Prim's Algorithm - Minimum Spanning Tree I have implemented Prim's Algorithm from Introduction to Algorithms. I have observed that the code is similar to Dijkstra's Algorithm, so I have used my Dijkstra's Algorithm implementation.

Please review this code and suggest improvements.

To compile on Linux: g++ -std=c++14 prims.cpp

#include <iostream>
#include <map>
#include <limits>
#include <list>
#include <queue>

class Graph
{

    struct Vertex
    {
        std::size_t id;
        int distance = std::numeric_limits<int>::max();
        Vertex * parent = nullptr;

        Vertex(std::size_t id) : id(id) {}
    };

    using pair_ = std::pair<std::size_t, int>;
    std::vector<Vertex> vertices = {};

    //adjacency list , store src, dest, and weight
    std::vector< std::vector< pair_> > adj_list;
    //to store unprocessed vertex min-priority queue
    std::priority_queue< pair_, std::vector<pair_>,
                         std::greater<pair_> > unprocessed;

  public:
    Graph(std::size_t size);
    void add_edge(std::size_t src, std::size_t dest, int weight);
    void prim(std::size_t vertex);
    std::size_t  minimum_cost() ;
};

Graph::Graph(std::size_t size)
{
    vertices.reserve(size);
    adj_list.resize(size);
    for (int i = 0; i < size; i++)
    {
        vertices.emplace_back(i);
    }
}

void Graph::add_edge(std::size_t src , std::size_t dest, int weight)
{
    if(weight >= 0)
    {
        if (src == dest)
        {
            throw std::logic_error("Source and destination vertices are same");
        }

        if (src < 0 || vertices.size() <= src)
        {
            throw std::out_of_range("Enter correct source vertex");
        }

        if (dest < 0 || vertices.size() <= dest)
        {
            throw std::out_of_range("Enter correct destination vertex");
        }

        int flag = 0, i = src;
        for (auto& it : adj_list[i])
        {
            if (it.first == dest)
            {
                flag = 1;
                break;
            }
        }
        if (flag == 0)
        {
            adj_list[src].push_back( {dest, weight} );
        }
        else
        {
            throw std::logic_error("Existing edge");
        }

    }
    else
    {
        std::cerr << "Negative weight\n";
    }
}

void Graph::prim(std::size_t vertex)
{
    vertices[vertex].distance = 0;
    vertices[vertex].parent = &vertices[vertex];

    unprocessed.push( std::make_pair(vertices[vertex].distance, vertex) );
     while (!unprocessed.empty())
     {
         int curr_vertex_dist = unprocessed.top().first;
         std::size_t curr_vertex = unprocessed.top().second;
         unprocessed.pop();

        for (auto& ver: adj_list[curr_vertex])
        {
            auto& next_dist = vertices[ver.first].distance;
            const auto curr_dist = ver.second;
            if (curr_dist < next_dist)
            {
                next_dist = curr_dist;
                //make src vertex parent of dest vertex
                vertices[ver.first].parent = &vertices[curr_vertex];
                unprocessed.push( std::make_pair(next_dist, ver.first));
            }
        }
    }
}

std::size_t  Graph::minimum_cost()
{
    std::size_t cost = 0;
    for (auto vertex: vertices)
    {
        cost = cost + vertex.distance;
    }
    return cost;
}

int main()
{
    Graph grp(9);
    grp.add_edge(0, 1, 4);
    grp.add_edge(0, 2, 8);
    grp.add_edge(1, 2, 11);
    grp.add_edge(1, 3, 8);
    grp.add_edge(3, 4, 2);
    grp.add_edge(4, 2, 7);
    grp.add_edge(2, 5, 1);
    grp.add_edge(5, 4, 6);
    grp.add_edge(3, 6, 7);
    grp.add_edge(3, 8, 4);
    grp.add_edge(5, 8, 2);
    grp.add_edge(6, 7, 9);
    grp.add_edge(6, 8, 14);
    grp.add_edge(7, 8, 10);
    grp.prim(0);
    std::cout << "The total cost is : " << grp.minimum_cost() << "\n";
}

1. Keeping references to potentially dangling pointers

I can see a potential problem with the line

vertices[ver.first].parent = &vertices[curr_vertex];

if the std::vector<Vertex> vertices would be reorganised due to changes in length.
The address you take there isn't stable.

Maybe a better solution would be to keep a

std::vector<std::unique_ptr<Vertex>> vertices;

instead of keeping copies of the Vertex instances.

Then you can change

vertices[ver.first].parent = &vertices[curr_vertex];

to

vertices[ver.first].parent = vertices[curr_vertex].get();

