SFTP Ruby script for downloading files from server I have written this Ruby script to download some file from server using SFTP. Help me improve this one.
Find the original script at this link

#This script is used to download file from the server to your local machine
#The implementation is based on pure Ruby implementation of SFTP using gem Net SFTP
#To install this on your local machine please run the following command in your terminal 'gem install net-sftp'
#Author : Ashish Wadekar <ashishawadekar@gmail.com>
#Date : 18 December 2016

#This is the gem / library required for the SFTP connection management
require 'net/sftp'

#The host address
HOST = "www.example.com"

#User for connection
USER = "admin"

#The location of the authentication key in pem format
SERVER_KEY_LOC = "/location/of/serverauthenticationkey.pem"

#The location on server where DB backups are done
SERVER_BACKUP = "/location/of/file/on/server"

#The location on local machine where the db has to be downloaded
LOCAL_BACKUP = "/location/on/local/machine/where/file/needs/to/be/downloaded"

#Array to hold the file names for easier sorting
files = Array.new

#Array to hold the file details for easier sorting
files_details = Array.new

#Inititalising the connection
Net::SFTP.start(HOST, USER, :keys => [SERVER_KEY_LOC]) do |sftp|
  puts "Connection to Server successful"
  entries = sftp.dir.entries([SERVER_BACKUP]).sort_by(&:name) #Sorting the files by name as timestamp is used as filename
  entries.each do |entry|
    files << entry.name
    files_details << entry.longname
  end

  download_size = files_details[-1].split(' ')[4].to_i

  puts "Total #{(files.size.to_i) - 2} backups found on server."
  puts "The latest file is : #{files[-1]}, Size : #{(download_size)/1048576} MB"
  puts "Now donwloading the file to your local machine at path : #{LOCAL_BACKUP}"

  sftp.download!(SERVER_BACKUP+files[-1], LOCAL_BACKUP+files[-1]) do |event, downloader, *args|
    case event
    when :open then
      # args[0] : file metadata
      puts "Starting download from server: #{args[0].remote} -> #{args[0].local}"
      puts "Perhaps you can sip a coffee till then!"
    when :get then
      # args[0] : file metadata
      # args[1] : byte offset in remote file
      # args[2] : data that was received
      STDOUT.write "Downloading complete: #{args[1].to_i/1048576}MB, #{args[1].to_i}bytes of #{download_size} bytes, #{((args[1].to_i / download_size) * 100.0)}% \r" unless args[1].to_i == 0

    when :close then
      # args[0] : file metadata
      puts "Done! Closing connection to server."
    when :mkdir then
      # args[0] : local path name
      puts "Creating directory #{args[0]}"
    when :finish then
      puts "Backup download complete!"
    end
  end
end

One of the core elements your code is missing is object-orientation. Take advantage of Ruby's powerful object model. That alone will vastly improve what you have.

What we're going to do is progressively breakdown everything into chunks of logic and encapsulate these chunks of logic in objects and methods.

Before that, let's go through each line and give feedback if anything seems a little off..

#The host address
HOST = "www.example.com"

It seems you've defined quite a few constants for things which, by nature, will change on a user-by-user basis. They are not constants, they are variables, and so it would be better to use variables.

However, if you do replace them with variables, you will not see any real benefit because the code is largely a procedural script. You have to define the constants/variables directly in the script and load that. You will see the benefit of using variables instead once the arguments and execution of the script are separated.

#Array to hold the file names for easier sorting
files = Array.new

You almost never need to initialize variables like this in Ruby. Look into inject for an alternative.

entries.each do |entry|
  files << entry.name
  files_details << entry.longname
end

This, along with the empty collections you defined above, give the impression that something is wrong. We are storing files and file details in separate arrays. There appears to be a focus on primitives, where we should instead be thinking about how we can encapsulate these in objects. There is nothing wrong with using arrays, but we should think about another approach when it becomes obvious that simple primitives are not providing the functionality we need in a clean way.

download_size = files_details[-1].split(' ')[4].to_i

The drawback of a simple array is made immediately obvious on the above line.

puts "Total #{(files.size.to_i) - 2} backups found on server."

We have littered about puts statements as a way to log the execution of the script. Perhaps we later want to log this to a file instead? That would be a good enough reason to extract it into its own object, and, if we want, later change the method of how we log.

puts "The latest file is : #{files[-1]}, Size : #{(download_size)/1048576} MB"

Another common mistake, though easily fixed, is the magic number 1048576. Give it some kind of meaning by hiding it behind a descriptive variable/constant or method.

# args[0] : file metadata

Along with magic numbers, we have magic indexes being used everywhere. You yourself can see this is difficult to follow and so have created a comment to remind you what you are dealing with. Again, this should be hidden behind a descriptive variable or method.

STDOUT.write "..."

We have now changed from puts to STDOUT.write. I haven't played around with this so unsure as to why, but lets assume this something we can also include in our logging object.

On a side-note - we have lots of comments in this code. As you begin to hide details behind descriptive names, you will see that the value in comments begins to diminish. If you have to litter comments everywhere then perhaps your code isn't readable enough?

Now let's start refactoring. With all that in mind, where do we begin? I can show you the end result, but that isn't useful in learning how to approach this in the future. The focus on creating small, composable objects and methods should be an ever-present design decision when programming Ruby. We are learning to think in objects.

The first thing I would do is throw the entire thing into a single object. Something like:

class RubySFTP
  HOST = "www.example.com"
  # etc

  def start
    files = Array.new
    files_details = Array.new

    Net::SFTP.start #etc
  end
end

This has immediately brought up the concept of RubySFTP. We would use as so: RubySFTP.new.start. It should be noted I could have created a class-method self.start and simply called RubySFTP.start, but that would take away the need to instantiate the object, which we will quickly make use of as we begin to breakdown our logic.

Again, focusing on small, descriptive methods, we can see start does not satisfy this at all. Because the logic is wrapped in a block, it is difficult to modify this to our tastes. For that reason, we will get rid of the block and simply use Net::SFTP.start(HOST, USER, :keys => [SERVER_KEY_LOC]).

That means we would then have:

  def start
    files = Array.new
    files_details = Array.new

    sftp = Net::SFTP.start # etc

    sftp.dir.entries # etc
  end

We are now initializing 3 variables in a single method. This can indicate a kind of code-smell when working with objects. We could clean this up greatly by extracting them into methods of the same name. When we do this it is important we don't reinitialize them every time they are called, so we use memoization.

For example, instead of sftp = [..], we have:

def sftp
  @sftp ||= Net::SFTP.start(HOST, USER, :keys => [SERVER_KEY_LOC])
end

If we do that with those other two variables (files and files_details), our start method now looks like:

def start
  sftp.dir.entries #etc
end

Now it is simply a case of running through things and seeing what we can extract into other methods..

entries = sftp.dir.entries([SERVER_BACKUP]).sort_by(&:name)

..becomes..

def sorted_entries
  @sorted_entries ||= sftp.dir.entries([SERVER_BACKUP]).sort_by(&:name)
end

..

download_size = files_details[-1].split(' ')[4].to_i

..becomes..

def download_size
  @download_size ||= files_details[-1].split(' ')[4].to_i
end

..and so on.. One other note here - though I am using memoization for each of these methods, this is only really a good idea if you are calling that method more than once. We might remove that later on, but noticing where you could benefit from memoization is a good habit to develop.

If we remove all the puts for now, let's see how this has affected the start method..

def start
  sorted_entries.each do |entry|
    files << entry.name
    files_details << entry.longname
  end

  sftp.download!(SERVER_BACKUP+files[-1], LOCAL_BACKUP+files[-1]) do |event, downloader, *args|
    # etc
  end
end

As you can see, this has clearly cleaned things up. We are hiding implementation details behind descriptive methods which is allowing us to think about the program from a better perspective.

Next, the way we are building files and files_details and the way we are accessing them is not that great - files_details[-1].split(' ')[4].to_i. We would much prefer if we could do something like file.download_size instead.

Ideally, what we want to do is wrap the files in our own object so we can build a more useful interface. We could also wrap the collection of files, but for simplicity we still use an array.

That means this:

entries.each do |entry|
  files << entry.name
  files_details << entry.longname
end

Should be, for one, in its own method, and, two, wrap the files (entry). That would look like:

def entries
  sorted_entries.map { |entry| Entry.new(entry) }
end

We would then build an Entry class and define a simple interface to extract what we're looking for. For example:

class Entry
  def initialize(entry)
    @entry = entry
  end

  def name
    @entry.name
  end

  def download_size
    @entry.longname.split(' ')[4].to_i
  end
end

Now our start method simply has:

def start
  sftp.download!(SERVER_BACKUP+entries[-1].name, LOCAL_BACKUP+entries[-1].name) do |event, downloader, *args|
    # etc
  end
end

The next thing we want to remove is those magic indexes (entries[-1]). We could do that by defining a new method on our RubySFTP object called last_entry..

def last_entry
  entries[-1]
end

Simple, but more descriptive. This might not matter so much for -1 as anybody working on this code should know that -1 is the last index, but it is something to keep an eye on.

While we're at it, we'll also extract SERVER_BACKUP+files[-1] and LOCAL_BACKUP+files[-1] into descriptive methods..

def last_entry_remote_path
  SERVER_BACKUP + last_entry
end

def last_entry_local_path
  LOCAL_BACKUP + last_entry
end

Now let's tackle the sftp.download! block.

I will not rewrite the whole thing, but you should consider using a custom handle as detailed in the docs here.

That means you would have a custom handler such as:

class CustomHandler
  def on_open(downloader, file)
    puts "starting download: #{file.remote} -> #{file.local} (#{file.size} bytes)"
  end

  def on_get(downloader, file, offset, data)
    puts "writing #{data.length} bytes to #{file.local} starting at #{offset}"
  end
end

..and your sftp call would look like:

sftp.download!(last_entry_remote_path, last_entry_local_path, progress: CustomHandler.new)

That about ends the core of our refactoring. Let's address the log for a second. Effectively what we want is Logger.log('etc'). We can then change the implementation without affecting the code which uses the logger. Here's a simple logger:

class Log
  def self.log(string)
    puts string
  end
end

You can see now how you can change puts to write to a file instead without actual editing the code which uses the logger.

Now let's fix the constants.

I said before that those constants weren't actually constants. Truth is, they ARE constants in the context of your script because they will not change throughout the execution of it. However, if you extract your script into objects which will be defined in different files, you introduce an element of reusability. You can instantiate your RubySFTP class many times, and pass through different variables as you see fit.

For that reason, we should convert those constants to variables and use as such so we can do:

RubySFTP.start(host, user, server_key, remote_path, local_path)

Though this isn't technically a requirement of your program, using objects has still allowed us to clean things up, and will serve you as it grows.

Before we update our class, we should think about reducing the amount of arguments being passed into it in our newly defined interface above. Generally, 3 arguments is enough. 5 is surely too much. One way to combat this might be to pass in an options hash. Having said that, for simplicity, I am going to keep the 5 arguments.

