Not duplicating any of @Peilonrayz's code review points ...

Stop reading entire files into memory when you can process the file line by line in one pass, and stop creating huge lists in memory which are then iterated over exactly once.  Both of these things creates a huge unnecessary memory pressure which can be avoided by looping and/or using generator expressions.

Using a simple loop over all lines in the file:

def average_spam_confidence(filename):
    with open(filename) as file:
        count = 0
        total = 0
        for line in file:
            if line.startswith("X-DSPAM-Confidence: "):
                try:
                    total += float(line[20:])
                    count += 1
                except:
                    pass   # Line ended with garbage
        return total / count

No regex.  No reading entire file in memory.  No creating a list of individual confidence values to sum up afterwards.

Plus, we've fixed a bug!  If "X-DSPAM-Confidence:" appears in the middle of a line, instead of at the start as required by the problem text, we don't try to process it.

But perhaps you wanted a more functional way of programming, where you:

find all the confidence lines,
extract the confidence values, and
compute the average

... all as separate steps which you can compose together, and reuse to solve other problems.  Fear not!  We can do that too!  Enter generator expressions:

First, let's open the file, and read all the matching lines into a list, using list comprehension:

with open(filename) as file:
    lines = [line for line in file if line.startswith("X-DSPAM-Confidence: ")]

That second statement loops through each line in the file, checks if the line begins with the desired text, and if so, includes it in the list that is being constructed.  We can later iterator over lines to further process each line individually.

That is almost what we want to do.  Well, that is exactly what we want to do, but we don't want to do it all at once.  If we change the [...] to (...), we move from list comprehension to a generator expression.

with open(filename) as file:
    lines = (line for line in file if line.startswith("X-DSPAM-Confidence: "))

Now, the second statement has done ... nothing.  We haven't read the first character of the file yet.  What we have returned is a generator expression, which when we ask for the first value will start reading lines until it finds one that matches, and then it will pause its execution and return that value.

Ok.  Let's extract just the confidence values:

    values = [ line[20:] for line in lines ]

Whoops!  That's list comprehension.  It will loop over all the lines that the lines generator can produce, skip over the prefix and return the rest.  Again, change those [...] to (...):

    values = ( line[20:] for line in lines )

Better!  Now those are still strings, so we'll need to convert them into float point values.  Too easy.  Just map them:

    confidences = map(float, values)

confidences is a generator.  If you said list(confidences), you'd create that in-memory list of float values for all the "X-DSPAM-Confidence:" values in the file.  And you could then sum(...) and len(...) the list of values to compute the average.  But we don't want to realize the list in memory, so ...

def average(data):
    total = 0
    count = 0
    for value in data:
        total += value
        count += 1
    return total / count

average_confidence = average(confidences)

... we ask for values from the confidences generator, add them up one at a time, counting as we go.  When the generator is exhausted, the for loop ends, and we return the average.

Putting it all together:

def average(data):
    total = 0
    count = 0
    for value in data:
        total += value
        count += 1
    return total / count

def average_spam_confidence(filename):
    with open(filename) as file:
        lines = (line for line in file if line.startswith("X-DSPAM-Confidence: "))
        values = ( line[20:] for line in lines )
        confidences = map(float, values)
        return average(confidences)

Or more simply:

import statistics

def average_spam_confidence(filename):
    with open(filename) as file:
        values = (line[20:] for line in file
                            if line.startswith("X-DSPAM-Confidence: "))
        return statistics.mean(map(float, values))

Note: the non-generator solution was more robust converting the confidence value strings into floats, via a try...except block.  The generator expression solution shown above omits that.  The robustness may be improved by using a more precise matching when searching for the "X-DSPAM" lines (regex).  Alternately, a generator function could be used, which discards the non-float values.

def map_to_float(data):
    for value in data:
        try:
            yield float(value)
        except:
            pass

confidences = map_to_float(values)