Since all the vertices are in private scope of the Graph class the std::unique_ptr<Vertex> instances stored to the vertices vector will never invalidate, as long you guarantee to remove all child Vertex instances when a parent Vertex instane is removed from the graph (Well, that's not an operation in question here, but needs to be considered for production code).

2. Inconsistent error handling

Here

else
{
    std::cerr << "Negative weight\n";
}

you just use kind of errorneous input being reported to the console, while you throw exceptions for other cases.

For this condition you should rather do

throwing an exception
applying an assert() call in 1st place
clarify from the function signature

If Negative weight is erroneous input you should make that clear in 1st place:

  void Graph::add_edge(std::size_t src , std::size_t dest, unsigned weight)
                                                        // ^^^^^^^^

This way violations would be covered by the compiler, before runtime detects that flaw.

3. Storing unnecessary information

In your code example parent is never used besides storing the information. I am aware that you might have been simplified the actual usage of parent with this review question, but with the code context you give, that member variable doesn't make any sense.


Server status checker I've created a program for work that checks the status of all our servers quickly. What we use to have to do is ssh into each server manually.

I'm looking for some critique on what I've created here. Any input on it would be greatly appreciated, such as better syntax I can use, better variables, easier ways to perform the rescue, etc. I will be leaving out a couple of things for security purposes.

#!/local/usr/bin/env ruby

require 'rubygems'
require 'net/ssh/gateway'
require 'work/mail/gem'

begin
  def scanning_all_ssh_servers(server_check, cmd)
    check = Net::SSH.start(server_check, @username, :password => @password)
    ping = check.exec!(cmd)
    case
    when ping.include?("1 received, 0% packet loss")
      puts "server online: #{server_check}"
    when ping.include?("0 received, +1 errors")
       puts "SERVER OFFLINE: #{server_check}"
    when check.include?("Connection closed")
      puts "SERVER OFFLINE: #{server_check}"
    else
      raise "Error loading server: #{server_check}. Exiting.."
    end
  end

  @username = "username"
  @password = "password"

  ssh_server_list = %w(these are all the servers that we can ssh too)
  ssh_server_list.each do |server_check|
    cmd = "ping -c 1 #{server_check}"
    scanning_all_ssh_servers(server_check, cmd)
  end
rescue => e
  x = AD::Mail.new
  x.from        = "email@email.email"
  x.to          = "email@email.email"
  x.subject     = "Server Loading Error"
  x.body        = e.message
  x.send
end

Read my rewrite from the bottom up.  To appreciate the benefit, you notice how it clarifies the high level logic: your whole script is really only 4 lines long.

Some notes:

Isolate your begin... rescue code.  This way all servers will be processed even if there's an error
Break up each of your tasks into smaller, more focused methods (see below)
Make sure your names are accurate: eg, scanning_all_ssh_servers really means "ping a particular server"
Watch out for hidden entanglements in your code: your scan function takes a cmd argument, but this is a false abstraction, since the cody of the function assumes the cmd is a ping.  In this case, better to just leave the argument out.
breaking out the mailer into its own method will make the high level logic clearer
You can shorten up the ping function, while making it clearer imo, by losing the case statment (see below).  But if you don't like that, you should probably still use the regex version of case to avoid the repeated ping.include? calls.
Another small point on naming: Avoid unnecessary adjectives.  In this context, all your "servers" are "ssh servers", so better to use names like server_list rather than ssh_server_list, which adds visual and auditory cruft without adding precision.

Rewrite:

def mail_error(e)
  x = AD::Mail.new
  x.from     = "email@email.email"
  x.to       = "email@email.email"
  x.subject  = "Server Loading Error"
  x.body     = e.message
  x.send
end

def ping_server(server)
  begin
    check = Net::SSH.start(server, @username, :password => @password)
    cmd = "ping -c 1 #{server}"
    check.exec!(cmd)
  rescue => e
    mail_error(e)
    :error
  end
end

def server_status(server)
  ping_result =  ping_server(server)

  online_text = /1 received, 0% packet loss/
  offline_text = Regexp.union('0 received, +1 errors', 'Connection closed')

  ping_result =~ online_text  ? :online  :
  ping_result =~ offline_text ? :offline : :error
end

def status_message(server, status)
  status == :online  ? "server online: #{server}"  :
  status == :offline ? "SERVER OFFLINE: #{server}" :
                       "Error loading server: #{server}. Exiting.."
end

@username = "username"
@password = "password"
server_list = %w(these are all the servers that we can ssh too)

server_list.each do |server|
  status = server_status(server)
  puts status_message(server,status)
end

UPDATE: Additional Explanation

Regexp.union allows you to easily create an "or"ed regexp from a number of strings.  So that when you match strings against the regexp you created, you'll get a match if any of the strings match.  It's equivalent to the regexp | "or" operator, but can often be a more readable way to create the regexp.

Re: your question about the rescue block, it depends, but I would think you only want the "server can't be reached" email when the "reach server" code fails.  That is, even though this code is simple, it's still possible you get some other error executing it, which should be reported as a script error (using a library like airbrake) rather than spawning a "server can't be reached" email.  Again, just separating you concerns.

Re: the distinction between ssh and rdp servers, that makes sense.


First Game: Simon Game I am working on a Simon game using python and tkinter, just now it is incomplete, but I don't know if I am doing it right because it looks messy, and I would like to have a check on my code for corrections and suggestions on how to improve it.

This is the first game I'm making, and I don't have to much experience in programming.

This is my actual entire code:

from Tkinter import *
import Tkinter
import random

base = Tkinter.Tk()

fr = Tkinter.Frame(base, bg="black", width="238", height="250")

score = Tkinter.Label(base, bg="black", fg="white", text="Score:")
score.place(x = 30, y = 15)

s = 0

scoreNum = Tkinter.Label(base, bg="black", fg="white", text = s)
scoreNum.place(x = 70, y = 15)

best = Tkinter.Label(base, bg="black", fg="white", text="Best:")
best.place(x = 155, y = 15)

b = 0

bestNum = Tkinter.Label(base, bg="black", fg="white", text = b)
bestNum.place(x = 188, y = 15)

clicks = []
color = 0

def yellowClick():

    yellow.configure(activebackground="yellow3")
    yellow.after(500, lambda: yellow.configure(activebackground="yellow"))

    global clicks
    global color

    color = 1
    clicks.append(color)

yellow = Tkinter.Button(base, bd="0", highlightthickness="0",
                       width="7", height="5", activebackground="yellow",
                       bg="yellow3", command = yellowClick)

yellow.place(x = 30, y = 50)

def blueClick():

    blue.configure(activebackground="medium blue")
    blue.after(500, lambda: blue.configure(activebackground="blue"))

    global clicks
    global color

    color = 2
    clicks.append(color)

blue = Tkinter.Button(base, bd="0", highlightthickness="0",
                     width="7", height="5", activebackground="blue",
                     bg="medium blue", command = blueClick)

blue.place(x = 125, y = 50)

def redClick():

    red.configure(activebackground="red3")
    red.after(500, lambda: red.configure(activebackground="red"))

    global clicks
    global color

    color = 3
    clicks.append(color)

red = Tkinter.Button(base, bd="0", highlightthickness="0",
                    width="7", height="5", activebackground="red",
                    bg = "red3", command = redClick)

red.place(x = 30, y = 145)

def greenClick():

    green.configure(activebackground="dark green")
    green.after(500, lambda: green.configure(activebackground="green4"))

    global clicks
    global color

    color = 4
    clicks.append(color)

green = Tkinter.Button(base, bd="0", highlightthickness="0",
                      width="7", height="5", activebackground="green4",
                      bg="dark green", command = greenClick)

green.place(x = 125, y = 145)

def scoreUp():

    global s

    s = s + 1

    scoreNum.configure(text = s)

r = random.randint(1, 4)

sequence = []

def checkSequence():

    global clicks
    global sequence

    if clicks == sequence:

        scoreUp()

def showSequence():

    global r
    global sequence

    if r == 1:

        yellow.configure(bg="yellow")
        yellow.after(1000, lambda: yellow.configure(bg="yellow3"))

        sequence.append(r)

        base.after(5000, checkSequence)

    elif r == 2:

        blue.configure(bg="blue")
        blue.after(1000, lambda: blue.configure(bg="medium blue"))

        sequence.append(r)

        base.after(5000, checkSequence)

    elif r == 3:

        red.configure(bg="red")
        red.after(1000, lambda: red.configure(bg="red3"))

        sequence.append(r)

        base.after(5000, checkSequence)

    elif r == 4:

        green.configure(bg="green4")
        green.after(1000, lambda: green.configure(bg="dark green"))

        sequence.append(r)

        base.after(5000, checkSequence)

base.after(2000, showSequence)

fr.pack()

base.resizable(False, False)
base.mainloop()

Actually I'm checking the sequence with time because I can't find another to do it.

Python has an official style-guide, PEP8, which you are encouraged to follow to improve readability (mostly by others, but also yourself). On e of its recommendations is using spaces around operators, but no spaces around = when assigning keywords. So these are good:

var = "value"
l = [2, 1, 3]
l.sort(reverse=True)

while these are bad:

var="value"
l=[ 1,2,3 ]
l.sort( reverse = True)

In general it is not best practice to use global, because it makes the state your program is in potentially undefined.

For example the function

def f():
    global c
    return c

It is not defined what this function will output at any given time, because anybody else might modify the global c.

Most of the times, global can be avoided entirely, just by making functions accept the values as parameters and returning the results where necessary.

In your case this is a bit more tricky, because many of these functions cannot take any parameter (because Tkinter just calls them as they are).

There is one work-around for it and one solution, though.

One is using lambdas, like this:

buttons = []
for i in range(10):
    buttons.append(Tkinter.Button(base, bd="0", highlightthickness="0",
                               width="7", height="5", activebackground=color,
                               bg=alt_color, command=lambda i=i: print i)

Here the i=i is evaluated whenever the function is called. A simple lambda: print i would have failed.

The actual solution to this problem, though is to use a class. This way you can pack all those global variables as attributes, can access them within the functions, and so on.

I won't go into details about how to write a class, because there are very good tutorials about this (add links).

There are a few more noteworthy changes.

I made functions to create and place a label, which is pretty straight-forward, and to create and place a button. The latter is a bit more complicated, because, as explained above, the button needs a parameter-less function. For this I used the fact that in python functions are first-class objects. This means that you can return functions from other functions. I therefore wrote a button click handler factory, that produces a custom function given its parameters (with the right color, highlightcolor, etc).

To save the data of the different buttons I created a collections.namedtuple, which is just a tuple, where you can access the members also by name (but also still by index if you need to).

Then I made the game slightly more complete by adding a variable length of the shown sequence, an actual saving and verification of the user sequence and the correct handling of checking whether it is correct.

The attribute self.sequence contains the generated (and shown) sequence, while if a user clicks a button it is added to self.user_sequence if the two are identical the user wins, score is updated (and best if needed). A new sequence can be generated, with a length one longer than before.

If the user clicks and the sequence is right so far, nothing happens. But if the user clicks a button which is not the next in the sequence, the game resets (keeping only best).

I also placed the actual execution of the game in a if __name__ == "__main__": guard to allow importing parts of the code from other scripts (So you can e.g. do from simon_game import Button, if this code is saved as simon_game.py).

Final code:

import Tkinter
import random
from collections import namedtuple

Button = namedtuple("Button", "color alt_color x y")
BUTTONS_DATA = [Button("yellow", "yellow3", 30, 50),
                Button("blue", "medium blue", 125, 50),
                Button("red", "red3", 30, 145),
                Button("green", "dark green", 125, 145)]

class SimonGame:
    def __init__(self, buttons_data=BUTTONS_DATA):
        self.base = Tkinter.Tk()
        self.frame = Tkinter.Frame(self.base, bg="black", width="238", height="250")

        self.score = 0
        self.score_label = self.place_label(30, 15, "Score:")
        self.score_val = self.place_label(70, 15, self.score)

        self.best = 0
        self.best_label = self.place_label(155, 15, "Best:")
        self.best_val = self.place_label(188, 15, self.best)

        self.start = Tkinter.Button(self.base, bd="0", text="start", command=self.show_sequence)
        self.start.place(x=90, y=12)

        self.sequence = []
        self.user_sequence = []

        self.buttons_data = buttons_data
        self.buttons = [self.create_button(*button) for button in self.buttons_data]

        self.frame.pack()
        self.base.resizable(False, False)
        self.base.mainloop()

    def place_label(self, x, y, text, fg="white", bg="black"):
        label = Tkinter.Label(self.base, bg=bg, fg=fg, text=text)
        label.place(x=x, y=y)
        return label

    def click_factory(self, button, color, alt_color):
        def click():
            button.configure(activebackground=alt_color)
            button.after(500, lambda: button.configure(activebackground=color))
            self.user_sequence.append(color)
            self.check_sequence()
        return click

    def create_button(self, color, alt_color, x, y):
        button = Tkinter.Button(self.base, bd="0", highlightthickness="0",
                           width="7", height="5", activebackground=color,
                           bg=alt_color)
        button['command'] = self.click_factory(button, color, alt_color)
        button.place(x=x, y=y)
        return button

    def show_sequence(self):
        self.reset_gui()
        for _ in range(self.score + 1):
            i = random.randrange(len(self.buttons))
            button = self.buttons[i]
            button_data = self.buttons_data[i]
            button.configure(bg=button_data.color)
            button.after(750, lambda: button.configure(bg=button_data.alt_color))
            self.sequence.append(button_data.color)
        print self.sequence

    def check_sequence(self):
        if self.sequence == self.user_sequence:
            self.score += 1
            self.best = max(self.best, self.score)
            self.reset_gui()
        elif not self.sequence[:len(self.user_sequence)] == self.user_sequence:
            self.score = 0
            self.reset_gui()

    def reset_gui(self):
        self.sequence = []
        self.user_sequence = []
        for button, data in zip(self.buttons, self.buttons_data):
            button.configure(bg=data.alt_color, activebackground=data.color)
        self.best_val.configure(text=self.best)
        self.score_val.configure(text=self.score)

if __name__ == "__main__":
    game = SimonGame()

What is left to do:
The graphical part is not very clean. Some buttons get stuck on on (in sequences where a button appears an even number of times) and the sequence plays out too fast. Also more visual feedback when getting a whole sequence right or messing it up would help.


Decorator & Visitor Pattern in Python I have implemented Visitor and Decorator Pattern in python. I am much used to java design pattern style and hence thought would try in python as well. Could anyone tell me if this is the correct way to do in Python.

Is there any better way to do this?

I have made coffee class and decorating it with Sugar and Milk. I have a visitor that will calculate the tax or the cost. The values are random in TaxVisitor and CostVisitor

from abc import ABC, abstractmethod

class Visitable(ABC):

    @abstractmethod
    def accept(self, visitor):
        pass

class CostVisitor(ABC):

    @abstractmethod
    def cost(self, node):
        pass

class TaxVisitor(CostVisitor):

    def cost(self,node):
        if isinstance(node, Milk):
            return 150
        if isinstance(node, Sugar):
            return 100
        if isinstance(node, Plain_Coffee):
            return 10

class CostVisitor(CostVisitor):

    def cost(self,node):
        if isinstance(node, Milk):
            return 15
        if isinstance(node, Sugar):
            return 10
        if isinstance(node, Plain_Coffee):
            return 1

class Coffee(Visitable):

    @abstractmethod
    def cost(self):
        raise ValueError('Implement')

    @abstractmethod
    def display(self):
        raise ValueError('Implement')

class Plain_Coffee(Coffee):

    def cost(self):
        return 2

    def display(self):
        return 'Coffee'

    def accept(self, visitor):
        return visitor.cost(self)

class CoffeeDecorator(Coffee):

    def __init__(self, m_base):
        self.m_base = m_base

    def accept(self, visitor):
        return visitor.cost(self) + self.m_base.accept(visitor)

class Milk(CoffeeDecorator):

    def __init__(self, m_base):
        CoffeeDecorator.__init__(self, m_base)

    def cost(self):
        return self.m_base.cost() + 1

    def display(self):
        return self.m_base.display() + ' milk'

class Sugar(CoffeeDecorator):

    def __init__(self, m_base):
        CoffeeDecorator.__init__(self, m_base)

    def cost(self):
        return self.m_base.cost() + .5

    def display(self):
        return self.m_base.display() + ' sugar'

coffee = Plain_Coffee()
coffee = Milk(coffee)
coffee = Sugar(coffee)

print(coffee.accept(CostVisitor()))

```

In general visitor patterns in other languages are manage with tables, maps or any other struct/object type, here is the improvement that you can make on your code

class TaxVisitor(CostVisitor):

    def cost(self,node):
        if isinstance(node, Milk):
            return 150
        if isinstance(node, Sugar):
            return 100
        if isinstance(node, Plain_Coffee):
            return 10

Can be changed to

class TaxVisitor(CostVisitor):
    self.__table = { "Milk" : 150, "Sugar" : 100, ...}

    def cost(self,node):
        if node.__class__.__name__ in self.__table:
            return self.__table[node.__class__.__name__]

This will make it easy to extender the number of instances by just update the table variable, hope is clear.


Decorator pattern for an application I've created a decorator in my Rails application. However, the initializer has 3 params. Is this generally acceptable in decorator or should I refactor this or try a different pattern?

class FileBinderDecorator
  attr_reader :binder, :credential, :page

  def initialize(binder, credential, page = 1)
    @binder = binder
    @credential = credential
    @page = page
  end

  def file_items
    file_items = credential.file_item_list(excluded_ids: binder.file_items.pluck(:id))
    file_items.page(page).per(7)
  end

  def method_missing(method_name, *args, &block)
    @binder.send(method_name, *args, &block)
  end

  def missing_responds_to?(method_name, include_private = false)
    @binder.respond_to?(method_name, include_private) || super
  end
end

manage.html.slim

.modal.binder-modal.fade data-selected-ids=[] [id="edit-files-modal"]
  .modal-dialog
    .modal-content
      .modal-header
        button.close aria-label="Close" data-dismiss="modal" type="button"
          span aria-hidden="true"  &times;
        h4.modal-title Edit Files
      .modal-body
        .container-fluid
          .row-fluid
            .credentials
              = select_tag 'credentials', options_from_collection_for_select(credentials_for_current_user_and_account(current_user, @account), :first, :last)
              .selectors
                .file-box-select-all
                  = link_to 'Select All', '#'
                .file-box-deselect-all
                  = link_to 'Deselect All', '#'
          .row-fluid
            .list-group[class="binder-#{decorator.id}"]
              = render partial: 'file_binders/filebox', locals: { file_items: decorator.file_items, binder: decorator.binder, credential: decorator.credential }
          .row-fluid
            .text-center
              .pagerx[class="pager-#{decorator.id}"]
                = render partial: 'file_binders/pager', locals: { file_items: decorator.file_items, binder: decorator.binder, credential: decorator.credential }
      .modal-footer
        button.add-selected.btn.btn-primary class="add-selected-#{decorator.id}" data-dismiss='modal' data-count=0 data-file-items=[]
          | Remove Selected
        button.btn.btn-cancel aria-label="Close" id="modalCloseButton#{decorator.id}" data-dismiss="modal" type="button" aria-hidden="true" Cancel

The whole idea behind decorators, proxies and drapers is that they encase a single object. Yours seems to be serving two masters and has far too much knowledge of the different objects in your system.

It hard to get a picture of your models from the question - but it seems like you are missing a key relationship since you are passing binder and
credential around together.

Also instead of creating your own proxying methods you can simply extend the Delegator class but first you have to figure what you are actually proxying and stick to that object.


Enumerating text (ranges) vertically and horizontally I'm writing a parser and I need to iterate through text vertically (down to bottom) and horizontally (left to right).

This alone would be easy but some text parts need to be rescanned by another parser and for this I need to reiterate over random lines or a text range.

I wrote two enumerators.

One for enumerating all lines or only some of them:

internal class LineEnumerator
{
    private const int IndexOutOfRange = -1;

    public LineEnumerator(IReadOnlyList<string> lines) : this(lines, null) { }

    public LineEnumerator(IReadOnlyList<string> lines, IReadOnlyList<int> lineNumbers)
    {
        Lines = lines;
        Index = -1;
        LineNumbers = lineNumbers;
    }

    public IReadOnlyList<string> Lines { get; }
    public IReadOnlyList<int> LineNumbers { get; }
    public int Index { get; private set; }
    public bool IsIndexInRange => Index > IndexOutOfRange && Index < (LineNumbers?.Count ?? Lines.Count);
    public int LineNumber
    {
        get
        {
            if (!IsIndexInRange) { return IndexOutOfRange; }
            return LineNumbers?[Index] ?? Index;
        }
    }
    public string Current => IsIndexInRange ? Lines[LineNumber] : null;

    public CharEnumerator Chars => IsIndexInRange ? new CharEnumerator(Current) : null;

    public bool MoveNext() => ++Index < (LineNumbers?.Count ?? Lines.Count);
}

and another one for enumerating chars or only a particular range:

internal class CharEnumerator
{
    private const int IndexOutOfRange = -1;

    public CharEnumerator(string line) : this(line, 0, line.Length) { }

    public CharEnumerator(string line, int index0, int length)
    {
        Line = line;
        Index0 = index0;
        Length = length;
        Index = IndexOutOfRange;
    }

    public string Line { get; }
    public int Index0 { get; }
    public int Length { get; }
    public int Counter { get; private set; }
    public int Index { get; private set; }
    public bool IsIndexInRange => Index >= Index0 && Counter <= Length;
    public char Current => IsIndexInRange ? Line[Index] : '\0';

    public bool MoveNext()
    {
        if (!IsIndexInRange)
        {
            Index = Index0;
            Counter++;
            return true;
        }
        Index++;
        Counter++;
        return (Counter <= Length);
    }
}

They shouldn't just enumerate the text but also keep track of where they are so that I can throw exceptions with exact location if something goes wrong thus the line enumerator needs to work with real indexes even for a range. The same applies to the char enumerator.

I feel like the enumerators violate SRP (single responsibility principle) and I should create two more enumerators for enumerating a range of lines or a range of characters. Is this necessary or would you leave it like that?

Examples

var lines = new[] { "foo", "bar", "baz", "qux" };
var lineEnumerator = new LineEnumerator(lines);
while (lineEnumerator.MoveNext())
{
    Console.WriteLine(lineEnumerator.Current);
}

  foo bar baz qux

Console.WriteLine();
var lineRangeEnumerator = new LineEnumerator(lines, new[] { 1, 3 });
while (lineRangeEnumerator.MoveNext())
{
    Console.WriteLine(lineRangeEnumerator.Current);
}

  bar qux

Console.WriteLine();
var line = "foo bar baz";
var charEnumerator = new CharEnumerator(line);
while (charEnumerator.MoveNext())
{
    Console.WriteLine(charEnumerator.Current);
}

  f o o   b a r   b a z

Console.WriteLine();
var charRangeEnumerator = new CharEnumerator(line, 4, 3);
while (charRangeEnumerator.MoveNext())
{
    Console.WriteLine(charRangeEnumerator.Current);
}

  b a r

I would go with standard IEnumerator and IEnumerable, so it could be consumed as:

    static void Main(string[] args)
    {
        var template = new Template("foo", "bar", "baz", "qux");

        foreach (var row in template)
            Console.WriteLine(row.Text);

        Console.WriteLine();
        foreach (var row in template.Select(1, 3))
            Console.WriteLine(row.Text);

        Console.WriteLine();
        foreach (var row in template.Select(0))
            foreach (var column in row)
                Console.WriteLine(column.Text);

        Console.WriteLine();
        foreach (var row in template.Select(0))
            foreach (var column in row.Range(0,3))
                Console.WriteLine(column.Text);
    }

Where Select allows to subset by indecies provided, while Range is working in terms of start and count. Row and Column objects have Index (current selection) and Indecies (allowed to be selected) properties:

public class Template : IEnumerable<Row>
{
    public Template(params string[] lines)
    {
        Lines = lines;
    }

    public IEnumerable<Row> Range(int start, int count) => Select(Enumerable.Range(start, count));
    public IEnumerable<Row> Select(params int[] indecies) => Select(indecies as IEnumerable<int>);
    public IEnumerable<Row> Select(IEnumerable<int> indecies)
    {
        foreach (var index in indecies)
            yield return new Row(Lines.ElementAtOrDefault(index), index, indecies);
    }

    public IEnumerator<Row> GetEnumerator() => Range(0, Lines.Length).GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
    string[] Lines { get; }
}

And:

public struct Row : IEnumerable<Column>
{
    public Row(string text, int index, IEnumerable<int> indecies)
    {
        Text = text;
        Index = index;
        Indecies = indecies;
    }

    public string Text { get; }
    public int Index { get; }
    public IEnumerable<int> Indecies { get; }

    public IEnumerable<Column> Range(int start, int count) => Select(Enumerable.Range(start, count));
    public IEnumerable<Column> Select(params int[] indecies) => Select(indecies as IEnumerable<int>);
    public IEnumerable<Column> Select(IEnumerable<int> indecies)
    {
        foreach (var index in indecies)
            yield return new Column(Text.ElementAtOrDefault(index), index, indecies);
    }

    public IEnumerator<Column> GetEnumerator() => Range(0, Text.Length).GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

And:

public struct Column
{
    public Column(char text, int index, IEnumerable<int> indecies)
    {
        Text = text;
        Index = index;
        Indecies = indecies;
    }

    public char Text { get; }
    public int Index { get; }
    public IEnumerable<int> Indecies { get; }
}


Deleting ranges of rows from a spreadsheet I'm currently trying to create a macro, to edit a document that is around 280k rows and 12 columns. For the first steps I found a macro to delete the rows I don't want based on certain criteria, but the code repeats itself a lot. Every time I run it, I change the criteria and link the macros.

Also, is there a better way to reuse this code, rather than to modify it each time?

Sub jbeck1()
Dim i As Long
Dim LR As Long

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

LR = Cells(Rows.Count, 1).End(xlUp).Row

For i = LR To 2 Step -1

If Range("E" & i) = "0020" Then
    Range("E" & i).EntireRow.Delete Shift:=xlUp
End If

Next i

Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True
Application.Run "jbeck2"

End Sub
Sub jbeck2()
Dim i As Long
Dim LR As Long

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

LR = Cells(Rows.Count, 1).End(xlUp).Row

For i = LR To 2 Step -1

If Range("E" & i) = "0021" Then
    Range("E" & i).EntireRow.Delete Shift:=xlUp
End If

Next i

Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True
Application.Run "jbeck3"
End Sub

Instead of looping through every single line and then deleting it if it matches your criteria, you could build a string that contains the exact ranges you want to delete.  Then once the loop is complete you could call a single Range().Delete call.  You also can make the macro take in a string that you wish to delete.

Sub MacroDelete(S As String)

Dim i As Long
Dim LR As Long

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

LR = Cells(Rows.Count, 1).End(xlUp).Row

Dim RangeToDelete As String
Dim Count As Integer
Dim numRowsToDelete As Integer

RangeToDelete = ""
Count = 0

' cheap way to get the total amount we want to delete
numRowsToDelete = Application.WorksheetFunction.CountIf(Range("E:E"), S) - Application.WorksheetFunction.CountIf(Range("E1:E2"), S)

For i = LR To 2 Step -1

' build the range string
If Range("E" & i) = S Then
    Count = Count + 1
    RangeToDelete = RangeToDelete & i & ":" & i
    If Count < numRowsToDelete Then
        RangeToDelete = RangeToDelete & ","
    End If
End If

Next i

' delete all the rows found matching S
Range(RangeToDelete).Delete Shift:=xlUp

Application.Calculation = xlCalculationAutomatic
Application.ScreenUpdating = True

End Sub

You can see @D_Zab's answer for an understanding on how to call subs or functions from other subs


Given any positive range, count the values that are complete squares import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;

public class Solution {
    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        System.out.println("Please choose the minimum of your range: ");
        double minimum = scanner.nextInt();
        System.out.println("Please choose the maximum of the range: ");
        double maximum = scanner.nextInt();
        List<Double> list = new ArrayList<>();
        List<Double> doubles = new ArrayList<>();
        int sum = 0;
        for (double x = minimum; x <= maximum; x++) {
            list.add(x);
        }
        for (int i = 0; i < list.size(); i++) {
            double sqrt = Math.sqrt((i));
            doubles.add(sqrt);
        }
        for (Double value : doubles) {
            if (value.doubleValue() == value.intValue()) {
                sum += 1;
            }
        }
        System.out.println(sum);
    }
}

Your algorithm is inefficient:

Floating-point arithmetic is slower than integer arithmetic.
To find all the perfect squares in a range, you need only find the square root of the minimum (and round up) and the square root of the maximum (and round down).

You should be able to find the answer in constant time, with no looping.


Using ASP.NET identity in a new app I need code review for using ASP.NET Identity in a new app.

Goals:

Use int instead of GUID for IDs.
Separate identity from view layer

I need code review if I did everything right. Yes it is working but maybe I did something that I shouldn't or maybe there is some place for improvement.

ApplicationUser

public class ApplicationUser : IdentityUser<int, CustomUserLogin, CustomUserRole,
    CustomUserClaim>
    {
        public ICollection<ProductCategory> ProductCategories { get; set; }
        public ICollection<Store> Stores { get; set; }
        public ICollection<Product> Products { get; set; }

        public async Task<ClaimsIdentity> GenerateUserIdentityAsync(
    UserManager<ApplicationUser, int> manager)
        {
            var userIdentity = await manager.CreateIdentityAsync(
                this, DefaultAuthenticationTypes.ApplicationCookie);
            return userIdentity;
        }
    }

    public class CustomUserRole : IdentityUserRole<int> { }
    public class CustomUserClaim : IdentityUserClaim<int> { }
    public class CustomUserLogin : IdentityUserLogin<int> { }

    public class CustomRole : IdentityRole<int, CustomUserRole>
    {
        public CustomRole() { }
        public CustomRole(string name) { Name = name; }
    }

    public class CustomUserStore : UserStore<ApplicationUser, CustomRole, int,
        CustomUserLogin, CustomUserRole, CustomUserClaim>
    {
        public CustomUserStore(ApplicationDbContext context)
            : base(context)
        {
        }
    }

    public class CustomRoleStore : RoleStore<CustomRole, int, CustomUserRole>
    {
        public CustomRoleStore(ApplicationDbContext context)
            : base(context)
        {
        }
    }

DbContext

public class ApplicationDbContext : IdentityDbContext<ApplicationUser, CustomRole,
    int, CustomUserLogin, CustomUserRole, CustomUserClaim>
    {
        public static ApplicationDbContext Create()
        {
            return new ApplicationDbContext();
        }

        public ApplicationDbContext()
            : base("DefaultConnection")
        {...

View models are still in view layer. All above is in data layer (class library).
In the view assembly in AccountController I changed all ApplicationUser to reference ApplicationUser in data layer assembly.

I also added extension to get UserID as int:

public static int GetUserIdInt(this IIdentity identity)
        {
            if (identity == null)
                throw new ArgumentNullException("identity");

            string stringUserId = identity.GetUserId();

            int userId;
            if (string.IsNullOrWhiteSpace(stringUserId) || !int.TryParse(stringUserId, out userId))
            {
                return default(int);
            }

            return userId;
        }

Your customization looks fine - that's the way it is recommended to do it.

The only note I could add is about your GetUserIdInt method which you can get rid of. You can just use this generic overload:

User.Identity.GetUserId<int>()


Knapsack01 problem, written in c# Is this code written in a decent style? Appreciate some feedback : )

private static int Max(int a, int b)
{
    return a > b ? a : b;
}

/// <summary>
/// Returns the solution to Knapsack01 problem. Uses bottom up dynamic programming.
/// </summary>
private static int Knapsack01(int capacity, int[] values, int[] weights)
{
    int[,] table = new int[values.Length + 1, capacity + 1];

    for (int row = 0; row <= values.Length; row++)
    {
        for (int column = 0; column <= capacity; column++)
        {
            if (row == 0 || column == 0)
            {
                // base case
                table[row, column] = 0;
            }

            else if (weights[row -1] <= column)
            {
                // item fits
                int valueAbove = table[row - 1, column];
                int spareCapacity = column - weights[row - 1];
                int currentValue = values[row - 1] + table[row - 1, spareCapacity];
                table[row, column] = Max(valueAbove, currentValue);
            }

            else
            {
                // item doesn't fit so copy value from above cell
                table[row, column] = table[row - 1, column];
            }
        }
    }

    return table[values.Length, capacity];
}

It's not exactly well-documented. You can sort of determine what the "Knapsack problem" is by looking at a few comments and some variable names, but if I were the developer, I'd probably add a comment block to describe how it works in case someone else came upon this code later and needed to understand it quickly.

I'm not quite sure why they didn't just use Math.Max instead of creating their own function (maybe they ported it from some other language that didn't have that function handy?).

Aside from those things, if it's functional and returns accurate results, then I don't see a huge problem with it.


Linspace method build in Python's C API Today I built a linspace function in Python's C API:

static PyObject *
linspace(PyObject * self, PyObject * args)
{
    int n, i;
    double start, end;

    if (!PyArg_ParseTuple(args, "ddi:linspace", &start, &end, &n))
        return NULL;

    if (n <= 1)
        return Py_BuildValue("[d]", end);

    double h;
    PyObject *pylist = PyList_New(n);

    h = (end - start) / (n - 1);

    for (i = 0; i < n; i++)
        PyList_SetItem(pylist, i, Py_BuildValue("d", (start + h * i)));

    return Py_BuildValue("O", pylist);
}

It behaves how I would like it to behave, however, when I benchmarked it against NumPy's linspace it was slower by about a factor of 80.

I have a few questions that I think may be affecting performance, but I can't seem to find help online:

Is there a memory leak? Or am I not incrementing or decrementing any references that I should be?
Can I do this with a C double array and then return that as a Python Object? Would this even be faster (I think it may)?
Am I missing something? I am new to the C API and I am not confident in it yet.

I can't see any obvious memory leaks. If you're worried, then you might start out by seeing what sys.getrefcount tells you.
You will need to package up your array-of-doubles as a new type of Python object. See section 2 of the Extending/Embedding manual.
Since you know that you are creating float objects, you could speed things up slightly by using PyFloat_FromDouble instead of the generic Py_BuildValue (which has to parse its first argument and then dispatch). But this is not going to beat NumPy, because packaging up an array-of-numbers as a new type of Python object is exactly what NumPy does, and that's why it runs so fast: it doesn't have to allocate a new Python object for every position in the list.


Linq C# pulling from table row into input fields Today is the first time I've used Linq, so I wanted to get a code review on this and make sure I'm going about it correctly.

I'm filling text boxes and dropdown lists from a table row.

protected void Page_Load(object sender, EventArgs e)
        {

            using (DBContext da = new DBContext())
            {
                string dob;
                var detail = from a in da.tblPatients
                             where a.ID == 1198
                             select a;
                foreach (var d in detail)
                {

                    txtFirstName.Text = d.FirstName;
                    txtLastName.Text = d.LastName;
                    txtPreferredName.Text = d.PreferredName;
                    dob = d.DOB;
                    if(!String.IsNullOrEmpty(dob))
                    {
                        try
                        {
                            int lastindexofdob = 0;
                            string month = "";
                            string day = "";
                            string year = "";
                            lastindexofdob = dob.LastIndexOf("/");
                            month = dob.Substring(0, dob.IndexOf("/"));
                            day = dob.Substring(month.Length + 1, (dob.LastIndexOf("/") - month.Length) - 1);
                            year = dob.Substring(dob.Length - 4);

                            ddlDOBMonth.SelectedIndex = ddlDOBMonth.Items.IndexOf(ddlDOBMonth.Items.FindByValue(month));
                            ddlDOBDay.SelectedIndex = ddlDOBDay.Items.IndexOf(ddlDOBDay.Items.FindByText(day));
                            ddlDOBYear.SelectedIndex = ddlDOBYear.Items.IndexOf(ddlDOBYear.Items.FindByText(year));
                        }
                        catch { //error catching will go here}

                    }
                    txtAddress.Text = d.Address;
                }
            }

        }

There are actually about 20 more input fields to do, but I wanted a code review at this point.

Your use of linq is fine.

You may consider using a more descriptive variable than a, such as patient

You should pass any query parameters likely to change as variables. Consider:

var patientID = 1198;
var detail = from patient in da.tblPatients
             where patient.ID == patientID
             select patient;

I find the form population logic and database design to be a touch more troubling. txtFirstName.Text = d.FirstName is perfectly fine. DOB, which I assume to be date of birth should be stored and used as a Date rather than a String.

If you are unable to change the data-layer, at least convert it to a Date for use using Date.Parse or similar.

Your try block is lazy coding. Check the results from your dob.IndexOf and 'dob.Substring' calls rather than relying on catching an error -- be defensive in your code. Consider the following:

DateTime dob = DateTime.MinValue;
if (DateTime.TryParse(d.DOB, dob))
{
    ddlDOBMonth.SelectedValue = dob.Month.ToString();
    ddlDOBDay.SelectedValue = dob.Day.ToString();
    ddlDOBYear.SelectedValue = dob.Year.ToString();
}


ASP.NET Core singleton acting as an HttpClient provider I have created an ASP.NET Core singleton service class to act as an HttpClient provider after reading that reusing an instance of HttpClient is encouraged. However, I also want to be sure that the BaseAddress property is kept up-to-date with the value specified in the configuration so I added a dependency on IOptionsMonitor so that any time the configuration is updated, the BaseAddress property is re-set.

HttpClientProvider

public class MachineDatabaseHttpClientProvider
{
    public HttpClient Client
    {
        get
        {
            try
            {
                readWriteLock.EnterReadLock();
                return client;
            }
            finally
            {
                readWriteLock.ExitReadLock();
            }
        }
    }

    private readonly ReaderWriterLockSlim readWriteLock = new ReaderWriterLockSlim();
    private readonly HttpClient client;

    public MachineDatabaseHttpClientProvider(IOptionsMonitor<ApplicationConfiguration> applicationConfigurationMonitor)
    {
        client = new HttpClient();
        client.BaseAddress = new Uri(applicationConfigurationMonitor.CurrentValue.MachineDatabase.BaseUrl);
        client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

        applicationConfigurationMonitor.OnChange(c => UpdateClientBaseAddress(c.MachineDatabase.BaseUrl));
    }

    private void UpdateClientBaseAddress(string baseUrl)
    {
        try
        {
            readWriteLock.EnterWriteLock();
            client.BaseAddress = new Uri(baseUrl);
        }
        finally
        {
            readWriteLock.ExitWriteLock();
        }
    }
}

ASP.NET Core Startup Class

public class Startup
{
    public Startup(IHostingEnvironment env) { ... }

    public void ConfigureServices(IServiceCollection services)
    {
        services.AddMvc().AddTypedRouting();
        services.Configure<ApplicationConfiguration>(Configuration.GetSection(ApplicationConfiguration.ConfigurationSectionKey));
        services.AddSingleton<MachineDatabaseHttpClientProvider>();
    }
}

Is the above code more complex than it needs to be? Is my use of ReaderWriterLockSlim sufficient to achieve thread-safety? Reading this answer on HttpClient thread-safety indicates that none of the properties of HttpClient are thread-safe. Should I offer Get and Post methods as a wrapper around the HttpClient so that I can control all access to the class?

Alternative proposal

I think, I'd approach it differently, but I may miss some important details.

As long as the configuration change does not happen too frequently OR client construction is cheap, we can actually create a new instance of the HttpClient on every configuration change event, rather than trying to keep a single object in a good state. IMO, this approach is cleaner in the functional programming sense: the Client property now becomes a pure function in a way -- no messing with state of the object.

Now, you probably still want to keep the same HttpClient instance for as long as the config does not change -- to avoid an unnecessary object construction. Your HttpClientProvider can then work as a client factory and client "cache" in a way. This class will be responsible for providing either an existing properly configured HttpClient instance, or generating a new one since configuration has changed.

As a side note, I assume you use an IoC container for injecting HttpClientProvider into the consumers. The provider is probably registered as a Singleton with the IoC container, right?

Nitpick

There's no need to immediately create a client instance on MachineDatabaseHttpClientProvider construction. This can be deferred until the Client instance is being accessed by the consumer (e.g. Lazy<HttpClient> may work).

P. S.

I know that my answer is twisting the problem definition. Hopefully, this is not an issue (especially if there are no other requirements that are forcing you to have a single instance of an HttpClient throughout the entire application lifetime).

--------------- UPDATE 1 ---------------

I am not a guru of multi-threading but I don't think you should have any issue in most of the cases even with a thread-unsafe code here. I have different concerns though (explained below).

Simplicity

If you lock around the _currentHttpClient object [obviously, not lock (_currentHttpClient) but rather lock (_currentHttpClientLock)], you will guarantee that NO unnecessary client is created "ever".
Otherwise, you may have an extra object created n times per configuration change when there're n threads asking for a client "simultaneously".

Warnings/Pitfalls

Non-locking approach is not an issue only as long as it's okay to swap the "hot" client.
I.e. the one used by a consumer for performing multiple client calls within a single body of work.
It also depends a lot on how the consumer is using the HttpClient.
Here are three different scenarios with their own problems.

Option 1

Here's a consumer that does not hold an HttpClient reference, but keeps retrieving the client from provider for EACH external call:

class ConsumingController
{
    private MachineDatabaseHttpClientProvider _provider;

    ConsumingController(MachineDatabaseHttpClientProvider provider)
    {
        _provider = provider;
    }

    // ...

    [HttpGet]
    public Task<ComplexObject> GetComplexObject(string id)
    {
        ComplexObject complexObject;

        // Moment 1
        complexObject = _myOwnCopyOfClient.Client.GetObject("base" + id);

        // Moment 2
        complexObject.Details = _myOwnCopyOfClient.Client.GetObject("datails-for-" + id);

        return complexObject;
    }
}

If the configuration changes between the moments 1 and 2 (say endpoint changes), the two requests will target different databases and it may result in funny results.
And I mean, funny in a bad way.

In a good case, you'll get an HTTP 404 for the second request, which is safe. All the consuming application needs to do is repeat the request...

In a worse case, there could be "detail" objects in the second DB that match the id from the first DB. This will result in construction of an object that presents data from two incompatible sources.

You application should be able to handle this situation.

Option 2

Another consumer design will simply hold the reference to the HttpClient instance:

class ConsumingController
{
    private HttpClient _myOwnCopyOfClient;

    ConsumingController(MachineDatabaseHttpClientProvider provider)
    {
        _myOwnCopyOfClient = provider.Client;
    }

    // ...

    [HttpGet]
    public Task<ComplexObject> GetComplexObject(string id)
    {
        ComplexObject complexObject;
        complexObject = _myOwnCopyOfClient.GetObject("base" + id);
        complexObject.Details = _myOwnCopyOfClient.GetObject("datails-for-" + id);
        return complexObject;
    }
}

Notice that if the consumer persists the reference to an HttpClient, there's NO effective WAY for the Provider to update the configuration of that instance.

Option 3

This may be the best option...

class ConsumingController
{
    private MachineDatabaseHttpClientProvider _provider;

    ConsumingController(MachineDatabaseHttpClientProvider provider)
    {
        _provider = provider;
    }

    // ...

    [HttpGet]
    public Task<ComplexObject> GetComplexObject(string id)
    {
        // This client instance is bound to the method execution scope...
        var requestScopeClient = _myOwnCopyOfClient.Client;

        ComplexObject complexObject;

        // Moment 1
        complexObject = requestScopeClient.GetObject("base" + id);

        // Moment 2
        complexObject.Details = requestScopeClient.GetObject("datails-for-" + id);

        return complexObject;
    }
}


Simple Blackjack/21 game in console I made a simple Blackjack/21 game in C++. It does not use any fancy graphics, just the console output. It is as simple as it can get.

#include <iostream>
#include <cstdlib>
#include <ctime>
#include <algorithm>

void printData(int pTotal, int dCard) {

    std::cout << "\nYour total is " << pTotal;
    std::cout << "\nDealer has a " << dCard << " showing.\n";

}

int getCard() {

    return std::rand() % 11 + 2;

}

void turn(int& pTotal, char choice, bool& stay) {

    // If the user wants to hit
    if (choice == 'H' || choice == 'h') {

        // If the user hits 21 or above
        if ((pTotal += getCard()) >= 21) {

            std::cout << "Your total is " << pTotal;
            stay = true;

        }

        return;
    }

    // Since the only other option can be to stay, then stay
    stay = true;

}

int main() {

    bool stay;
    char choice = NULL;

    while (true) {

        std::cout << "================================";
        stay = false;

        // Initialize srand and get random card numbers
        std::srand(std::time(0));
        int dCard = getCard();
        int pTotal = getCard() + getCard();
        int dTotal = dCard + getCard();

        // Player's turn
        while (!stay) {

            printData(pTotal, dCard);
            std::cout << "[H]it or [S]tay?\n";
            std::cin >> choice;
            turn(pTotal, choice, stay);

        }

        stay = false;
        std::cout << "\n\nIt is now the dealer's turn!\n";

        // Dealer's turn
        while (dTotal <= 21 && !stay) {

            if (dTotal >= 17)
                stay = true;
            else
                dTotal += getCard();

        }

        // Display winner
        std::cout << "\n\nThe player has " << pTotal << ".\nThe dealer has " << dTotal << ".\n\n";
        if ((std::abs(21 - pTotal)) < (std::abs(21 - dTotal))) std::cout << "The player wins!\n";
        else std::cout << "The dealer wins!\n";

    }

}

Is this code efficient?
Is the code easy to read/understand and to follow?
Are variable names appropriate?

I want my money back!
Your total is 15
Dealer has a 4 showing.
[H]it or [S]tay?
s

It is now the dealer's turn!

The player has 15.
The dealer has 23.   <<=====

The dealer wins!     <<=====

Your casino is cheating! Also you're not handling aces, all face cards are supposed to have a value of 10, and there are 13 ranks in a suit but your getCard function can only return 11 different values (2 to 12).
This means that you can lose right off the bat:
$ ./a.out
================================
Your total is 24       <<======     :-(
Dealer has a 9 showing.
[H]it or [S]tay?

In turn, if the player enters h and doesn't bust, you're not setting stay - this happens to work given the code flow, but it looks odd. Set it before returning.
If the user enters gibberish, you always interpret that as stay. That's not user-friendly.
In main:
bool stay;
char choice = NULL;

That's not a valid initializer for a char - chars are not pointers. And leaving a variable (stay) uninitialized is not a good idea. You should move stay inside the loop, only declare it when you give it its first value. (I'd also use two separate variables for the dealer and the player.)
Remove choice from main altogether. Only turn needs it, so handle the user input in there completely. (And have turn return hit/stay so you don't need that out parameter.)
This shouldn't be inside the loop:
std::srand(std::time(0));

You should initialize the RNG only once, do that at the very start of main. C++11 introduced new random generation facilities, you should look into them.
These lines are too long:
// Display winner
std::cout << "\n\nThe player has " << pTotal << ".\nThe dealer has " << dTotal << ".\n\n";
if ((std::abs(21 - pTotal)) < (std::abs(21 - dTotal))) std::cout << "The player wins!\n";
else std::cout << "The dealer wins!\n";

Try something like:
// Display winner
std::cout << "\n\nThe player has " << pTotal
          << ".\nThe dealer has " << dTotal << ".\n\n";

if ((std::abs(21 - pTotal)) < (std::abs(21 - dTotal))) {
  std::cout << "The player wins!\n";
} else {
  std::cout << "The dealer wins!\n";
}

And once you fix your logic so that a bust directly ends the players turn, and a dealer bust makes the player win, you can get rid of the std::abs since both scores will be at most 21.


Is my WriteConsole solution correct? Here is the book question:

  Modify the CatFile function in the program so that it uses
  WriteConsole rather than WriteFile when the standard output handle
  is associated with a console.

And here is my solution:

static VOID CatFile(HANDLE hInFile, HANDLE hOutFile)
{
    DWORD nIn, nOut;
    TCHAR buffer[BUF_SIZE];
    LPTSTR pBuffer = &buffer[0];

    while (ReadFile(hInFile, buffer, BUF_SIZE, &nIn, NULL) &&
        (nIn != 0) &&
        (WriteFile(hOutFile, buffer, nIn, &nOut, NULL) || WriteConsole(hOutFile, pBuffer, _tcslen(pBuffer), &nOut, NULL)));
    return;
}

The part from the || sign in the code above is my addition to the code.

My question is to know if that would work once WriteFile wouldn't be TRUE since hOutFile is a handle of a console?

Another part of the question is just about the WriteConsole function: are these the correct arguments?

No comment on what your call as I don't know what WriteFile() or WriteConsole() do.

But that is seriously abusing the && and !! operators.
I don't think I would ever let that code pass a review:

while (ReadFile(hInFile, buffer, BUF_SIZE, &nIn, NULL) &&
    (nIn != 0) &&
    (WriteFile(hOutFile, buffer, nIn, &nOut, NULL) || WriteConsole(hOutFile, pBuffer, _tcslen(pBuffer), &nOut, NULL)));

// Why not write it like this:

// While we manage to read a good value:
while (ReadFile(hInFile, buffer, BUF_SIZE, &nIn, NULL) && (nIn != 0))
{
     if (!(   (WriteFile(hOutFile, buffer, nIn, &nOut, NULL))
          || (WriteConsole(hOutFile, pBuffer, _tcslen(pBuffer), &nOut, NULL))
          )
        )
     {
          // Write Error
          break;
     }
}
// Now check for read errors to report.


Matching and Merging CPU info between worksheets Goal: Transfer Sheet Test1 2nd Column to Sheet TestS 2nd Column.

Where:

For each CPU in Test1. If CPU name in Sheet Test1 match CPU name in sheet TestS, copy 2nd column of that CPU in Test1 and paste in 2nd column of that CPU in TestS.

Else, create new row with the unfound CPU name and 2nd column after the last CPU of sheet TestS.

Final look in TestS looks like:

This is my current working code where variables needs to be declared (string, integer etc):

   Sub automateCpu()

   Dim noofComps As Long
   noOfComps = Worksheets("Test1").Range("A1").CurrentRegion.Rows.Count-1
   Dim x As Long
   x = 0
   Dim d As Integer
   d = 0
   Sheets("Test1").Select
   Range("A1").Select
   Do While x > noOfComps
      Selection.Offset(1, 0).Select
      Dim curComputer As String
      curComputer = Selection.Value
      Dim test1Row As Integer
      d = d + 1
      test1Row = ActiveCell.Row

      Sheets("TestS").Select
      Range("A1").Select
      Dim y As Integer
      y = 0
      Dim z As Integer
      z = 0
      Dim LastRowSummary As integer
      LastRowSummary = Worksheets("TestS").Range("A1").CurrentRegion.Rows.Count - 1
      Do While y < LastRowSummary
           Selection.Offset(1, 0).Select
           If Selection.Value = curComputer Then
              z = 1
              Dim SummaryRow As Integer
              SummaryRow = ActiveCell.Row

              Sheets("Test1").Select
              Range("B" & test1Row).Select
              Dim curLogValue As Integer
              curLogValue = Selection.Value

              Sheets("TestS").Select
              Range("B" & SummaryRow).Select
              Selection.Value = curLogValue

              Sheets("Test1").Select
              Range("A" & test1Row).Select
              Exit Do
         End If
         y = y + 1
      Loop

      If z = 0 Then
         d = d + 1
         Selection.Offset(1, 0).Select

         Selection.Value = curComputer
         SummaryRow = ActiveCell.Row

         Sheets("Test1").Select
         Range("B" & test1Row).Select
         Dim curLogValue As Integer
         curLogValue = Selection.Value

         Sheets("TestS").Select
         Range("B" & SummaryRow).Select
         Selection.Value = curLogValue

         Sheets("Test1").Select
         Range("A" & test1Row).Select
      End If
      z = 0
      d = d + 1
      x = x + 1
   Loop

   End Sub

Pseudo Code:

1) Loop CPU in Test1

2) Find CPU in TestS by looping each computer

3) If CPU Matches for both Test1 & TestS paste 2nd Column

4) Else, create new row for new computer and 2nd column.

My question is how can I make this code more efficient?
Or any alternate method that can reduce the amount of codes etc.

Three initial points:

Always use Option Explicit at the top of the module. Always!
Properly indent your code to help with readability and exposure of some of the logic.
Avoid using .Activate and .Select in your code. It is not necessary unless you want to bring something to the user's attention.

You have d, x, y, z but no real understand what they mean. Particularly d, which is not used anywhere except to be incremented!

Taking these points into account, you end up with:

Sub automateCpu()
Dim noofComps As Long
Dim test as Range
Dim testS as Range
Dim test1 as Range
    set test1 = Worksheets("Test1").Range("A1")
    noOfComps = testRange.CurrentRegion.Rows.Count-1
Dim x As Long
    x = 0
Dim d As Integer
    d = 0
    Do While x > noOfComps
        Set test1 = test1.Offset(1,0)
        Dim curComputer As String
        curComputer = testRange.Value
        Dim test1Row As Integer
        d = d + 1
        test1Row = test1.Row
        Dim testS as Range
        Set testS = Sheets("TestS").Range("A1") ' Why "Sheets" instead of "Worksheets" as in the other case? Consistency!
        Dim y As Integer
        y = 0
        Dim z As Integer
        z = 0
        Dim LastRowSummary As integer
        LastRowSummary = testS.CurrentRegion.Rows.Count - 1
        Do While y < LastRowSummary
            Set testS = testSRange.Offset(1, 0)
            If testS.Value = curComputer Then
                z = 1
                Dim SummaryRow As Integer
                SummaryRow = testS.Row
                Set test1 = Sheets("Test1").Range("B" & test1Row)
              Dim curLogValue As Integer
              curLogValue = test1.Value

              Set testS = Sheets("TestS").Range("B" & SummaryRow)
              testS.Value = curLogValue

              Set test1 = Sheets("Test1").Range("A" & test1Row)
              Exit Do
         End If
         y = y + 1
      Loop

      If z = 0 Then
         d = d + 1
         set test1 = test1.Offset(1, 0)
         test1.Value = curComputer
         SummaryRow = test1.Row

         set test1 = Sheets("Test1").Range("B" & test1Row)
         Dim curLogValue As Integer
         curLogValue = test1.Value

         Set testS = Sheets("TestS").Range("B" & SummaryRow)
         testS.Value = curLogValue

         Set test1 = Sheets("Test1").Range("A" & test1Row)
      End If
      z = 0
      d = d + 1
      x = x + 1
   Loop
   End Sub

But, looking at the code, you can see that there is a lot of shuffling of data into temporary variables. We can also clean this up with direct assignments.

Sub automateCpu()
Dim noofComps As Long
Dim test as Range
Dim testSA as Range
Dim test1A as Range
Dim testSB as Range
Dim test1B as Range
    set test1A = Worksheets("Test1").Range("A1")
    noOfComps = testRange.CurrentRegion.Rows.Count-1
Dim x As Long
    x = 0
    Do While x > noOfComps
        Set test1A = test1A.Offset(1,0)
        Dim curComputer As String
        curComputer = testRange.Value
        Dim test1Row As Integer
        test1Row = test1A.Row
        Set testSA = Sheets("TestS").Range("A1") ' Why "Sheets" instead of "Worksheets" as in the other case? Consistency!
        Dim y As Integer
        y = 0
        Dim z As Integer
        z = 0
        Dim LastRowSummary As integer
        LastRowSummary = testS.CurrentRegion.Rows.Count - 1
        Do While y < LastRowSummary
            Set testSA = testSRange.Offset(1, 0)
            If testSA.Value = curComputer Then
                z = 1
                Dim SummaryRow As Integer
                SummaryRow = testSA.Row
                Set test1B = Sheets("Test1").Range("B" & test1Row)
                Set testSB = Sheets("TestS").Range("B" & SummaryRow)
                testSB.Value = test1B.Value

                Set test1A = Sheets("Test1").Range("A" & test1Row)
                Exit Do
            End If
            y = y + 1
         Loop

         If z = 0 Then
             set test1A = test1A.Offset(1, 0)
             test1.Value = curComputer
             SummaryRow = test1A.Row

             set test1B = Sheets("Test1").Range("B" & test1Row)
             Set testSB = Sheets("TestS").Range("B" & SummaryRow)
             testSB.Value = test1B.Value

             Set test1A = Sheets("Test1").Range("A" & test1Row)
         End If
         z = 0
         x = x + 1
     Loop
End Sub

Probably still some tidying up to do - but this gives the idea.


Extracting elements from multiple sorted lists efficiently I was asked this interview question recently. I was able to come up with this which runs in \$O(k \log n)\$.

  Given k <= n sorted arrays each of size n, there exists a data structure requiring \$O(kn)\$ preprocessing time and memory that answers iterated search queries in \$O(k + \log n)\$ time.

I have k sorted Lists, each of size \$n\$. I currently have hard-coded 5 sorted Lists each of size 3, but in general that can be a very high number.

I would like to search for a single element in each of the \$k\$ Lists. Obviously, I can binary search each array individually, which will result in \$O(k \log n)\$ where \$k\$ is number of sorted arrays.

Can I do it in \$O(k + \log n)\$ where \$k\$ is the number of sorted arrays? I think there might be some better way of doing it as we're doing the same searches \$k\$ times as of now.

private List<List<Integer>> dataInput;

public SearchItem(final List<List<Integer>> inputs) {
    dataInput = new ArrayList<List<Integer>>();
    for (List<Integer> input : inputs) {
        dataInput.add(new ArrayList<Integer>(input));
    }
}

public List<Integer> getItem(final Integer x) {
    List<Integer> outputs = new ArrayList<Integer>();
    for (List<Integer> data : dataInput) {
        int i = Collections.binarySearch(data, x); // binary searching the item
        if (i < 0)
            i = -(i + 1);
        outputs.add(i == data.size() ? null : data.get(i));
    }
    return outputs;
}

public static void main(String[] args) {
    List<List<Integer>> lists = new ArrayList<List<Integer>>();

    List<Integer> list1 = new ArrayList<Integer>(Arrays.asList(3, 4, 6));
    List<Integer> list2 = new ArrayList<Integer>(Arrays.asList(1, 2, 3));
    List<Integer> list3 = new ArrayList<Integer>(Arrays.asList(2, 3, 6));
    List<Integer> list4 = new ArrayList<Integer>(Arrays.asList(1, 2, 3));
    List<Integer> list5 = new ArrayList<Integer>(Arrays.asList(4, 8, 13));

    lists.add(list1);
    lists.add(list2);
    lists.add(list3);
    lists.add(list4);
    lists.add(list5);

    SearchItem search = new SearchItem(lists);
    System.out.println(dataInput);

    List<Integer> dataOuput = search.getItem(5);

    System.out.println(dataOuput);
}

Whatever output I am seeing with my above code approach should come with the new approach as well which should work in \$O(k + \log n)\$. Is this possible to achieve? Can anyone provide an example of how this would work based on my example?

This is sooo off-topic for CR... but, since it is related to a previous answer, and since I want to see a blown mind ....:

consider the following code:

class Value<T> {
    private final T value;
    private final int[] arraypointers;
    private final int arraycursor = 0;

    Value(T value, int maxindex) {
        this.value = value;
        this.arraypointers = new int[maxindex];
    }

    public void addIndex(int pointer) {
        arraypointers[arraycursor++] = pointer;
    }

    ... some other stuff.
}

OK, the above class will be used as follows... consider the example data system, the value 4 appears in list1 and list5.

This would be stored as:

Value v = new Value(4, k);
v.addIndex(1);
v.addIndex(5);

Now, start with a LinkedList:

LinkedList<Value<Integer>> values = new LinkedList<>();

Then, iterator though each of your loops, and merge the values in to the linked list:

for (int datapointer = 0; datapointer < datalists.size(); datapointer++) {
    ListIterator<Value<Integer>> valit : values.listIterator();
    List<Integer> = datalists.get(datapointer);
    for (Integer addval : data) {
        boolean found = false;
        while (valit.hasNext()) {
            if (addval.compareTo(valit.next().value) >= 0) {
                found = true;
                Value<Integer> val = valit.previous();
                if (val.value.equals(addval) {
                    // update existing value
                    val.addIndex(datapointer);
                    // leave the iterator point backwards to
                    // allow for dup values in the data.
                } else {
                    // add a new value
                    Value<Integer> val = new Value(addval, k);
                    val.addIndex(datapointer);
                    valit.add(val);
                    // leave the iterator pointing backwards.
                    // but need to move it back one.
                    valit.previous();
                }
            }
        }
        if (!found) {
            Value<Integer> val = new Value(addval, k);
            val.addIndex(datapointer);
            valit.add(val);
            valit.previous();
        }

    }
}

then, convert the LinkedList in to an ArrayList

List sortedvalues = new ArrayList<Value<Integer>>(values);

Right, here we now have a sorted list of Values<Integer>. Each Value has pointers back to the list(s) they came from.

The space complexity for this is \$O(kn)\$ and we got there by doing a complexity \$O(kn)\$ nested loop (the inside while loop does not count because it is on an iterator that is outside the for loop, and it is part of the same complexity as the inner for loop)...

OK, so that is the \$O(kn)\$ preprocessing.

The lookup is a case of doing a binary search on the ArrayList (\$O(\log n)\$) and then iterating over the index pointers (\$O(k)\$).

Thus, the search is \$O(k + log n)\$.

Voila!

Working solution

Right, putting all the pieces together in a working solution:

Value.java

import java.util.Arrays;

class Value<T extends Comparable<T>> implements Comparable<Value<T>> {

    private final T value;

    private final T[] indices;

    public Value(T value, T[] data) {
        super();
        this.value = value;
        this.indices = data;
    }

    public void setIndex(int index, T val) {
        if (indices[index] == null) {
            indices[index] = val;
        }
    }

    public T[] getIndices() {
        return Arrays.copyOf(indices, indices.length);
    }

    public int compareToValue(T o) {
        return value.compareTo(o);
    }

    @Override
    public int compareTo(Value<T> o) {
        return value.compareTo(o.value);
    }

    @Override
    public int hashCode() {
        return value.hashCode();
    }

    @Override
    public boolean equals(Object obj) {
        return obj instanceof Value && value.equals(((Value<?>)obj).value);
    }

    @Override
    public String toString() {
        return String.format("%s -> %s", value, Arrays.toString(indices));
    }
}

MultiListIndex.java

package listsearch;

import java.lang.reflect.Array;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.ListIterator;

public class MultiListIndex<T extends Comparable<T>> {

    private final List<Value<T>> index;
    private final Class<T> clazz;
    private final int width;

    public MultiListIndex(Class<T> clazz, Collection<List<T>> data) {
        this.clazz = clazz;
        this.width = data.size();
        this.index = preprocess(new ArrayList<>(data));
    }

    private final List<Value<T>> preprocess(List<List<T>> data) {
        LinkedList<Value<T>> processed = new LinkedList<>();
        Value<T> target = null;
        for (int listid = 0; listid < data.size(); listid++) {
            ListIterator<Value<T>> valit = processed.listIterator();
            Iterator<T> datait = data.get(listid).iterator();
            while (datait.hasNext()) {
                final T toadd = datait.next();
                boolean found = false;
                while (valit.hasNext()) {
                    final int compare = (target = valit.next()).compareToValue(toadd);
                    if (compare >= 0) {
                        // we have a match, or gone past.
                        valit.previous();
                        found = true;
                        if (compare == 0) {
                            target.setIndex(listid, toadd);
                        } else {
                            Value<T> newtarget = new Value<>(toadd, Arrays.copyOf(target.getIndices(), width));
                            valit.add(newtarget);
                            newtarget.setIndex(listid, toadd);
                            if (newtarget != valit.previous()) {
                                throw new IllegalStateException("Bad math!");
                            }
                        }
                        break;
                    }
                    target.setIndex(listid, toadd);
                }
                if (!found) {
                    Value<T> newtarget = new Value<>(toadd, buildArray(clazz, width));
                    valit.add(newtarget);
                    newtarget.setIndex(listid, toadd);
                }
            }
        }
        return new ArrayList<>(processed);
    }

    @SuppressWarnings("unchecked")
    private static final <T> T[] buildArray(Class<T> clazz, int size) {
        return (T[])Array.newInstance(clazz, size);
    }

    public List<T> searchValues(T value) {
        Value<T> key = new Value<>(value, null);
        int pos = Collections.binarySearch(index, key);
        if (pos < 0) {
            pos = -pos - 1;
        }
        if (pos >= index.size()) {
            return Arrays.asList(buildArray(clazz, width));
        }
        return Arrays.asList(Arrays.copyOf(index.get(pos).getIndices(), width));
    }

}

MultiListMain.java

package listsearch;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class MultiListMain {

    public static void main(String[] args) {
        List<List<Integer>> lists = new ArrayList<List<Integer>>();

        List<Integer> list1 = new ArrayList<Integer>(Arrays.asList(3, 4, 6));
        List<Integer> list2 = new ArrayList<Integer>(Arrays.asList(1, 2, 3));
        List<Integer> list3 = new ArrayList<Integer>(Arrays.asList(2, 3, 6));
        List<Integer> list4 = new ArrayList<Integer>(Arrays.asList(1, 2, 3));
        List<Integer> list5 = new ArrayList<Integer>(Arrays.asList(4, 8, 13));

        lists.add(list1);
        lists.add(list2);
        lists.add(list3);
        lists.add(list4);
        lists.add(list5);

        MultiListIndex<Integer> search = new MultiListIndex<Integer>(
                Integer.class, lists);
        // System.out.println(dataInput);

        System.out.println(search.searchValues(0));
        System.out.println(search.searchValues(1));
        System.out.println(search.searchValues(2));
        System.out.println(search.searchValues(5));

    }

}

Output

[3, 1, 2, 1, 4]
[3, 1, 2, 1, 4]
[3, 2, 2, 2, 4]
[6, null, 6, null, 8]


A practical example of evenly distributing n lists into a single list I had previously asked about how to evenly distribute the items in n lists into a single list and was referred to this question.

I made a practical example of my solution for this in VBA for Excel, since my application for this was resorting my Spotify lists which can be easily pasted into Excel for manipulation. Assumptions are that you have a headerless worksheet (wsSource) of songs with columns A, B, C representing Artist, Song, SpotifyURI respectively, a "Totals" worksheet (wsTotals) containing the sum of songs for each Artist from wsSource sorted in descending order, and a "Destination" worksheet where the new list will be created.

Here's a link to the sample file.

Could I get some suggestions to improve this? I was going to get rid of the totals worksheet and have this portion done in code, but I have to go and I wanted to go ahead and put this out there.

Sub WeaveSort()

Dim wb As Workbook
Dim wsDest As Worksheet
Dim wsSource As Worksheet
Dim wsTotals As Worksheet
Dim i As Integer
Dim iLast As Integer
Dim iDest As Integer
Dim iSource As Integer
Dim iOldRow As Integer
Dim iNewRow As Integer
Dim dDiff As Double
Dim dDiffSum As Double

    Set wb = ThisWorkbook
    Set wsTotals = wb.Worksheets("Totals")
    Set wsSource = wb.Worksheets("Source")
    Set wsDest = wb.Worksheets("Dest")
    iLast = wsTotals.Range("A1").End(xlDown).Row - 1

    For i = 2 To iLast
        iSource = wsTotals.Range("B" & i).Value
        iDest = wsDest.Range("A99999").End(xlUp).Row

        If i = 2 Then
            wsDest.Range("A1:C" & iSource).Value2 = wsSource.Range("A1:C" & iSource).Value2
            wsSource.Range("A1:C" & iSource).Delete (xlShiftUp)
            GoTo NextI
        End If

        dDiff = iDest / iSource
        dDiffSum = 0
        iNewRow = 0

        For iOldRow = 1 To iSource
            dDiff = iDest / iSource
            dDiffSum = dDiffSum + dDiff
            iNewRow = Round(dDiffSum, 0)
            wsSource.Rows(iOldRow).Copy
            wsDest.Rows(iNewRow).Insert xlShiftDown
            iDest = iDest + 1
        Next iOldRow

        wsSource.Range("A1:C" & iSource).Delete (xlShiftUp)
NextI:
    Next i

End Sub

One word.... VELOCIRAPTORS.

You seem like a nice guy, I don't want the raptors to get you, so let's take Heslacher's advice and move this code outside of the loop.

    If i = 2 Then
        wsDest.Range("A1:C" & iSource).Value2 = wsSource.Range("A1:C" & iSource).Value2
        wsSource.Range("A1:C" & iSource).Delete (xlShiftUp)
        GoTo NextI
    End If

Since we'll handling the special case of the first row outside the loop, we need to change which row the loop starts on. While we're at it, let's get rid of that magic number and replace it with a variable. Note that I also replaced i with the more meaningful name row.

iSource = wsTotals.Range("B2").Value
wsDest.Range("A1:C" & iSource).Value2 = wsSource.Range("A1:C" & iSource).Value2
wsSource.Range("A1:C" & iSource).Delete (xlShiftUp)

Dim startRow As Long
startRow = 3
For row = startRow To iLast
    iSource = wsTotals.Range("B" & i).Value
    iDest = wsDest.Range("A99999").End(xlUp).Row

Now that the dreaded GoTo has been banished, let's see what else we can clean up.

There's no sense in assigning ThisWorkbook to a variable. There's no need to keep an extra reference to it in memory. Just do this. (Note that although I hate hungarian notation, I find using wb for workbook and ws for worksheet acceptable. The intention is clear to any VBA developer.)

Set wsTotals = ThisWorkbook.Worksheets("Totals")
Set wsSource = ThisWorkbook.Worksheets("Source")
Set wsDest = ThisWorkbook.Worksheets("Dest")

Your method of finding the last row has unpredictable results. Use this instead.

lastRow = wsTotals.Range("A" & wsTotals.Rows.Count).End(xlUp).row

Same deal with finding the destination row.

iDest = wsDest.Range("A" & wsDest.Rows.Count).End(xlUp).row

Applying most of Vogel612's naming suggestions, this is the code up to this point.

Sub WeaveSort()

    Dim wsDest As Worksheet
    Dim wsSource As Worksheet
    Dim wsTotals As Worksheet
    Dim row As Integer
    Dim lastRow As Integer
    Dim destinationRow As Integer
    Dim sourceRow As Integer
    Dim oldRow As Integer
    Dim newRow As Integer
    Dim difference As Double
    Dim differenceSum As Double

    Set wsTotals = ThisWorkbook.Worksheets("Totals")
    Set wsSource = ThisWorkbook.Worksheets("Source")
    Set wsDest = ThisWorkbook.Worksheets("Dest")

    lastRow = wsTotals.Range("A" & wsTotals.Rows.Count).End(xlUp).row - 1

    sourceRow = wsTotals.Range("B2").Value
    wsDest.Range("A1:C" & sourceRow).Value2 = wsSource.Range("A1:C" & sourceRow).Value2
    wsSource.Range("A1:C" & sourceRow).Delete (xlShiftUp)

    Dim startRow As Long
    startRow = 3
    For row = startRow To lastRow
        sourceRow = wsTotals.Range("B" & row).Value
        destinationRow = wsDest.Range("A" & wsDest.Rows.Count).End(xlUp).row

        difference = destinationRow / sourceRow
        differenceSum = 0
        newRow = 0

        For oldRow = 1 To sourceRow
            difference = destinationRow / sourceRow
            differenceSum = differenceSum + difference
            newRow = Round(differenceSum, 0)
            wsSource.Rows(oldRow).Copy
            wsDest.Rows(newRow).Insert xlShiftDown
            destinationRow = destinationRow + 1
        Next oldRow

        wsSource.Range("A1:C" & sourceRow).Delete (xlShiftUp)
    Next row

End Sub

We introduced a little duplication when we got rid of the raptors. Let's introduce a sub to delete the data from the source. To make things easy, we'll declare the different worksheets at the module level.

Option Explicit

Private wsDest As Worksheet
Private wsSource As Worksheet
Private wsTotals As Worksheet

Private Sub DeleteFromSource(ByVal row As Long)
    wsSource.Range("A1:C" & row).Delete (xlShiftUp)
End Sub

Changing WeaveSort to:

Sub WeaveSort()

    Dim row As Integer
    Dim lastRow As Integer
    Dim destinationRow As Integer
    Dim sourceRow As Integer
    Dim oldRow As Integer
    Dim newRow As Integer
    Dim difference As Double
    Dim differenceSum As Double

    Set wsTotals = ThisWorkbook.Worksheets("Totals")
    Set wsSource = ThisWorkbook.Worksheets("Source")
    Set wsDest = ThisWorkbook.Worksheets("Dest")

    lastRow = wsTotals.Range("A" & wsTotals.Rows.Count).End(xlUp).row - 1
    sourceRow = wsTotals.Range("B2").Value

    wsDest.Range("A1:C" & row).Value2 = wsSource.Range("A1:C" & row).Value2
    DeleteFromSource sourceRow

    Dim startRow As Long
    startRow = 3
    For row = startRow To lastRow
        sourceRow = wsTotals.Range("B" & row).Value
        destinationRow = wsDest.Range("A" & wsDest.Rows.Count).End(xlUp).row

        difference = destinationRow / sourceRow
        differenceSum = 0
        newRow = 0

        For oldRow = 1 To sourceRow
            difference = destinationRow / sourceRow
            differenceSum = differenceSum + difference
            newRow = Round(differenceSum, 0)
            wsSource.Rows(oldRow).Copy
            wsDest.Rows(newRow).Insert xlShiftDown
            destinationRow = destinationRow + 1
        Next oldRow

        DeleteFromSource sourceRow
    Next row

End Sub

Ultimately leaving this clever solution unchanged, but much more understandable.


Inventory - how to handle items? I've been designing a game server, where a player has inventory, bank, and other features with items.

There are some definitions of what item can be in my game:

Item is either stackable or not stackable.
If item is stackable, item's amount can be up to 100 per stack.

So since each item can have an amount if it's stackable, my regular idea was having two arrays, int[] itemIds and int[] amount, where itemIds holds the IDs of the items, and amount holds the amount for item in that index.

I must keep the indexes the same, because when the client selects an item, it sends over the slot id he clicked on.

But then I thought, I could just do an Item object, with the tools in that class for that item, and have a private property isStackable and amount.

But then my mate said the following thing:

  If you want to change some value of an Item object, you'll need to
  call a new construct and then get the old value then add the new value
  and then pass it to the construct. with primitives you can do that
  instant, which gives also performance positives

But then I thought of an idea, have a class named ItemStorage which will contain two arrays, items and amount like I described above:

public class ItemStorage {

    private int[] items;
    private int[] amount;

    public void setItems(int[] a, int[] b) {
        this.items = a;
        this.amount = b;
    }

    public int[] getItems() {
        return this.items;
    }

    public int[] getAmount() {
        return this.amount;
    }

}

I have a class called Inventory, it's my domain object for handling the inventory, it extends the interface Storage<ItemStorage>.

public interface Storage<T> {
    public T getStorage();
}

Now, since it implements it, the class must have the method getStorage() which will return ItemStorage, so to modify items, you do inventory.getStorage().getItems()[0] = 50;

But I am not sure about it, I am sure that List<Item> or Item[] will be much friendly, but I am not really sure why my mate dislikes it.

What do you think about my idea, and his opinion?  Do you have another idea I can use for this?

How the items are handled in the game

The game contains over 25,000 items, each item is unique. The game uses each item as an id to represent it's uniqueness. The first item out of that item list, will have an id of 1.

Lets look at item 1445, It's a sword.

When the client wants to wear the item, he has to click on it in his inventory. This click will send a packet to the server, with the slot id he clicked in the inventory.

There's an example, similar to my inventory:

Let's base on that picture.

You see 16 slots in that picture, starting from 0 to 15. Player clicks on slot 4, and the client will pass the slot 4 to the server. The server will check through the inventory, what item is sitting on slot 4 in the array, it will see the amulet with id 555 for example (like in the picture).

Therefore I need slots to be supported in my design.

About your existing code
There's only two things I have to complain about in your current ItemStorage.

public void setItems(int[] a, int[] b) See those parameter names? a and b. They represent completely different things, they should have been named itemIds and amount.

Item(id)s in one array, and amount in another. What if each item would have enchantments? (which, for the sake of simplicity, also is an int for now) Would you add private int[] enchantments? Then what if you would add damage for items? Or other properties? It's a bad bad idea to store it like this. It's not all about performance, it's also about extensibility.

But then I thought, I could just do an Item object, with the tools in that class for that item, and have a private property isStackable and amount.

Yes, yes, YES!!

If you want to change some value of an Item object, you'll need to call a new construct and then get the old value then add the new value and then pass it to the construct. with primitives you can do that instant, which gives also performance positives

No, no, NO!!
If by "construct" you mean constructor, then no - use a setSomeValue(int newValue) method instead. If by "construct" you mean a set method like setSomeValue(int newValue), then sure, if you want to change some value you'd have to do item.setSomeValue(item.getSomeValue() + 3); but that's fine. This won't affect performance much. Java even use a Just in Time compiler to boost the performance of such calls.
If you want to you could even add a method like this to your Item class:
public void changeSomeValue(int change) {
    this.someValue += change;
}

This is especially useful for values you change often without caring much about the actual value at the moment, such as changing a player's HP. (I personally often call methods such as player.heal(4) or player.damage(6))
Designing an Inventory and Items
I think you should look at how the API of Bukkit (the Minecraft server) has been made. Bukkit have a class for ItemStack and also an interface for Inventory. You can probably find a lot of inspiration from Bukkit, although you probably won't even need half of it.
Here are some example classes that I think you could use:
public class ItemStorage {
    private Item[] items;
    private SomeObject owner; // Player, Chest, whatever... if you need it.
}

public enum ItemType {
    WEAPON(1, 1), SHIELD(2, 1), MONKEY(3, 4), MUG(4, 42), FISH(5, 64);
    // You never know what you might need in your game

    private final int id;
    private final int maxStackSize;
    private ItemType(int id, int maxStackSize) {
        this.id = id;
        this.maxStackSize = maxStackSize;
    }
}

public class Item {
    private ItemType type;
    private int amount;
}

If you want to somehow dynamically add items, then an enum won't work well for you, use an int in that case. Although I think an enum is good enough for now.


Listing the representations of integer M as a sum of three squares M = x^2 + y^2 + z^2 I am trying a number as the sum of 3 squares.  For my particular project I will require a complete list since the next step is to compute nearest neighbors (here).

In any case we'd like to find \$ M = x^2 + y^2 + z^2 \$ in all possible ways.  Here's what I wrote in Python:

M = 1062437
m = int(np.sqrt(M))

x = np.arange(int(np.sqrt(m)))
y = M - x**2

sq = [(i,j, int(np.sqrt(M - i**2 - j**2))) for i in range(m) for j in range(m) if i**2 + j**2 in y]

This is easy to explain in pseudocode.

build a list of remainders \$\{ M - k^2 : 0 \leq k \leq \sqrt{M} \}\$
loop over all possible \$0 < i,j < \sqrt{M} \$ check that \$i^2 + j^2 \$ is in the list

This is one of several possible strategies.  In my case, I don't even need a fixed M it could be a range, such as 10**6 < M < 10**6 + 10*4.

I do not  mind looping over values of M at this point. Can we recycle our computations somehow?

You might be interested in this question.

While looking for solution to

$$ x^{2} + y^{2} + z^{2} = M $$

you can assume

$$ x  \leq y  \leq z$$.

and perform rotation if needed afterward.

This is good because, you can start iterating over x in a smaller range. You know that

$$ M = x^{2} + y^{2} + z^{2} \ge x^{2} + x^{2} + x^{2}  = 3 * x^{2}$$

, therefore $$x \le sqrt(M / 3)$$

Similarly, when iterating over y, you can start at xand go up to a smaller higer bound because we have :

$$ M = x^{2} + y^{2} + z^{2} \ge x^{2} + y^{2} + y^{2}  = x^{2} + 2 * y^{2}$$

, therefore $$x \le y \le sqrt((M - x^{2})/ 2)$$

Then, when once x and y are fixed, you don't need to iterate over z : just compute

$$ z = sqrt(M - x^{2} - y^{2}) $$

and check that it is indeed an integer.

Here's a proof-of-concept piece of code : I've altered your code a bit to be able to compare the results you originally had with the one I get :

sq = set(tuple(sorted([i,j, int(np.sqrt(M - i**2 - j**2))])) for i in range(m) for j in range(m) if i**2 + j**2 in y)
print(len(sq))

sol = []
lim_x = int(np.sqrt(M/3))
for x in range(1 + lim_x):
    rem_x = M - x*x
    lim_y = int(np.sqrt(rem_x / 2))
    for y in range(x, 1 + lim_y):
        rem_y = rem_x - y*y
        z = int(np.sqrt(rem_y))
        if z*z == rem_y:
            assert x <= y <= z
            assert x*x + y*y + z*z == M
            sol.append((x, y, z))
print(len(sol))

assert all(s in sol for s in sq)

You have 26 different ordered solutions out of the 290 I found (in a smaller time range).

Finally, you might be able to adapt the equations above (and the code written from it) if you want to handle ranges of M values.