The long initialize method is a code smell. It doesn't look right for a reason - we are passing in too many arguments.

class RubySFTP
  def initialize(host, user, server_key, remote_path, local_path)
    @host = host
    @user = user
    @server_key = server_key
    @remote_path = remote_path
    @local_path = local_path
  end

  def self.start(host, user, server_key, remote_path, local_path)
    new(host, user, server_key, remote_path, local_path).start
  end

  def start
    # etc
  end
end

With all of those changes, here is the finished code, broken down into a few objects - RubySFTP, Entry, and CustomHandler. Keep in mind that this can be broken down further.

class RubySFTP
  def initialize(host, user, server_key, remote_path, local_path)
    @host = host
    @user = user
    @server_key = server_key
    @remote_path = remote_path
    @local_path = local_path
  end

  def self.start(host, user, server_key, remote_path, local_path)
    new(host, user, server_key, remote_path, local_path).start
  end

  def start
    # prime for extraction!
    Logger.log("Connection to Server successful")
    Logger.log("Total #{entries.size.to_i} backupds found on server.")
    Logger.log("etc")
    Logger.log("etc")

    sftp.download!(
      last_entry_remote_path,
      last_entry_local_path,
      progress: CustomHandler.new
    )
  end

  private

  def sftp
    @sftp ||= Net::SFTP.start(@host, @user, keys: [server_key])
  end

  def sorted_entries
    @sorted_entries ||= sftp.dir.entries([@remote_path]).sort_by(&:name)
  end

  def entries
    sorted_entries.map { |entry| Entry.new(entry) }
  end

  def last_entry
    entries[-1]
  end

  def last_entry_remote_path
    @remote_path + last_entry.name
  end

  def last_entry_local_path
    @local_path + last_entry.name
  end
end

Entry

class Entry
  def initialize(entry)
    @entry = entry
  end

  def name
    @entry.name
  end

  def download_size
    @entry.longname.split(' ')[4].to_i
  end
end

CustomHandler -- from here

class CustomHandler
  def on_open(downloader, file)
    puts "starting download: #{file.remote} -> #{file.local} (#{file.size} bytes)"
  end

  def on_get(downloader, file, offset, data)
    puts "writing #{data.length} bytes to #{file.local} starting at #{offset}"
  end

  def on_close(downloader, file)
    puts "finished with #{file.remote}"
  end

  def on_mkdir(downloader, path)
    puts "creating directory #{path}"
  end

  def on_finish(downloader)
    puts "all done!"
  end
end

Couple of final notes - I might have missed a couple of things during this review and refactoring, but I hope it gives you a good foundation to work from. Secondly, I have left some things for you to clean up yourself so you can apply the advice given -- for e.g. files.size.to_i - 2 should be extracted into a total_files method.

Let me know if you have any questions or have noticed something that doesn't add up and I'll try to help.


Dice rolling simulator, any ideas on implementing any features? This is one of my mini project of dice rolling and I want to improve it further with more advanced Python. Maybe you have any ideas to improve the code itself?

This code will ask the minimum and maximum number for the dice from the user and the number of times it will roll them, giving random numbers.

import random
#Size choosing for dice
while True:
    #Choosing the minimum number
    Min = input("Please input the minimum number of your dice: ")
    try:
        Min = int(Min)
    except ValueError:
        print("Invalid input")
        continue
    #Choosing the maximum number
    Max = input("Please input the maximum number of your dice: ")
    try:
        Max = int(Max)
    except ValueError:
        print("Invalid input")
        continue
    #Check if the minimum and maximum is valid
    if Min > Max:
        print("Minimum is bigger than Maximum.\n Please reinput data...")
        continue
    elif Min == Max:
        print("Minimum is equaled to Maximum,\n Please reinput data...")
        continue
    print(f"Your dice is from {Min} to {Max}.")
    break

#Random number generater
while True:
    Rollcount = input("How many times do you want to roll the dice: ")
    try:
        Rollcount = int(Rollcount)
        break
    except ValueError:
        print("Invalid input")
        continue
for i in range(0,Rollcount):
    roll = random.randint(Min,Max)
    print(roll)

I normally would agree with Graipher here, however with your code I think there's a simpler and more readable solution.

The first number doesn't have to be larger than the second.

This is an artificial limitation that can be fixed by sorting the inputted values.
Not allowing the same value to be selected is an artificial limitation. random.randint includes both bounds, meaning random.randint(n, n) is valid input.
You can bundle the first two try statements together to simplify the code.

This makes the code DRY as the two loops are logically doing two different things.

Whilst longer it keeps the functionality to change the lower bound if you enter it incorrectly, without having to kill the process.

import random

while True:
    try:
        min_, max_ = sorted([
            int(input("Please enter one of the bounds: "))
            int(input("Please enter the other bound: "))
        ])
        break
    except ValueError:
        print("Invalid input")

while True:
    try:
        amount = int(input("How many times do you want to roll the dice: "))
        break
    except ValueError:
        print("Invalid input")

for _ in range(amount):
    print(random.randint(min_, max_))

Note: You can move both the while try loops into a function, and pass a lambda, however I don't think that's as readable.

min_, max_ = while_try(lambda: sorted([
    int(input("Please enter one of the bounds: "))
    int(input("Please enter the other bound: "))
]))


This LINQ Model Query seems extremely inefficient I'm new to LINQ, but I have some background in T-SQL. I know there are probably 100 different ways to design a T-SQL statement that would run this much more efficiently, but I'm not sure how I would do it in LINQ and I would like to stick with LINQ.

This works perfectly well, I just hate the way it looks and am unsure of how to fix it.

I am using MVC3, C#, LINQ, Entity Framework 4.3

public class CollectionRepository : ICollectionRepository {
    private CollectionEntities db = new CollectionEntities();

    public IEnumerable<Collection> GetCollectionByUid(long id) {
        var _collection = (from c in db.UserCollections
                          where c.uid == id
                          orderby c.CreateDate descending
                          select new Collection {
                              Name = c.CollectionName,
                              Type = (from t in db.CollectionTypes
                                      where t.ctypeid == c.Type
                                      select t.CollectionTypeName).FirstOrDefault(),
                              Created = c.CreateDate,
                              Count = (from f in db.Figures
                                       where f.CollectionID == c.cid
                                       select f).Count()
                          }).ToList();
        return _collection;
    }
}

The constructors it points to:

public class Collection {
    public string Name { get; set; }
    public string Type { get; set; }
    public DateTime Created { get; set; }
    public int Count { get; set; }
}

As this is EF code, assuming you have all the associations set up correctly, you should be able to write it something like this:

public IEnumerable<Collection> GetCollectionByUid(long uid) {
    var result =
       (from collection in db.UserCollections
        where collection.uid == uid
        orderby collection.CreateDate descending
        select new Collection {
            Name = collection.CollectionName,
            Type = collection.CollectionType.CollectionTypeName,
            Created = collection.CreateDate,
            Count = collection.Figures.Count(),
        }).ToList();
    return result;
}

My only real criticism is the variable names.

Local variables or parameters should not be prefixed with an underscore.  IMHO, it should only be allowed for private instance fields.  So rather than using _collection, it would be better off as collection (though I used result for the name here instead).