Note (from @Roland Illig's comment): Because generator expressions delay the execution of their operation, any resources they use must remain available until their processing has been finished.  They cannot be used to compute the average spam confidence if the file they are reading from is closed before the average has been computed.  In the above examples, the generator expressions are fully consumed within the body of the with open(...) as file: block, so the file was kept open.

This does not mean the generator expressions must all occur within the with statement.  They can be spread out across many functions, but their execution must be constrained to the interval when the file is open:

def find_occurrences_in_file(input_file):
    """Return a generator for occurrences of X-DSPAM lines"""
    return (line for line in input_file if line.startswith("X-DSPAM-Confidence: "))

def retrieve_values(input_list):
    """Return a generator for float values from a list"""
    return map(float, (line.split()[-1] for line in input_list))

def average(input_list):
    """Compute average of a list/sequence/iterable/generator of values..."""
    return statistics.mean(input_list)

def average_spam_confidence(file_name):
    with open(file_name) as file:
        # File is open for this entire with statement.
        occurrences = find_occurrences_in_file(file)
        values = retrieve_values(occurrences)
        print(average(values))
        # File is closed here - but "average(...)" has exhaustively read
        # from all of the generators already.


If I understood you correctly what you need is to make this method faster.
Firstly, you need to answer one simple question is that really what you NEED, this method does not look that time consuming at all (unless you have hundreads of elements in that string). Have you benchmarked it and saw that it consumes a lot of time?

EDIT: it came to me that you might be asking about performance of your idea, if so - for me it looks good enough.

If you really need to make this method faster here is what I can see:

1. Do not use foreach loop. If you decided to make uniform.Split('.') a list take advantage of it. Change your loop to for

2. Do not use LINQ at all, it is known for being time consuming (since it enumerates everythingcall enumerator in every function).

partsToReplace.Any() && !partsToReplace.Contains(part)

can be changed to for loop when you do Any by List.Count method and Contains by simply checking every element. Exactly the same thing is applicable to if (figure.Contains(part + "-")) and var indexOfItem = figureParts.ToList().FindIndex(value => value.StartsWith(part));

in the latter I think you do not need to make ToList() but I have no compiler at hand to check it.

Note
Those changes are micro optimalisations, if your application is slow it is probably because of some other part. Plase do propper benchmark to check this.

Also : I think there should be List<string> figureParts = figure.Split('.').ToList();


There is a bug here:

foreach (@output) {
    if (($_ =~ m/(\d+)% (.*)/) && ( $1 > $threshold )) {
      chomp($svr); $msg = "$svr: $2 volume  is $1 percent full\n" }
    }

If there are more than one partitions full, $msg will contain only the last one. I think you want to append to $msg instead of overwriting. I suggest to rewrite like this:

chomp(my $svr = `hostname`);

foreach (@output) {
    if (m/(\d+)% (.*)/ && $1 > $threshold) {
        $msg .= "$svr: $2 volume is $1 percent full\n";
    }
}

Explanation of other improvements:

Removed unnecessary brackets from the if condition
Simplified $_ =~ m// to just m// which does the same
Instead of chomp($svr) for each line, do it only once, right after initialization
Reindented, so it's easier to see the block of code that belongs to the loop

Lastly, you can drop "defined" in if ( defined $msg ) {


It is unclear what n and st are in your code and you are mixing things together which makes fixing your code harder.

I therefore suggest refactoring your code:

extract the factorial calculation in its own method, without all the string conversion and input reading stuff:

private static BigInteger factorial(int n) {
  BigInteger fact = new BigInteger("1");
  for (int i = 1; i <= n; i++) {
    fact = fact.multiply(BigInteger.valueOf(i));
  }
  return fact;
}

adapt your main method accordingly

Once you have done that, you can start optimising the factorial method, for example by caching the results as you calculate them.


One thing that always comes to my mind when seeing code like that is one of the first principles in programming that everyone learns.

Don't repeat yourself.

In your code, the part that stands out is mod year n == 0; let's get away with it first:

isLeapYear :: Int -> Bool
isLeapYear year = divisibleBy 400 || (divisibleBy 4 && not $ divisibleBy 100)
    where
        divisibleBy x = mod year x

Using such short helpers is very convenient in Haskell. In my opinion it's one of the most important reasons for the code staying readable (contrary to some newcomers' opinions who just pile up code in one giant expression).

Now, what I don't like here is the full name for the year; I know, I know, it's descriptive, but bear with me:

type Year = Int

isLeapYear :: Year -> Bool
isLeapYear y = divisibleBy 400 || (divisibleBy 4 && not (divisibleBy 100))
    where
        divisibleBy x = mod y x == 0

Now, this is the point where I'd really, really leave it be.


I have a few nitpicks:

If you use assertFalse and assertTrue instead of assertEquals(something, True/False) where appropriate, the output of your unit tests will make more sense when they fail. (http://docs.python.org/2/library/unittest.html#assert-methods)
The argument order of assertEquals is expected, actual. Again, if you use the correct order the error messages will work as intended.
A more common style for when you want a block of code to execute a fixed number of times (like in for count in range(0, 3):) is: for _ in xrange(3):. The _ tells the reader that the value is unimportant, and xrange uses less memory since it doesn't create a list.


Simple surcharge algorithm could use DRY

From a short review;

You are dealing with amounts, but you call your variable num1 and num2
Your code now has tons of magic numbers, you should use named constants
I understand you are not yet in to 'functions within functions', but that is how you apply DRY in this case

So the DRY version could look like this;
function addSurcharge(amount){
  const FLOOR = 10;
  const CEILING = 20;

  if (amount <= FLOOR) {
    return amount + 1;
  } else if (amount<= CEILING) {
    return amount + 2;
  } else {
    return amount + 3;
  }
}

function addWithSurcharge(amount1, amount2) {
  return addSurcharge(amount1) + addSurcharge(amount2);
}


First, you have improved yourself. But I still see the habit of your

 int loop = 0;

        while (loop == 0){...

structure in both the programs you have written so far (Your program doesn't actually iterate as you don't change the loop variable). Actually, while loops are faster than any other loops. On the other hand, it is also important to write more clean & clear code.

As your loop-body is not more than 5000 or more lines, it's better to use a do-while(){...} looping structure.

Polymorphism (Factory Pattern + Strategy Pattern)

You can use Polymorphism in your program to make it nice. (If you don't know what is polymorphism)

Switch cases are good but replacing switch-case with polymorphism is fantastic. Look, you have different things to do in each switch case. Basically, different shapes formed with different ways.

Shape Interface

public interface Shape {

    double calculateArea();
    void buildShape(Scanner scanner);
}

ShapeClub.java (Main class)

public class ShapeClub {

    public static void main(String[] args){

        Scanner scanner = new Scanner (System.in);
        boolean retry = false;

        do{
            System.out.println("Which shape do you want to find the area?");
            Shape unknownShape = ShapeFactory.getShape(scanner.next());
            unknownShape.buildShape(scanner);

            System.out.print("Do you want to try a different shape?[Y/N]:");
            retry = scanner.next().equalsIgnoreCase("Y");
        }while (retry);
        scanner.close();
    }
}

ShapeFactory.java (Factory class)

public class ShapeFactory {

    private ShapeFactory(){}

    public static Shape getShape(String shapeName){
        switch (shapeName){
            case "rectangle":
                return new Rectangle();
            case "circle":
                return new Circle();
            default:
                return null;
        }
    }
}

Rectangle.java (Concrete class)

public class Rectangle implements Shape {

    private double length = 0.0;
    private double width = 0.0;
    private String unit ="";

    @Override
    public double calculateArea() {
        return length*width;
    }

    @Override
    public void buildShape(Scanner scanner) {
        System.out.println("Please enter the leghth of the rectangle: ");
        length = scanner.nextDouble ();
        System.out.println("Please enter the width of the rectangle: ");
        width = scanner.nextDouble();
        System.out.println("What is the unit of the shape?");
        unit = scanner.next();
        System.out.println(calculateArea()+unit);
    }
}

Circle.java (Another concrete class)

public class Circle implements Shape {

    private double radius = 0.0;
    private String unit ="";

    @Override
    public double calculateArea() {
        return Math.PI*(Math.pow(radius, 2));
    }

    @Override
    public void buildShape(Scanner scanner) {
        System.out.println("Please enter the radius of the circle: ");
        radius = scanner.nextDouble();
        System.out.println("What is the unit of the shape?");
        unit = scanner.next();
        System.out.println(calculateArea()+unit);
    }
}

Hope you understood it. :)


An opinion question, always tough to answer.

I like the first way better, 1 less parameter and it is easier to grok for me.

Incidentally, ABAP uses forward slashes for namespaces as well.


Generate short database identifier names from an integer

The trick is to find a formula for where a new place has to be added. In this case the formula is:

$$ d_k = 26*(36^k-1)/35 $$

If n >= dk then the name of n has at least k+1 digits.

The turnover points are 26, 962, 34658, 1247714, ...

name 24          = y
name 25          = z
name 26          = aa
name 27          = ab

name 960         = z8
name 961         = z9
name 962         = aaa
name 963         = aab

name 34656       = z98
name 34657       = z99
name 34658       = aaaa
name 34659       = aaab

name 1247712     = z998
name 1247713     = z999
name 1247714     = aaaaa
name 1247715     = aaaab

name 44917728    = z9998
name 44917729    = z9999
name 44917730    = aaaaaa
name 44917731    = aaaaab

After finding k, just represent n - dk as a k+1 digit number base 36.

Some Python code:

theChars = "abcdefghijklmnopqrstuvwxyz0123456789"

# the magic formula:
def dk(k):
  return (26 * (36**k-1)) / 35

def name(n):
  # find the largest k s.t. dk(k) <= n
  k = 0
  while dk(k) <= n:
    k += 1
  k -= 1
  # take k+1 digits mod 36
  q = n - dk(k)
  digits = ""
  while k >= 0:
    r = q % 36
    q = q / 36
    digits = digits + theChars[r]
    k -= 1
  return digits[::-1]  # shortcut to reverse a string

def testForDuplicates(a,b):
  seen = set()
  for n in xrange(a,b):
    m = name(n)
    if m in seen:
      print "duplicate found at n =", n, ", name:", m
    seen.add(m)

def printRange(a,b):
  for n in xrange(a,b+1):
    print n, " ->", name(n)

printRange(20,30)
printRange(955,965)
printRange(34650,34662)
printRange(1247710,1247720)
printRange(44917727,4491773)

testForDuplicates(0,1000000) # takes about 6 secs


Building a list of dates between two dates

How about...

def date_count(start, end, day_of_month=1):
    dates = [start]
    next_date = start.replace(day=day_of_month)
    if day_of_month > start.day:
        dates.append(next_date)
    while next_date < end.replace(day=day_of_month):
        next_date += relativedelta(next_date, months=+1)
        dates.append(next_date)
    return dates

And by the way it seems like a nice opportunity to use yield, if you wanted to.

def date_count2(start, end, day_of_month=1):
    yield start
    next_date = start.replace(day=day_of_month)
    if day_of_month > start.day:
        yield next_date
    while next_date < end.replace(day=day_of_month):
        next_date += relativedelta(next_date, months=+1)
        yield next_date

Another possibility - discard the first value if it is earlier than the start date:

def date_count(start, end, day_of_month=1):
    dates = [start.replace(day=day_of_month)]
    while dates[-1] < end.replace(day=day_of_month):
        dates.append(dates[-1] + relativedelta(dates[-1], months=+1))
    if dates[0] > start:
        return [start] + dates
    else:
        return [start] + dates[1:]

Or use a list comprehension to iterate over the number of months between start and end.

def date_count(start, end, day_of_month=1):
    round_start = start.replace(day=day_of_month)
    gap = end.year * 12 + end.month - start.year * 12 - start.month + 1
    return [start] + [round_start + relativedelta(round_start, months=i)
                      for i in range(day_of_month <= start.day, gap)]

Finally, I don't know dateutil but it seems you can use rrule:

from dateutil import rrule
def date_count(start, end, day_of_month=1):
    yield start
    for date in rrule(MONTHLY,
                      dtstart=start.replace(day=day_of_month),
                      until=end.replace(day=day_of_month)):
        if date > start:
            yield date


Speed up a program to find the minimum Hamming distance

Considering that this is meant to be a puzzle, I'll write my review as a sequence of escalating hints.

Everybody who is suggesting the use of threads or std::vector or matrix libraries is on the wrong track! How do you think error correction is implemented in your CD player or hard disk firmware? No threads or matrix libraries there, I assure you.
Don't simulate the computer; use the computer.
All operations are modulo 2.  The puzzle guarantees k ≤ 16. Under these conditions, you can massively parallelize vector dot products. (Think SIMD or AltiVec, except you don't need SIMD or AltiVec.)
I've added the bitwise tag to this question.

I'll conclude with a remark that you've basically written C code.  The only thing that makes it C++ is your use of iostream.  C++ can be much more expressive than that.


Checking string length limitation and add items to it in a shorter way with Linq

Reduce nesting by putting a negated condition first:

if (! needDefaultAdd)
{
    return text;
}


Caling AJAX Method for multiple hidden ids aspx

You should create a function that would wrap the ajax call and use it as many times as you wish.

var callMyMethod = function(nodeType, clientId, target) {
  var ids = $('#' + clientId).val();
  ajaxMethod('/MyPage/MyMethod', {
    custId: custId, // is it a global variable?
    vehicleNodeType: nodeType,
    selectedIds: ids
  }, target);
};

callMyMethod('<%=VechileNodeTypeEnum.Car.ToString()%>', '<%=hfCarId.ClientID%>', '#carDiv');
callMyMethod('<%=VechileNodeTypeEnum.Van.ToString()%>', '<%=hfVanId.ClientID%>', '#vanDiv');
callMyMethod('<%=VechileNodeTypeEnum.Truck.ToString()%>', '<%=hfTruckId.ClientID%>', '#truckDiv');
callMyMethod('<%=VechileNodeTypeEnum.MotorCycle.ToString()%>', '<%=hfMotorCycleId.ClientID%>', '#motorCycleDiv');

If there is some correlation between for example hfCarId.ClientID and #carId, you can remove the target argument from the function.

It should be also possible to generate these calls by iterating over Car, Van, Truck and MotorCycle but I'm not that familiar with .net to suggest it to you.


A couple of things to try:

You assemble a map at every level of nesting. It is probably quicker
to pass up sequences of key/value pairs.
Even the single entries return a map. We can cope with a
MapEntry directly using reduce instead of for.

We might end up with ...

(defn flatten-record [record]
  (reduce
   (fn [a [k v]]
     (if (map? v)
       (into a (flatten-record v))
       (conj a (clojure.lang.MapEntry. (match-table k k) v))))
   []
   record))

I'm assuming it's quicker to build a MapEntry than a small vector or list.

The above returns a sequence. We have to convert the product into a map:

(def example-input {:foo {:foo-host "host-1" :user {:foo-user "user-1" :foo-id "id-1"}}
                    :bar {:bar-var "potato"}})

(into {} (flatten-record example-input))
;{:host "host-1", :user "user-1", :id "id-1", :var "potato"}

The recursive call of flatten-recordis not in tail position, so neither recur nor trampolineare available.


Formatting a price in Swift using an extension vs. a class

Should I use an extension instead of classical class to encapsulate "formatter", "serializers", etc?

No, it should not be an extension to the numeric type.

A couple of observations:

This formatter should really work with a variety of types (Double, Float, Decimal, etc.), so burying it inside a particular numeric type’s definition isn’t right. Theoretically you could consider an extension to a protocol, e.g. Numeric, instead, but attaching it to a particular type is certainly not right.
Formatters often have properties of their own. E.g. for a currency formatter, you’d probably have a locale for number formatting, currency symbol, preferred decimal places for that currency, etc., so that speaks to having the formatter as a type, itself, not just some computed property for the numeric type that is being displayed. It also affords the idea of having a single instance of a formatter that you use repeatedly for every underlying object that needs to be displayed.
Note, once you realize that you’re likely to want to configure a formatter once and use it repeatedly, it no longer makes sense to make the value a property of this formatter type. It should be a parameter that we pass to our method for creating a string representation.
By the way, formatters are not just “represent this object as a string”, but also “parse this string into an object” (and sometimes even “is this a valid substring for this type” which you might use to validate input as the user enters it). Once you consider formatters in this context, the notion of burying this in the underlying numeric types starts to run afoul of the “single responsibility principle”. A formatter is, effectively, an object for translating model objects to and from presentations in the UI.
For all of the aforementioned reasons, Foundation has a long tradition of separating data types and their formatters. See Formatter. And there are a litany of existing formatters. You should have a fairly compelling reason before deviating from this well established pattern.
Needless to say, I wouldn’t advise writing your own currency formatter at all when there are existing formatters that do the job very well. As Apple says:

  Before you decide to create a custom formatter, make sure that you cannot configure the public subclasses to satisfy your requirements.

I’d suggest just using NumberFormatter with a numberStyle of one of the currency types (e.g. .currency, .currencyAccounting, etc.).

But I assume this is for illustrative purposes only.
While your code snippet was a “formatter”, you mention serialization.

That’s a different situation where you are often serializing objects with multiple properties of different types. And the serialization is often being handled by an existing serializer objects (e.g., for encoding, JSONEncoder and PropertyListEncoder, etc.). To support serialization for your custom types, you define your type to conform to the Codable protocol.
While presentation and parsing of values in the UI is often handled by separate formatter objects, it’s worth noting that there are a few string representations that we might add to our custom types for debugging purposes. For example, we might conform to CustomStringConvertible so that we can print our values.  Value types (struct) have a decent default string representation, but especially for reference types (class) it can be nice to customize this.

But don’t be tempted to use description as a backdoor for formatting strings in your UI. As Apple says,

  Accessing a type’s description property directly or using CustomStringConvertible as a generic constraint is discouraged.

But for diagnostic purposes, this can be useful. Also see CustomDebugStringConvertible.


Printing the status of a database insertion as JSON

This looks like you are trying to create a REST interface. If this is true, the following remarks apply:

First of all, your path should not include the action. The resource should be named "record" and the action is determined by the HTTP verb, i.e. "insert" is implied by POST, retrieve by GET, modify/update by PUT, etc.
Success or failure should primarily be communicated via HTTP status. For a POST, customary responses are either code 200 (OK) with the complete object as a result (often modified by the insertion, e.g. having a unique id now) or 204 (no content) to signify that everything is OK without sending the object back.
Likewise, failure should be communicated as a status code in the 500 range (server error, e.g. if the database connection fails) or 400 range (bad request, e.g. wrong data types transmitted by the client)

Generally, look around for "REST best practices", you'll find lots of information on the topic.


From a once over:

Your code is not consistently indented, especially the first dozen lines need some TLC
$.component = function( name, object ) { needs commenting, it is not clear what this is trying to achieve.
evt is an unfortunate parameter name, I would go for the classic e or the readable event
$targ is needlessly short, just use $target
You could probably use helper functions for if(evt) evt.preventDefault(); and if(evt) evt.preventDefault(); evt.stopPropagation();
Note that if(evt) evt.preventDefault(); evt.stopPropagation(); is exactly the kind of bug that makes people enforce {} around if blocks!
toggle() could use a ternary condition with !this.isShowing
'show-mobile-nav' should probably not be hardcoded since you use it more than once, instead you should use a properly named variable

All in all, some pretty cool code.


By moving the build statement into a service you actually aren't saving yourself much, even after the refactor you still have a bunch of code that isn't DRY. Depending on how big your application is I would move pretty much all of the logic for your create actions into either ApplicationController, or, more likely a module. The module could look like this:

module ShoutController
  def create
    shout = current_user.shouts.build(content: build_content)
    redirect_params = shout.save ? {} : {notice: 'not created'}
    redirect_to dashboard_path, redirect_params
  end
end

Then your controllers would simply be:

class PhotoShoutsController < ApplicationController
  include ShoutController

  private

  def build_content
    PhotoShout.new(params.require(:photo_shout).permit(:image))
  end
end

And

class TextShoutsController < ApplicationController
  include ShoutController

  private

  def build_content
    TextShout.new(params.require(:text_shout).permit(:body))
  end
end


The basic idea of your cache, as I understand it, is that it is a map of weak pointers connected to shared pointers with custom deleters that remove them from the map. As you noticed yourself, that's a risky pattern, because if any resources outlive the cache, you get UB.

Is it really necessary for objects to remove themselves from the cache? What if you just let them destroy themselves naturally, and leave the empty weak pointer in the cache. Then your lifetime worries fade away. From time to time you can purge the cache of entries where expired() == true if you really want, and other functions of the cache will have to take into account expired entries (like size() for example). It even makes your cache more flexible, because if you're not using the deleter you allow the possibility of users providing their own.

I also question the wisdom of keeping the "creator" function object in the cache, for a couple of reasons:

It makes the cache itself so much clunkier to use, because you have to specify all the arguments to the create function in the template parameter list.
It makes the cache less flexible, because there may be multiple ways to create (for example) a texture (such as from a file, or from a memory buffer), so you might want different create function signatures.

Rather than hard-coding the create function's signature in the type, and deciding on the create function for everything in cache at cache construction time, why not just pass an optional create function when and where it's needed?

I'll bring these points up again as I go through the code.

PtrCache(const std::function<Val*(Args...)>& creator) :
  _creator(creator),
  _mutex(std::make_unique<std::mutex>())
{}

Single-argument constructors should pretty much always be explicit.

Since this is a pure sink function, you should really take the arguments by value, not by const&. And then you should move them into their final place:

explicit PtrCache(std::function<Val*(Args...)> creator) :
  _creator{std::move(creator)},
  _mutex{std::make_unique<std::mutex>()}
{}

But I would suggest not having the _creator member variable, so you don't need this constructor at all. Or the Args... template parameter for the class either.

PtrCache(PtrCache&& other) :
  _cache(std::move(other._cache)),
  _creator(std::move(other._creator)),
  _mutex(std::move(other._mutex))
{}
PtrCache& operator=(PtrCache&& other)
{
  if (this != &other) {
    _cache = std::move(other._cache);
    _creator = std::move(other._creator);
    _mutex = std::move(other._mutex);
  }
  return *this;
}

There's no need to write out either of these, because they're just the default implementations. You can just do:

PtrCache(PtrCache&& other) noexcept = default;
PtrCache& operator=(PtrCache&& other) noexcept = default;

Also, both can be noexcept (and move ops always should be).

bool getOrCreate(const Key& key, std::shared_ptr<Val>& ret, Args... args)
{
  std::lock_guard<std::mutex> lock(*_mutex);
  auto it = _cache.find(key);
  if (it != _cache.end()) {
    ret = std::shared_ptr<Val>(it->second); // Construct from weak_ptr
    return true;
  }
  ret = std::shared_ptr<Val>(_creator(args...), [this, key](Val* ptr) { // Construct from creator and pass custom deleter which removes the element from the cache.
    std::lock_guard<std::mutex> lock(*_mutex);
    _cache.erase(key);
    delete ptr;
  });
  _cache[key] = ret;
  return false;
}

This is the meat of the cache, and so the best place to talk about broader problems. But the function has plenty of problems of its own.

First, out-arguments are a bad idea. They're clunky as all hell. Every use of this function - which is the main function of the cache - requires two lines:

std::shared_ptr<GLTexture> result;
texture_cache.getOrCreate("my_texture.png"s, result, "my_texture.png"s);

The natural place for the output of a function is the return value. If you want multiple return values - so you can get that bool as well - use a custom return struct, or a tuple:

std::tuple<std::shared_ptr<Val>, bool> getOrCreate(const Key& key, Args... args);

Now this is much easier to use, especially since C++17:

auto [ result, created ] = texture_cache.getOrCreate("my_texture.png"s, my_texture.png"s);

At a higher level, you've hard-coded the create function into the cache, which makes it less flexible. Rather than doing that, you can take a function object in getOrCreate(), like this:

auto [ tex1, created1 ] = texture_cache.getOrCreate("texture_1", [] { return load_texture_from_file("texture_1.png"); });
auto [ tex2, created2 ] = texture_cache.getOrCreate("texture_2", [] { return generate_procedural_texture(seed_value); });
// and so on

The signature of getOrCreate() now becomes:

template <typename Func>
std::tuple<std::shared_ptr<Val>, bool> getOrCreate(const Key& key, Func&& func);

and the creation line in the function is:

ret = std::make_shared<Val>(func());

Or even better, let the cache objects construct themselves:

template <typename Args...>
std::tuple<std::shared_ptr<Val>, bool> getOrCreate(const Key& key, Args&&... args)
{
    // ...
    ret = std::make_shared<Val>(std::forward<Args>(args)...);
    // ...
}

// used as:
auto [ tex1, created1 ] = texture_cache.getOrCreate("texture_1", from_file, "texture_1.png");
auto [ tex2, created2 ] = texture_cache.getOrCreate("texture_2", noise_texture, seed_value);
// assumes texture_cache holds "texture" objects that have the following constructors, or something similar:
// texture::texture(from_file_t, std::filesystem::path);
// texture::texture(noise_texture_t, std::uint_fast32_t);

There are multiple ways to do all this that are way more flexible than setting the create function and its signature when the cache is created.

This function also seems a little over-complex, using find() and then []. Your map values are weak pointers. There's really no need for all that dancing around to get-or-set them in the map. You can just do:

bool getOrCreate(const Key& key, std::shared_ptr<Val>& ret, Args... args)
{
    std::lock_guard<std::mutex> lock(*_mutex);
    auto& wkptr = _cache[key];
    ret = wkptr.lock();
    if (ret)
        return true;
    // Either the value wasn't in the map, or it was but it expired
    // (so it was effectively removed from the map). Either way,
    // make a new thing and store it in the cache.
    ret = std::make_shared<Val>(_creator(args...));
    wkptr = ret;
    return false;
}

Also, along with everything I mentioned above about the create function stuff, if you're going to make this function a template - either to take a create function object or an argument parameter pack to pass to the value type's constructor - you might as well take advantage of that and use perfect forwarding for the key as well.

Also-also, you use make_unique(), and that's good... but it's not that important. It is, however, important to use make_shared(). You should aim for that in the function's design.

inline void clear()
{
    std::lock_guard<std::mutex> lock(*_mutex);
    _cache.clear();
}

The inline keyword serves no purpose here. Functions defined in the class are inline by default. What would be handy here are const and noexcept. At least const, because locking might throw an error. Same goes for size().

There's another nasty little issue with having a clear() function, though. Your cache objects erase themselves from the cache when the last one goes out of scope, right? Consider this scenario:

Subsystem A creates a cache object with key "foo", and gets an instance.
Subsystem (any!) calls clear() on the cache. Subsystem A's "foo" instance is still valid, of course. It just doesn't have an entry in the cache anymore.
Subsystem B creates a new cache object with the key "foo", and gets an instance.
Subsystem A lets its "foo" instance expire... which triggers the deleter to erase "foo" from the cache.
Subsystem B asks for another "foo" object from the cache, expecting to get an alias of the one it got in step 3 because it knows no-one's called clear() since it got the "foo" object in step 3... instead it gets a whole new "foo".

In other words, the cache becomes unpredictable. Even if I'm holding an object I got from a specified key and I know for sure no one's cleared the cache since I got it, I still can't be sure I'll get the same object from the same key (or any object at all from that key).

std::map<Key, std::weak_ptr<Val>> _cache;

Is there any reason you're not using an unordered map here?

Summary

As you've noted, having cache objects remove themselves from the cache when they expire invites problems. There's no good reason to do that. Just leave the empty weak pointers in the cache, and cull them from time to time (or not; just leave them there and ignore them).
Defining the create function arguments at instantiation time, and the create function itself at construction time, is inflexible, and it complicates the class. It's much more flexible to be able define creation parameters when and where I need them. The class should be PtrCache<Key, Val>, maybe with an allocator option. Having Args... in there is clunky and restrictive.
The most flexible option is probably to let your cache objects' constructors do what constructors are for, and define getOrCreate() as a template function taking arguments that get perfect-forwarded to the constructor if and only if something actually gets constructed.
Use make_shared().
Don't manually redefine implicitly generated functions; just = default them.


Style

final int notFound = -1;
final int minimumArrayLength = 2;

These two fields can be better placed as private static final fields, instead of inside the method. On a related note, you may also want to consider turning this into an "utility class" and have the method as static too:

public class MagnitudePole {

    private static final int notFound = -1;
    private static final int minimumArrayLength = 2;

    private MagnitudePole() {
        // empty
    }

    // using an arguably better-sounding name... MagnitudePole.solveFor(int[])
    public static int solveFor(int[] A) {
        // ...
    }
}

For the currentValue > A[magnitudePole] else-if condition, you can join that with the inner if:

There's a bug here all right... The else should only be done on the original outer condition, so if the inner ifconditions fail it shouldn't reach there. Slightly edited the code block so as not to invalidate the comments on this part. :)

if (currentValue < A[magnitudePole]) {
    isPoleValid = false;
} else if (currentValue > A[magnitudePole] && currentValue > highestSeen && !isPoleValid) {
    isPoleValid = true;
    magnitudePole = i;
} else {
    /* this should only be reached when
        currentValue <= A[magnitudePole],
        bug in preceding else-if! */
}

Finally, in the else part, you can also re-order the conditions around as such:

} else {
    isPoleValid = currentValue >= highestSeen;
    if (isPoleValid) {
      magnitudePole = i;
    }
}

Logic + unit testing

You perform a check for the array to contain at least two elements, which means single-element arrays are conveniently bypassed. Do you actually want to consider them as magnitude poles, since all the LHS and RHS elements ("none", or \$\emptyset\$) are equal to it? :)

I will also suggest writing unit tests as a means of documenting what are the expected outputs for various inputs - valid, invalid, and edge cases in all. The slight benefit is that you can safely optimize your logic later on, and have a safety mechanism to ensure the modifications still work per expectation.


Moving character in Unity3D based on accelerometer input

I know absolutely nothing about c# or unity3d, so if there's a language specific assist, I have no idea about it!

I notice each condition checks Input.acceleration.y, so that's one bit of information that repeats. I also notice we have three speeds, yet we have seven conditionals!

First of all, the negatives are confusing. Find the absolute value first.

I'm going to assume a number followed by an "f" means float. So maybe we need something like:

float acceleration = Math.Abs(Input.acceleration.y);

Great, now it's going to be easier to handle this!

Now all we need is a simple check like so:

if (acceleration < 0.2f) {
    maxSpeed = 17;
} else if (acceleration < 0.9f) {
    maxSpeed = 25;
} else {
    maxSpeed = 40;
}

Your original numbers were a little weird, but if you meant for such a strange pattern, correct me!


Swapping dynamic page div's around on button click

You could improve it a little like this, at least you are not selecting the same elements over and over.

I do think it could improve more, if you swapped the actual div's rather then the html content and umrn value.

var $bed0 = $('#bed-div-' + selected[0].substring(4));
var $bed1 = $('#bed-div-' + selected[1].substring(4));

//get the html of the two bed divs
patientname1 = $bed0.html();
patientname2 = $bed1.html();
pt1UMRN = $bed0.attr('umrn');
pt2UMRN = $bed1.attr('umrn');

//swap the elements around
$bed0.html(patientname2);
$bed1.html(patientname1);

//update the umrn attribute
$bed0.attr({ umrn: pt2UMRN});
$bed1.attr({ umrn: pt1UMRN });

//message the user
$bed0.effect("highlight", {}, 2000);
$bed1.effect("highlight", {}, 2000);


Counting the number of distinct words in the input

Here are some things that may help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Use all the required #include files

The code uses std::string but doesn't include the corresponding file.  The code should have this line added:

#include <string>

Decompose the program into smaller parts

Right now, all of the code is in main which isn't necessarily wrong, but it means that it's not only hard to reuse but also hard to troubleshoot.  Better is to separate the code into small chunks.  It makes it both easier to understand and easier to fix or improve.

Use an appropriate data structure

The current code uses a std::vector to hold the words and linearly searches for each new word.  Far better would be to use a std::unordered_map for this.  Here's a version which not only counts each unique word, but also counts the number of occurrences of each word:

#include <iostream>
#include <string>
#include <unordered_map>

int main() {
    std::unordered_map<std::string, unsigned> dict;
    for (std::string word; std::cin >> word; ) {
        ++dict[word];
    }
    std::cout << "number of distinct words are: " << dict.size() << "\n";
}

There is a significant performance benefit.  I used both the original and this version to count all the words in the Project Gutenberg eBook of Dracula, by Bram Stoker. Both correctly reported 19027 distinct words, but the original took 1.011 s and the version above took 0.057 s (17 times faster).

Don't use std::endl unless you really need to flush the stream

The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.

Use a newer book

The book you have was fine in its day, but it is now woefully out of date.  I'd recommend Stroustrup's book "A Tour of C++" instead, since the current edition covers C++11 which is a very much improved and much different language than previous versions of C++.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


Refactoring a LINQ query that (sometimes) returns null

Your current code generates an INNER JOIN, which as you've noticed, excludes any Applicant without an ApplicantAddress.

You want to select from Applicant and generate a LEFT JOIN on ApplicantAddress, this SO answer shows how to use DefaultIfEmpty() to do that:

var query = from u in usergroups
            join p in UsergroupPrices on u equals p.UsergroupID into gj
            from x in gj.DefaultIfEmpty()
            select new {
                UsergroupID = u.UsergroupID,
                UsergroupName = u.UsergroupName,
                Price = (x == null ? String.Empty : x.Price)
            };

It's hard to tell exactly what/how to change in your code to make it work, because we're not seeing the DbContext and it's not clear how Request turns out hitting the database; I'd rather not say anything than assume what's going on.

In this specific case:

private static Expression<Func<Request, RequestResultModel>> requestResultExpression = request =>
    request.Applicant
            .ApplicantAddresses
            .DefaultIfEmpty()
            .OrderBy(address => address.IsPreferred)
            .Select(address => new RequestResultModel
            {
                Id = request.Id,
                ApplicantName = request.Applicant.FullName,
                ReviewerName = request.Reviewer.FullName,
                RefferrerName = request.Refferrer.Name,
                City = address.Address.City,
                Province = address.Address.Province.Code,
                DisciplineCode = request.Discipline.Code,
                Event = request.Event,
                StatusName = request.Status.Name,
                Submitted = request.Submitted
            })
            .FirstOrDefault();


my question is about idea in general

It's flawed, in several ways.

Premature Optimization

The code smell I get is that the whole point is avoiding instantiating and then disposing Array elements.  Reasons we stay away from optimizing up front

We end up spending too much time on it
Corrupting our business design for the sake of something that we don't even know is an issue.
We don't know yet where and how much actual performance is affected.
We cannot know if our up front optimization is in fact better than what we would have done; it could be worse!

Resizing?

I thought this is what we're trying to avoid.

Where is the "resizing"? I assume it will be in Add() and Remove(). If there is no resizing - the size is fixed at instantiation, then the class name is wrong.

The conventional wisdom says resizing Array is less performant than resizing a List. In fact the C#/.NET team went to great lengths to make sure List automatic resizing performs well. Do you have the time and other resources to make your home-spun Array resizing worth not using what the .NET framework already gives you?

Wrong Perspective

I see the client having to write his code in terms of Arrays and array elements when it should be in terms of your business objects - TradeOrder, for example.

The structure - the array - should not be the emphasis in your design. If you keep going down this road you are in for ugly maintenance problems over time.

Give the business classes "collection friendly" capabilities

I imagine sorting, searching, preventing duplicates, uniqueness, etc. might be important qualities when we make a collection of things. So TradeOrder should implement IEquatable and IComparable.

ResizeableArray, or any other TradeOrder user's code, should not be doing this:

if (trade1.stockName == trade2.stockName && trade1.shares == trade2.shares && ...)

Client code should be able to do this:

if (trade1.Equals(trade2))

Should ResizableArray inherit Array?

Array already implements foreach, for example. You'll get all the built-in goodness and it will be exposed at the 'class level' to the client. There's lots of nifty search and sort methods that take advantage of ICompareable implementation.

  Some fields are never changes so as I reuse elements I need to assign those fields just once

Memory Leaks

As ResizableArray gets used it will have empty and occupied elements scattered throughout the Array. You will end up writing code to scan the array for every Add(). Otherwise you'll be adding new elements when there are empty elements available.

It looks like we leave TradeOrder objects in unused array elements. This is the case at instantiation, clearly. Also removing things involves Count but not nulling the reference there. I guarantee you'll be spending lots of extra code and lots of debugging time trying to keep the Count in synch with the actual active objects.

We don't know what an empty element is

The Count is being incremented/decremented but we're leaving objects in place. I'm assuming that at some point we're done with a given object, in which case it should be disposed of. Besides the memory issue, how do we know what array elements are in use and which ones we can over-write?

  // made public to use in "foreach", but better to be private
      public T[] array;

NO.

ResizeableArray should implement IEnumerable.

You are forcing client code to be written like this:

foreach (var trade in tradeOrders.array)

When they should write:

foreach (var trade in tradeOrders)

This is a violation of OO principles. Single Responsibility, Law of Demeter (least knowledge), Encapsulation ... a discussion for another time.

As a practical matter do not force the client to have to know how to manipulate the class properties. ResizableArray should know how to iterate itself.

The client should not tell the ResizeableArray object to resize, or how.


Program in JAVA to generate random alphanumeric string

for
        (double i = 0; i < length; i++)

And related loops should have the "for" on the same line (as this is a common coding convention.)

(randroll <= (1.0 / 36.0 * j))

This doesn't have to be a double; instead, the random number can be generated as an integer (to select which element from the array.)

randchar = '@';

Unless the random string is not random, I would not initialize the variables with sample data. I'd just leave them blank and then adjust the loop to always run at least once (a do-while loop) so that it becomes initialized.

for
            (int j = 1; j <= 36; j++)
            {
                if
                (randroll <= (1.0 / 36.0 * j))
                {
                    randchar = charstring.charAt(j - 1);
                    break;
                }
            }

I would remove the inner if-statement and un-hardcode the values so it can work with strings with any size. Applying these suggestions, it can be simplified to:

import java.util.Random;
class Main {
  public static void main(String[] args) {
    int strLen = 100;
    String randString = "";
    Random r = new Random();
    String[] chars = "abcdefghijklmnopqrstuvwxyz0123456789".split("");
    while (randString.length() < strLen)
        randString += chars[randBetween(r, 0, chars.length - 1)];

    System.out.println(randString);
  }

  /*
  Generates a random number from min to max inclusive
  */
  public static int randBetween(Random r, int min, int max) {
    return r.nextInt((max - min) + 1) + min;
  }
}

This approach is not optimal as the string is constantly being appended to, meaning that the string has to be re-copied every iteration.

Java introduced Streams, which allows reading forever from certain generators. Knowing this, we can read a stream of random numbers up until the string length that the user wants, and then get the character at the random string length:

import java.util.Random;
class Main {
  public static void main(String[] args) {
    int strLen = 100;
    String chars = "abcdefghijklmnopqrstuvwxyz0123456789";

    StringBuilder randomOutput = new StringBuilder();
    new Random().ints(strLen, 0, chars.length())
                .forEach(c -> randomOutput.append(chars.charAt(c)));

    System.out.println(randomOutput);
  }
}

StringBuilder is used to append the random character as it doesn't have to be re-copied for every loop iteration.


You have made some uncommon stylistic choices which are not to your benefit, and a few things aren't doing what I think you think they're doing.

First, the stylistic elements. Your whitespace is excessive, there's no need to push everything that far to the right and out of line with the beginning of the RHS. I also would move the colon down to begin the next line, this is mostly a matter of personal style. The parentheses around ys are unnecessary and while harmless noise for the compiler will distract your human reader. My version would look like this.

group' xs = takeWhile (== head' xs) (sorted xs)
          : group' (dropWhile (== head' xs) (sorted xs))
    where sorted ys   = mergesort' ys
          head' (x:_) = x

