Handling an e-commerce order form I wrote this form object to handle an e-commerce order form. My areas of concern are the verbosity of the populate method, and the repetition of the validations. I omitted some address fields and validations for brevity.

class OrderForm
  include ActiveModel::Model

  def persisted?
    false
  end

  attr_accessor :params

  delegate :email, :bill_to_shipping_address, to: :order
  delegate :name, :street, :city, :state, :post_code, to: :shipping_address, prefix: :shipping
  delegate :name, :street, :city, :state, :post_code, to: :billing_address,  prefix: :billing

  validates :email, length: { maximum: 60 }, email_format: true

  validates :shipping_name, :shipping_street, :shipping_city, presence: true
  validates :shipping_post_code, numericality: { only_integer: true }

  validates :billing_name, :billing_street, :shipping_city, presence: true, unless: -> { bill_to_shipping_address }
  validates :billing_post_code, numericality: { only_integer: true }, unless: -> { bill_to_shipping_address }

  def initialize(item, params = nil, customer = nil)
    @item, @params, @customer = item, params, customer
  end

  def submit
    populate
    if valid?
      order.save!
      true
    else
      false
    end
  end

  def order
    @order ||= @item.build_order do |order|
      order.customer = @customer if @customer
    end
  end

  private

  def shipping_address
    @shipping_address ||= order.build_shipping_address
  end

  def billing_address
    @billing_address ||= order.build_billing_address
  end

  def populate
    order.email = params[:email]
    order.bill_to_shipping_address = params[:bill_to_shipping_address]

    shipping_address.name          = params[:shipping_name]
    shipping_address.street        = params[:shipping_street]
    shipping_address.city          = params[:shipping_city]
    shipping_address.state         = params[:shipping_state]
    shipping_address.post_code     = params[:shipping_post_code]

    if order.bill_to_shipping_address?
      billing_address.name          = params[:shipping_name]
      billing_address.street        = params[:shipping_street]
      billing_address.city          = params[:shipping_city]
      billing_address.state         = params[:shipping_state]
      billing_address.post_code     = params[:shipping_post_code]
    else
      billing_address.name          = params[:billing_name]
      billing_address.street        = params[:billing_street]
      billing_address.city          = params[:billing_city]
      billing_address.state         = params[:billing_state]
      billing_address.post_code     = params[:billing_post_code]
    end
  end
end

The Order object has billing_address and one shipping_address objects. They both inherit from an Address.

Here's the controller:

  def new
    @order_form = OrderForm.new(@item)
  end

  def create
    @order_form = OrderForm.new(@item, params[:order], current_user)
    if @order_form.submit
      # process payment
    else
      render 'new'
    end
  end

I was about to propose some changes to your code to reduce the line-count (and get rid of those @cached objects, bad idea IMHO), but I think it's the big picture what it's failing here. Some notes:

You are creating an OrderForm abstraction when in fact Rails gives all the infrastructure you need to write this in Order in a simple and very concise way.
All those validations belong to each model, validating them from an outside class is not a good practice.
Use accepts_nested_attributes_for (in models) + fields_for (in views) so the associations are automatically (and transparently) handled by Rails.

[EDIT] Regarding the non-cached approach, a glimpse of what I'd write:

order = @item.order || build_order
order.customer = @customer
shipping_address = order.shipping_address || order.build_shipping_address
shipping_address.attributes = params.slice(:shipping_name, :shipping_street,...)


C++ split function (equivalent to python string.split()) I wrote the equivalent to the split() in python for C++.  I would like to know how I can make it better:

static constexpr auto whitespace = " \t\v\r\n\f";
static constexpr auto npos = std::string::npos;

std::vector<std::string> split(std::string_view str, std::string_view sep = "", std::size_t maxsplit = std::numeric_limits<std::size_t>::max())
{
    std::vector<std::string> result;
    if (sep.empty())
    {
        for (std::size_t start = str.find_first_not_of(whitespace), splits = 0; start != npos; ++splits)
        {
            if (auto end = str.find_first_of(whitespace, start); end != npos && splits < maxsplit)
            {
                result.emplace_back(str.substr(start, end - start));
                start = str.find_first_not_of(whitespace, end);
            }
            else
            {
                result.emplace_back(str.substr(start));
                break;
            }
        }
    }
    else
    {
        for (std::size_t start = 0, splits = 0; start < str.size(); ++splits)
        {
            if (auto end = str.find(sep, start); end != npos && splits < maxsplit)
            {
                result.emplace_back(str.substr(start, end - start));
                start = end + sep.size();
            }
            else
            {
                result.emplace_back(str.substr(start));
                break;
            }
        }
    }
    return result;
}

I tried to keep all variables as local as possible and use standard algorithms.

I would use a const reference here:

static constexpr auto npos = std::string::npos;

// I would do this.
static constexpr auto const& npos = std::string::npos;

No point in duplicating storage.
Also declaring global variables is a frowned upon. Especially if they are in the global namespace. At a minimum these should be inside your own namespace, but preferably they should be inside the function.

Dyslexia strikes.
I read the next part to have std::string not std::string_view.
Prefer to pass parameters (especially those that are not mutated) by const reference. This prevents a copy.

std::vector<std::string> split(std::string_view const& str, std::string_view const& sep = "", std::size_t maxsplit = std::numeric_limits<std::size_t>::max())

                                               ^^^^^^^^                     ^^^^^^^^

I don't like this:

if (auto end = str.find_first_of(whitespace, start); end != npos && splits < maxsplit)

I did find that this is a C++17 extension. But it puts way too much information on one line. Personally I would split this across two lines.

auto end = str.find_first_of(whitespace, start);
if (end != npos && splits < maxsplit)

As a side note splitting the line by white space can be written more simply as:

 std::stringstream  stream(str);
 return std::vector<std::string>(std::istream_iterator<std::string>(stream),
                                 std::istream_iterator<std::string>());


Source formatting of Markdown Table I use MultiMarkdown tables more than I thought I ever would. In my .md files, I would like the source table code to be neat, evenly spaced, and resemble (in an alignment sense) the final HTML rendered table.

Solution

Select the source of the table, press a hotkey on my computer, replace the untidy table with the tidy table.

Notes:

This script (macro?) is for my own use; I will always write the source table the same way
The script is written for Python 2.7
I use a Mac app that allows me to assign a hot key to an action (e.g. a script), and the following seems to work with that at the moment!

Example

I write something like the following:

| Header 1 | Header 2 | ... | Header m |
| :--- | :--: | :--: | --: |
| $a_{11}$ | $a_{12}$ | ... | $a_{1m}$ |
| ... | ... | ... | ... |
| $a_{n1}$ | $a_{n2}$ | ... | $a_{nm} |

select it, press my hot key, and it is replaced by something like this (in the source):

| Header 1 | Header 2 | ...  | Header m |
| :---     |   :--:   | :--: |      --: |
| $a_{11}$ | $a_{12}$ | ...  | $a_{1m}$ |
| ...      |   ...    | ...  |      ... |
| $a_{n1}$ | $a_{n2}$ | ...  | $a_{nm}$ |

As mentioned, the script seems to work.

How can I improve the script?

I would really appreciate some criticism of the script I've written. I am not a coder, but try on occasion to write little tool type things like this.

The Script

#!/usr/bin/env python

"""MMD Table Formatter.

Silly script that takes a MMD table as a
string and returns a tidied version of the table
"""

import sys
import StringIO

query = sys.argv[1]

# For "cleaned" table entries:
rows = []

# This NEEDS TO BE CLOSED AFTER!!
s_obj = StringIO.StringIO(query)
# Clean the entries:
for line in s_obj:
    l = line.split('|')
    rows.append([entry.strip() for entry in l])`enter code here`
# CLOSE
s_obj.close()

# Max length of each "entry" is what we'll use
# to evenly space "columns" in the final table
cols = zip(*rows)
col_widths = []
for columns in cols:
    row_widths = map(lambda x: len(x), columns)
    col_widths.append(max(row_widths))

# Let's align entries as per intended formatting.
# Second line of input string contains alignment commmands:
# ":---"    left aligned
# "---:"    right aligned
# ":--:"    centered (also accepts "---")
alignment = []
for r in rows[1]:
    if r.startswith(":") and not r.endswith(":"):
        alignment.append("lalign")
    elif r.endswith(":") and not r.startswith(":"):
        alignment.append("ralign")
    else:
        alignment.append("centered")

# Prepare for output string:
out = []
for row in rows:
    for entry_and_width in zip(row, col_widths, alignment):
        if entry_and_width[1] == 0:
            continue
        if entry_and_width[2] == "centered":
            outstring = "| " + entry_and_width[0].center(entry_and_width[1]) + ' '
            out.append(outstring)
        if entry_and_width[2] == "lalign":
            outstring = "| " + entry_and_width[0].ljust(entry_and_width[1]) + ' '
            out.append(outstring)
        if entry_and_width[2] == "ralign":
            outstring = "| " + entry_and_width[0].rjust(entry_and_width[1]) + ' '
            out.append(outstring)
    out.append("|\n")

query = "".join(out)

sys.stdout.write(query)

As far as I can see, there is no need for the StringIO (?).
You can just use query.split('\n'). Regardless, that for loop can be condensed into a list comprehension:

rows = [[el.strip() for el in row.split('|')] for row in query.splitlines()]

If the StringIO is really needed, I would use with..as:

with StringIO.StringIO(query) as s_obj:
    rows = [[el.strip() for el in row.split('|')] for row in s_obj]

For the column widths you can use that len is already a function, so there is no need for lambda x: len(x). This way you can also inline all of it into one list comprehension:

col_widths = [max(map(len, column)) for column in zip(*rows)]

For the alignments, I would define a function that returns the alignment, given the content of a cell. First I had this function return your strings. But then I realized that you are already using entry.ljust further down. I then changed it to return the function to use here. Note that str.ljust("ab", 2) and "ab".ljust(2) are equivalent, so later we just call align(entry, width).

def get_alignment(cell):
    """"
    :---"    left aligned
    "---:"    right aligned
    ":--:"    centered (also accepts "---"), default
    """
    if cell.startswith(":") and not cell.endswith(":"):
        return str.ljust
    elif cell.endswith(":") and not cell.startswith(":"):
        return str.rjust
    return str.center

To get all alignments, we just use map again:

alignments = map(get_alignment, rows[1])

Finally, the output part. Since you already build a nice tuple with zip, you should use tuple unpacking to give the element readable names. entry_and_widths is a confusing name, especially since it also contains the alignments!

Here we can now get rid of most of your code, since it boils down to:

entry = align(entry, width)
out.append("| {} ".format(entry)

Here I used str.format to make it a bit easier and avoid the costly string addition. Note that we could also use str.format to do the adjusting for us (using e.g. "{>2}".format(entry) instead of "{}".format(str.rjust(entry, 2)), but that would mean nesting formats, which starts to get ugly very quickly.

I also use the fact that 0 compares to False to make the code skipping a column if it is empty shorter.

out = []
for row in rows:
    for entry, width, align in zip(row, col_widths, alignments):
        if not width:
            continue
        out.append("| {} ".format(align(entry, width)))
    out.append("|\n")
query = "".join(out)

Final code:

#!/usr/bin/env python

"""MMD Table Formatter.

Silly script that takes a MMD table as a
string and returns a tidied version of the table
"""
import sys

def get_alignment(cell):
    """"
    :---"    left aligned
    "---:"    right aligned
    ":--:"    centered (also accepts "---"), default
    """
    if cell.startswith(":") and not cell.endswith(":"):
        return str.ljust
    elif cell.endswith(":") and not cell.startswith(":"):
        return str.rjust
    return str.center

query = sys.argv[1]

# For "cleaned" table entries:
rows = [[el.strip() for el in row.split('|')] for row in query.splitlines()]

# Max length of each "entry" is what we'll use
# to evenly space "columns" in the final table
col_widths = [max(map(len, column)) for column in zip(*rows)]

# Let's align entries as per intended formatting.
# Second line of input string contains alignment commmands:
alignments = map(get_alignment, rows[1])

# Prepare for output string:
out = []
for row in rows:
    for entry, width, align in zip(row, col_widths, alignments):
        if not width:
            continue
        out.append("| {} ".format(align(entry, width)))
    out.append("|\n")

query = "".join(out)

sys.stdout.write(query)


Removing repetitive code in ASP.NET MVC Controller actions? I want to keep things DRY, so I'm trying to figure out: Is it possible to remove repetitive parameters and code from ASP.NET MVC Controller actions?  eg given these Actions:

public ActionResult List(string key, string signature) { GetSignature(key); // etc }
public ActionResult Add(string key, string signature) { GetSignature(key); // etc }
public ActionResult Update(string key, int id, string signature) { GetSignature(key, id.ToString()); // etc}
public ActionResult Delete(string key, int id, string signature) { GetSignature(key, id.ToString()); // etc}
public ActionResult Action1(string key, string x, string y, string z, string signature) { GetSignature(key, x, y, z);//etc}

Is there any way to refactor out the repetitive key and signature parameters, and the call to GetSignature()?

There are two obstacles to refactoring I can think of:

It's MVC so the Actions are the first port of call - it may not be possible to have a refactored method that is called before the Action.
The GetSignature() method is called using all of the Action's parameters, so it may be difficult to refactor.

I like the direction Scott started but would recommend to take it another layer down so that the GenerateSignature method does not have to do discovery with the thing object (Do I have an Id or do I have string X/Y/Z or do I only have Key/Signature, etc).

public class MySignatureCreateRead
{
    public string Key { get; set; }
    public string Signature { get; set; }
}

public class MySignatureUpdateDelete : MySignatureCreateRead
{
    public int Id { get; set; }
}

public class MySignatureAction : MySignatureCreateRead
{
    public string X { get; set; }
    public string Y { get; set; }
    public string Z { get; set; }
}

Which puts the ActionResults like:

public ActionResult List(MySignatureCreateRead signature) { GetSignature(signature); /* etc */ }
public ActionResult Add(MySignatureCreateRead signature) { GetSignature(signature); /* etc */ }
public ActionResult Update(MySignatureUpdateDelete signature) { GetSignature(signature); /* etc */ }
public ActionResult Delete(MySignatureUpdateDelete signature) { GetSignature(signature); /* etc */ }
public ActionResult Action1(MySignatureAction signature) { GetSignature(signature); /* etc */ }

This also gives you objects to strongly bind views to.


Determine column averages I'm trying to write terse Perl code to calculate the average of each column in a file. The file can have >= 1 columns.

use strict;
use warnings;
use List::Util qw/sum/;

my @aoc = ();
while (<DATA>) {
  my $c = 0;
  push @{ $aoc[$c++] }, $_ foreach (split);
}
print join (" ", map { sum(@$_)/@$_ } @aoc), $/;

#output: 2.66666666666667 26.6666666666667 266.666666666667

__DATA__
1 10 100
3 30 300
4 40 400

Can this code be any terser? I tried to remove while (<DATA>) by replacing split with map { $c = 0; split } <DATA> but this does not reset $c to 0 for each loop.

Any ideas on how to reduce this code further?

Following @mpapec suggestion, your code becomes (in pseudocode):

@columns = transpose(map (split " ", readlines("filename")))
print join (" ", map { sum(@$_)/@$_ } (columns), $/;

Where readlines reads a file into an array of lines.

To find the transpose function you will have to follow the link @mpapec gave you.

Your code is now down to just two lines and uses functional programming, that seems to be encouraged in Perl, so it looks better to me.


List subclasses of a class Description

Given a class, return all its subclasses (recursively).
As you can see I've eliminated recursion using a stack.

What I want reviewed

Is there a better way to do this?
How can I make this code more generic and easier to use?
Is it pythonic?
Better way to eliminate recursion?

Code

def all_subclasses(cls):

    if cls == type:
        raise ValueError("Invalid class - 'type' is not a class")

    subclasses = set()

    stack = []
    try:
        immediate_subclasses = cls.__subclasses__()
    except (TypeError, AttributeError) as ex:
        raise ValueError("Invalid class" + repr(cls)) from ex

    for subclass in immediate_subclasses:
        stack.append(subclass)

    while stack:
        sub = stack.pop()
        subclasses.add(sub)
        try:
            sub_subclasses = sub.__subclasses__()
        except (TypeError, AttributeError) as _:
           continue
        if sub_subclasses:
            stack.extend(sub_subclasses)

    return list(subclasses)

Tests

import unittest

from class_util import all_subclasses

def names(classes):
    return sorted([cls.__name__ for cls in classes])

class A:
    @classmethod
    def all_subclasses(cls):
        return all_subclasses(cls)

class B(A):
    pass

class C(B):
    pass

class D(C):
    pass

class E(C):
    pass

class F(E, C):
    pass

class AllSublassesTestCase(unittest.TestCase):
    def test_nested_classes(self):
        self.assertEqual(names(A.all_subclasses()), ["B", "C", "D", "E", "F"])

    def test_work_with_buitins(self):
        self.assertTrue(names(all_subclasses(dict)))
        self.assertTrue(names(all_subclasses(tuple)))
        self.assertTrue(names(all_subclasses(list)))

    def test_value_error_is_raised_on_invalid_classes(self):
        self.assertRaises(ValueError, all_subclasses, type)
        self.assertRaises(ValueError, all_subclasses, "")
        self.assertRaises(ValueError, all_subclasses, None)
        self.assertRaises(ValueError, all_subclasses, [])

if __name__ == "__main__":
    unittest.main()

While working on the stack you are using stack.extend. You can also use this in the part where you add the immediate subclasses.
There is no need to check if a list is empty before using extend, if it is empty it will just do nothing.
If you don't need the exception, just don't catch it with as _.
Not sure if you should be doing if cls is type instead of if cls == type.

def all_subclasses(cls):

    if cls == type:
        raise ValueError("Invalid class - 'type' is not a class")

    subclasses = set()

    stack = []
    try:
        stack.extend(cls.__subclasses__())
    except (TypeError, AttributeError) as ex:
        raise ValueError("Invalid class" + repr(cls)) from ex

    while stack:
        sub = stack.pop()
        subclasses.add(sub)
        try:
            stack.extend(sub.__subclasses__())
        except (TypeError, AttributeError):
           continue

    return list(subclasses)

One way to optimize this further is to make sure you don't visit a class multiple times:

    while stack:
        sub = stack.pop()
        subclasses.add(sub)
        try:
            stack.extend(s for s in sub.__subclasses__() if s not in subclasses)
        except (TypeError, AttributeError):
           continue

This should prevent having to visit (almost) every class twice with convoluted hierarchies like this:

    A
  /   \
 B     C
  \   /
    D
 / | | \
E  F G ...


General way to add persistence to a class in Python The idea here is to write a function that gives you back a persistent version of a class that you supply. So you if you run

PersistentList = make_persistent(list, "PersistentList", ['append', 'extend', 'insert', 'pop', 'remove', 'reverse', 'sort'])

You will get back a class that behaves like a list but automatically persists itself after each operation. So given this test code:

import time
def get_hhmmss():
    return time.strftime('%H:%M:%S', time.localtime())

def test_list():
    PersistentList = make_persistent(list, "PersistentList", get_mutators(list, tuple))
    filepath = os.path.expanduser("~/Desktop/test_list")
    with contextlib.closing(PersistentList(filepath)) as pl:
        pl.append(get_hhmmss())
        pl += ['foo']
        pl += ['bar']
        print(pl)

def test_set():
    PersistentSet = make_persistent(set, "PersistentSet", get_mutators(set, frozenset))
    filepath = os.path.expanduser("~/Desktop/test_set")
    with contextlib.closing(PersistentSet(filepath)) as ps:
        ps.add(get_hhmmss())
        ps.add('spam')
        print(sorted(ps))

You can get this output:

>>> test_set()
['21:38:27', 'spam']
>>> test_set()
['21:38:27', '21:38:31', 'spam']
>>> test_set()
['21:38:27', '21:38:31', '21:38:34', 'spam']
>>> test_list()
['21:38:39', 'foo', 'bar']
>>> test_list()
['21:38:39', 'foo', 'bar', '21:38:43', 'foo', 'bar']
>>> test_list()
['21:38:39', 'foo', 'bar', '21:38:43', 'foo', 'bar', '21:38:47', 'foo', 'bar']

Here's the code that accomplishes it. I ask in part because I'm tempted to try to share this but assume that if it were a reasonable way to do things it would be out there by now.

import pickle, errno, os, functools, contextlib

def touch_new(filepath):
    "Will fail if filepath already exists, or if relevant directories don't already exist"
    os.close(os.open(filepath, os.O_WRONLY | os.O_CREAT | os.O_EXCL))

def get_mutators(mutable_class, frozen_class):
    "Convenience function for identifying mutators. Methods such as __iadd__ won't wrap properly so this function throws them out; you'll need to call `_save` or `close` to sync after such operations"
    def qualifying_methodname(methodname):
        return not(methodname.startswith("__") and methodname.endswith("__"))
    return sorted(mn for mn in set(dir(mutable_class)) - set(dir(frozen_class)) if qualifying_methodname(mn))

# Inspiration: http://stackoverflow.com/a/9449852/2829764
def make_persistent(original_class, new_classname, mutator_methodnames):
    class NewClass(original_class):
        def __init__(self, filepath):
            self._filepath = filepath
            self._closed = False
            try:
                with open(filepath, "rb") as ifile:
                    loaded = pickle.load(ifile)
                if type(loaded) != original_class: # Don't even allow subclasses
                    raise TypeError("{} exists but does not contain a {}".format(filepath, original_class))
                original_class.__init__(self, loaded)
            except IOError as ioe:
                if ioe.errno != errno.ENOENT:
                    raise
                touch_new(filepath)
                original_class.__init__(self)
            for attr in mutator_methodnames:
                setattr(self, attr, self._autosave(getattr(self, attr)))
        def _autosave(self, func):
            @functools.wraps(func)
            def _func(*args, **kwargs):
                if self._closed:
                    raise ValueError("Invalid operation on closed "+self.__class__)
                ret = func(*args, **kwargs)
                self._save()
                return ret
            return _func
        def _save(self):
            pickle.dump(original_class(self), open(self._filepath, "wb"))
        def close(self):
            if not self._closed:
                self._save()
                self._closed = True
    NewClass.__name__ = new_classname
    return NewClass

No time to do a proper review, but some quick notes:

There are no docstrings! How am I supposed to use your code?
Doesn't follow the Python style guide (PEP8): in particular, line lengths are too long, so we have to scroll horizontally to read it here.
You should use super to call superclass methods, otherwise your code won't play nicely with multiple inheritance.
This relies on original_class.__init__ taking one optional argument which is copied to self. This seems rather inflexible: it works for some built-in constructors like list but it is not very general. It would be better to leave __init__ alone and use a factory function to do the construction.
It seems clumsy to have to specify the names of all the methods that you need. Python has the tools (dir, __getattribute__, etc.) to avoid this.
Copying attributes from one object to another works for methods but not for properties.
It would be better to actually use the context manager protocol (__enter__ and __exit__) and not contextlib.closing, which requires the object to have a close method which might conflict with original_class.close.
Consider using three-argument type instead of making a class with the wrong name and then rewriting its __name__ attribute.
At the moment you write the object to disk after every change, which is costly. Better to postpone this until the object is finished with.
Consider saving the object via the __del__ method.
Test cases should be organized into unit tests so they are runnable via the unittest module.
Consider catching FileNotFoundError instead of catching IOError and then re-raising if the error code is not ENOENT.


Calculating the 'binary gap' of a number Inspired by this question and therefore this one I decided that clearly the best most readable way to solve this is with Regular Expressions.

A binary gap within a positive integer N is any maximal sequence of consecutive zeros that is surrounded by ones at both ends in the binary representation of N.
For example, number 9 has binary representation 1001 and contains a binary gap of length 2. The number 529 has binary representation 1000010001 and contains two binary gaps: one of length 4 and one of length 3. The number 20 has binary representation 10100 and contains one binary gap of length 1. The number 15 has binary representation 1111 and has no binary gaps.
For example, given N = 1041 the function should return 5, because N has binary representation 10000010001 and so its longest binary gap is of length 5.

I decided to remove the limit of positive numbers only.
Here's the code:
public static int ComputeLargestBinaryGap(int value)
{
    var regexp = new Regex("(?<=1)(0+)(?=1)");
    var valueAsBinary = Convert.ToString(value, 2);
    return
        regexp.Matches(valueAsBinary)
            .Cast<Match>()
            .Select(m => m.Value)
            .DefaultIfEmpty(string.Empty)
            .Max(s => s.Length);
}

You'll notice that this can be trivially extended to also work for long or any number representation you can convert to a string binary representation.
I appreciate that's a pretty small amount of code to review but would be intrigued as to whether there's anything I could improve.
For the record, my first actual solution was bit shifting an unsigned int but I stand by the Regular Expression solution.

I think using Regex will be too much for this little task. A simple split by 1s should suffice.

public static int ComputeLargestBinaryGap2(int value)
{
    return Convert
            // convert to binary
            .ToString(value, 2)
            // remove leading and trailing 0s, as per requirement
            .Trim('0')
            // split the string by 1s
            .Split(new [] { '1' })
            // find the length of longest segment
            .Max(x => x.Length);
}

We can also solve the problem from a mathematical approach, with logical operators and bit-shiftings :

public static int ComputeLargestBinaryGap3(int value)
{
    // casting it to uint while keeping the same binary value, like reinterpret_cast<unsigned int>
    //var unsigned = BitConverter.ToUInt32(BitConverter.GetBytes(value), 0);
    var unsigned = unchecked((uint)value);
    // flag used to ignore counting trailing 0's
    var pastTrailing0 = false;

    int max = 0, count = 0;
    while(unsigned > 0)
    {
        if ((unsigned & 1) == 1)
        {
            if (count > max)
                max = count;

            count = 0;
            pastTrailing0 = true;
        }
        else if (pastTrailing0)
        {
            count++;
        }

        unsigned = unsigned >> 1;
    }

    return max;
}

EDIT: After this answer was posted, OP updated on his definition of "best", which leans toward readability. I'll add my comments on his code here :

There is no benefit in declaring the variable, regexp , for the regular express, without giving a meaningful name to it, other than stating the obvious. It should be renamed to binaryGap.
valueAsBinary is a quite misleading name. One would think it the value stored in the binary format, which is pretty much how every number is stored... However, it stores the binary string of value. Therefore, it should be renamed to binaryString, or valueAsBinaryString.
The Match class already have a Length property that could be used instead of the long way match.Value.Length. This property is cached, so we are not re-measuring the length of the string, as you can see here.

And, here is the final result :

public static int ComputeLargestBinaryGap(int value)
{
    var binaryGap = new Regex("(?<=1)(0+)(?=1)");
    var binaryString = Convert.ToString(value, 2);
    return
        binaryGap.Matches(binaryString)
            .Cast<Match>()
            .Select(m => m.Length)
            .DefaultIfEmpty(0)
            .Max();
}


IService or defining IProductService, ICustomerService As per tutorial here, I have the following classes:
(In reference to this tutorial's section)
namespace Supermarket.API.Domain.Services
{
    public interface ICategoryService
    {
         Task<IEnumerable<Category>> ListAsync();
    }
}

IProductService.cs
namespace Supermarket.API.Domain.Services
{
    public interface IProductService
    {
         Task<IEnumerable<Product>> ListAsync();
    }
}

ICustomerService.cs
namespace Supermarket.API.Domain.Services
{
    public interface ICustomerService
    {
         Task<IEnumerable<Customer>> ListAsync();
    }
}

I have about 10 Domains that I want to have the same interface as above (i.e. the service ability to GET items for e.g. Customer / Product / OrderItem / Order)
In view of the DRY principle, I am thinking of changing to the below:
public interface IService<T>
{
    /// <summary>
    /// Gets a collection of entities
    /// </summary>
    /// <returns></returns>
    Task<IEnumerable<T>> ListAsync();
}

Whereby at implementation time, it looks like the below:
public class CustomerService : IService<Customer>
{
    public async Task<IEnumerable<Customer>> ListAsync()
    {
        var entities =  await // Imeplementation of DAL to follow

        return entities;
    }
}

None of the tutorials that I have read so far does anything similar to keep the interface DRY. Is this a good idea? Thanks.

It makes perfect sense to define such generic interface as long as there is a generic consumer that can do some useful work with any of the implementations.
And I suppose there is in this case.
If I were to guess, I'd say the tutorial did not include generics just because that is a feature that might not be familiar to the audience that the tutorial was pointing at.
As @Hayden already mentioned, IService<T> is not a very useful name. Let me call it IEntityListProvider<T> instead.
I can imagine the common consumer could be a single route controller.
Let me first a define a simple routing framework to provide some context:
// IRequest and IResponse are models of HTTP request resp. response
interface IRequestHandler
{
  public Task<IResponse> HandleAsync(IRequest request);
}

interface IRouterBuilder
{
  // register a GET route handler
  public void Get(string path, IRequestHandler handler);

  // build the router instance
  public IRequestHandler Build();
}

now we can implement the generic consumer like this:
class EntityListRouteHandler<T> : IRequestHandler
{
  private IEntityListProvider<T> Provider;

  // add constructor to take the dependency

  Task<IResponse> HandleAsync(IRequest request)
  {
    // get the list from provider
    // and convert the list to a response in a way
    // that's going to be the same for all entities
  }
}

and somewhere in router factory you would do somethig like this:
RouterBuilder.Get('/categories', new EntityListRouteHandler<Category>(categoriesProvider));
RouterBuilder.Get('/products', new EntityListRouteHandler<Product>(productProvider));
RouterBuilder.Get('/customers', new EntityListRouteHandler<Customer>(customersProvider));

Now I would like to expand on @Hayden's answer.
Ad 1) he mentions that if a PasswordService were to exist, it would probably not need a ListAsync method. Well, that's true, but it's misleading because it is irrelevant. Such a service would simply not implement the IEntityListProvider<T> interface.
Ad 2) if some service needs some more methods, then I would ask mayself is there someone who needs those methods at the same time? If yes just create a new interface extending the base one. If not, just define a new separate interface. Whether the concrete class implements two interfaces or you simply provide two different classes to implement each of the two interfaces is up to you. But a separate class for each interface feels like it better follows SRP.
I would add one more note regarding pagination. It is not very wise to provide the lists of entities without any limit. The momory consumption of such program is then out of control unless there is some logic that prevents too many entities to exist in the first place. Your ListAsync method should have arguments for pagination and possibly filtering and sorting as those are very common things to do with list resources. The type of these arguments may as well be generic arguments of the interface. I.e. Task<IEnumerable<T>> IEntityListProvider<T, F, S, P>::ListAsync(F filter, S sort, P page) or so...


struct,template arguments, C++ I am writing color model logic in C++. I ask review in  defining limits for the color model, by saying Limits I mean this.
Consider a struct which represents RGB color model.
template<typename T>
struct RGB {
   T r,g,b;
};

So I declared a type RGB<T> where T can be any arithmetic type (the assertion is done, don't worry). So now I can create types using RGB_Float = RGB<flaot> or using RGB_888 = RGB<unsigned char> and so on.
As you already guessed I am doing image processing and sometimes there comes a time where I want a type RGB<float> where each component limits are [0-1] and sometimes I want same RGB<float> with the limits [10-20] (An example) . Also I need each type to know its limits, at compile time. Syntax is like RGB_Float::Limits::max() \\Returns max RGB_Float and RGB_Float::Limits::min() \\ Returns min RGB_Float. So I need to change my struct RGB<T> this way.
template<typename T, typename LIMITS>
struct RGB {
    using Limits = LIMITS;
    T r,g,b;

}

And before declaring any RGB<T,LIMITS> type I need to create a struct like
RGB_FloatLimits {
     static RGB<float,RGB_FloatLimits> max( return {1.0f,1.0f,1.0f};
     ... // min implementation too.
}

And then using RGB_Float = RGB<float,RGB_FloatLimits>;
I know that this is a bad solution but I think it's the only if you use C++17. I spent a week trying to pass the limits as a template argument to a color model. Please review this solution and I am 90% sure that there is a better way to do this. Thx in advance.
Note In any implementation of the limits sizeof the color model must remain the same.(So we must store the max and min statically)

C++11
Pre C++20 you have to use the type traits based approach, which is essentially what you have done. I dont think this is a bad solution, but probably the way to go if you need to.
C++20 solution (if integral min and max are ok also for C++11 )
In C++ 20 this all becomes easier as it finally allows for class and float non-type template arguments.
template<typename T, T min, T max>
struct RGB {
    T r,g,b;
}

works for T = int, T = char, T = float and so on.
This is the most straightforward solution I could think of that does not need any helper classes. If you only need min and max this is probably good.
If you could live with integer min and max then this also works pre C++20. Simply use int for the min and max template arguments.
The drawback is that in every function that accepts an RGB you need three  template parameters (T, min and max), which could create a lot of noise in the code and make it less readable.
In case you need more than two config parameters
In case min and max is not a sufficient description of the number range, you could add addtional template parameters. However I would not advise this but make the parameters now a class.
In C++20 this is very straightforward again, as you can also use class values as template parameters. This is a bit more clear than the previous type based traits approach (which is also the one you are using).
template <class T>
struct RGBconfig<T> {
   T min, max;
}

template<typename T, RGBconfig<T> config>
struct RGB {
    T r,g,b;
}

use as
constexpr RGBconfig<float> float1020 = {10, 20}

using RGBfloat1020 = RGB<float, float1020>

alternatively one could do
using RGBfloat1020 = RGB<float, RGBconfig<float> {10,20} >

Reduce to one template parameter => include type in RGBconfig
Perhaps it is even possible to reduce it down to one template argument
template <class T>
struct RGBconfig<T> {
   using type = T;
   T min, max;
}

template<auto config>
struct RGB {
    typename decltype(config)::type    r,g,b;
}

then use as
constexpr RGBconfig<float> float1020 = {10, 20};
using RGBfloat1020 = RGB<float1020>;

or
using RGBfloat1020 = RGB<RGBconfig<float> {10,20} >;


Checking for rvm Ruby versions As it is going to be used as part of some automation and not just at the command line I was wondering if it could be improved at all.

Improve would mean any of:

handle sad cases better
handle error better
report errors better
better able to work on multiple linux OS including OSX

Improve would NOT mean:

shorter code
short variable names
Linux shell version specific tricks or shortcuts
allowing for rvm not being present (checked elsewhere)

function make_sure_rvm_rubies_installed() {

rvm list > $TMP/local_ruby_versions.txt
local_ruby_versions=(1.8.7 1.9.3)

for version in "${local_ruby_versions[@]}"
do
  cat $TMP/local_ruby_versions.txt | grep -q $version
  version_search_result=$?
  if [ $version_search_result = 0 ]
  then
    echo "Required RVM Ruby version $version confirmed as present on this machine"
  else
    echo "*** EXITING SMOKE TEST *** - not all required ruby versions are present in RVM"
    echo "Please install RVM ruby version: $version and then re-run this program"
    exit
  fi
done
if [ $? = 0 ]
  then echo "All required Ruby Versions confirmed as present locally"
fi
}

make_sure_rvm_rubies_installed

The name make_sure_rvm_rubies_installed describes very well what the function does,
but it's a bit too much.
Maybe check_installed_rubies would be good enough.

You didn't indent the body of the function.
It's better to indent the inner part of large { ... } blocks for readability.

No need to use the function keyword when declaring functions, though you can if you like.

Instead of cat ... | grep it's better to omit the cat and use the file as a parameter of grep.

No need to save the exit code of the grep in the version_search_result variable,
you can do this directly:

  if grep -q $version $TMP/local_ruby_versions.txt
  then
    echo "Required RVM Ruby version $version confirmed as present on this machine"
  else
    echo "*** EXITING SMOKE TEST *** - not all required ruby versions are present in RVM"
    echo "Please install RVM ruby version: $version and then re-run this program"
    exit
  fi

The exit there is a bit unexpected.
The script could be reusable if you return 1 instead,
to give a chance to the caller to recover gracefully if necessary.
(In your example use case it doesn't matter, but this is a tip for the future.)

I'm not sure what you're expecting from the $? check in this code:

for version in "${local_ruby_versions[@]}"
do
  # ...
done
if [ $? = 0 ]
  then echo "All required Ruby Versions confirmed as present locally"
fi

When a version is not found the original loop exits the entire script (I recommended above to return from the function).
So in case of failures this if is not reached.
Otherwise the last command to evaluate is an echo,
which should just succeed,
so this if condition seems pointless, you can omit it.

Although arrays are cool, and it's great that you can use them,
in simple situations like this I would use a normal variable:

local_ruby_versions='1.8.7 1.9.3'

for version in $local_ruby_versions; do ...; done

You create the file temporary file $TMP/local_ruby_versions.txt but you don't clean it up when finished.


Outputting years with no repeated digits I'm working through a course now, and was tasked with the following:

Write a function, no_repeats(year_start, year_end), which takes a range of years and outputs those years which do not have any repeated digits.
You should probably write a helper function, no_repeat?(year) which returns true/false if a single year doesn't have a repeat.

I did not know about each_char yet, so I wrote the following:
def no_repeats(year_start, year_end)
  (year_start..year_end).to_a.keep_if {|year| no_repeat?(year)}
end

def no_repeat?(year)
  year_array = year.to_s.split(//)
  year_array == year_array.uniq
end

I'm wondering if:

there is an easier way to make an array from a string with no spaces
there is a way to convert each character in a number to an item in a single array without converting the number to a string first
there is a better way to do this problem

This is the model solution for a beginner:

def no_repeats(year_start, year_end)
  no_repeats = []
  (year_start..year_end).each do |yr|
    no_repeats << yr if no_repeat?(yr)
  end

  no_repeats
end

def no_repeat?(year)
  chars_seen = []
  year.to_s.each_char do |char|
    return false if chars_seen.include?(char)
    chars_seen << char
  end

  return true
end

I'm also wondering about the advantages and disadvantages of my code compared to the model solution. I kind of get that shorter code is better, and readability matters. I don't know how to test code for time yet.

The model solution they propose is very unidiomatic, your code is far better. I'd just made some minor changes:

Use String#chars instead of String#split(//).
Use Enumerable#select? instead of Array#keep_if to avoid modifying data in-place. It's better to work with immutable streams of data (with methods from Enumerable).

I'd write:

def no_repeats(year_start, year_end)
  (year_start..year_end).select { |year| no_repeat?(year) }
end

def no_repeat?(year)
  year_digits = year.to_s.chars
  year_digits == year_digits.uniq
end


Improving the time complexity of my back pack algorithm Is there anyone who can help me improve the time complexity of this backpack algorithm, for which I already use sliding array to improve the space complexity.

The problem is as follows:

  Given n items with size A[i], an integer m denotes the size of a backpack. How full you can fill this backpack?

Example:
If we have 4 items with size [2, 3, 5, 7], and the backpack size is 11, we can select [2, 3, 5], so that the max size for this backpack is 10. If the backpack size is 12, we can select [2, 3, 7] and fill it completely.

The function should return the max size we can fill in the given backpack.

class Solution:
# @param m: An integer m denotes the size of a backpack
# @param A: Given n items with size A[i]
# @return: The maximum size
def backPack(self, m, A):
    # write your code here
    n = len(A)
    f = [[False for x in range(m+1)] for y in range(2)]
    for i in range(n+1):
        f[i%2][0] = True

    for i in range(1, n+1):
        for j in range(1, m+1):
            f[i%2][j] = f[(i-1)%2][j]
            if j >= A[i-1] and f[(i-1)%2][j-A[i-1]]:
                f[i%2][j] = True

    max = 0
    for i in range(m+1):
        if f[n%2][i]:
            max = i

    return max

OO is not always appropriate

In this case the class is not needed, just write a free standing function for simplicity.

Make the code speak, not the comments

For example:

# @param m: An integer m denotes the size of a backpack

Can be omitted if you write:

def backPack(backpack_size: int, ...):

and:

# @return: The maximum size

Can be omitted if you write:

def backpack_max_size(...):


Runs a MySQL query, returns Dictionary or false; I've been practicing C# and ASP.NET, and have been working on a somewhat larger-scale project than I am currently used to. I am sure there are horrible things wrong with this code that I will find out later, and would rather get some input on it now.

One of the things I'm concerned with is the fact that RunSelectCommand can return false if the database fails to connect, or if the query returns nothing.

Currently, in order to check for that, I do if (!(data is bool)) and if it's not I assume there is data to work with, and I return an error from the web server if not.

If any more information is needed, please let me know! I am very excited for some real peer-reviewed code.

public object RunSelectCommand(MySqlCommand command, List<MySqlParameter> parameterCollection = null)
{
    using (MySqlConnection conn = new MySqlConnection(connectionString))
    using (var cmd = conn.CreateCommand())
    {
        try
        {
            conn.Open();
        }
        catch (MySqlException ex)
        {
            return ex;
        }
        cmd.CommandText = command.CommandText;
        if (parameterCollection != null) // if parameters for query are specified, add them here.
        {
            cmd.Parameters.AddRange(parameterCollection.ToArray());
        }
        using (var reader = cmd.ExecuteReader())
        {
            if (reader.Read())
            {
                Dictionary<string, string> data = new Dictionary<string, string>(); // This is designed for returning only 1 row at a time
                for (int i = 0; i < (reader.FieldCount - 1); i++) // TODO seperate SelectAll command
                {
                    data.Add(reader.GetName(i), reader[i].ToString()); // Iterates through reader and adds data to Dictionary for output to JSON
                }
                conn.Close();
                return data;
            }
            else
            {
                conn.Close();
                return false;
            }
        }
    }
}

Usage example:

[HttpGet("checkapi/{text}")]
    public object CheckApi(string text) // returning object automatically converts to JSon
    {
        MySqlCommand C = new MySqlCommand("SELECT * FROM api WHERE apikey = @apikey");
        List<MySqlParameter> collection = new List<MySqlParameter>()
        {
            new MySqlParameter() { ParameterName = "@apikey", Value = text }
        };
        return mysqlWrapper.RunSelectCommand(C, collection);
    }

As you suspected, this code is pretty bad. You are returning three different types of data here. Just, don't.

1)

try
{
    conn.Open();
}
catch (MySqlException ex)
{
    return ex;
}

Don't return the exception, re-throw it and catch it in the caller.

try
{
    conn.Open();
}
catch (MySqlException ex)
{
    throw;
}

In this block, you are catching the exception, then returning the exception to the user as a piece of data. A better way to do this would be to just not catch the exception and let the caller catch it:

try
{
    return mysqlWrapper.RunSelectCommand(C, collection);
}
catch (Exception ex)
{
    return ex;
}

However, now you are exposing your call stack to the user, which you don't want to do. This information could provide hints to someone trying to break into your system, and is not allowed under some security protocols (SOC 2, for example). Instead of returning the whole exception, build a new piece of data and return just the relevant bits (or if they just need one piece, then just return that piece).

2)

Don't return false. Either return an empty object (if the caller doesn't care if there were no results), or throw an exception. Alternately, you could use a ValueTuple and return (bool success, Dictionary<string, string> data) from the method. The first successful path would return (true, {data}) and the second would return (false, null).

3)

You might have a SQL Injection attack going on here. Check out what happens if the client sends a request with the text parameter "test' or 1 = 1 --". It looks like the resulting query might be SELECT * FROM api WHERE apikey = 'test' or 1 = 1 --', but I don't know this well enough to be sure.


Rotating an array by mentioned digit to rotate I have developed the program to rotate the array in java request you to please check it and let me know if there are any flaws in it or where further it can be improved

    package com.dataStructures.Arrays;

// Java program for reversal algorithm of array rotation
//Let the array be arr[] = [1, 2, 3, 4, 5, 6, 7], d =2 and n = 7
//        A = [1, 2] and B = [3, 4, 5, 6, 7]
//
//        Reverse A, we get ArB = [2, 1, 3, 4, 5, 6, 7]
//        Reverse B, we get ArBr = [2, 1, 7, 6, 5, 4, 3]
//        Reverse all, we get (ArBr)r = [3, 4, 5, 6, 7, 1, 2]

import java.util.Arrays;

class ArrayRotate {

    static void leftRotate(int[] arr, int d) {

        if (d == 0)
            return;
        int n = arr.length;
        reveresArray(arr, 0, d - 1);
        reveresArray(arr, d, n - 1);
        reveresArray(arr, 0, n - 1);
    }

    static void reveresArray(int[] arr, int start, int end) {
        int temp;
        while (start < end) {
            temp = arr[start];
            arr[start] = arr[end];
            arr[end] = temp;
            start++;
            end--;
        }
    }

    static void printArray(int[] arr) {
        Arrays.stream(arr).mapToObj(item -> item + " ").forEach(System.out::print);
    }

    public static void main(String[] args) {
        int[] arr = {1, 2, 3, 4, 5, 6, 7};
        int n = arr.length;
        int d = 2;
        leftRotate(arr, d); // Rotate array by d
        printArray(arr);
    }
}

while the solution is functional. it is overly complex. rotation is a simple algorithm of index arithmetic: start with the first cell, with each iteration, you determine the source and destination indexes of one value, move that value into place, take the evicted value and set the next iteration to determine its new place.

so we start with idx 0 and determine its place is at idx 5. in the next iteration we start with idx 5 and determine its new place and so on.

static void leftRotate(int[] arr, int d) {
    int srcIdx, dstIdx;
    srcIdx = 0;
    int val = arr[srcIdx];
    // loop only used to count (i not used as index)
    for (int i = 0; i < arr.length ; i++) {
        dstIdx = srcIdx - d;
        if (dstIdx < 0) dstIdx = arr.length + dstIdx;  // rotate dst idx from start to end of arr
        int temp = arr[dstIdx];
        arr[dstIdx] = val;
        val = temp;  // set next iteration value to move
        srcIdx = dstIdx;  // set next iteration srcIdx
    }
}

also, printArray is redundant. use Arrays.tostring()

EDIT

thanks to @Imus, there is a nasty bug in my "simple" algorithm described above.
The bug occurs when the array is divisible by d. in this case, following src-dst-src-dst chain results in processing the same indexes in a loop, while missing other indexes.  The solution (that I found) is to detect such cases of "closed-loop" and advance the pointers one cell ahead. admittedly, it does make the solution more complex.

below is a fixed solution.

static void leftRotate(int[] arr, int d) {
    int srcIdx, dstIdx, loopDetectIdx;
    srcIdx = 0;
    loopDetectIdx = 0;  // detects processing in "closed loop"
    int val = arr[srcIdx];
    for (int i = 0; i < arr.length ; i++) {
        dstIdx = srcIdx - d;
        // rotate dst idx from start to end of arr
        if (dstIdx < 0) dstIdx = arr.length + dstIdx;
        int temp = arr[dstIdx];
        arr[dstIdx] = val;
        System.out.println(srcIdx + "-" + dstIdx + " " + Arrays.toString(arr));
        val = temp;
        srcIdx = dstIdx;
        // if we already processed srcIdx, move all pointers one cell forward
        if (srcIdx == loopDetectIdx) {
            srcIdx++;
            loopDetectIdx++;
            val = arr[srcIdx];
        }
    }
}


Finite State Automaton implementation I've been studying compiler construction theory. Right now, I'm studying Finite State Automaton and I've tried to create my own implementation. I'm not sure if my implementation is right.

<?php
/**
 * FINITE STATE AUTOMATON
 * ======================
 * - states
 * - starting state
 * - accept state
 * - transition table
 *
 * Input string examples:
 * 10112 - Accepted
 * 222 - Rejected
 *
 * Diagram: http://i.imgur.com/pDl1VZM.png
 */
$s1 = 'S1';
$s2 = 'S2';

$states = [];
$starting_state = $s1;

$transition_table = [
    'S1' => [
        '2' => 'S3',
        '1' => 'S1',
        '0' => 'S2',
    ],
    'S2' => [
        '1' => 'S1',
        '0' => 'S2',
    ],
    'S3' => [],
];
$input_string = '10112';

$current_state = $starting_state;
foreach (str_split($input_string) as $character) {
    if (isset($transition_table[$current_state][$character])) {
        $current_state = $transition_table[$current_state][$character];
        $states[] = $current_state;
    }
}
if (count($states) == strlen($input_string)) {
    echo 'Accepted';
} else {
    echo 'Rejected';
}
echo PHP_EOL;

Did I get the whole idea? Are there something that I've missed?

Is this FSA thing related to regular expressions? It seems to be similar.

It's quite good, I especially like the $current_state explanatory variable. Actually, I'd create another one to remove some duplication:

$transitions = $transition_table[$current_state];
if (isset($transitions[$character])) {
    $current_state = $transitions[$character];
    $states[] = $current_state;
}

Some other notes:

The code currently ignores invalid transitions. You could break the loop immediately if a transition is invalid:

foreach (str_split($input_string) as $character) {
    $transitions = $transition_table[$current_state];
    if (isset($transitions[$character])) {
        $current_state = $transitions[$character];
        $states[] = $current_state;
    } else {
        break;
    }
}

Inverting the isset would make the code flatten a little bit:

foreach (str_split($input_string) as $character) {
    $transitions = $transition_table[$current_state];
    if (!isset($transitions[$character])) {
        break;
    }
    $current_state = $transitions[$character];
    $states[] = $current_state;
}

$s2 is unused, I guess you could remove it:

$s2 = 'S2';

  Are there something that I've missed?

The linked page mentions classes and I guess it should have been an object oriented solution. Check the Wikipedia article about the Java implementation, it shows a great example.


Calculate the running median I'm trying to solve a challenge where you need to calculate the median every time you add a number.

say you have a list of numbers: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10

When you scan in the first number (1), you calculate the median of that.
When you scan in the second number (2), you calculate the median of 1 and 2, and so on.

My code is working, but is having timeout issues on the larger test cases:

import java.io.*;
import java.util.*;

public class Solution {

    public static void main(String[] args) {
        Scanner s = new Scanner(System.in);
        int n = s.nextInt();

        PriorityQueue<Integer> pq = new PriorityQueue<>();

        while(n-- > 0) {
            pq.add(s.nextInt());
            System.out.println(calculateMedian(pq));
        }
    }

    public static float calculateMedian(PriorityQueue<Integer> pq) {
        PriorityQueue<Integer> temp = new PriorityQueue<>(pq);
        int index = 0;
        float number = 0.0f;
        int halfQueueSize = (temp.size() >> 1);

        if(temp.size() % 2 != 0) {
            while(index++ <= halfQueueSize) {
                number = temp.poll();
            }
            return number;
        } else {
            while(index++ < halfQueueSize) {
               number = temp.poll();
            }
            return ((number+temp.poll())/2.0f);
        }
    }
}

Any tips as to how this can be optimized would be much appreciated!

The PriorityQueue was no good idea. What are you doing?

After the nth element, you copy all the previous n-1 elements into a temporary queue, which has complexity \$O(n)\$. Then you drain half the queue, which has complexity \$O(n \log n)\$. Summed up you get \$O(n^2 \log n)\$.

You're just misusing the queue as a sorting vehicle. That's not optimal as Arrays.sort has a faster algorithm (quicksort is faster, except when it goes wrong and this one tries pretty hard to avoid going wrong).

It would be probably faster to use an int[], track the size manually and re-sort the array after each element added. This has the same complexity, but could be faster as you at least avoid the copying.

Inserting the element in the proper position would lead to a total complexity of \$O(n^2)\$.

A much faster solution (I hope \$O(n \log n)\$) could be implemented using a TreeSet with higher and lower. There are also smart algorithms allowing to compute median without sorting the collection as you sort of need to sort only the middle part.

Review

public static float calculateMedian(PriorityQueue<Integer> pq) {

Don't use float unless you really have to conserve memory. They're pretty imprecise and they're no faster than double, at least on modern Desktop CPUs.

    int halfQueueSize = (temp.size() >> 1);

    if(temp.size() % 2 != 0) {

Do you optimize division by 2 as shift or don't you? Note that it hardly matters as this is surely not the most time-consuming operation here.

    if(temp.size() % 2 != 0) {
        while(index++ <= halfQueueSize) {
            number = temp.poll();
        }
        return number;
    } else {
        while(index++ < halfQueueSize) {
           number = temp.poll();
        }
        return ((number+temp.poll())/2.0f);
    }

You really shouldn't have two nearly identical branches. It makes it hard to spot the difference. With something like

    int halfQueueSize = (temp.size() >> 1) + (temp.size() & 1);

you can use < in both branches and pull the loop out.


DateTime range splitting algorithm I have the algorithm which splits DateTime range into multiple ranges based on night time range.
For example:
Night start: 22:00, Night end: 06:00
Start time: 2020-10-26 21:00:00 End time: 2020-10-27 07:00:00
Expected output:

2020-10-26 21:00:00 - 2020-10-26 22:00:00
2020-10-26 22:00:00 - 2020-10-27 06:00:00
2020-10-27 06:00:00 - 2020-10-27 07:00:00

It basically splits DateTime range by day and night/to day/night batches.
I wonder if there's something I could do to make the code shorter and possibly better performance-wise.
Code:
internal class Helper
{
    public static readonly TimeSpan NightStartTime = new TimeSpan(22, 0, 0);
    public static readonly TimeSpan NightEndTime = new TimeSpan(6, 0, 0);

    public static bool IsNightTime(TimeSpan time) => time >= NightStartTime || time < NightEndTime;
    public static bool IsDayTime(TimeSpan time) => !IsNightTime(time);
}

internal class Program
{
    private static void Main(string[] args)
    {
        var startTime = DateTime.Parse("2020-10-26 21:00:00");
        var endTime = DateTime.Parse("2020-10-27 07:00:00");

        DateTime? currentStartTime = startTime;
        DateTime? currentEndTime = null;
        var currentTimeIsDayTime = Helper.IsDayTime(startTime.TimeOfDay);
        for (var currentTime = startTime; currentTime <= endTime; currentTime = currentTime.AddMinutes(1))
        {
            var isDayTime = Helper.IsDayTime(currentTime.TimeOfDay);
            if (currentTimeIsDayTime != isDayTime)
            {
                currentEndTime = currentTime;
                currentTimeIsDayTime = isDayTime;
            }

            if (currentTime == endTime)
            {
                currentEndTime = endTime;
            }

            if (currentStartTime != null && currentEndTime != null)
            {
                Console.WriteLine($"{currentStartTime} - {currentEndTime}");

                currentStartTime = currentEndTime;
                currentEndTime = null;
            }
        }
    }
}

You can make it shorter by tackling the problem smarter, as in shift or work segments in relation to a DateTime or TimeofDay.  Trying brute force checks of minute-by-minute is a horrifically bad solution.  My answer may point you in the right direction but it is NOT TOTALLY CORRECT FOR ALL POSSIBLE INPUTS.
I would separate the logic into its own class.
public enum Shift { Day, Night }

// CAUTION: this class does not perform validation to make sure end times are after start time, or that
// start and end times are of the same DateTimeKind.
// This does not work for Kind = Local, specifically on fall back or spring forward transition days.
// It does work for Kind = Utc or Unspecified, since they do not observe DST.
public class WorkShift
{

    // A WorkShift may not be complete shift, but rather segments.
    public Shift Shift { get; }
    public DateTime StartTime { get; }
    public DateTime EndTime { get; }

    public WorkShift(Shift shift, DateTime startTime, DateTime endTime)
    {
        this.Shift = shift;
        this.StartTime = startTime;
        this.EndTime = endTime;
    }

    public static readonly TimeSpan NightShiftStart = new TimeSpan(22, 0, 0);
    public static readonly TimeSpan NightShiftEnd = new TimeSpan(6, 0, 0);

    // Shifts have an inclusive start time but an exclusive end time.
    public static bool IsNightShift(TimeSpan timeOfDay) => timeOfDay >= NightShiftStart || timeOfDay < NightShiftEnd;
    public static bool IsDayShift(TimeSpan timeofDay) => !IsNightShift(timeofDay);

    public static bool IsNightShift(DateTime time) => IsNightShift(time.TimeOfDay);
    public static bool IsDayShift(DateTime time) => IsDayShift(time.TimeOfDay);

    public static IEnumerable<WorkShift> GetShiftSegments(DateTime startTime, DateTime endTime)
    {
        var segmentEnd = startTime;
        while (segmentEnd < endTime)
        {
            var segmentStart = segmentEnd;
            var shift = IsNightShift(segmentStart) ? Shift.Night : Shift.Day;
            segmentEnd = GetShiftEndTime(shift, segmentStart);
            if (segmentEnd > endTime)
            {
                segmentEnd = endTime;
            }
            yield return new WorkShift(shift, segmentStart, segmentEnd);
        }
    }

    private static  DateTime GetShiftEndTime(Shift shift, DateTime segmentStart)
    {
        // Nice thing about Day is it occurs on the same date, but Night may crossover midnight into the next date.
        if (shift == Shift.Day)
        {
            return DateTime.SpecifyKind(segmentStart.Date + NightShiftStart, segmentStart.Kind);
        }

        if (segmentStart.TimeOfDay >= NightShiftStart)
        {
            // ending shift would be tomorrow.
            return DateTime.SpecifyKind(segmentStart.Date.AddDays(1) + NightShiftEnd, segmentStart.Kind);
        }

        return DateTime.SpecifyKind(segmentStart.Date + NightShiftEnd, segmentStart.Kind);
    }
}

Now the Main test can become:
static void Main(string[] args)
{
    var startTime = DateTime.Parse("2020-10-26 21:00:00");
    var endTime = DateTime.Parse("2020-10-27 07:00:00");
    Console.WriteLine($"Time Range From {startTime} To {endTime}");

    var workSegments = WorkShift.GetShiftSegments(startTime, endTime).ToList();

    for (var i = 0; i < workSegments.Count; i++)
    {
        Console.WriteLine($"   [{i}] = {workSegments[i].Shift},  Start:{workSegments[i].StartTime}, End:{workSegments[i].EndTime}");
    }

    Console.WriteLine("\nPress ENTER to close");
    Console.ReadLine();
}

Sample console output is:
Time Range From 10/26/2020 9:00:00 PM To 10/27/2020 7:00:00 AM
   [0] = Day,  Start:10/26/2020 9:00:00 PM, End:10/26/2020 10:00:00 PM
   [1] = Night,  Start:10/26/2020 10:00:00 PM, End:10/27/2020 6:00:00 AM
   [2] = Day,  Start:10/27/2020 6:00:00 AM, End:10/27/2020 7:00:00 AM

Press ENTER to close

CAUTIONS AND WARNINGS
A WorkShift instance may or may not represent a complete Day or Night work shift, but instead a segment depending upon the starting or ending time.
As the code comments say, there are no validation checks to ensure that the ending time occurs after the starting time.  Nor are there any checks to ensure that both the starting and ending times have the same DateTimeKind.
I did not check to see if the code works correctly for Local Kind.  My intuition tells me to be suspicious or very leery about it.  However, it should work when Kind is Utc or Unspecified, where neither observe DST and therefore neither will have a 23-hour Spring Forward Day or a 25-hour Fall Back Day.
It is important that both start and ending times belong the the same DateTimeKind when subtracting start from end.  If they belong to different kinds, the time range span may be incorrect.
Even if you are super careful about the Kind, keep in mind that certain operations may return a invalid date and time.  For instance, DateTime.Date returns a value at midnight with the same Kind as the input DateTime.  But if your local time is Brazil (or Brasil), then the Spring Forward transition day does not have a midnight.
There are so many gotcha's with dates, times, and local time zones, that I would urge you to read up on https://nodatime.org/,


For-loop for mixing audio (left channel, stereo to mono) This is a very simple for loop, but I find it very, complex-looking.  I hope you can give me some insight on how it may be improved.

int Tindex = 0;
int16_t Test[960];
int TestSize = SampleSize / 2; //Test is 16Bit, Half size of 32bit
int TestChannels = 2; //Stereo
float copy = 0; //Used to store Audio data used to Mixdown to Mono
bool left = true; //Only use Left Channel
bool MixMono = true; //MixMono has priority over left
if (MixMono || left) //Change the approprite variables if it's Mono instead of Stereo
{
    int16_t Test[960 / 2];
    TestSize = TestSize / 2; //If Mono is used, Half Channel/Size.
    TestChannels = 1; //Mono
}
int ChannelCheck = 0;

for (int i = 0; i < bufferSize; i += 1)         //For Loop for Downmixing and BitDepth Conversion
{
    int s = (micBuffer[i] * 32768.0f); //32bit Float to 16Bit Int

    ChannelCheck++;

    if (MixMono)//Mixdown Stereo to Mono
    {
        copy += s;
        if (ChannelCheck == 2)
        {
            Test[Tindex] = copy / 2;
            copy = 0;
            Tindex++;
            ChannelCheck = 0;
        }

    }
    else if (left)
    {
        if (ChannelCheck == 1)
        {
            Test[Tindex] = s;
            ChannelCheck -= 2;
            Tindex++;
        }
    }
    else
    {
        Test[Tindex] = s;
        Tindex++;
    }
}
//End of For Loop

I tried commenting most stuff, so it should be self-explained.

It's pretty much a mix stereo to mono.  It combines 2 indices from the float array (add them), then divide them (to keep the correct volume).

16-bit conversion just multiplies by Int16 Max; not much there.  I would like to have some kind of dithering, so if anyone knows some easy references, please offer them.

But as you can see, it's overly complicated with checks all over the place.  I think it can be improved, but I can't really figure out a good way to do it.

Provide a complete sample

For future reference, it's much easier to review code if it's complete.  That is, instead of just writing a loop, you could instead wrap in a small but complete program.  Your code refers to SampleSize, bufferSize, and micBuffer but none of those are defined in the sample you've posted.

Establish and use consistent naming

Some of your variable names, such as SampleSize are capitalized, while others, such as bufferSize are not.  One common C++ convention is to use lowercase names for variables (as with bufferSize) and capitalized names for classes and structure types such as Vector.  There is no significance to these conventions to the compiler, but a consistently applied convention makes it easier for others to read and understand your code.

Avoid the use of hard-coded "magic numbers"

There are numbers, such as 960 and 32768.0f in your code that don't have an obvious meaning.  Better would be to declare them as constants with a meaningful name, or at least to have more explanatory comments.

Eliminate unused variables

This code declares a variable Test at the top of the sample, and then redefines it within the scope of the if statement.  The second declaration of Test is never used within the code.  TestChannels and TestSize are also unused within this code. You can (and should) eliminate unused variables like this.Your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so.

Remove loop invariants from the loop

A loop invariant is a value used within a loop construct that doesn't change.  Right now your code checks the value of MixMono and left for every iteration through the loop. Since they don't change within the loop, it's better to check them just once and then select a loop, since you're looking for performance.

As an example of this, here's what your MixMono loop would look like when extracted:

if (MixMono)
{
    for (int i = 0; i < bufferSize; ++i)
    {
        int s = micBuffer[i] * scaleFactor; //32bit Float to 16Bit Int
        ChannelCheck++;
        copy += s;
        if (ChannelCheck == 2)
        {
            Test[Tindex] = copy / 2;
            copy = 0;
            Tindex++;
            ChannelCheck = 0;
        }
    }
}

Now that it's extracted, it's easier to see what's happening and to identify inefficiencies.  One thing to note is that we're doing two multiplies, an add and a division for each mono sample.  The math is currently (left * 32768 + right * 32768)/2 with each channel being a float and the result being an int16_t.  Mathematically, this is the same as (left+right)*16384 so as long as you can assure that the samples are within range and that you always have an even number of samples, you can simplify this loop to this:

const float scaleFactor = 32768;
const float halfScaleFactor = scaleFactor/2;
if (MixMono)
{
    for (int i = 0; i < bufferSize; i+=2)
    {
        Test[Tindex++] = (micBuffer[i] + micBuffer[i+1]) * halfScaleFactor;
    }
}

The other loops can be similarly compressed and the s, ChannelCheck and copy variables completely eliminated:

else if (left)
{
    for (int i = 0; i < bufferSize; i+=2)
    {
        Test[Tindex++] = micBuffer[i] * scaleFactor;
    }
}
else
{
    for (int i = 0; i < bufferSize; ++i)
    {
        Test[Tindex++] = micBuffer[i] * scaleFactor;
    }
}

Consider using pointers

Depending on the architecture and compiler, using pointers instead of array indexing may be faster.  If your application requires more speed than indexing can provide, you might want to test using pointers and see if that will allow your code to meet its performance requirements.  As always, you should prefer clearly written code and only attempt such speedups if you have measured the performance of the code and found it insufficient.

Consider using a struct

Another possibility is to define a struct like this:

struct Sample
{
    float left;
    float right;
};

and then processing your micBuffer data by casting it as an array of such structures.  Your code might be easier to read and understand this way and may be faster.  An example is this:

Sample *micBufferEnd = (Sample *)&micBuffer[bufferSize];
if (MixMono)
{
    for (Sample *s = (Sample *)micBuffer; s < micBufferEnd; ++s)
    {
        Test[Tindex++] = (s->left + s->right) * halfScaleFactor;
    }
}

This works by treating the samples a pair at a time.  When the ++s is executed at the end of each loop, it advances the pointer by the size of the structure which happens to be the size of two floats.

Note that as @JerryCoffin noted in his comment, that this is not guaranteed to work because the compiler is free to insert padding between values.  For instance, on a 64-bit machine, the compiler may decide to align each member on a 64-bit boundary, possibly resulting in a gap between member items.  Compilers often implement something like a #pragma pack to force the behavior used here, but that's not portable, and it's even possible that it could change from version to version of an individual compiler.  An alternative, also noted in the comments, was to define the structure differently.  One way to do it might be this:

class Sample
{
private:
    float chan[2];
public:
    float left() const { return chan[0]; }
    float right() const { return chan[1]; }
};

Using it would be almost identical except that left and right are now member functions rather than data items.

Sample *micBufferEnd = (Sample *)&micBuffer[bufferSize];
if (MixMono)
{
    for (Sample *s = (Sample *)micBuffer; s < micBufferEnd; ++s)
    {
        Test[Tindex++] = (s->left() + s->right()) * halfScaleFactor;
    }
}


Thread which executes code after Thread.sleep() and can be cancelled only if it's sleeping I use Java8, I have a subclass of Thread called DelayedThread which wraps a Runnable and executes the Runnable after certain amount of time. It has a method called cancelOnlyIfStillSleeping() which interrupts Thread.sleep but I don't want to interrupt it once Runnable has been called because I'm afraid of interrupting Runnable which might react to the interruption in an unexpected way. The code looks like this:
class DelayedThread extends Thread {

    private final long delay;
    private final Runnable task;
    private boolean starting;

    private final Object lock = new Object();

    DelayedThread(long delay, Runnable task) {
        this.delay = delay;
        this.task = task;
    }

    @Override
    public void run() {
        try {
            Thread.sleep(delay);
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
            return;
        }

        synchronized (lock) {
            if (isInterrupted()) {
                return;
            }
            starting = true;
        }

        task.run();
    }

    /**
     * Interrupts this thread only if it's still sleeping. Once it finished sleeping
     * the thread won't get interrupted.
     */
    void cancelOnlyIfStillSleeping() {
        synchronized (lock) {
            if (starting) {
                return;
            }
            interrupt();
        }
    }
}

Does this code look safe? Isn't there any race condition where my code doesn't work as intended? Or is there any way better to achieve this?

I haven't analysed your code in detail, but it looks threadsafe on a quick scan.
I'd just suggest that you avoid reinventing the wheel. The java.util.Timer class will, I think, fit your use-case and is clearer that you are cancelling rather than interrupting your scheduled task. I think we can be fairly confident that it has been well-enough designed, implemented and reviewed to be threadsafe ...


Alphabetically sorted numbers This exercise involves taking an array of numbers and sorting them alphabetically. For example, [1,2,3,4] would return as [4,1,3,2]

I mapped the array first, returning an array of words, then sorted those words using Ruby's built-in sort function. Then, I mapped that array to return an array of integers.

It works, obviously. I want your suggestions and thoughts, different ideas and better ways I could do this in the future.

I know I could one-line the map functions, but thought this is better for readability. Example: alphabeticize = numbers.map {|num| alphas.fetch(:"#{num.to_s}")}.sort

def alphabetic_number_sort(numbers)
  alphas = {"0": "zero", "1": "one", "2": "two", "3": "three", "4": "four", "5": "five", "6": "six", "7": "seven", "8": "eight", "9": "nine", "10": "ten", "11": "eleven", "12": "twelve", "13": "thirteen", "14": "fourteen", "15": "fifteen", "16": "sixteen", "17": "seventeen", "18": "eighteen", "19": "nineteen"}

  alphabeticize = numbers.map do |num|
    alphas.fetch(:"#{num.to_s}")
  end.sort

  sorted_alphabetically = alphabeticize.map do |word|
    alphas.key(word).to_s.to_i
  end

  return sorted_alphabetically
end

p alphabetic_number_sort((0..19).to_a) == [
  8, 18, 11, 15, 5, 4, 14, 9, 19, 1, 7, 17,
  6, 16, 10, 13, 3, 12, 2, 0
]

sort_by cleans this up quite a bit, and avoids the unnecessary conversions to and from strings:

def sort_alphabetically(numbers)
  alphas = %w(zero one two three four five six seven eight nine ten eleven twelve thirteen fourteen fifteen sixteen seventeen eighteen nineteen)
  .map.with_index {|x,i| [i, x]}.to_h

  numbers.sort_by {|x| alphas[x]}
end

Notes:

Point on naming: use a verb, or else a noun describing what the function returns: sort_alphabetically or alphabetically_sorted. I know technically alphabetically_sorted isn't a noun, but I see it as an abbreviation for alphabetically_sorted_list, which is too verbose.
If speed matters, you should compute the hash outside the function, so it's only done once.


Emulating class extending I'm using the following piece of JavaScript to emulate class extending. Is this a valid way to go or does it have some drawbacks which should definitely be fixed?

var Validator = function (inputId) {
    "use strict";

    var self = this;

    self.inputId = inputId;

    window.document.getElementById(inputId).onblur = function () {
        self.check();
    };

};

Validator.prototype.inputId = '';

/**
 * Validator fails by default
 *
 * @returns {Boolean}
 */
Validator.prototype.isValid = function () {
    "use strict";

    return false;

};

/**
 * Check whether the input is valid
 *
 * @returns {Validator}
 */
Validator.prototype.check = function () {
    "use strict";

    // Check and mark whether validation passes
    if (!this.isValid()) {

        this.markInvalid();

    } else {

        this.markValid();

    }

    return this;

};

/**
 * Mark the input as valid
 *
 * @returns {Validator}
 */
Validator.prototype.markValid = function () {
    "use strict";

    var input = window.document.getElementById(this.inputId);

    // Remove "error" class name
    input.className = input.className.replace(
        /(?:^|\s)error(?!\S)/g,
        ''
    );

    return this;

};

/**
 * Mark input as invalid
 *
 * @returns {Validator}
 */
Validator.prototype.markInvalid = function () {
    "use strict";

    var input = window.document.getElementById(this.inputId);

    // Add "error" class when it is not set already
    if (!input.className.match(/(?:^|\s)error(?!\S)/g)) {
        input.className += ' error';
    }

    return this;

};

Than this is used as a concrete validator implementation

// Inherit from validator
var VatCheck = Validator;

/**
 * Validate vat number
 *
 * @note only format is checked, not whether the number is in use
 *
 * @returns {Boolean}
 */
VatCheck.prototype.isValid = function () {
    "use strict";

    var input = window.document.getElementById(this.inputId);

    // Check number by using Vat.js
    return !!checkVATNumber(input.value);

};

The reason I came to this solution is because only one or two methods of the parent class have different behavior, and the use of the strategy pattern seems somewhat over-engineering

There is one bad problem in the code that can be answered by the basis of the pattern presented here:

// Inherit from validator
var VatCheck = Validator;

This does not inherit anything, instead it causes VatCheck name to point to the exact same object (constructor function) as the Validator. Thus all you are doing is overwriting Validator.prototype.isValid (changing the base implementation), and aliasing the base implementation by different names.

If you repeat this pattern again, the end result is that all validators have their isValid check method do the action of the last validator type that you define.

Instead you need to do VatCheck.prototype = new Validator() but then you need to also redo the constructor.


Check whether string of brackets are well-formed The task

I asked similar questions here and here.

  Given a string of round, curly, and square open and closing brackets,
  return whether the brackets are balanced (well-formed).

  For example, given the string "([])", you should return true.

  Given the string "([)]" or "((()", you should return false.

My solution

...is inspired by @200_success' answer. I only made it more concise.

const isBracketBalanced = brc => {
  const braces = {
    "(": ")",
    "[": "]",
    "{": "}",
  }, expectCloseStack = [];

  const bracketIsBalanced = b => braces[b] ?
           expectCloseStack.push(braces[b]) :
           expectCloseStack.pop() === b;

  return [...brc]
    .every(bracketIsBalanced) &&
    !expectCloseStack.length;
};

console.log(isBracketBalanced("([])[]({})"));

There are some issues with the code:

isBracketBalanced - Should be plural. It checks more than one bracket.
bracketIsBalanced - Is basically the same name as the parent function, except it does more than just check if the bracket is balanced. It also manipulates the stack.
braces - Didn't we just call them brackets?
expectCloseStack declaration - This is perhaps just my personal preference, but to me, writing the declaration after a comma like that makes me look twice to see if it is part of the object that was declared above it.

All in all, I find this solution a bit harder to read than the original. Also, it's actually not more concise.


Functional Javascript Practice with AJAX I am currently doing projects in freecodecamp.com to brush up on my javascript skills and just finished a weather report widget. It does the following:

Gets api from openweathermap.org and displays the weather based on
user's latitude and longitude
Displays the city, weather description and wind speed
Can toggle between celsius and ferenheit

I want to become a better javascript developer and use best-practice tools and design patterns. Therefore, I would kindly ask if anyone has design pattern suggestions for making my code more efficient, less redundant, and more functional.

Also, the console displays getCurrentPosition() and watchPosition() are deprecated on insecure origins why is this, and is there an alternative for getting the user's position from directly from the browser?

The following is my javascript code:

if (navigator.geolocation) {
   navigator.geolocation.getCurrentPosition(function(position) {

      var latitude = position.coords.latitude;
      var longitude = position.coords.longitude;

      $.getJSON("http://api.openweathermap.org/data/2.5/weather?lat=" + latitude + "&lon=" + longitude + "&APPID=eee5ab7fffb62d126756d9b810ee1875", function(data) {

     var temp = JSON.stringify(data.main.temp);
     //Convert to Ferenheit
     var temp2 = temp * 9 / 5 - 459.67;
     //Round to 2nd decimal place
     var temp3 = Math.round(temp2 * 10) / 10;
     //Display
     $('#temperature').html(temp3 + " F");

     //Description
     var description = data.weather[0].description;

     //Wind speed
     var wind = JSON.stringify(data.wind.speed);

     //HTML disaply
     $(".report").html("
" + data.name + "
" + "
" + description + "

" + wind + " knots
");

     //Toggle Logic
     $('#celsius').on('click', function() {
       var celsius = temp - 273.15;
       var celsius2 = Math.round(celsius * 10) / 10;
       $('#temperature').html(celsius2 + " C");
       $('#celsius').removeClass('btn-default').addClass('btn-primary');
       $('#ferenheit').removeClass('btn-primary').addClass('btn-default');
     });

     $('#ferenheit').on('click', function() {
       var temp = JSON.stringify(data.main.temp);
       var temp2 = temp * 9 / 5 - 459.67;
       var temp3 = Math.round(temp2 * 10) / 10;
       $('#temperature').html(temp3 + " F");
       $('#ferenheit').removeClass('btn-default').addClass('btn-primary');
       $('#celsius').removeClass('btn-primary').addClass('btn-default');
     });

     //Icons logic
     if (description == "broken clouds" || "scattered clouds") {
       $("i").addClass("wi-cloudy");
     } else if (description == "few clouds") {
       $("i").addClass("wi-cloud");
     } else if (description == "clear sky") {
       $("i").addClass("wi-day-sunny");
     } else if (description == "shower rain" || "rain") {
       $("i").addClass("wi-rain");
     } else if (description == "thunderstorm") {
       $("i").addClass("wi-storm-showers");
     } else if (description == "snow") {
       $("i").addClass("wi-snowy");
     } else if (description == "mist") {
       $("i").addClass("wi-dust");
     };
   });
 });
}

You can find the rest of my code in the gist here.

Move your APPID and API url from your code an into constants. That way, they're readily visible and easily configurable.
Move the creation of your API url into a function, that way it's easily modifiable. In addition, use $.param to construct the query portion of your URL. jQuery encodes the params for you. If you can do ES6, you can simplify using shorthand literal notation and template strings

function getApiUrl(lat, lon){
  return `${API_URL}?${$.param({lat, lon, APPID})}`;
}

Not entirely sure why you need to stringify data.main.temp when the logic comes after it expects a number.
data.main.temp appears to be in Kelvin. It would be better if you name the variable appropriately to show that it is in Kelvin.
Move your temperature calculations into functions, like convertFromKelvinToCelsius and convertKelvinToFahrenheit.
It appears that you only get the data once, but your conversions to Celsius and Fahrenheit happen on click. Why not convert them just once, and on click, just display the pre-calculated values?
In the same spirit, why not render the HTML in advance and just show/hide depending on the click. This avoids you from putting the values in html all the time.
Construct your data first, then do the HTML. Makes it easier to have all your logic up top, and rendering down below. That way, your data is prepared and all in one place.
The <i> part, you're repeating code. Why not determine the class name only and addClass to the <i> once instead of repeating the addClass for each condition.

I'll leave the code to you. Feel free to open up another review for a followup.


Convert Multivalued map to HashMap I have a Multivalued map (javax.ws.rs.core.MultivaluedMap<String, String>) which I want to convert to regular HashMap so I got below code:

private Map<String, String> convertMultiToRegularMap(MultivaluedMap<String, String> m) {
    Map<String, String> map = new HashMap<String, String>();
    if (m == null) {
        return map;
    }
    for (Entry<String, List<String>> entry : m.entrySet()) {
        String qKey = entry.getKey();
        List<String> values = entry.getValue();
        if (values.size() > 1) {
            String val = "";
            int i = 0;
            for (String s : values) {
                if (i > 0) {
                    val += ",";
                }
                val += s;
                i++;
            }
            map.put(qKey, val);
        } else {
            map.put(qKey, values.get(0));
        }
    }
    return map;
}

Is there any improvement I can do here? I am using Java7.

The main part of the code is converting the List<String> value into a String delimited by a comma. Instead of using an external i variable, you can use a StringBuilder and append the comma or not depending on whether it is empty or not.

Regarding your current code:

Try to avoid concatenating String with +. You should use a StringBuilder when necessary.
You don't need to make a separate code path for the case where the list is empty in the map value: the regular path handles it also.
You don't need to store the key and the value in local variables.
It is indeed a very good idea to return a new empty map instead of null when the incoming map is null.
If the value stored in the multimap is an empty list, then your code will throw an exception because it tries to access the element 0 when there is no such element.

This would be a proposed code for Java 7:

private Map<String, String> convertMultiToRegularMap(MultivaluedMap<String, String> m) {
    Map<String, String> map = new HashMap<String, String>();
    if (m == null) {
        return map;
    }
    for (Entry<String, List<String>> entry : m.entrySet()) {
        StringBuilder sb = new StringBuilder();
        for (String s : entry.getValue()) {
            if (sb.length() > 0) {
                sb.append(',');
            }
            sb.append(s);
        }
        map.put(entry.getKey(), sb.toString());
    }
    return map;
}

As a side-note, when you'll upgrade to Java 8, this code can be made simpler by using the new String.join(delimiter, elements) method.


Checking a MySQL connection and refreshing it I am using the following function to check if a MySQL connection is in an invalid state or if it has reached a predefined maximum operation count:

 /// <summary>
 /// Holds the amount of operations for the currently mysql-connection open
 /// </summary>
 private int connectionUsageCount = 0;

 /// <summary>
 /// The maximum usage count of the connection until it has to be refreshed
 /// </summary>
 private const int MAX_CONNECTION_USAGE_COUNT = 100;

 /// <summary>
 /// Holds the cached MySql Connection to the databse server.
 /// </summary>
 private MySqlConnection mySqlConnection;

 /// <summary>
 /// Checks the status of the sql connection and refreshes it when necessary.
 /// </summary>
 private void CheckSqlConnection()
 {
     // Check if the connection is in an invalid state or
     // the connection has to be refreshed and refresh it
     // when necessary
     if (mySqlConnection == null
         || mySqlConnection.State == System.Data.ConnectionState.Broken
         || mySqlConnection.State == System.Data.ConnectionState.Closed
         || connectionUsageCount >= MAX_CONNECTION_USAGE_COUNT)
     {
         // If the connection was already initialized, close and dispose it
         if (mySqlConnection != null)
         {
             // If connection is open, close it
             if (mySqlConnection.State != System.Data.ConnectionState.Broken
                 && mySqlConnection.State != System.Data.ConnectionState.Closed)
             {
                 mySqlConnection.Close();
             }

             mySqlConnection.Dispose();
             mySqlConnection = null;
         }

         // Create new connection
         mySqlConnection = new MySqlConnection(this.ConnectionString.ToString());
     }
 }

Can anyone review this piece of code and tell provide me with some feedback? I am not sure if I have covered all possibilities that would cause an error when using the mySqlConnection.

As per this SO question you need to be aware that calling Close will not necessarily actually close the connection but only put it back into the connection pool.
There is no reason to do mySqlConnection = null;. You assign a new value to mySqlConnection in the next statement so setting it to null achieves nothing.
These docs might be somewhat out-dated but they state that you can call Close multiple times and that no exception is generated so you can probably skip the check and just call Close unconditionally.


Follow up to Top Active Answerers on Stack Exchange site This is a follow up to Top Active Answerers on a Stack Exchange site

Actual SEDE Query --> Top Active Answerers on site

Do I have a good understanding of the RANK function?  It seems pretty simple.

Should I separate this into 2 queries or is having 2 rank columns an acceptable thing?  If it is acceptable have I displayed it properly? or does it look messy?

--@DaysSinceActivity is the amount of days that you would like to scan for activity
--@NumberOfUsers is the number of users you want to return.

SELECT
    TOP ##NumberOfUsers:int?1000##
    Users.Id AS [User Link],
    Users.DisplayName AS UserName,
    COUNT(Posts.Id) AS [Total Answers],
    RANK() OVER (ORDER BY (COUNT(Posts.Id)) DESC) AS [Total Answer Rank],
    CAST(AVG(CAST(Score AS float)) AS NUMERIC(6,2)) AS [Average Answer Score],
    RANK() OVER (ORDER BY (CAST(AVG(CAST(Score AS float)) AS NUMERIC(6,2))) DESC) AS [Avg Score Ranking]
FROM
    Posts
INNER JOIN
    Users ON Users.Id = OwnerUserId
WHERE
    PostTypeId = 2
    AND CommunityOwnedDate IS NULL
    AND ClosedDate IS NULL
    AND Users.LastAccessDate > DATEADD(DAY, -##DaysSinceActivity:int?31##, GETDATE())
GROUP BY
    Users.Id, Users.DisplayName
HAVING
    COUNT(Posts.Id) > 10
ORDER BY
    [Average Answer Score] DESC

The Ranking on those two rows seems really redundant because they are calculated fields.

The DisplayName in the GROUP BY is because I didn't know where else to put that, when I removed it I got an error saying

  Column 'Users.DisplayName' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause.

I will probably use it in the ORDER BY statement, I think I can get away with that.

This is a substantial improvement over the previous version. The code is consistent, I do not see any bugs, it's generally good.

There are two further opportunities for improvement that I can see.

I can tell you do this to get an accurate average, and round it to 2 decimal places. The concept is good:

CAST(AVG(CAST(Score AS float)) AS NUMERIC(6,2))

but, there's a trick with float that simplifies things a bit. Consider:

CAST(AVG(Score * 1.0) AS NUMERIC(6,2))

I know it is small, but the implicit cast makes things easier to read.
You duplicate calculations once for the base value, then again for the rank, and again in the having clause. These complex aggregates can be simplified by using a CTE. Calculate the raw data in one CTE and do the calculations in that, then, use that raw data as part of the outer select. Describing it is harder than just doing it. Consider the following query which produces the same results as yours:

WITH RawData AS (

    SELECT
        Users.Id AS UserId,
        Users.DisplayName AS UserName,
        COUNT(Posts.Id) AS AnswerCount,
        CAST(AVG(1.0 * Score) AS NUMERIC(6,2)) AS AvgScore
    FROM
        Posts
      INNER JOIN
        Users ON Users.Id = OwnerUserId
    WHERE
        PostTypeId = 2
        AND CommunityOwnedDate IS NULL
        AND ClosedDate IS NULL
        AND Users.LastAccessDate > DATEADD(DAY, -##DaysSinceActivity:int?31##, GETDATE())
    GROUP BY
        Users.Id, Users.DisplayName

)
SELECT
    TOP ##NumberOfUsers:int?1000##
    UserId AS [User Link],
    UserName AS [UserName],
    AnswerCount AS [Total Answers],
    RANK() OVER (ORDER BY AnswerCount DESC) AS [Total Answer Rank],
    AvgScore AS [Average Answer Score],
    RANK() OVER (ORDER BY AvgScore DESC) AS [Avg Score Ranking]
FROM
    RawData
WHERE
    AnswerCount > 10
ORDER BY
    AvgScore DESC

Note how the complex aggregates happen in just one place.