Parameter names are very much part of the documentation as well (especially so in C# and .NET in general).  The method is called GetCollectionByUid which suggests you provide a uid.  The parameter name should reflect that.  Call it uid since that's what it is.

Personal preference but I also prefer to write out the full/reasonable-length variable names in queries.  I reserve significantly shortened variable names (i.e., one character variable names) in lambda expressions.


Filtering out empty Optionals I want to convert my list of optional strings to a list of strings by getting rid of the empty Optionals.

Is there a shorter version for achieving this than the following code (except statically importing the methods of Collectors)?

List<Optional<String>> stringsMaybe = Arrays.asList(Optional.of("Hi"),
                                      Optional.empty(), Optional.of(" there!"));

List<String> strings = stringsMaybe
            .stream()
            .filter(Optional::isPresent)
            .collect(Collectors.mapping(Optional::get, Collectors.toList()));

It's more idiomatic to use .map on the stream instead of Collectors.mapping:

stringsMaybe.stream()
    .filter(Optional::isPresent)
    .map(Optional::get)
    .collect(toList());

Without introducing a helper method or a custom collector, that's the shortest and clearest way to do this.

Since Java 9, Optional offers a stream method, enabling you to do .flatMap(Optional::stream) instead of .filter(...).map(...).


follow-up review of a custom Undo Manager This is a follow up review of question 'a custom Undo Manager'.

After reviewing all the comments and answers, my code has been revised (completely rewritten) based on the review.

Summary

The intent of the code is to provide a means to keep track of events in c#. Each Action has an undo and redo event (Command) and is pushed onto a redo or undo Stack<Command>. An object can create an instance of the StateManager, which should be private in order keep the undo and redo events encapsulated. For example purposes, I created a class called Car, which has a property of type StateManager which handles all the events. Your comments and suggestions are appreciated.

public interface ICommand
{
    // Executes this command.
    void Execute();

    // Undoes this command.
    void Undo();
}

/// <summary>
/// The Command class holds a reference to an Event and the Undo Event
/// </summary>
public class Command:ICommand
{
    private readonly Action _action;
    private readonly Action _undoAction;

    public Command(Action action, Action undoAction)
    {
        _undoAction = undoAction;
        _action = action;
    }

    public void Execute()
    {
        _action();
    }

    public void Undo()
    {
        _undoAction();
    }

}

public interface IStateManager
{
    // Executes the specified commmand and adds it to the Undo stack.
    void ChangeState(Command commmand);

    // Undoes the last command in the Undo stack.
    void RestorePreviousState();

    // Redoes the last command in the Redo stack.
    void RedoPreviousState();
}

public class StateManager:IStateManager
{
    private Stack<Command> _undoCommands = new Stack<Command>();
    private Stack<Command> _redoCommands = new Stack<Command>();

    public void ChangeState(Command command)
    {
        command.Execute();
        _undoCommands.Push(command);
    }

    public void RestorePreviousState()
    {
        if (this._undoCommands.Any()) {

            var command = _undoCommands.Pop();
            command.Undo();
            _redoCommands.Clear();// clear the redo stack
            _redoCommands.Push(command);// push event on redo stack
        }

    }

    public void RedoPreviousState() {

        if (this._redoCommands.Any())
        {
            var commandPop = this._redoCommands.Pop();
            commandPop.Execute();
            this._undoCommands.Push(commandPop);  // put the event on the undo stack

        }
    }
}

public class Car
{
    /// <summary>
    /// StateManager - manages the state of properties/methods that call  StateManager.ChangeState.
    /// The StateManager is set to private for encapsulated - The State of the object is only handled
    /// by the object that instantiates it.
    /// </summary>
    private StateManager _manager { get; set; }

    private String color;

    public String Color
    {
        get { return color; }
        set {
            string currentValue = color;
            _manager.ChangeState(new Command(() => color = value,
                                                    () => color = currentValue));
        }
    }
    public Car()
    {
        _manager = new StateManager();
    }

    /// <summary>
    /// Undo the last event.
    /// </summary>
    public void Undo() {

        _manager.RestorePreviousState();
    }

    /// <summary>
    /// Redo the last event
    /// </summary>
    public void Redo()
    {
        _manager.RedoPreviousState();
    }

}

I think you should not clear _redoCommands stack when you are re-doing command. Actually you should be able to continue re-doing command until you will restore state when you called first undo command. You should clear redo-commands when completely new command arrives:

public void ChangeState(Command command)
{
    command.Execute();
    _undoCommands.Push(command);
    _redoCommands.Clear(); // new command arrived, you cannot redo anymore
}

public void RestorePreviousState()
{
    if (!_undoCommands.Any())
        return;

    var command = _undoCommands.Pop();
    command.Undo();
    _redoCommands.Push(command);
}

public void RedoPreviousState()
{
    if (!_redoCommands.Any())
       return;

    var command = _redoCommands.Pop(); // commandPop a little strange name :)
    command.Execute();
    _undoCommands.Push(command);
}

Also I'm thinking on improving naming... because RestorePreviousState() looks like restoring state which was before last state change - i.e. two call of this method should leave object in initial state.

UPDATE: I'd go with UndoStateChange and RedoStateChange without word previous - I find such terms less confusing. Also I'd think about making public properties for checking available state changes. Because currently it's not clear whether state changed or not. You call UndoStateChange and nothing is returned whether state changed or there is nothing to undo.


Is there a more semantic way to markup Price Lists? Example as Table and as Description List I have a pricing list that I can make sense of as both a table and as a description list.  Using a table feels hackish.  I thought a dl might be appropriate because description lists "are useful for displaying metadata as a list of key-value pairs" according to the docs at MDN

As a table

Server Cost

                        2VPU + 4GB Memory

                        $ 0.066666 / hr

                        10 GB Storage Disks

                        $ 0.088888 / hr

                        $0.155554 / hr

Monthly Licensing Cost

                    @0.00 per CPU x 1

                    $ 0.00 / month

As a description list

Server Cost

2VPU + 4GB Memory

$ 0.066666 / hr

10 GB Storage Disks

$ 0.088888 / hr

subtotal

$ 0.155554 / hr

Licensing Cost

@0.00 per CPU x 1

$ 0.00 / month

Here's a codepen with the examples:

http://codepen.io/anon/pen/bAoLe

You can see that they display more or less the same, I'm interested to know what is the more semantic way of representing data like this.

I'd say stick with the table.

Yes, tables are frowned upon - if you use them for purely visual purposes. But this isn't "just" layout; it's information structure. And these are tabular data sets so semantically speaking tables are the right choice. There's nothing hacky about it; the table element was made for this sort of thing.

Meanwhile, dls are for terms and their definition(s). Semantically speaking, is an item defined by its cost? Eh, not quite. Of course this is a rather pedantic interpretation I'm applying, and a dl solution wouldn't be completely crazy, but still.

Flexibility is another issue: If an item only has name and cost, sure, a list will work. But what if that's not all? What if you need to add a column for the number of units ordered/in stock/shipped/whatever. Or an SKU column? Or anything else, really?

Yes, you can have multiple dd elements following a dt but you'll have to do all the layout yourself for n "pseudo-columns" of unknown content, whereas a table just works it out for you including resizing itself, wrapping text where necessary and all that. Again: It's made for this.

Think of it this way: If it wasn't a web page, and you just had to make a document, wouldn't you use a spreadsheet, or at least use table in a word processor? If yes, use a table in on the webpage too.

Other stuff:

I'd add a thead element with th elements for column headers, and wrap the rest in a tbody
I'd probably use a span for the total cost instead of strong. Give the span a class like cost or amount or total for styling purposes. I'm assuming you just want the price to stand out visually, but using a strong is a bit, well, strong. A span, meanwhile, doesn't imply much at all about its content. Even a good ol' b tag could be used here. b simply says "this text is bold" without implying why it's bold - which is why you don't often see the b tag anymore: no semantic purpose. But that's kinda what you want here; just some bold text for visual purposes.


isStraight evaluation function The context is Weekend Challenge #2 (Poker hand evaluation).

Hand.prototype.isStraight = function()
{
  for( var i = 1 ; i < this.cards.length ; i++ )
    if( this.cards[i].value + 1 != this.cards[i-1].value )
      return false;
  return true;
}

cards is an array with card objects { suit : "x" , value : "0->12" }.  The cards are sorted during the creation of the hand.

My approach to detect straightness seems like the hard way.  Can anyone suggest something else?

Okay, Poker is tough.  I will revise this question once I have my new isStraight.

It's not that hard.

Instead of sorting the cards, looping over them and skipping double results, let's convert them to a more useful format:

var bitmap = 0;

for(var i = 0; i < cards.length; i++)
{
    var value = cards[i].value;

    // set i+1 bit in the bitmap
    bitmap |= 1 << (value + 1);

    // if it's an ace, also set the low bit
    if(value === 12)
        bitmap |= 1;
}

Now, if there's a card of value i in your hand, the i+1 bit will be set in the bitmap. An Ace is treated as two cards with values 13 and 0.
Next, we scan the bitmap for 5 consecutive bits set, which is equivalent to 31 (1 | 2 | 4 | 8 | 16). We start with the highest straight, which is 9.
The lowest straight is 0, if there is no straight i is -1.

for(var i = 10; i--; )
{
    if((bitmap & 31 << i) === (31 << i))
    {
        break;
    }
}

The method works for games with more than 5 cards. Note that in any variant of Poker, 5 cards make a hand, so even in Omaha with 9 cards, 5 cards give you a straight.

Note: The method is roughly equivalent to creating a set (and checking for 5 consecutive elements). As you can see, with bit-wise operators it is much simpler to check for 5 consecutive cards.

Another note: There's a harder-to-understand but slightly faster method for testing a straight given the bitmap. I might add it later, but for now I think this method hard enough to understand for beginners.


Groupby count, then sum and get the percentage I wrote this code.

It works, but I think there is a more elegant and Pythonic way to this task.

Groupby and count the different occurences
Get the sum of all the occurences
Divide each occurrence by the total of the occurrences and get the percentage

#Creating the dataframe
##The cluster column represent centroid labels of a clustering alghoritm
df=pd.DataFrame({'char':['a','b','c','d','e'], 'cluster':[1,1,2,2,2]})

#Counting the frequency of each labels
cluster_count=df.groupby('cluster').count()

#Calculating the sum of the frequency
cluster_sum=cluster_count.sum()

#Normalizing the frequency
cluster_prct=cluster_count.char.apply(lambda x: 100*x/cluster_sum)

print(cluster_prct)

Output:

cluster
1        40.0
2        60.0

I think your code is already nearly optimal and Pythonic. But there is some little things to improve:

cluster_count.sum() returns you a Series object so if you are working with it outside the Pandas, it is better to specify the column: cluster_count.char.sum(). This way you will get an ordinary Python integer.
Pandas has an ability to manipulate with columns directly so instead of apply function usage you can just write arithmetical operations with column itself: cluster_count.char = cluster_count.char * 100 / cluster_sum (note that this line of code is in-place work).

Here is the final code:

df = pd.DataFrame({'char':['a','b','c','d','e'], 'cluster':[1,1,2,2,2]})
cluster_count=df.groupby('cluster').count()
cluster_sum=sum(cluster_count.char)
cluster_count.char = cluster_count.char * 100 / cluster_sum

Edit 1: You can do the magic even without cluster_sum variable, just in one line of code:

cluster_count.char = cluster_count.char * 100 / cluster_count.char.sum()

But I am not sure about its perfomance (it can probably recalculate the sum for each group).


Javascript function to convert SQL resultset to a string function get_sql_string(dbConn,query_string,col_delim,row_delim) {
    var result = dbConn.executeCachedQuery( query_string );
    var result_string = '';
    var column_count = result.getMetaData().getColumnCount();
    if ( col_delim ){
        col_delim = col_delim;
    }
    else {
        col_delim = ',';
    }

    if ( row_delim ){
        row_delim = row_delim;
    }
    else {
        row_delim = '\n';
    }

    if(result.size() > 0){
        while (result.next()) {
            i = 1;
            var line = '';
            while (i <= column_count) {
                line = line + result.getString(i) + col_delim;
                i++;
            }
            result_string = result_string + line.slice(0, -1) + row_delim;
        }
        result_string = result_string.slice(0, -1);
        result.close();
        if (result_string.toString() != 'null' || result_string != null) {
            return result_string.trim();
        }
        else {
            return '';
        }
    }
    else {
        dbConn.close();
        return '';
    }
    dbConn.close();
}

Executes a SQL select query and creates a string from the results

@param0 {String} dbConn - connection string for database
@param1 {String} query_string - query to execute
@param2 {String} col_delim - column delimiter separates each value in a row, default value is a commma
@param3 {String} row_delim - row delimiter separates earch row in the resultset, default value is a newline character
@return {String} returns the resultset as a string with column and row delimiters

Above is a function in that converts a SQL resultset to a string. I want to learn how to make this code more succint for maintainability. Can someone help? Thanks!

You write this is a

  Javascript function to convert SQL resultset to a string

however that is not true. It's a function that

executes an SQL query
converts its result set to a string
(sometimes) closes its DB connection

Generally its bad idea to have a function to do more than one thing. Especially the closing of a DB connection that (I presume) is passed in open is very out of place and unexpected to the user of this function, more so because it only happens when the result set of the query is empty. (The last dbConn.close() is never executed).

Conversely the result set is only closed if it isn't empty. I don't know which DB library you are using, so it may be ok, but I doubt it.

Generally you should be using try ... finally blocks to make sure you are closing your resources.

Next you have pointless assignments such as col_delim = col_delim; when handling default argument values. All current JavaScript implementations support default
parameters:

 function get_sql_string(dbConn, query_string, col_delim = ",", row_delim = "\n") {

Also look into using const/let instead of var.

Another potential bug is the use of .slice(0, -1). Besides being inefficient, it assumes that the delimiters are exactly one character long, which may not be the case. At the very least you should be using .slice(0, -col_delim.length), better would be not to add this final delimiter in the first place. This could be done by collecting the results/lines in arrays and using .join(col_delim).

String concatenation (especially in a loop) is anyway very inefficient and using join would help here, too.

Other than that: Code formatting is very inconsistent. For example:

if( vs if (
if (...){ if (...) {

etc. Choose one style and stay with it (preferably the latter version). Consider using an editor/IDE that can format the code for you.

Example reimplentation (untested):

function executeQueryAndFormatResult(dbConn, queryString, colDelim = ",", rowDelim = "\n") {
  try {
    const result = dbConn.executeCachedQuery(queryString);
    return formatResult(result, colDelim, rowDelim);
  } finally {
    if (result) {
       result.close();
    }
  }
}

function formatSqlResult(result, colDelim = ",", rowDelim = "\n") {
   const columnCount = result.getMetaData().getColumnCount();
   const lines = [];
   while (result.next()) {
      const row = [];
      row.length = columnCount; // Allows JS engine to reserve the needed number of entries ahead of time
      for (let i = 1; i <= columnCount; i++) {
         row[i-1] = result.getString(i);
      }
      lines.push(row.join(colDelim));
   }
   return lines.join(rowDelim);
}

EDIT: Do the columns actually start with index 1? It seems unusual, but I've adjusted my code.


Parse and sort domain names and their prices I set out to write a small Python script to tell me cheapest domains. I manually scraped for data from iwantmyname.com into a plain text file as follows:

.COM
Commercial
$14.90
The most registered domain extension on the planet.
.CO 48% off!
Colombia
$19.90
then $39.00/year
It's .com without the m.
.APP
Generic
$29.00
Your app needs a new home
.NET
Network
$17.10
Think garage startups, beards before they were cool, and hobbyist tinkering.

This file has about 2050 lines.

First I wrote a script to get the domains and prices by reading lines that start with a . for domain and a $ for the associated price.

from operator import itemgetter

domain_prices_raw = []

with open(r'domain_prices.txt') as f:
    domain_prices_raw = f.readlines()

domain_prices_raw = [x.strip() for x in domain_prices_raw]

domains = []
prices = []

for line in domain_prices_raw:
    if line.startswith('.'):
        domains.append(line.split(' ')[0])
    if line.startswith('$'):
        prices.append(float(line.replace('$', '')))

sorted_domain_prices = sorted(
    list(zip(domains, prices)), key=itemgetter(1))

with open('sorted_domain_prices.txt', 'w') as f:
    for domain, price in sorted_domain_prices:
        f.write("{0}\t{1}\n".format(domain, price))

A bit later I realized that some domains have a low price only for the first year and subsequently have higher price per year. So I updated the script to read the lines that start with then.

Example from the file:

.CO 48% off!
Colombia
$19.90
then $39.00/year

Updated script is as follows:

from operator import itemgetter

domain_prices_raw = []

with open(r'domain_prices.txt') as f:
    domain_prices_raw = f.readlines()

domain_prices_raw = [x.strip() for x in domain_prices_raw]

domains = []
prices = []
prices_after_first_year = []

for line in domain_prices_raw:
    if line.startswith('.'):
        domains.append(line.split(' ')[0])
    if line.startswith('$'):
        prices.append(float(line.replace('$', '')))

    prices_after_first_year.append(float(line.split(' ')[1].split(
        '/')[0].replace('$', '')) if line.startswith('then') else 0)

sorted_domain_prices = sorted(
    list(zip(domains, prices, prices_after_first_year)), key=itemgetter(1))

with open('sorted_domain_prices.txt', 'w') as f:
    for domain, price, after in sorted_domain_prices:
        f.write("{0}\t{1}\t{2}\n".format(domain, price, after))

The script ran quickly enough and I was happy that I could get a working solution, if only for a few minutes.

I know I used zip to associate adjacent items and that took care of most of the logic that I would have had to write if I weren't aware the existence of something like zip from the knowledge of other programming languages.

Could the script be improved other ways to be more pythonic, more performance oriented and if nothing else, just a more expressive and or OO way to solve the problem?

Bonus question: Could a CS degree have guided me to arrive at a proper solution?

One thing that makes a script like this more readable, maintainable and extendable is giving names to things. The easiest way to do that is using functions.

First I would make a function that reads and cleans your file:

def get_domain_prices(file_name):
    with open(file_name) as f:
        for line in f:
            yield line.strip()

This is a generator producing each cleaned line as needed. There is no need to read the whole file into memory here since you process it line by line anyway.

Next, your if conditions are mutually exclusive (a line can only start with one character), so you could use elif the second time to save yourself one check if the first one is true.

For the parsing of the "then $39.00/year" I would use a regex. This format is fixed enough for this to be easy and also complicated enough that splitting and replacing becomes cumbersome. The regex just searches for the string "then $x.yy/year", where before the decimal point there must be one or more digits and after the decimal point there are exactly two digits.

import re

for line in get_domain_prices('domain_prices.txt'):
    if line.startswith('.'):
        if len(prices_after_first_year) < len(domains):
            prices_after_first_year.append(prices[-1])
        domains.append(line.split()[0])
    elif line.startswith('$'):
        prices.append(float(line.replace('$', '')))
    elif line.startswith("then"):
        price = float(re.search(r'then \$(\d+.\d\d)/year', line).groups()[0])
        prices_after_first_year.append(price)

I also merged this checking into the elif chain and put a safe guard in the first condition to check if the previous domain had th price after one year set and if not add the price of that domain.

sorted can take a zip object, there is no need to cast it to list first (even in Python 3). If you re-structured the order in the zip (and in the write) you don't need to set the key and rely on the normal sorting of tuples (beware that this might produce a different order for domains with the same price, because then they are ordered by the next element of the tuple).

Since I guess you are using Python 3 (and if you are not yet, you should), I used f-strings (Python 3.6+) below to make the writing a bit easier.

sorted_domain_prices = sorted(zip(prices, prices_after_first_year, domains))
with open('sorted_domain_prices.txt', 'w') as f:
    for price, after, domain in sorted_domain_prices:
        f.write(f"{domain}\t{price}\t{after}\n")

Putting it all together:

import re

def clean_lines(file_name):
    with open(file_name) as f:
        for line in f:
            yield line.strip()

def parse_domain_prices(file_name):
    for line in clean_lines(file_name):
        if line.startswith('.'):
            if len(prices_after_first_year) < len(domains):
                prices_after_first_year.append(prices[-1])
            domains.append(line.split(' ')[0])
        elif line.startswith('$'):
            prices.append(float(line.replace('$', '')))
        elif line.startswith("then"):
            price = float(re.search(r'then \$(\d+.\d\d)/year', line).groups()[0])
            prices_after_first_year.append(price)
    return prices, prices_after_first_year, domains

def write_domain_prices(sorted_domain_prices, file_name):
    with open(file_name, 'w') as f:
        for price, after, domain in sorted_domain_prices:
            f.write(f"{domain}\t{price}\t{after}\n")

if __name__ == "__main__":
    data = parse_domain_prices('domain_prices.txt')
    write_domain_prices(sorted(zip(*data)), 'sorted_domain_prices.txt')

Here I modified the order in which the tuple is returned to be able to directly pass it to zip. I also added a if __name__ == "__main__": guard to allow importing parts of this script from another script without running it.

As a last step you should consider adding docstrings that describe what each function does so you can still remember in 6 months.


Singly Linked list implementation in C++ How can I improve this code
Please tell proper indentation and usage of camel-case.

#include <iostream>
using std::cout;
struct Node
{
    int data;
    Node* next;
};
Node* Create(int data){
    Node* root=new Node();
    root->data=data;
    root->next=NULL;
    Node* head=root;
    return head;
}
void push_back(int data,Node* head){
    if(!head->next)
    {
        Node* newNode=new Node();
        newNode->data=data;
        newNode->next=NULL;
        head->next=newNode;
    }
    else
        push_back(data,head->next);
}
Node* push_front(int data,Node* head){
    Node* newNode=new Node();
    newNode->data=data;
    newNode->next=head;
    head=newNode;
    return head;
}
void pull_back(Node* head){
    Node* temp=head;
    if(!temp->next)
        delete temp;
    else
        pull_back(temp->next);
}
Node* pull_front(Node* head){
    Node* temp=head->next;
    delete head;
    Node* NewHead=temp;
    return NewHead;
}
void print(Node* head){
    while(head)
        cout<<head->data<<" ",
        head=head->next;
    cout<<"\n";
}
Node* Reverse(Node* head)
{
    Node* prev   = NULL;
    Node* current = head;
    Node* next;
    while (current != NULL)
    {
        next  = current->next;
        current->next = prev;
        prev = current;
        current = next;
    }
    head = prev;
    return head;
}
int Search(int data,Node* head,int count=1){
    if(head->data==data)
        return count;
    if(!head->next)
        return -1;
    return Search(data,head->next,count+1);
}
Node* MakeCircular(Node* head)
{
    Node* temp=head;
    while(!temp->next)
        temp=temp->next;
    temp->next=head;
    return head;
}
bool DetectCircle(Node* head){
    //using floyd's cycle
    Node* fast=head;
    Node* slow=head;
    if(!(slow && slow->next))return false;
    while(fast && fast->next){
        slow=slow->next;
        fast=fast->next->next;
        if(slow==fast)
            return true;
    }
    return false;
}
int main(int argc, char const *argv[])
{
    Node* head=Create(12);
    push_back(32,head);
    push_back(123,head);
    head=push_front(-21,head);
    print(head);
    head=Reverse(head);
    print(head);
    return 0;
}

I see a number of things that may help you improve your code.

Prefer references to raw pointers

In modern C++, we tend not to use raw pointers very often.  In this case, It would probably be better to have two different classes, one would be a LinkedList class and the other would be a Node class.  That way, instead of starting with a pointer, you can start with an object.

Use objects

You have a Node structure and then separate functions that operate on Node data. With only a slight syntax change, you would have a real object instead of C-style code written in C++.

Use whitespace to improve readability

Lines like this:

head=push_front(-21,head);

become much easier to read with a little bit of whitespace:

head = push_front(-21, head);

Use nullptr rather than NULL

Modern C++ uses nullptr rather than NULL.  See this answer for why and how it's useful.

Use consistent naming

The code includes print() (lowercase) and Search() (uppercase).  It would be better for both functions to have similar case.  I tend to use capital letters to name clases or structs such a Node and then lowercase letters for functions such as print or search.

Match new with delete

If you allocate memory using new, you must also free it using delete or your program will leak memory.  This program leaks memory.

Use more descriptive names

The code's DetectCircle function seems to return true if the list is circular.  Generally, it's good to name boolean functions so that their sense is unambiguous.  For example, I'd probably call it isCircular.

Omit return 0

If your program completes successfully, the return 0 at the end of main() will be generated automatically, so it's not needed in C++ programs.


Pinging a list of hosts I have a simple script that will take a list of hosts and ping each host (there's about 200) a single time before moving on. This is not the most effecient method I am sure, as it is very linear. And it takes a few minutes to complete. I would ideally like to run this script every minute (so each IP address is checked every minute (the actual running of the script is controlled externally).

I was looking for some pointers on potentially making the script more effecient/dynamic.

import sys
import os
import platform
import subprocess

plat = platform.system()
scriptDir = sys.path[0]
hosts = os.path.join(scriptDir, 'hosts.txt')
hostsFile = open(hosts, "r")
lines = hostsFile.readlines()
if plat == "Windows":
    for line in lines:
        line = line.strip( )
        ping = subprocess.Popen(
            ["ping", "-n", "1", "-l", "1", "-w", "100", line],
            stdout = subprocess.PIPE,
            stderr = subprocess.PIPE
        )
        out, error = ping.communicate()
        print out
        print error

if plat == "Linux":
    for line in lines:
        line = line.strip( )
        ping = subprocess.Popen(
            ["ping", "-c", "1", "-l", "1", "-s", "1", "-W", "1", line],
            stdout = subprocess.PIPE,
            stderr = subprocess.PIPE
        )
        out, error = ping.communicate()
        print out
        print error

hostsFile.close()

Here's a solution using threads:

import sys
import os
import platform
import subprocess
import Queue
import threading

def worker_func(pingArgs, pending, done):
    try:
        while True:
            # Get the next address to ping.
            address = pending.get_nowait()

            ping = subprocess.Popen(ping_args + [address],
                stdout = subprocess.PIPE,
                stderr = subprocess.PIPE
            )
            out, error = ping.communicate()

            # Output the result to the 'done' queue.
            done.put((out, error))
    except Queue.Empty:
        # No more addresses.
        pass
    finally:
        # Tell the main thread that a worker is about to terminate.
        done.put(None)

# The number of workers.
NUM_WORKERS = 4

plat = platform.system()
scriptDir = sys.path[0]
hosts = os.path.join(scriptDir, 'hosts.txt')

# The arguments for the 'ping', excluding the address.
if plat == "Windows":
    pingArgs = ["ping", "-n", "1", "-l", "1", "-w", "100"]
elif plat == "Linux":
    pingArgs = ["ping", "-c", "1", "-l", "1", "-s", "1", "-W", "1"]
else:
    raise ValueError("Unknown platform")

# The queue of addresses to ping.
pending = Queue.Queue()

# The queue of results.
done = Queue.Queue()

# Create all the workers.
workers = []
for _ in range(NUM_WORKERS):
    workers.append(threading.Thread(target=worker_func, args=(pingArgs, pending, done)))

# Put all the addresses into the 'pending' queue.
with open(hosts, "r") as hostsFile:
    for line in hostsFile:
        pending.put(line.strip())

# Start all the workers.
for w in workers:
    w.daemon = True
    w.start()

# Print out the results as they arrive.
numTerminated = 0
while numTerminated < NUM_WORKERS:
    result = done.get()
    if result is None:
        # A worker is about to terminate.
        numTerminated += 1
    else:
        print result[0] # out
        print result[1] # error

# Wait for all the workers to terminate.
for w in workers:
    w.join()


Login system, PDO and me I have run into a bit of a dilemma.  Binding user input and building a prepared statement is all well and good, but what if I need the user input as a variable for a compare?

Can I just create a function to clean the input?

This is the line that worries me:

$password = $_POST['password']

What I have working so far:

// QUERY
public function query($query){
    $this->stmt = $this->dbh->prepare($query);
}

// RESULT SET
public function resultset(){
$this->execute();
    return $this->stmt->fetchAll(PDO::FETCH_ASSOC);
}

// EXECUTE
public function execute(){
    return $this->stmt->execute();
}

Check that the password matches with the database password if username exists

<?php
if (isset($_POST['login'])) {

    $dbpass = '';
    $password = $_POST['password'];

    //Select username, password, salt and active
    $database->query('SELECT username, password, salt, active FROM wcx_admin WHERE username = :username');

    $database->bind(':username', $_POST['username']);

    $rows = $database->resultset();

    if(isset($rows[0])) {
        $salt = $rows[0]['salt'];
        $dbpass = $rows[0]['password'];
        $password = hash("sha512", $password . $salt);
    }
    if($password !== $dbpass) {
        echo 'Incorrect Username or Password';
    }
}

?>

Binding Function

// BIND
public function bind($param, $value, $type = null){
if (is_null($type)) {
    switch (true) {
        case is_int($value):
            $type = PDO::PARAM_INT;
            break;
        case is_bool($value):
            $type = PDO::PARAM_BOOL;
            break;
        case is_null($value):
            $type = PDO::PARAM_NULL;
            break;
        default:
            $type = PDO::PARAM_STR;
    }
}
$this->stmt->bindValue($param, $value, $type);
}

You should also make heavy use of the filter functions of PHP.

<?php

if (filter_input(INPUT_POST, "form_id", FILTER_UNSAFE_RAW) === filter_input(INPUT_SESSION, "form_id", FILTER_UNSAFE_RAW)) {
  $username = filter_input(INPUT_POST, "username", FILTER_SANITIZE_STRING, FILTER_FLAG_STRIP_LOW);
  $password = filter_input(INPUT_POST, "password", FILTER_UNSAFE_RAW);

  if ($username && $password) {
    $database->query("SELECT `username`, `password`, `active` FROM `admin` WHERE `username` = :username AND `password` = :password LIMIT 1");
    $database->bind(":username", $username);
    $result = $database->resultset();

    if (isset($result[0]) && password_verify($password, $result[0]["password"])) {
      echo "Welcome!";
    }
  }
}


C# threading and synchronizing methods So I started this question, I believe i have synchronized the methods correctly, need some insight on whats wrong and what should be done. I have to modify ths class Purse  so that the method AddCoin is synchronized and  implement a synchronized RemoveCoin method with a Coin argument, I believe that is done just fine. Where im really having trouble is implementing thread that adds pennies, another thread that adds quarters, and another thread that removes and prints randomly selected coins, pennies, quarters, any one. Then finally synchronize the threads to wait for each other when necessary. Here is what I have so far:
Purse.cs
using System;
using System.Collections;
using System.Threading;

namespace TestingProject
{
    /// <summary>
    /// A purse holds a collection of coins.
    /// </summary>
public class Purse
{
    ///  Constructs an empty purse.
    public Purse()
    {
        coins = new ArrayList();

    }

    ///   Add a coin to the purse.
    ///   @param aCoin the coin to add
    public void Add(Coin aCoin)
    {
        Monitor.Enter(coins);
        coins.Add(aCoin);

        Monitor.Exit(coins);
    }
    public void RemoveCoin(Coin aCoin)
    {
        Monitor.Enter(coins);
        coins.Remove(aCoin);

        Monitor.Exit(coins);
    }

    ///  Get the total value of the coins in the purse.
    ///  @return the sum of all coin values
    public double GetTotal()
    {
        double total = 0;
        for (int i = 0; i < coins.Count; i++)
        {
            Coin aCoin = (Coin)coins[i];
            total = total + aCoin.GetValue();
        }
        return total;
    }

    private ArrayList coins;
}

}

Coin.cs
using System;
using System.Collections;

namespace TestingProject
{
    /// <summary>
    /// A coin with a monetary value.
    /// </summary>
public class Coin   {

    /// Constructs a coin.
    /// @param aValue the monetary value of the coin
    /// @param aName the name of the coin
    public Coin(double aValue, String aName)
    {
        value = aValue;
        name = aName;
    }

    public Coin()
    {
    }

    /// Gets the coin value.
    /// @return the value
    public double GetValue()
    {
        return value;
    }

    /// Gets the coin name.
    /// @return the name
    public String GetName()
    {
        return name;
    }

    public override bool Equals(Object otherObject)
    {
        Coin other = (Coin)otherObject;
        return name==other.name
            && value == other.value;
    }

    // C# requirement:
    // since we override Equals, MUST also override GetHashCode ( !! )
    public override int GetHashCode()
    {
        return base.GetHashCode ();
    }

    private double value;
    private string name;
}
}

pursetest.cs
using System;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.VisualBasic;

namespace TestingProject
{

class PurseTest
{
    public static void Main(string[] args)
    {

        // Random object used by each thread
        Random random = new Random();

        Purse purse =new Purse();

        Coin coin = new Coin();

        // output column heads and initial buffer state
        Console.WriteLine("{0,-35}{1,-9}{2}\n",
           "Operation", "Buffer", "Occupied Count");

        Thread purseThread =
           new Thread(new ThreadStart(purse.Add))
           {
               Name = "Purse"
           };

        Thread coinThread =
           new Thread(new ThreadStart(coin.GetValue));
        coinThread.Name = "coin";

        // start each thread
        purseThread.Start();
        coinThread.Start();

    }
}
}

Taking a look at your Coin class, there are a few things that aren't quite idiomatic C#.
The most obvious thing is that you're using what looks nearly like Java Doc comments. C# uses XML doc comments so this:
/// Constructs a coin.
/// @param aValue the monetary value of the coin
/// @param aName the name of the coin

Should be:
/// Constructs a Coin.
/// <param name="aValue">The monetary value of the Coin.</param>
/// <param name="aName">The name of the Coin.</param>

In C#, we have properties which mean you don't need to write explicit GetXyz and SetXyz. I'm not sure whether you're using an old version of .Net (e.g. using ArrayList) but it's normal to have those as auto-implemented properties like this:
 public double Value { get; }
 public string Name { get; }

If you can't use auto-properties, you'll have to do a bit more typing. I'm going to assume you're stuck on an older .Net and try to avoid newer features.
private readonly double value;
public double Value
{
    get
    {
        return value;
    }
}

That means you get rid of GetValue and GetName methods. I'd also suggest that you remove the empty constructor to force creation with a value and a name. That only leaves your Equals and GetHashcode implementations.
Let's consider a couple of test cases:
new Coin().Equals("blah"); // InvalidCastException :(
new Coin().Equals(null); // NullReferenceException :(

Oh dear, we've got a problem here! Let's fix those bugs:
public override bool Equals(Object otherObject)
{
    return Equals(otherObject as Coin);
}

public bool Equals(Coin coin)
{
    if (ReferenceEquals(null, coin))
         return false;

    return coin.Value == this.Value && coin.Name == this.Name;
}

That's nice and clear and most importantly, is correct! This is a common pattern and you can implement IEquatable at the same time if you want.
I think a coin should have a name in your model so you should validate that in the constructor.
So we're up to here:
///
/// A coin with a monetary value.
///
public class Coin : IEquatable<Coin>
{
    private readonly double value;

    public double Value
    {
        get
        {
            return value;
        }
    }

    private string name;

    public string Name
    {
        get
        {
            return name;
        }
    }

    /// Constructs a coin.
    /// <param name="value">The monetary value of the Coin</param>
    /// <param name="name">The name of the Coin</param>
    public Coin(double value, string name)
    {
        if (name == null)
            throw new ArgumentNullException("name");
        this.value = value;
        this.name = name;
    }

    public override bool Equals(Object otherObject)
    {
        return Equals(otherObject as Coin);
    }

    public bool Equals(Coin coin)
    {
        if (ReferenceEquals(null, coin))
            return false;

        return coin.Value == this.Value && coin.Name == this.Name;
    }

    // C# requirement:
    // since we override Equals, MUST also override GetHashCode ( !! )
    public override int GetHashCode()
    {
        return base.GetHashCode();
    }

}

So, only one thing left to talk about: GetHashCode. We must override GetHashCode and implement it correctly. I'll link you to this SO post: https://stackoverflow.com/questions/371328/why-is-it-important-to-override-gethashcode-when-equals-method-is-overridden
Your GetHashCode should look like this:
public override int GetHashCode()
{
    int hash = 13;
    hash = (hash * 7) + Value.GetHashCode();
    hash = (hash * 7) + Name.GetHashCode();
    return hash;
}

Now we've got a class that correctly implements equality.
ETA:
As has been pointed out in the comments, value is a contextual keyword so you may want to think twice before calling a field value. I don't see the problem myself as syntax highlighting in VS, for example, would know it wasn't a keyword. Calling it theValue would be worse for readability in my opinion. If you're on C# 6 or later, you should prefer the auto property so you don't need the field. Either way, if you write this class well at the beginning, the field is an implementation detail and you'll never read this source code again.


Parent-child relationship I am implementing a parent-child relationship, represented by hashes that look like this:

parent = { 'parent' => nil, 'id' => 3 }
child = { 'parent' => parent, 'id' => 7 }

These hashes are data given to me, for example, through a JSON API.

(As per a question in the comments, the JSON for the child would look like:)

{
  "parent": { "parent": null, "id": 3, ...etc.},
  "id": 7,
  ...etc.
}

Parent and child have more keys like name, etc. (they are a lot so I am only showing the id as an example). They both have the same keys. There are only parents and children, i.e., no grandchildren or grandparents, etc. There is only one "level". Each child has only one parent, but each parent may have several children.

I wrapped the data in a class to represent it, so that my application can use an object. Both parent and child are represented by the same class. So I did this:

class Item
  def initialize(data)
    @data = data
  end

  def parent
    @parent ||= Item.new(data['parent']) if data['parent']
  end

  def id
    data['id']
  end

  private

  attr_reader :data
end

Every key in the hash has its method to retrieve it, and Item is the class that represents parents and children. I use it like this:

describe 'children' do
  let(:subject1) { Item.new(child) }

  it 'is an item' do
    subject1.class.must_equal(Item)
  end

  it 'has an id' do
    subject1.id.must_equal(7)
  end

  it 'has a parent' do
    subject1.parent.id.must_equal(3)
  end
end

describe 'parent' do
  let(:subject2) { subject1.parent }

  it 'is also an item' do
    subject2.class.must_equal(Item)
  end

  it 'does not have a parent' do
    assert_nil(subject2.parent)
  end
end

With this, children know who is their parent, and for a parent to know its children we can iterate through all the children and compare their parent's id with the id we are searching.

However, I have a bad feeling about calling Item.new inside of Item. Is like a lot of things could go wrong. I thought of using a Item factory through dependency injection, but really all the knowledge about how to build a Item is in Item, so why introduce that dependency. Is this like a weird kind of recursion?

It doesn't help that I haven't seen this implementation anywhere (I'm not a computer scientist though). Is this bad code? I have a weird feeling, but I can't quite word why.

I don't think you should worry about calling new like that, but you can avoid it by embracing objects and Ruby, it's not perl, you're not stuck with hashes! Make your data into objects and if you need a hash representation of it then write a method that converts the object into a hash.

If you want to be able to keep track of objects that's when I'd use a hash, by putting it into a class variable, or even better, a class instance variable.

Additional:

Since you've mentioned JSON in the comments, or perhaps you'd be passed the hash, then I'd still use objects, just add a way to coerce a hash into an object. A library I've forked called Grackle communicates with Twitter and can receive either JSON or XML. It does this by loading handlers, which means the same data object class can receive many differing types of input and still produce an object with a standard interface.

module Handlers
  module JSONHandler
    require 'json'

    def from_json json
      hash = JSON.parse(json)
      recursive hash
    end

    # You might want to add an extra check to stop an infinite loop
    # Beware the data!
    def recursive hash
      return nil if hash.nil?
      return hash unless hash.respond_to? :fetch
      Item.new id: hash["id"], parent: recursive(hash["parent"])
    end
  end
end

class Item
  extend Handlers::JSONHandler

  def self.items
    @items ||= {}
  end

  def initialize(id:, parent:nil)
    @parent = parent
    @id = id
    self.class.items[@id] = self
  end

  attr_reader :parent, :id

  def to_h
    h = {id: @id, parent: (@parent && parent.id) }
    h.reject{|k,v| v.nil? }
  end

  alias_method :to_hash,:to_h
end
# => Item
parent = Item.new id: 3
# => #<Item:0x007fa94a4010c8 @parent=nil, @id=3>
child = Item.new id: 7, parent: parent
# => #<Item:0x007fa94a9abe38 @parent=#<Item:0x007fa94a4010c8 @parent=nil, @id=3>, @id=7>
Item.items
# => {3=>#<Item:0x007fa94a4010c8 @parent=nil, @id=3>, 7=>#<Item:0x007fa94a9abe38 @parent=#<Item:0x007fa94a4010c8 @parent=nil, @id=3>, @id=7>}
Item.items[3]
# => #<Item:0x007fa94a4010c8 @parent=nil, @id=3>
Item.items[7]
# => #<Item:0x007fa94a9abe38 @parent=#<Item:0x007fa94a4010c8 @parent=nil, @id=3>, @id=7>
Item.items[7].parent
# => #<Item:0x007fa94a4010c8 @parent=nil, @id=3>
parent.to_h
# => {:id=>3}
child.to_hash
# => {:id=>7, :parent=>3}
json_parent = parent.to_h.to_json
# => "{\"id\":3}"
json_child = child.to_h.to_json
# => "{\"id\":7,\"parent\":3}"
parent2 = Item.from_json json_parent
# => #<Item:0x007fb543e26940 @parent=nil, @id=3>
child2 = Item.from_json json_child
# => #<Item:0x007fb5431d7270 @parent=3, @id=7>

and to make calling it easy:

def self.Item(*args, **keywords)
  case args.first
  when String # it's JSON
    Item.from_json args.first
  when Hash
    # you might want to nick the recursive bit from the handler
    # or write an Hash handler etc
    Item.new id: args.first["id"], parent: args.first["parent"]
  when Item
    args.first
  else
    Item.new id: keywords[:id], parent: keywords[:parent]
  end
end

Item(parent)
# => #<Item:0x007fb9b2765160 @parent=nil, @id=3>
Item(child)
# => #<Item:0x007fb9b272f330 @parent=3, @id=7>
Item(json_parent)
# => #<Item:0x007fb9b26fdd30 @parent=nil, @id=3>
Item(json_child)
# => #<Item:0x007fb9b26c7dc0 @parent=3, @id=7>
Item(parent.to_h)
# => #<Item:0x007fb9b26965b8 @parent=nil, @id=3>
Item(child.to_h)
# => #<Item:0x007fb9b2664928 @parent=3, @id=7>
Item(id: 3)
# => #<Item:0x007fb9b262ed50 @parent=nil, @id=3>
Item(id: 7, parent: parent)
# => #<Item:0x007fb9b25fc990 @parent=#<Item:0x007fb9b23bd7b0 @parent=nil, @id=3>, @id=7>


Google Custom Search placeholder function in JavaScript Problem:

We have a Google Custom Search function that replaces the input placeholder attribute and it works okay.

Would you be so kind and help me with reviewing it, and if possible, provide an efficient function to replace this function with (also hoping that it would work with older version of IE and other browsers, not very necessary though):

Placeholder Script:

// google search placeholder
(function() {
    var cx = '!!!!!!!!!!!!!!!!!!!';
    var gcse = document.createElement('script');
    gcse.type = 'text/javascript';
    gcse.async = true;
    gcse.src = 'https://cse.google.com/cse.js?cx='+ cx;
    var s = document.getElementsByTagName('script')[0];
    s.parentNode.insertBefore(gcse, s);
})();

window.onload = function(){
    document.getElementById('gsc-i-id1').placeholder = 'e.g., United Stocks AMD';
};

where cx is a dynamic variable string parameter, defined as the custom search engine ID to use for a request (Custom Search API, ID for custom search).

Result Display Script:

<gcse:searchresults-only></gcse:searchresults-only>

Popular Query Script:

Added CSS:

/*google custom search*/
.cse .gsc-search-button input.gsc-search-button-v2,input.gsc-search-button-v2{height:26px!important;margin-top:0!important;min-width:13px!important;padding:5px 26px!important;width:68px!important}
.cse .gsc-search-button-v2,.gsc-search-button-v2{box-sizing:content-box;min-width:13px!important;min-height:16px!important;cursor:pointer;border-radius:20px}
.gsc-search-button-v2 svg{vertical-align:middle}
.gs-title{line-height:normal!important}
.gsc-search-box-tools .gsc-search-box .gsc-input{padding:5px!important;color:#4169E1!important;border-radius:20px}
.gsc-input-box{background:none!important;border:none!important}

Screenshot Before Search:

Screenshot After Search:

Screenshot for "Code Review" search:

So after testing it,  you don't really need to define it in your javascript. You can simply add it in your HTML before all your other scripts.

That's what s.parentNode.insertBefore(gcse, s); does. It finds the first occurence of a script tag in your HTML (which is kind of funny as it won't work if there are no script tags in your HTML), and adds your new script before it.

So if the first occurence of a script tag appears in the HEAD, then it'll be placed in the head'

<script src="https://cse.google.com/cse.js?cx=017643444788069204610:4gvhea_mvga" async type="text/javascript"></script>
<gcse:search></gcse:search>

Making your javascript more efficient will be difficult as 1. there isn't much happening and 2. it's already very short.

But you could do something like this (which doesn't change much)...

// google search placeholder
(function(cx) {
    const gcse = document.createElement('script');
    gcse.type = 'text/javascript';
    gcse.async = true;
    gcse.src = `https://cse.google.com/cse.js?cx=${cx}`;
    const s = document.getElementsByTagName('script')[0];
    s.parentNode.insertBefore(gcse, s);
})('!!!!!!!!!!!!!!!!!!!');


Generating permutations I have an algorithm for generating permutations in Python

from collections import Counter

def permutations(A):
    def permutations(A):
        if len(A) == 0:
            return
        if len(A) == 1:
            yield A
        else:
            for i in range(1, len(A)):
                for p in permutations(A[i:]): # Pretend slicing is O(1)
                    for q in permutations(A[:i]): # Pretend slicing is O(1)
                        yield p + q # Pretend O(1)
                        yield q + p # Pretend O(1)
    return set(permutations(A))

P = permutations((1, 2, 3))
C = Counter(P)
print(P)
print(C)

Which has output

{(3, 1, 2), (1, 3, 2), (3, 2, 1), (2, 3, 1), (1, 2, 3), (2, 1, 3)}
Counter({(2, 1, 3): 1, (1, 3, 2): 1, (3, 1, 2): 1, (3, 2, 1): 1, (2, 3, 1): 1, (1, 2, 3): 1})

Assuming I don't care about the time it takes to slice/concatenate arrays, is this the most efficient algorithm for generating permutations? If not, what can I do to improve it?

Well, the obvious method would be to use the itertools module which has exactly what you want. Here are the docs, just in case you'll need to look up for something else.

from itertools import permutations

def generate_permutations():
    return list(permutations([1,2,3]))

The above will return:

  [(1, 2, 3), (1, 3, 2), (2, 1, 3), (2, 3, 1), (3, 1, 2), (3, 2, 1)]

Below, you can find the algorith of this module:

def permutations(iterable, r=None):
    pool = tuple(iterable)
    n = len(pool)
    r = n if r is None else r
    if r > n:
        return
    indices = range(n)
    cycles = range(n, n-r, -1)
    yield tuple(pool[i] for i in indices[:r])
    while n:
        for i in reversed(range(r)):
            cycles[i] -= 1
            if cycles[i] == 0:
                indices[i:] = indices[i+1:] + indices[i:i+1]
                cycles[i] = n - i
            else:
                j = cycles[i]
                indices[i], indices[-j] = indices[-j], indices[i]
                yield tuple(pool[i] for i in indices[:r])
                break
        else:
            return

As you can see, this is using just a while loop do do what you did in 3 separate for loops. For this kind of operations, I strongly recommend using the itertools module.


Implementation of arbitrary-precision integer addition Implementation:

string Add(string a,string b)
{
    int maxLen = Math.Max(a.Length,b.Length);
    a = a.PadLeft(maxLen+1,'0');
    b = b.PadLeft(maxLen+1,'0');

    int[] arr1 = a.Select(x => int.Parse(x.ToString())).ToArray();
    int[] arr2 = b.Select(x => int.Parse(x.ToString())).ToArray();
    int[] sum = new int[arr1.Length];

    int carry = 0;
    for(int i = sum.Length-1;i >= 0;i--)
    {
        int total = arr1[i] + arr2[i] + carry;
        sum[i] = total % 10;
        if(total > 9) carry = 1;
        else carry = 0;
    }
    return string.Join("",sum.SkipWhile(x => x == 0));
}

Example:

void Main()
{
    string a = "12121213213213902139210903";
    string b = "1213212222132132113";
    Console.WriteLine(Add(a,b)); // 12121214426426124271343016
}

How can I improve this implementation?

How can I improve this implementation?

You could extract the string parsing to a separate int[] ToIntArray(String) method.
You could add a separate int[] Add(int[],int[]) method.
You could add a separate String ToString(int[]) method.

Right now your method is

parsing the String's to int[]
adding two int[]
composing a string out of the adding

So after extracting the methods your former Add() method would look like

string Add(string a,string b)
{
    int maxLen = Math.Max(a.Length,b.Length);
    a = a.PadLeft(maxLen+1,'0');
    b = b.PadLeft(maxLen+1,'0');

    int[] arr1 = ToIntArray(a);
    int[] arr2 = ToIntArray(b);

    int[] sum = Add(arr1,arr2);

    return ToString(sum);
}

But wait, we can do much better using more OOP.

Let us introduce a new class IntArray ( which can be private ). In this class we override the ToString() method, add constructors for int[] and string and also add a + operator.

private class IntArray
{

    public int[] array { get; private set; }
    public IntArray(int[] arr)
    {
        array = arr;
    }

    public IntArray(String s)
    {
        array = s.Select(x => int.Parse(x.ToString())).ToArray();
    }

    public static IntArray operator +(IntArray summand1, IntArray summand2)
    {
        int[] sum = new int[summand1.array.Length];
        int carry = 0;

        for (int i = sum.Length - 1; i >= 0; i--)
        {
            int total = summand1.array[i] + summand2.array[i] + carry;
            sum[i] = total % 10;
            if (total > 9)
            {
                 carry = 1;
            }
            else
            {
                carry = 0;
            }
        }

        return new IntArray(sum);
    }
    public override string ToString()
    {
        return string.Concat(array.SkipWhile(x => x == 0));
    }

}

This will simplify your former Add() method to

string Add(string a, string b)
{
    int maxLen = Math.Max(a.Length, b.Length);
    a = a.PadLeft(maxLen + 1, '0');
    b = b.PadLeft(maxLen + 1, '0');

    IntArray arr1 = new IntArray(a);
    IntArray arr2 = new IntArray(b);

    return (arr1 + arr2).ToString();
}

Update: Based on d347hm4n's comment and after checking the reference source, I changed to implementation of ToString() from String.Join() to String.Concat() method.


Simple locale loader I have a class that loads locales from a database. I'm not sure if locale is the right word. It's basically loading texts for a game, generic sayings like alerts and error messages.

I'm also not sure if it should be called WhateverHandler or WhateverController, maybe even WhateverManager. I tend to get really OCD about my code, so some advice would be nice.

public class LocaleHandler
{
    private static readonly ILogger Logger = new ConsoleLogger(typeof(LanguageLocale));

    private readonly ConcurrentDictionary<string, string> _localeKeyValue;

    public LocaleHandler()
    {
        using (var dbConnection = PlusEnvironment.GetDatabaseManager().Connection)
        {
            _localeKeyValue = FetchLocale(dbConnection);
        }

        Logger.Trace($"Fetched {_localeKeyValue.Count} language locales from the database.");
    }

    private static ConcurrentDictionary<string, string> FetchLocale(DatabaseConnection dbConnection)
    {
        dbConnection.SetQuery("SELECT * FROM `server_locale`");

        using (var reader = dbConnection.ExecuteReader())
        {
            if (!reader.HasRows)
            {
                return new ConcurrentDictionary<string, string>();
            }

            var locale = new ConcurrentDictionary<string, string>();

            while (reader.Read())
            {
                locale.TryAdd(reader.GetString("key"), reader.GetString("value"));
            }

            return locale;
        }
    }

    public bool TryGetLocale(string key, out string value)
    {
        return _localeKeyValue.TryGetValue(key, out value);
    }
}

First, since you are concerned about naming: this class doesn't really handle, manage, or control anything.  It does two things: queries a database, and makes those results available via a TryGet interface.  In fact, you could split it into two classes, LocaleLoader and Locale.  Though I wonder if "locale" is even the correct term since there is no locale supplied (like "en-us"), it's more of a string table.  On the topic of naming, "_localeKeyValue" is not very good; it describes the implementation and not the purpose.  Something like "Strings" would be better.  Even "_localeDict" would be an improvement.

As noted in the comments, the ConcurrentDictionary is unnecessary; Dictionary supports concurrent readers as long as there are no writers.  In your case, all reading occurs after all writing.

@paparazzo is right about checking for no results being unnecessary.  The result is the same, it's no faster, and it's more code.

Consider allowing the constructor to be passed in a database connection to use.  This makes it more flexible and removes a dependency.

TryGetLocale: are you really getting a locale?  I think you are getting a string, translation, or localization.  Without seeing how this code is used, I question the usefulness of the TryGet-style interface.  For (what I imagine to be) my similar code, I like to just return the key if it's not found.  Something like:

public String GetString(String key)
{
    String value;
    return _localeKeyValue.TryGetValue(key, out value) ? value : key;
}

This means that when the code encounters an untranslated string it's obvious to the devs and testers what is missing.  The other possibility would be to throw an exception when you can't find the key, but on the off-chance that you ship software with a missing string it's better for the user to see a message box like "IMG_CONVERSION_JPEG_SUCCESS" than "Illegal argument: key".


Find the Center of the Largest Blob I have a vector of points that form blobs inside a 2d region. This 2d region represents an image. I need to find the center point of the largest blob very quickly. My algorithm that I use takes ~1 minute for a 50x50 image and it took so long that I canceled it for a 2560x1440 image. I want to be able to calculate a 2560x1440 image in just a couple seconds. Is this possible? I don't mind if the center is just an approximation. It doesn't have to be exact.

Here is an image I created from the points:

The white points are the ones that I am interested in. I need the center of the largest blob. The points that are identified could look wildly different, so take this image with a grain of salt.

I took the code from here and adapted it for my purpose. The point.h file contains an iteration value. I create a 2d array where every point outside the blobs has an iteration value of -1 and the rest have a value greater than or equal to 0 and passed that into the method.

Point.h

class Point
{
private:
    int x, y, iteration;
public:
    Point(int Xin, int Yin) : x(Xin), y(Yin), iteration(-1) {}
    Point(int Xin, int Yin, int Iterationin) : x(Xin), y(Yin), iteration(Iterationin) {}
    int getX() { return x; };
    int getY() { return y; };
    int getIteration() { return iteration; };
};

Center Point

int calculateFramePerturbation2::fill(vector<vector<int>> arr, int r, int c, vector<Point>& points) {
    int count = 0;
    if (r < arr.size() && arr[r][c] >= 0) {
        for (int i = c; i >= 0 && arr[r][i] >= 0; i--) {
            Point pt(r, i, arr[r][i]);
            points.push_back(pt);

            arr[r][i] = -1;
            count += fill(arr, r + 1, i, points) + 1;
        }
        for (int i = c + 1; i < arr[r].size() && arr[r][i] >= 0; i++) {
            Point pt(r, i, arr[r][i]);
            points.push_back(pt);

            arr[r][i] = -1;
            count += fill(arr, r + 1, i, points) + 1;
        }
    }
    return count;
}

Point calculateFramePerturbation2::print_components(vector<vector<int>> arr) {
    vector<int> pointAreas;
    vector<vector<Point>> allPoints;

    for (int r = 0; r < arr.size(); ++r) {
        for (int c = 0; c < arr[r].size(); ++c) {
            if (arr[r][c] >= 0) {
                vector<Point> points;

                fill(arr, r, c, points);
                //cout << fill(arr, r, c, points) << " ";
                //cout << points.size() << endl;
                allPoints.push_back(points);
                pointAreas.push_back(points.size());
            }
        }
    }

    //Identify the blob with the highest area.
    int max = -1;
    int index2 = 0;
    for (int i = 0; i < pointAreas.size(); i++) {
        if (pointAreas[i] > max) {
            max = pointAreas[i];
            index2 = i;
        }
    }

    cout << endl;
    cout << max << " " << index2 << endl;

    //Find the center point
    int totalX = 0;
    int totalY = 0;
    for (int i = 0; i < allPoints[index2].size(); i++) {
        totalX += allPoints[index2][i].getX();
        totalY += allPoints[index2][i].getY();
    }

    Point finalPoint(totalX / allPoints[index2].size(), totalY / allPoints[index2].size(), -1);
    return finalPoint;
}
void run(vector<Point> pointsRemaining) {
    //I'd prefer a solution where I could just take the points remaining array (it defines the blobs instead of converting it into a 2d array.

    vector<vector<int>> glitch2DArray;

    glitch2DArray.resize(WIDTH, vector<int>(HEIGHT, -1));
    for (int i = 0; i < pointsRemaining.size(); i++) {
        glitch2DArray[pointsRemaining[i].getX()][pointsRemaining[i].getY()] = pointsRemaining[i].getIteration();
    }

    Point finalPoint = print_components(glitch2DArray);
}

The observation of @juvian, which pointed to the obvious mistake of copying vectors, is good. But I think that the main point is that of recognizing this is a pretty famous problem in Image Processing: Connected Components Labeling. Flood fill is one of the slowest solutions, because of its poor cache usage.

Here I'm using the SAUF algorithm published in Two Strategies to Speed up Connected Component Labeling Algorithms.

In particular this solution works by making two scans of the image and using Union-Find to manage label equivalences. One thing to note is that the second scan is not needed to solve the specific problem, because you can stop without getting the labeled image and just use the vector of blobs. This saves about 20% of the total time.

This takes few milliseconds for the specific image.

I'm sorry for posting this extremely long piece of code, but a lot of additional code is needed for having a working example. What is needed is a way to store images, the UF data structure (here everything is static, but this is not needed), the PerformLabeling function. The PNG image was converted to PBM format for easy loading.

#include <vector>
#include <fstream>
#include <string>
#include <cstdint>
#include <tuple>

using namespace std;

template <typename T>
struct mat {
    size_t rows_, cols_;
    vector<T> data_;

    mat(size_t rows = 0, size_t cols = 0) : rows_(rows), cols_(cols), data_(rows*cols) {}

    const T& operator()(size_t r, size_t c) const { return data_[cols_*r + c]; }
    T& operator()(size_t r, size_t c) { return data_[cols_*r + c]; }

    const T* operator[](size_t r) const { return &data_[cols_*r]; }
    T* operator[](size_t r) { return &data_[cols_*r]; }
};

bool loadPBM(const string& filename, mat<uint8_t>& img)
{
    ifstream is(filename, ios::binary);
    if (!is)
        return false;

    string type;
    size_t w, h;
    is >> type >> w >> h;
    if (type!="P4" || is.get()!='\n')
        return false;

    img = mat<uint8_t>(h, w);

    for (size_t r = 0; r < h; ++r) {
        uint8_t nbits = 0;
        uint8_t buffer;
        for (size_t c = 0; c < w; ++c) {
            if (nbits == 0) {
                if (!is.get(reinterpret_cast<char&>(buffer)))
                    return false;
                nbits = 8;
            }
            img(r, c) = 255 * (1 - (buffer >> --nbits & 1));
        }
    }

    return is.good();
}

bool savePGM(const string& filename, const mat<uint8_t>& img)
{
    ofstream os(filename, ios::binary);
    if (!os)
        return false;

    os << "P5\n" << img.cols_ << " " << img.rows_ << "\n255\n";
    os.write(reinterpret_cast<const char*>(&img.data_[0]), img.cols_*img.rows_);

    return os.good();
}

// Union-find (UF)
class UF {
public:
    static void Alloc(unsigned max_length) {
        P_ = new unsigned[max_length];
    }
    static void Dealloc() {
        delete[] P_;
    }
    static void Setup() {
        P_[0] = 0;// First label is for background pixels
        length_ = 1;
    }
    static unsigned NewLabel() {
        P_[length_] = length_;
        return length_++;
    }
    static unsigned GetLabel(unsigned index) {
        return P_[index];
    }
    static unsigned GetLength() {
        return length_;
    }
    static unsigned Merge(unsigned i, unsigned j)
    {
        // FindRoot(i)
        while (P_[i] < i) {
            i = P_[i];
        }
        // FindRoot(j)
        while (P_[j] < j) {
            j = P_[j];
        }
        if (i < j)
            return P_[j] = i;
        return P_[i] = j;
    }
    static unsigned Flatten()
    {
        unsigned k = 1;
        for (unsigned i = 1; i < length_; ++i) {
            if (P_[i] < i) {
                P_[i] = P_[P_[i]];
            }
            else {
                P_[i] = k;
                k = k + 1;
            }
        }
        return k;
    }
private:
    static unsigned *P_;
    static unsigned length_;
};
unsigned* UF::P_;
unsigned UF::length_;

struct blob {
    size_t x = 0, y = 0, count = 0;
    size_t left = SIZE_MAX, right = 0, top = SIZE_MAX, bottom = 0;
    blob() {}
    void accumulate(size_t r, size_t c) {
        x += c;
        y += r;
        ++count;

        if (left > c)
            left = c;
        if (right < c)
            right = c;
        if (top > r)
            top = r;
        if (bottom < r)
            bottom = r;
    }
    void accumulate(const blob& b) {
        x += b.x;
        y += b.y;
        count += b.count;

        if (left > b.left)
            left = b.left;
        if (right < b.right)
            right = b.right;
        if (top > b.top)
            top = b.top;
        if (bottom < b.bottom)
            bottom = b.bottom;
    }
};

template <typename Blob>
size_t PerformLabeling(const mat<uint8_t>& img, mat<unsigned>& img_labels, vector<Blob>& blobs)
{
    size_t h = img.rows_;
    size_t w = img.cols_;

    img_labels = mat<unsigned>(h, w); // Allocation + initialization of the output image

    UF::Alloc(((img.rows_ + 1) / 2) * ((img.cols_ + 1) / 2) + 1); // Memory allocation of the labels solver
    UF::Setup(); // Labels solver initialization

    vector<Blob> temp_blobs(((img.rows_ + 1) / 2) * ((img.cols_ + 1) / 2) + 1);

    // Rosenfeld Mask
    // +-+-+-+
    // |p|q|r|
    // +-+-+-+
    // |s|x|
    // +-+-+

    // First scan
    for (size_t r = 0; r < h; ++r) {
        // Get row pointers
        const unsigned char *img_row = img[r];
        const unsigned char *img_row_prev = img_row - img.cols_;
        unsigned *img_labels_row = img_labels[r];
        unsigned *img_labels_row_prev = img_labels_row - img_labels.cols_;

        for (size_t c = 0; c < w; ++c) {
#define CONDITION_P c > 0 && r > 0 && img_row_prev[c - 1] > 0
#define CONDITION_Q r > 0 && img_row_prev[c] > 0
#define CONDITION_R c < w - 1 && r > 0 && img_row_prev[c + 1] > 0
#define CONDITION_S c > 0 && img_row[c - 1] > 0
#define CONDITION_X img_row[c] > 0

#define ACTION_1 // nothing to do
#define ACTION_2 img_labels_row[c] = UF::NewLabel(); // new label
#define ACTION_3 img_labels_row[c] = img_labels_row_prev[c - 1]; // x <- p
#define ACTION_4 img_labels_row[c] = img_labels_row_prev[c]; // x <- q
#define ACTION_5 img_labels_row[c] = img_labels_row_prev[c + 1]; // x <- r
#define ACTION_6 img_labels_row[c] = img_labels_row[c - 1]; // x <- s
#define ACTION_7 img_labels_row[c] = UF::Merge(img_labels_row_prev[c - 1], img_labels_row_prev[c + 1]); // x <- p + r
#define ACTION_8 img_labels_row[c] = UF::Merge(img_labels_row[c - 1], img_labels_row_prev[c + 1]); // x <- s + r

            if (CONDITION_X) {
                if (CONDITION_Q) {
                    ACTION_4
                }
                else {
                    if (CONDITION_R) {
                        if (CONDITION_P) {
                            ACTION_7
                        }
                        else {
                            if (CONDITION_S) {
                                ACTION_8
                            }
                            else {
                                ACTION_5
                            }
                        }
                    }
                    else {
                        if (CONDITION_P) {
                            ACTION_3
                        }
                        else {
                            if (CONDITION_S) {
                                ACTION_6
                            }
                            else {
                                ACTION_2
                            }
                        }
                    }
                }
            }
            else {
                ACTION_1
                continue;
            }

            temp_blobs[img_labels_row[c]].accumulate(r, c);
        }
    }

    size_t n_labels = UF::Flatten();

    blobs.resize(n_labels);
    for (size_t i = 0; i < UF::GetLength(); ++i) {
        blobs[UF::GetLabel(i)].accumulate(temp_blobs[i]);
    }

    // Second scan
    for (size_t r = 0; r < img_labels.rows_; ++r) {
        unsigned *img_row_start = img_labels[r];
        unsigned *img_row_end = img_row_start + img_labels.cols_;
        for (; img_row_start != img_row_end; ++img_row_start) {
            *img_row_start = UF::GetLabel(*img_row_start);
        }
    }

    UF::Dealloc(); // Memory deallocation of the labels solver

    return n_labels;
}

int main() {
    mat<uint8_t> img;
    if (!loadPBM("mD6sL.pbm", img))
        return EXIT_FAILURE;

    mat<unsigned> img_labels;

    vector<blob> blobs;
    auto n = PerformLabeling(img, img_labels, blobs);

    blob bmax;
    for (const auto& b :  blobs) {
        if (bmax.count < b.count)
            bmax = b;
    }
    bmax.x /= bmax.count;
    bmax.y /= bmax.count;

    mat<uint8_t> out(img.rows_, img.cols_);
    copy(begin(img_labels.data_), end(img_labels.data_), begin(out.data_));

    for (int i = -50; i <= 50; ++i) {
        out(bmax.y + i, bmax.x) = 255;
        out(bmax.y, bmax.x + i) = 255;
    }
    for (size_t i=bmax.left;i<=bmax.right;++i) {
        out(bmax.top, i) = 255;
        out(bmax.bottom, i) = 255;
    }
    for (size_t i = bmax.top; i <= bmax.bottom; ++i) {
        out(i, bmax.left) = 255;
        out(i, bmax.right) = 255;
    }

    if (!savePGM("out.pgm", out))
        return EXIT_FAILURE;

    return EXIT_SUCCESS;
}


Obfuscate text and preserve line breaks Problem

A function is needed, that obfuscates text but preserves line breaks. Line breaks may be encoded by "\r\n" or "\r", even mixed. myObfuscator is an object that obfuscates text, ignoring line breaks (would be replaced by any other character).

Example:

  ABC\r\nDEF\rGHE

should be obfuscated to something like

  XXX\r\nXXX\rXXX

where XXX depends on myObfuscator (not important for the question).

Implementation

My approach works, but seems a little too complicated.

public string Obfuscate(string text)
{
    var stringBuilder = new StringBuilder();
    var lineCount = 0;
    var lines = text.Split(new[] { "\r\n" }, StringSplitOptions.None);
    foreach (var line in lines)
    {
        var innerLineCount = 0;
        if (lineCount > 0) stringBuilder.Append("\r\n");
        foreach (var innerLine in line.Split(new[] { "\r" }, StringSplitOptions.None))
        {
            if (innerLineCount > 0) stringBuilder.Append("\r");
            var obfuscatedLine = string.IsNullOrEmpty(innerLine) ? string.Empty : myObfuscator.Obfuscate(innerLine);
            stringBuilder.Append(obfuscatedLine);
            innerLineCount++;
        }
        lineCount++;
    }
    return stringBuilder.ToString();
}

Is there a more elegant solution?

I would omit the StringBuilder here and would go with string.Join() by joining the processed innerLines and the processed lines like so

public string Obfuscate(string text)
{

    var lines = text.Split(new[] { "\r\n" }, StringSplitOptions.None);

    for (int i = 0; i < lines.Length; i++)
    {

        var innerLines = lines[i].Split(new[] { "\r" }, StringSplitOptions.None);
        for (int j = 0; j < innerLines.Length; j++)
        {
            innerLines[j]= string.IsNullOrEmpty(innerLines[j]) ? string.Empty : myObfuscator.Obfuscate(innerLines[j]);
        }

        lines[i] = string.Join("\r", innerLines);

    }
    return string.Join(Environment.NewLine, lines);

}

What I would add about your code would be some proper argument validation because the method is public. In addition I would move the check wether ther innerLine is NullOrEmpty inside the obfuscator.