Now to the meat of it. As it is, your definition of sorted is just giving a different name to mergesort'. The two uses in the main body of the function will cause sorted xs to be recomputed each time (ie twice!). The correct definition is not to create an inner function which is passed a list to be sorted as a parameter, but to bind a sorted version of the outer function's parameter.

group' xs = takeWhile (== x) sorted : group' (dropWhile (== x) sorted)
    where sorted = mergesort' xs -- Computed once, the result is used twice
          x = head sorted

You'll notice that I gave the same treatment to head', this is less of a performance hit and more a matter of consistency. If you wanted to be really pedantic you could bind the section as well, but (and this is an art not a science) I feel that this version is less readable.

group' xs = takeWhile p sorted : ...
    where ...
          p = (== head sorted)

This is still doing more work than it needs though! Once you've made the first recursive call, the tail passed to group' will always already be sorted, and doesn't need to be resorted with each recursive step. One way to handle this would be to pull the main functionality of group' into an inner function named something like group'' and change the body of group' to just be group'' (mergesort' xs). Now the list is only sorted once.

I think it makes more sense though to separate the two functions entirely. There is in fact no requirement that the list being grouped is sorted, so let's relax that constraint.

group' :: (Eq a) => [a] -> [[a]]
group' []     = []
group' (x:xs) = (x : takeWhile (== x) xs) : group' (dropWhile (== x) xs)

elementSets :: (Ord a) => [a] -> [[a]] -- Same functionality as original group'
elementSets = group' . mergeSort'

And just so you see what's happening,

> group' [2,2,1,2,2,2]
[[2,2],[1],[2,2,2]]
> elementSets [2,2,1,2,2,2]
[[1],[2,2,2,2,2]]

Note that we can now find the head of the list through pattern matching instead of calling head because we don't need to assume the list may be out of order. We do however have to remember to add that element back to the prefix of what we're returning!

There's one more improvement we can make though! As it is, our group' function will walk the prefix of the list equal to the first element twice each recursive step (once for takeWhile, and once for dropWhile). We can eliminate that inefficiency by using span which returns the prefix matching the predicate and the remainder in one go.

group' (x:xs) = (x : prefix) : group' remainder
    where (prefix, remainder) = span (== x) xs

And there we have it.

Regarding your question about whether head''s pattern matching was exhaustive, the answer is that it is not exhaustive because the definition of head' contained no case which would match every possible pattern (ie head' []). It was however "safe" to use because of how group' was defined, and the case of the empty list was handled through pattern matching at the outer level. In general you shouldn't rely on tricky business like that, it can be difficult to notice when coming back to code you or someone else has written, and can blow up on you if you have to change the definition of the function.


Don't localize your Java code.  You don't want to hand over your source code to your translator, and you don't want to recompile the code to add a support for a new language.  Rather, you should internationalize your code and do all the localization through resources, as recommended in the Android Developers documentation.

Furthermore, localizing strings in isolation doesn't work in the general case.  For example, "monthly" may need to be translated as « mensuel », « mensuelle », « mensuels », « mensuelles », or « mensuellement », depending on context.  Word order also tends to differ between languages.  Therefore, the only sane approach is to translate entire phrases at the point of use.

res/values/strings.xml

<resources xmlns:xliff="urn:oasis:names:tc:xliff:document:1.2">
    <string name="expenses">Your {0,choice, 1#weekly| 2#bi-weekly| 3#monthly| 4#quarterly| 5#annual} expenses are {1,number,currency}.</string>
</resources>

res/values-fr/strings.xml

<resources xmlns:xliff="urn:oasis:names:tc:xliff:document:1.2">
    <string name="expenses">Vos dépenses {0,choice, 1#hebdomadaires| 2#bimensuelles| 3#mensuelles| 4#trimestrielles| 5#annuelles} sont {1,number,currency}.</string>
</resources>

Java code

import java.text.*;
…

Format expensesFmt = new MessageFormat(getResources().getString(R.string.expenses));
textView.setText(expensesFmt.format(new Object[] { MONTHLY.ordinal(), 1337 }));

… to get

Your monthly expenses are $1337.00.

or

Vos dépenses mensuelles sont 1337,00€.


Inefficient Parsec parser to skip unreachable block

First of all, you can batch the skipping of braces (and return ()):

unparsedBlock :: Parser ()
unparsedBlock = braces (skipMany (skipMany (noneOf "{}") <|> unparsedBlock))

Unfortunately, this is also probably quite inefficient, as this still works on a character-by-character basis, and uses stuff like manyAccum under the hood.

If you really need more speed, then you will need to write your own Parser function that does the skipping on the source, while at the same time updating the position in the stream.

That is (one of) the reason(s) attoparsec is much faster : it doesn't keep track of the position in the input stream, and has combinators that work directly on the source (such as skipWhile).


You're trying to solve a solved problem.

Just use CLPlacemark out of Core Location.  It has already solved all the problems you've brought up in your question: how to handle international addresses, what all the necessary fields are, how to handle missing fields, and how to interop with Swift.

Worse case scenario, if CLPlacemark isn't quite what you need, you can either subclass it or model your place object closely around this object.


Bug:

public void insertEnd(Object item){
    SListNode lastNode = this.head;
    while(lastNode.next != null){
        lastNode = lastNode.next;
    }
    lastNode.next = new SListNode(item,lastNode.next);
    size++;
}

Make a new list, add to end. this.head is null, so lastNode.next is a NullPointerException.

Bug:

public void insertAt(Object item, int position){
    if (position == 1){
        insertFront(item);
        size++;
    }else if(position < 1){
        System.exit(1);
    }else{
        SListNode currentNode = this.head.nth(position);
        if(currentNode != null){
            currentNode.insertAfter(item);
            size++;
        }else{
            System.exit(1); //position value exceeds
        }
    }
}

Make new list, insertAt("hello", 1), insertAt("world", 2). The application shuts down.

Please test your code before putting it up for review. You'll get higher grades too, especially if you show your tests to your professor or whoever is teaching you.


Implementing PLC "Timer Pulse" function block in C++

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

Let the compiler write default functions

The Tpulse destructor literally does nothing and is what the compiler would have generated anyway.  To indicate that, eliminate the function in Tpulse.cpp and declare it as default in Tpulse.h:

virtual ~Tpulse() = default;

If you define one special member function, define them all

Sometimes called the "rule of five," if you delete or define any of the special member functions, you should delete or define them all. See C.21

Use override where appropriate

If you're intending to override a virtual base function, you should explicitly say so to help catch errors.  In this case it's the Update function which should be marked override:

void Update() override;

See C.128  Also note that unlike in C, Update(void) and Update() mean the same thing in C++.  I prefer the shorter form, but sometimes coding guidelines (often written by or for C programmers) require the long form.

Use namespaces consistently

In some places within Tpulse::Update, TestBitClr() is written with a Utils:: namespace prefix and in other place not.  This is inconsistent and confusing to the reader who may be left wondering if there are two versions.  Instead, I'd recommend either always using the explicit namespace or putting a using namespace Util; within the Update function.

Prefer std:: namespace versions of functions and types

Rather than uint32_t, I'd recommend using std::uint32_t and explicitly adding #include <cstdint>.  This makes it clear which type you mean and will be robust even if someone later introduces a local uint32_t which does, unfortunately, sometimes happen in embedded systems projects.

Use parameter names in function templates

It's best to make the interface clear and explicit, and with a function call like this:

Tpulse(uint32_t*, uint32_t, uint32_t, uint32_t);

It's not clear what the various uint32_t values represent.  If they were written like this, it would be better:

Tpulse(std::uint32_t *bitsArray, std::uint32_t input, std::uint32_t output, std::uint32_t pulseTime):

See I.4.

Think about the possible correct use of volatile

In embedded systems we often encounter one of the few correct uses of volatile.  In this case, I'm wondering about the m_BitsArray pointer.  If, as I suspect, this is memory-mapped I/O, then this "memory" is not really solely under the control of the C++ environment since external asynchronous signals may cause those bits to change outside program control.  For that reason, it may be that it should be declared as volatile to indicate this fact.  See CP.200.  On the other hand, if this array might also be use by other threads within your program, you will need to add explicit synchronization of some kind.  See CP.8

Use const where practical

It appears to me that m_Input and m_Output should never change within the lifetime of a Tpulse object, so I'd suggest that both of those member data functions should be declared const.  The same is probably true of m_PulseTime.

Minimize the interface

It seems to me that the Tpulse class could eliminate m_ElapsedTime and m_CurrentTime in favor of only using m_StartTime and perhaps a bool running.  If you need other named variables within Update they can be local.

Use helper functions

The code could be much simpler to read and understand with the use of a few private helper functions:

bool LogicBlocks::Tpulse::input() const {
    return Utils::TestBitSet(m_BitsArray, m_Input);
}

bool LogicBlocks::Tpulse::output(bool value) {
    if (value) {
        Utils::SetBit(m_BitsArray, m_Output);
    } else {
        Utils::ClrBit(m_BitsArray, m_Output);
    }
    return value;
}

Simplify the code

The Update code is a little more complex than it needs to be.  Essentially, either the timer is running and we do timer things, or it's not yet running, but we receive an input that tells us to start it.  The only other slightly tricky thing is that we don't allow the timer to restart until it's expired and the input is low.

So we can simplify the code, using a bool running member variable and the helper functions shown above:

void LogicBlocks::Tpulse::Update(){
    if (running) {
        auto elapsed = Utils::GetTick() - m_StartTime;
        if (elapsed >= m_PulseTime) {
            output(false);
            running = input();
        }
    } else if (input()) {
        m_StartTime = Utils::GetTick();
        running = output(true);
    }
}

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.

In this case I wrote this main to do some testing:

#include "Tpulse.h"
#include "Utils.h"
#include <iostream>
#include <iomanip>
#include <chrono>
#include <thread>

int main() {
    using namespace Utils;
    using namespace std::chrono_literals;
    uint32_t reg{0};
    constexpr uint32_t inbit{0x80};
    constexpr uint32_t outbit{0x2};
    LogicBlocks::Tpulse tp(&reg, inbit, outbit, 5);
    for (int i=0; i < 20; ++i) {
        std::this_thread::sleep_for(1s);
        if (i == 2 || i == 9) {
            SetBit(&reg, inbit);
        } else if (i == 4 || i == 16) {
            ClrBit(&reg, inbit);
        }
        std::cout << std::dec << "t = " << i << ", reg = 0x" << std::hex << reg;
        tp.Update();
        std::cout << ", updated to 0x" << reg << '\n';
    }
}

Here's the output:

t = 0, reg = 0x0, updated to 0x0
t = 1, reg = 0x0, updated to 0x0
t = 2, reg = 0x80, updated to 0x82
t = 3, reg = 0x82, updated to 0x82
t = 4, reg = 0x2, updated to 0x2
t = 5, reg = 0x2, updated to 0x2
t = 6, reg = 0x2, updated to 0x2
t = 7, reg = 0x2, updated to 0x0
t = 8, reg = 0x0, updated to 0x0
t = 9, reg = 0x80, updated to 0x82
t = 10, reg = 0x82, updated to 0x82
t = 11, reg = 0x82, updated to 0x82
t = 12, reg = 0x82, updated to 0x82
t = 13, reg = 0x82, updated to 0x82
t = 14, reg = 0x82, updated to 0x80
t = 15, reg = 0x80, updated to 0x80
t = 16, reg = 0x0, updated to 0x0
t = 17, reg = 0x0, updated to 0x0
t = 18, reg = 0x0, updated to 0x0
t = 19, reg = 0x0, updated to 0x0


Avoid the sort
You are almost there. But there is room for some improvements
Sparse array
Using an array to store number frequency works. Javascript will know to create a sparse array, and will sort such arrays using only the elements in it.
A spare array is an array that is not continuous. This is done to reduce the memory use. However there is a penalty for the memory saved, and that is it takes longer to access each item, as the index is used to find the real index, which then can get the item.
For a standard array, the index is used directly to get the item.
Sort is complex
The sort is an expensive operation and as you are only needing the top two you can store the top two results as you count the frequency of numbers.
This also means you don't need to store the number you are counting with each count, just how many there are.
The logic goes like this, see rewrite for implmentation
if frequency greater than second place
   if frequency greater than first place
      if first place the same value
          set the new first place frequency
      else
          move first place to second
          set first place to the new first
   else
       set the new second place

When done just return the first and second values.
It could also be done using a Map, but you would do that if the items to count were not able to be indexes (eg words).
More than one correct answer
The is a possibility that all values have the same frequency. That means that any two will be correct. Or as your second test [2, 2, 3, 2, 4, 3, 4] there are two correct answers [2,3] or [2,4]. As there are no rules for this any of the correct answers will do.
for loops are better than Array.forEach
Using a for loop to iterate over the array is more performant than using the array.forEach function.
A rewrite.
The rewrite avoids the sort
const topTwoElement = nums => {
    const arr = [];

    const first = {count : 0, value : null};
    const second = {count : 0, value : null};
    for (const num of nums) {

        if (arr[num]) { arr[num] += 1 }
        else { arr[num] = 1 }

        // to avoid having to index into the sparse array in the
        // code to test for top two
        const count = arr[num];

        if (count > second.count) {
            if (count > first.count) {
                if (first.value === num) {
                    first.count = count
                } else {
                    second.count = first.count;
                    second.value = first.value;
                    first.count = count;
                    first.value = num;
                }
            } else {
                second.count = count;
                second.value = num;
            }
        }
    }
    return [first.value, second.value];
}


Class for removing Non Alpha/Numeric Character String and Counting Words

The function RemoveNonAlphaNumeric promises to remove non-alphanumeric characters. It does this in an incomplete way.

The comma is not removed.
The apostrophe is not removed.
The em-dash — is not removed.

There's probably a function Character.IsDigit and Character.isLetter that is more appropriate. In the end, whether it is appropriate or not depends on what the code is supposed to do at all. What do you need it for, why do you want to remove non-alphanumeric characters in the first place?


Find The Duplicates using binarySearch python

Given that len(arr1) == n and len(arr2) == m, the bisection method has a \$O(n \log m)\$ time complexity. Notice that it is important which array is scanned, and which is bisected. You really want arr1 to be shorter than arr2 (do you see why?).
That said, in case M ≈ N the bisection method has \$O(n \log n)\$ time complexity. However in this case you can achieve \$O(n)\$. The same linear algorithm applied to the general case yields \$O(n + m)\$. Picking the right one (\$O(n \log m)\$ vs \$O(n + m)\$) is the core of the question as I see it.

I intentionally do not spell the linear algorithm out. It is quite simple.
Returning value from b_search misses an opportunity to optimize further. Notice that both arrays are sorted, therefore you don't need to search the entire arr2 again and again: you are only interested in values to the right of where the last search ended.

Consider instead returning an index. It should not be an index of the element found, but one that partitions arr2 into strictly smaller, and larger or equal than val. A bisect module conveniently provides a bisect_left method:

    from bisect import bisect_left

    def find_duplicates(arr1, arr2):
        output = []
        index = 0
        for val in arr1:
            index = bisect_left(arr2, val, index)
            if arr2[index] == val:
                output.append(val)


Methods creating transform functionality on Collections

I see the Supplier-less option as being a very broken one. Consider the following code:

transform(linkedList.sublist(0, linkedList.size() / 2), i -> (i + 1) + "-" + i);

Or, any of the other collections that do not have a meaningful default constructor:

transform(Arrays.asList(argv), a -> a.toLowerCase());

The code will cause more headaches than the small requirement to add the supplier will require.

The Generics of the mapping are also broken when there is no supplier, because the actual type of the return list should be more specific....

Lose the first static method.

The second method looks fine, except for the ordering of the arguments. I would make the supplier the middle, not the last argument (and I would introduce some newlines to make it not-so-long-of-a-line):

public static <E, R, C extends Collection<R>> C transform(
          final Collection<E> collection,
          final Supplier<C> collectionSupplier,
          final Function<? super E, ? extends R> mapper) {
    ....
}

The logical reading of that order is better, take this collection, map to that collection, and use this function to do it..... maybe it is not such a big deal...?

The use-case seems to read better for me:

List<String> lcargs = transform(Arrays.asList(args), ArrayList::new, a -> a.toLowerCase());


Pouring water between two jugs to get a certain amount in one of the jugs (2)

Your code is not a “good” practice at recursion.  Every “recursive” call to jugSolver is the last statement that is executed in the current call, thus the whole function can easily be replaced by a simple loop:

print(amt1, amt2)
while amt1 != t  and  amt2 != t:
    if amt2 = jug2:
        amt2 = 0
    elif amt1 != 0:
        if amt1 <= jug2 - amt2:
            amt1, amt2 = 0, amt1+amt2
        else:
            amt1, amt2 = amt1 - (jug2-amt2), jug2
    else:
        amt1 = jug1
    print(amt1, amt2)

Your assumption that pouring from the smaller jug into the bigger jug always results in the minimum number of steps is flawed.  Consider t=2.  Your way:

5, 0
0, 5
5, 5
3, 7
3, 0
0, 3
5, 3
1, 7
1, 0
0, 1
5, 1
0, 6
5, 6
4, 7
4, 0
0, 4
5, 4
2, 7

Compare with:

0, 7
5, 2

So you’ll need to relook at solving the problem with different possible moves.


Recursive function to get all Zendesk tickets

My usual advice for implementors of recursive algorithms Is this: figure out how to do it without recursion (although i dont know how to prove it Is Always better, i can prove it Is never worse. And i can prove it Is Always possible.)

This advice Is yet stronger in your case because there Is nothing recursive about pagination. Its you who made it recursive artificially.

Further, the api has reasons to provide the data with a page limit. They dont want to run to memory issues when serving those data to you.

Nor should you want to run into memory issues when processing the data. If Its not restricting you, you should also process the data page by page discarding the previous page before loading another one.


Finding football team with the smallest difference in goals

Class names in Java or Kotlin always start with a capital, so TeamAndScoreDifference.  It makes it difficult to read the code otherwise because I assume it's a variable.

The name of readLines() is not quite right since it does many other things.  It does 3 things: read the lines, transform them to pairs and sorts them.  It's awkward that the sorting is in this method, but picking the first element was left in the main method.

I don't like the pattern that you use in the body of readLines() where you initialize an empty list and then fill it with a for-loop (a forEachLine actually).  This is what is usually done in C and Java, but modern functional languages proceed differently.  The pattern that is being executed here is just a mapping: you take a list of objects and return a list of modified objects.  That is done better with the map function; see my code below.

Also it's not clear why your readLines() should be an extension method and not just a plain function.

You don't need to define this function anyway since File.readLines() already exists.

Other programmers might have used a plain Pair instead of a data class TeamAndScoreDifference.  But I'm not recommending one way or another.

makePair could probably have used a better name , but I can't come up with one right now.  You could have used pattern matching to extract the values from the text.  If you don't know how to use regex you will have to learn it at some point as a programmer, either in Java, Kotlin, or any other language.  But it seems that your file has columns of predetermined width, so your solution does work as it is.

The if-else clause in Kotlin is an expression which returns a value, so you can make the final return less awkward at the end of makePair.

You forgot to consider that the answer might not be unique.

You write too many comments.  They just describe what the code is doing.  You can put official documentation for a public API, otherwise you only put comments to explain something quite complicated that another programmer might have a hard time comprehending.  So there are very rarely comments within the code.  Another problem with comments is that they can sometimes be contradictory with the code.  You have such an instance with
//by lowest (GoalsScored - GoalsTaken) where the code instead uses the absolute value.

import java.io.File

fun main(arg: Array<String>) {
    val lines = File("football.dat").readLines()
    val teamsWithScoreDiff = lines.map(::makePair).filterNotNull()
    val minDiff = teamsWithScoreDiff.map(TeamAndScoreDifference::scoreDiffere).min()
    val minTeams = teamsWithScoreDiff.filter { it.scoreDiffere == minDiff }
    println("Team(s) with minimal score difference of %minDiff: %{minTeams}")
}

data class TeamAndScoreDifference(val team: String, val scoreDiffere: Int)

fun makePair(line: String): TeamAndScoreDifference? {
    val teamName = line
            .take(23)
            .substringAfter(". ")
            .substringBefore(" ")
    val goalsScoredAndTaken: String = line
            .take(52)
            .substringAfterLast("   ")
    val goalsScored =
            goalsScoredAndTaken
                    .substringBefore(" ")
                    .toIntOrNull()
    val goalsTaken =
            goalsScoredAndTaken
                    .substringAfterLast("  ")
                    .toIntOrNull()
    return if (goalsScored != null && goalsTaken != null)
        TeamAndScoreDifference(teamName, Math.abs(goalsScored - goalsTaken))
    else
        null
}


Performing unit conversions using intermediate conversions

The post (and the original assignment!) are a bit confused about these concepts:

the ratio between two units of measurement
the exchange rate between two currencies
the weight of an edge in a graph

The confusion is understandable because we sometimes use the word "cost" for all three concepts. But there are some important differences:

When combining ratios or exchange rates, you multiply them, but when combining weights you add them.
When converting units of measurement, it doesn't matter what path you take.

For example, here's a possible set of inputs to your program:

yard, foot, 3
chain, foot, 66
mile, chain, 80
mile, yard, 1760

If you need to use this to convert a mile into feet, it doesn't matter whether you take the path mile → yard → foot or mile → chain → foot, the answer is the same (5280) either way. Different paths can only yield different results if there are errors in the input.

However, when exchanging currency, it might be better value to take one route rather than another. Due to arbitrage the difference between paths is likely to be small in the real world, but it can exist and so ought to be taken into account in any computer model.

This has some consequences:

Converting currencies and converting units of measurement are two different problems, and you shouldn't expect to be able to write one program that handles both.
A general graph is the wrong data structure for units of measurement, because it could have multiple paths between pairs of units, which means you would have to worry about data inconsistency. A better data structure would be a forest, so that there is at most one path between any pair of units and the data cannot be inconsistent.
If you are going to represent a collection of ratios as a weighted graph, then the weights need to be the logarithms of the ratios, so that the total weight of a path (the sum of the weights of the edges) is the log of the product of the ratios, as required.
Logarithms of ratios below 1 are negative, and Dijkstra's algorithm doesn't work with negative edge weights.
A graph with some negative edge weights might have a cycle with a negative sum. This means that there is no path of lowest weight in the graph. For example, suppose that you can exchange 2 marks for a crown, 2 guilders for a mark, and 2 crowns for a guilder. Then you can get arbitrary amounts of money by going around this cycle as often as you like. The program will need to have some way of handling this case—for example, maybe it indicates an error in the input.


Asynchronous lookup over HTTP and checking the status

I suggest to remove the result variable. You can just return the result. You don't have to store it in a temporary variable:

if (response.IsSuccessStatusCode)
{
    return await response.Content.ReadAsAsync<ApiResult>();
}
else {
    return null;
}

In this case I would even use the ternary ? : operator:

return response.IsSuccessStatusCode
    ? await response.Content.ReadAsAsync<ApiResult>()
    : null;

You could also write it like this:

if (!response.IsSuccessStatusCode)
    return null;

return await response.Content.ReadAsAsync<ApiResult>();


How are my javascript module pattern and promise pattern?

I assume that you are not going to use readily available third-party libraries for this one. If I had the option to, I'd be using jQuery as it already supports promises, and AJAX.

Anyways, let's start reviewing. I pretty much explain in the comments so watch out for them. Any explanations are usually out of the comments though.

//actually, there is no point doing the localization thing since they
//are objects (and passed by reference). Anything that changes the
//original object, these references you have here also change
//
//However, it's still useful for assigning aliases so I'll let this one pass
(function (window, document, bigmap, ajax) {
    "use strict";

    //personal preference though
    //I prefer chaining the vars with commas
    //declarations on top, those with values last
    //also, you can merge declarations and assignments, like in svg_canvas
    var drawings,
        svg_canvas = bigmap.initialize("map", window.innerWidth - 21, window.innerHeight - 21),
        attr = {
            fill: "#fafafa",
            stroke: "#505050",
            "stroke-width": 1,
            "stroke-linejoin": "round"
        };

    //promises are used for async to maintain a "sync" feeling in the code
    //however, if operations are not async, avoid using a promise
    //if bigmap.draw is not async, then I'd avoid chaining them in .then()'s
    //because it's overhead
    //
    //but I assume they are async since they are SVG drawing operations
    ajax.post("php/paths.php", null)
        .then(function (value) {
            drawings = JSON.parse(value);
        })
        .then(function () {
            bigmap.draw(svg_canvas, drawings.walls, null);
        })
        .then(function () {
            bigmap.draw(svg_canvas, drawings.depts, attr);
        });

}(this, this.document, parent.bigmap, parent.ajax));

On the promises, I'd avoid the module pattern and use prototypes. Imagine this: Every call to promise() returns an object with it's own properties as well as functions. When using prototypes, each promise will have it's own properties, but will share the functions. Forget privates, this one saves memory. However, just like me, if you hate the new Constructor() approach, you can mask the operation in a function call.

//constructor
function PromiseBuilder(){
    this.pending = [];
    this.ready = false;
    this.result;
}

//our shared functions

//this one stores the callback
PromiseBuilder.prototype.then = function (callback) {
    //personal preference, but I use single quotes
    if (typeof callback === 'function') {
        //ready is boolean. instead of checking for false,
        //you can use loose checking and ! which means *if not* ready
        if (!ready) {
            this.pending.push(callback);
        } else {
            //so you won't lose the instance, use call and pass this
            //as "this" in your function
            callback.call(this,result);
        }
    }
    //according to the PromisesA spec [http://wiki.commonjs.org/wiki/Promises/A]
    //each then should return a new promise
    //I haven't read the full spec or try implementing it
    //but you should, if you want it to become a proper PromiseA type promise
    return this;
};

//I guess this one acts as the "resolve" - you should name it that way
PromiseBuilder.prototype.resolve = function () {

    //saving the instance and all arguments
    var instance = this,
        resolveArgs = arguments;

    if (!ready) {

        ready = true;

        //ES5 foreach? or are you using a shim/fix?
        this.pending.forEach(function (value, index, ar) {

            //instead of being restrained in one argument,
            //why not pass the entire arguments array to the callback
            //by using .apply()
            value.apply(instance,resolveArgs);
        });

        this.pending = undefined;
    }
};

PromiseBuilder.prototype.isReady = function () {
    return this.ready;
};

//our masking function
function promise() {
    return new PromiseBuilder();
};

Same as well for AJAX, we use literal namespacing instead. since the ajax response is now bound to the promise there is no need to have instances for our ajax.

var ajax = {
    post : function(url,parameters){
        var XHR = new XMLHttpRequest(),
            p = promise(),
            parameters = params || ''; //parameters is either itself or blank if none

        XHR.open("post", url, true);
        XHR.setRequestHeader("content-type", "application/x-www-form-urlencoded");
        XHR.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
        XHR.onreadystatechange = function () {
            if (XHR.readyState === 4 && XHR.status === 200) {
                promise.keep(XHR.responseText);
            }
        };
        XHR.send(parameters);

        return promise;
    }
}


for (let file of await enumerateFiles(String.raw`C:\Windows\Web\Screen`))

The await looks like a leaky abstraction. Could your generator naturally behave synchronously?  A quote from Exploring ES6

  22.1.5 Use case: receiving asynchronous data

  Generators can receive input from next() via yield. That means that you can wake up a generator whenever new data arrives asynchronously and to the generator it feels like it receives the data synchronously.


Your first solution could be more elegantly written as:

f :: [String]
f = [concatMap (replicate n) "abc" | n <- [0..]]

The basic operating principle would be the same, though.

A general sorting algorithm would be O(n log n) at best, so I don't believe that sort would be a good idea.


Abstract classes with operator overloading

To answer your direct question, yes an interface would be better than an abstract class.  Plus you may want to consider that Area could be a property rather than a method.

To address things you didn't ask about:

For the Triangle class, you may want to give simpler names to the base and height.  So Base instead of theBase and Height instead of theHeight.  And they should be public properties, perhaps with private setters.

public double Base { get; private set; }
public double Height { get; private set; }

For Rectangle class, the Length and Width could also be public.

public double Length { get; private set; }
public double Width { get; private set; }

Also the constructor should use lower casing for the arguments:

public Rectangle(double length, double width)

The operator could be shortened:

public static Rectangle operator+ (Rectangle r1, Rectangle r2)
{
    return new Rectangle(r1.Length + r2.Length, r2.Width+ r2.Width);
}

Or if you are using C# 6.0:

public static Rectangle operator+ (Rectangle r1, Rectangle r2) => new Rectangle(r1.Length + r2.Length, r2.Width+ r2.Width);

The last line uses an Expression-bodied function member found in C# 6.0.  A single line method body can be replaced with a 'lambda arrow' and you can omit the return since its implied.

Putting it altogether

Using C# version 6.0 syntax

interface IShape
{
    double Area { get; }
}

public class Triangle: IShape
{
    public double Base { get; private set; }
    public double Height { get; private set; }
    public Triangle(double theBase, double theHeight)
    {
        Base = theBase;
        Height = theHeight;
    }
    public double Area => (Base * Height) / 2;
}

public class Rectangle : IShape
{
    public double Length { get; private set; }
    public double Width { get; private set; }
    public Rectangle(double length, double width)
    {
        Length = length;
        Width = width;
    }
    public double Area => Length * Width;

    public static Rectangle operator+(Rectangle r1, Rectangle r2) => new Rectangle(r1.Length + r2.Length, r2.Width+ r2.Width);
}


@Autowired is not required on constructor in Spring over 4.3 (class has to be annotated as bean and have only one constructor).

Inject SimpMessagingTemplate template in a constructor. Now you mix annotation and constructor injection.

IMHO you shouldn't put any business login in controller methods. I would move it all to separate service. For example I would write updatePerson() like this:

@MessageMapping("/person/update")
public void updatePerson(@Payload Person requestPerson) {
    personService.updatePerson(requestPerson);
    this.template.convertAndSend("/topic/person", personService.viewPersons());
}

In updatePerson() and createPerson() you should have separate objects instead of Person class. E.g. CreatePersonRequest and UpdatePersonRequest

I would change viewPersons() to getAllPersons()

The rest look fine :)


I gotta start by saying it's weird to ask how much damage you want to take, my answer would always be zero, you know!

Your variable names could be more descriptive.

I understand that DMG means damage, SH would be.. something health?

Well I guess you see my point, if we don't know what the variable name means, it's harder to figure out what it does.

Considering the PEP8 guidelines towards Python, your variable should never contain upper case.

So I would change :

DMG -> damage
SH -> total_health
Output -> remaining_health
PA -> play_again

In your input you ask how much damage should be dealt. There are much more options that are invalid than valid ones. I mean, I can enter 0,20,40,60,80,100 but I can't enter any other number, so there are much more chances I face the "That isn't a command" error message than anything else.

If the only options are the values from 0 to a 100 that are multiples of twenty, you should specify it in your message :

"How much damage would you like to take? [0,20,40,60,80,100]"

There is no real advantage for the Output variable at this moment. You want to print how much damage is dealt and an health bar that corresponds to the remaining life.

if damage == 20:
    print("You took 20 damage!")

Or, even better, use the .format method.

print("You took {0} damage!".format(str(damage))

Then you could print the health bar. I don't want to go deep in details but you could write a function like print_remaining_health_bar(total_health,remaining_health) that would print according to the parameters instead of repeating this piece of code every time.

All in all you could remove the whole if/else business (except for your input validation, which is important) and have :

print("You took {0} damage!".format(str(damage))
remaining_health = total_health - damage
print_remaining_health_bar(total_health,remaining_health)

There's also a small problem with your code. When asked if I want to play again, if I enter an invalid command, I'd just restart the game.


Am working on a few minor improvements, but I think the major saving to be had is in removing the line

square2, isquare2 = square[:], isquare[:]

Instead, do

sofar = square[x], isquare[y]
for o in options[sofar]:
    square[x], isquare[y] = o
    for s in addone(square, isquare, position+1):
        yield s
square[x], isquare[y] = sofar