Print words in decreasing order of frequency in Scala

Using just a priority queue

Yes, it's possible, although you would need a special implementation of priority queue, which internally is a hash map + a heap. This trick is commonly used in Dijkstra's algorithm. See https://stackoverflow.com/a/17581306/1370445.

Unlike in Dijkstra's algorithm, in your task, you don't need to be able to alternately update values on the heap and remove the top element. So I would discourage you from using this data structure, and encourage to instead focus on achieving high code quality (including low code volume), using the data structures and operations available in Scala's standard library.

Here are my recommendations regarding possible improvements to the code, some of them obsolete the others in the context of this specific piece of code, but I think it's worth to mention them anyway:

Use UpperCamelCase for class names

Scala has an official naming convention (https://docs.scala-lang.org/style/naming-conventions.html) which recommends to name classes with UpperCamelCase, so wordFreq should be WordFreq.

Use Scala syntax consistently

Scala let's you replace the dot and parentheses with a space in some case. x method param is equivalent to x.method(param). In the snippet of code you've provided you sometimes use that feature ("aa bbb  ccc aa ddd aa ccc" split " +"), sometimes use just part of it c compareTo(that.c) and sometimes you don't use it (myHeap.enqueue(wordFreq(w,c))). I would recommend you to decide which style of code you prefer (I prefer dots and parentheses) and use it consistently, at least within a single source file.

Embrace immutability

Scala provides two sets of collections, mutable and immutable. In most cases it's preferable to use immutable collection as they make reasoning easier and eliminate whole class of bugs. I won't dive deeper into that topic, as it is covered in detail by many articles on the internet, for example:

https://www.quora.com/What-are-the-advantages-and-disadvantages-of-immutable-data-structures
https://www.yegor256.com/2014/06/09/objects-should-be-immutable.html

Prefer Ordering over Ordered

Ordered binds together the data structure and the way of ordering/comparing them. This is reasonable for thing which have some notion of "natural order" like Strings, or Integers. Ordering lets you deliver the way of ordering separately from the data structure declaration, resulting in more composable code.

So instead of

case class wordFreq(w: String, c: Int) extends Ordered[wordFreq] {
  override def compare(that: wordFreq): Int = c compareTo(that.c)
}

I would write

case class WordFreq(w: String, c: Int)
val mostFrequentFirstWordFreqOrdering: Ordering[WordFreq] =
  (x: WordFreq, y: WordFreq) => y.c.compareTo(x.c)

you could make the mostFrequentFirstWordFreqOrdering implicit so that the compiler will pick it up automatic, but since there's more than one reasonable way to sort WordFreq I would prefer to stay explicit and pass the ordering by-hand.

Use sorted/sortBy instead of hand-crafting a heap sort

val myHeap = PriorityQueue[wordFreq]()
wordCount.toSeq foreach { case (w,c) => myHeap.enqueue(wordFreq(w,c))  }
myHeap.dequeueAll

Is basically a heap sort implementation.  It could be replaced by the sorted or the sortBy methods of SeqLike. It could look like

wordCount.toSeq.sortBy{case (word, count) => -count}

or if you find the - and inelegant way of reversing the order, alternatives are:

wordCount.toSeq.sorted(Ordering.by[(String, Int), Int]{case (word, count) => count}.reverse)

or

wordCount.toSeq.sortBy{case (word, count) => count}.reverse

the last one being less efficient in terms of allocations.

countBy

val wordCount = HashMap[String, Int]()
for  (word <- words)  {
  wordCount += (word -> (wordCount.getOrElse(word,0) + 1))
}

can be replaced with

words.groupBy(x => x).mapValues(_.length)

In fact I observe this pattern so often, that in some project I have an "extension method" countBy added to Traversable like this:

implicit class RichTraversable[+A](val traversable: Traversable[A]) extends AnyVal {
  def countBy[K, That](accessor: A => K): Map[K, Int] =
    traversable.groupBy(accessor).mapValues(_.length)
}

Separate calculations from making side effects

The line println(myHeap.dequeueAll.map(_.w).mkString(",")) does two things. It finishes the process of sorting the results myHeap.dequeueAll.map(_.w) and prints them in human readable format println(results.mkString(",")). Two things should be done in two lines, or better, two methods, or better functions (..., but functional IO is a longer topic).

Split by more than just spaces
In real-world text words may be separated by more than just spaces - new-lines, commas, semicolons, etc.. \W is a regex pattern for "non-word character" (see https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html). Using "\\W+" would be better than " +", although most likely there would still be some edge-cases where it would go wrong.

Final result

After all the suggested modifications the code could look like this:

object FindWordFreq extends App {
  val words = "aa bbb  ccc, aa ddd\naa\rccc" split "\\W+"
  val wordCounts = words.groupBy(x => x).mapValues(_.length)
  val wordsByDescendingNumberOfOccurrences = wordCounts.toSeq.sortBy{case (_, count) => count}.reverse
  println(wordsByDescendingNumberOfOccurrences.mkString(","))
}


Multiprocess, multithreaded read write on a single file

Is there anything i can do in these methods to get any boost in performance?

Yes, You can reimplement them, using the 'memory-mapped files' (MMf) to solve both problems of parallel concurrency and access sharing.

using (MemoryMappedFile file = MemoryMappedFile.CreateFromFile("test.bin"))
using (MemoryMappedViewStream stream = file.CreateViewStream())
{
    Read(stream);
}

There are two ways of accessing data on a memory mapped file – using a MemoryMappedViewStream and an MemoryMappedViewAccessor.
 The former is optimised to provide sequential access (e.g. when reading or writing a block of data) and the later is optimised for random access reading and writing.

Also, for 'production code' consider the following:

Performance can be greatly improved by always writing data sequentially and in large chunks
The chunk size is less important for small amounts of data (less than a kilobyte)
There is a cost to creating and disposing accessors, this cost becomes acceptable for reading files below 32K.
There is benefit of keeping hold of a stream, wich will remain significant.
When reading a large stream the performance is equivalent to raw memory access


Finding large Fibonacci Number in Python

Issue
Your issue is you compute fibonacci(i) for one number multiple times.
To understand this, consider computing fibonacci(5). The function would call fibonacci(4) and fibonacci(3) at the end. Now what happens in fibonacci(4)? It would call fibonacci(3) and fibonacci(2). Now you can notice, that the function gets called 2 times for the number 3, which is a serious issue, since that can go VERY deep in the recursion and you can gain a massive overhead because of this.
Solution
Because fibonacci is a pure function, it returns the same value every time it is called with the same parameters. So there's a simple trick you can do to avoid so much time on recalculating what you already found. You can store the result of fibonacci for a specific number in an array, and if it's called again, just return that instead of computing everything again. This trick is called memoization. The Fibonacci sequence is often used as the introductory example for dynamic programming and memoization.
See this nice video explaining the concept of memoization and dynamic programming
Code
This is a simple implementation of memoization for this problem.
cache = {}

def fibonacci(n):
    if n in cache:
        return cache[n]

    if n == 1 or n == 2:
        return 1
    else:
        result = fibonacci(n-1) + fibonacci(n-2)
        cache[n] = result
        return result


Multithreaded Matrix Multiplication in C++, improving efficiency?

Welcome to Code Review!
Algorithm
Currently, a thread is started for every row of the result matrix.  This works best if the matrix has a large number of columns and the number of rows is close to the number of native threads.
A more efficient strategy in general might be to partition the rows to several chunks according to the native thread count instead.  Without careful benchmarking, it's hard to say for sure.
Encapsulation
Matrix has a class invariant — for a non-empty matrix, data points to an array of rows pointers that point to arrays of cols elements each.  To ensure synchronization, the data members can be kept private.
Memory issues
Matrix fails to observe the rule of zero:

Classes that have custom destructors, copy/move constructors or
copy/move assignment operators should deal exclusively with ownership
(which follows from the Single Responsibility Principle). Other
classes should not have custom destructors, copy/move constructors or
copy/move assignment operators.
This rule also appears in the C++ Core Guidelines as C.20: If you can
avoid defining default operations, do.

To fix this issue, simply change the pointers to std::vector.
Storage of matrices
Currently, you are storing a pointer to a dynamic array of pointers, which, in turn, point to dynamic arrays, incurring double indirection overhead and causing memory segmentation.  A more efficient method is to store all elements in a flattened array and provide a helper function to access the element at a given place:
class Matrix {
    std::vector<int> elements;
    std::size_t n_rows;
    std::size_t n_cols;
public:
    Matrix()
        : Matrix(0, 0)
    {
    }
    Matrix(std::size_t n_rows, std::size_t n_cols)
        : elements(n_rows * n_cols), n_rows(n_rows), n_cols(n_cols)
    {
    }

    std::size_t row_count() const noexcept {
        return n_rows;
    }
    std::size_t col_count() const noexcept {
        return n_cols;
    }

    int& operator()(std::size_t i, std::size_t j) {
        return elements[i * n_cols + j];
    }
    const int& operator()(std::size_t i, std::size_t j) const {
        return elements[i * n_cols + j];
    }
};

Note that this does not preserve the aliasing semantics of your Matrix and performs deep copying instead, so the by-value parameters should be changed to references.
Miscellaneous
using namespace std; is considered bad practice in C++.
If you turn on high warning levels, you will see warnings about mixing signed and unsigned integers in comparisons, which is because you are mixing int and unsigned int.  Instead of them, use std::size_t to store sizes, which is what the language functionalities and the standard library use.
In ReadMatrix, file streams close automatically when they go out of scope, so there's no need to do that manually.  std::ifstream can be used to simplify the construction of the file stream.
Instead of printing messages, throw exceptions when errors occur.


Message Queue for inter-thread communication

Nothing in-depth, just a few things that threw me off as a developer that would be using your class:

If you design a queue, use proper interfaces.
There are a few interfaces in Java, designed to offer standardized access to a Queue

public class MessageQueue<T> { // A queue should implement the Queue<E> interface, preferably BlockingQueue<E>

Mention constants in your documentation, together with their reason.
I have a hard time understanding your limit of 255 messages. As a developer that cannot look at private variables, I would be thrown off with errors once I go over that limit. Document it. Tell others about this limitation!

private static final int MAX_SIZE = 255; // Mention this constant in your documentation

Is there a reason to use the Boolean object type in favor of the boolean primitive? Using Boolean involves unnecessary boxing and might, if used excessively and in time-sensitive applications, introduce lower performance.

public synchronized boolean hasMessage() { // Use primitives when objects are not necessary

This isn't meant as an exhaustive list, but only my subjective opinion.


In the wild I don't find much recursion, it seems mostly used during exercises. I'm assuming this is one as well. Just hope you already handed it in.

one pointer for you, you are passing nrElements. Is this the same as elements.length? you might be able to remove it altogether from the interface of your methods.
Second pointer: your printAllKLengthRec returns an array , but it also updates the (same) array passed into it as an input argument. you can remove the return value completely. It is not needed. trust that the method will update the array you pass as input.

the way I would solve this myself would be something like:

public List<String> getStrings(char[] chars, int k) {
    int[] indexes = new int[k]; // prefills with 0
    List<String> result = new ArrayList<String>();
    boolean done = false;

    while (!done) { // stopcondition is inside the loop
        // construct 1 string with current indexes
        StringBuffer buffer = new StringBuffer();
        for (int i = 0; i < k; ++i) {
            buffer.append(chars[indexes[i]]);
        }
        result.add(buffer.toString());

        indexes[0]++;

        for (int i = 0; i < k; ++i) {
            if (indexes[i] == chars.length) { // overflow should go into i+1, set this index back to 0
                if (i + 1 == k) {
                    // loop done. would overflow.
                    done = true;
                    break;
                } else {
                    indexes[i] = 0;
                    indexes[i + 1]++;
                }
            }
        }
    }

    return result;
}

It is more lines, but it's nonrecursive which might make things a bit more manageable. For one you can split subparts into a separate method (for instance the indexes could become a class of it's own) and reuse it for other methods that need the same. I find recursion really only fixes it's one issue, but can't easily be reused for other things. Unless you write a good datastructure around it, like tree lookups and such.

just measured, my implementation was faster 3 times out of 4, but :) that's the problem with measuring using System.nanotime. All sorts of stuff can happen during execution to make the result not untrustworthy.


Adding action id to a history repository

The problem is that the action id is static so it will be shared with all instances of the ActionIdContext class. That means that 2 separate pieces of work excuting at the same time could log everything with the same action id.

var client1 = new Client(someService);
var client2 = new Client(someService);
var client3 = new Client(someService);

Task.Run(() => client1.DoSomeGroupAction());
Task.Run(() => client2.DoSomeGroupAction());
Task.Run(() => client3.DoSomeGroupAction());

You have no idea how this code will log the actions - it could log them all with the same action id, it could log some with no action id and some with a common action id. It could also log some with one action id and others with another.

The point I'm trying to make is, although a nice idea in theory, you can't locally instantiate state then share it via a static property and expect it to work.

Can't you add a LogHistories(Guid actionId, param History[] histories) method or something?

You should implement the full IDisposable.Dispose pattern;

public static Guid? ActionId
{
    get { return actionId; }
    set { actionId = value; }
}

would be better as an auto property.

HistoryBuilder is a bit weird - Builders are generally used with immutables or objects with very complex constructors (in my experience).


Simplifying member functions in Blackjack game

If you want to make things more consice, you could change a few things:

Number of card printed on 2 digits:

if (totalCardsValue <= 9)
    ss << "(0" << totalCardsValue << ")  ";
else
    ss << "(" << totalCardsValue << ")  ";

could be written

ss << ((totalCardsValue <= 9) ? "(0" : "(") << totalCardsValue << ")  ";

or

ss << "(" << std::setw(2) << std::setfill('0') << totalCardsValue << ")  ";

No need for temporary std::stringstream.
I think you could just std::cout directly without using std::stringstream ss.
Removing duplicated code: void Hand::displayPlayerHand() does the same as void Hand::displayCPUHand(true). This would probably better if you were naming this method display and calling the parameter displayAllCards or displayOnlyFirst. Also, you might want to give it a default value.

Corresponding code for Hand.cpp is:

void Hand::display(bool displayAllCards) const
{
    // displays the first card
    std::cout << cards[0] << " ";

    // displays the rest of the cards if required
    if (displayAllCards)
    {
        for (unsigned i = 1; i < cards.size(); i++)
            std::cout << cards[i] << " ";
    }
}

Removing duplicated code: things can be changed in Player.cpp too as Player::displayCPUStats and  Player::displayPlayerStats really look alike.

Edited to finish my answer: Indeed, displayPlayerStats() is nothing but displayCPUStats(true). Here again, it might be worth changing the names of methods and variables.

Corresponding code for Player.cpp is:

void Player::displayStats(bool displayAllCards) const
{
    std::cout << "\n* " << name << ": ";
    std::cout << "($" << chips << ") ";

    if (displayAllCards)
    {
        std::cout << ((totalCardsValue <= 9) ? "(0" : "(") << totalCardsValue << ")  ";
    }
    playerHand[0].displayHand(displayAllCards);
}


Inadvertent case dependency?

Should not the below code also work with 'Q', 'U'?

if (*s == 'q' && *(s+1) == 'u')

char functions (advanced issue)`

The Std C library functions treat  char * data as if it was accessed by unsigned char *.

This is important in select cases.

tolower(int ch), is...() are well defined for values in the unsigned char range and EOF, else UB.  When *s < 0, tolower(*s) is a problem.  Casting to (unsigned char) fixes that.

isvowel(const char *s) {
  // switch (tolower(*s)) {
  switch (tolower((unsigned char) *s)) {

With some extended ASCII encoding this is useful, yet with UTF8, the issue moot as tolower() is wholly inadequate.

Deeper pedantic detail

(unsigned char) *s is the wrong solution with non-2's complement and signed char (not seen these days) as code should access the data an unsigned char.

isvowel(const char *s) {
  const char *us = (const char *) s;
  switch (tolower(*us)) {

With 2's complement the result is the same with either approach.  IAC, I doubt much C code these days would survive a new non-2's complement integer encoding so no real reason to worry about this deep detail.

Formatting

Code lacks {} in simple blocks.  I find the later style easier to review and maintain.  As with such style issues, code to your group's coding standard.

} else {
    while (argc-- > 0)
        piglatin(*argv++);
}

vs.

} else {
    while (argc-- > 0) {
        piglatin(*argv++);
    }
}

Consider local variables, initialization and code reduction

//bool upper;
//...
//    upper = false;
//    if (isupper(*s))
//        upper = true;
      bool upper = isupper(*s);


Regex for domain validation name validation

Your regex will suffer from catastrophic backtracking with certain inputs. Just try matching aaaaaaaaaaaaaaaaaaaaaaaaaaaa, and you'll see the visible slowdown.

I suggest (as was already mentioned in the comments) that you break up the regex at the | into two separate regexes, one for IP addresses and the other for domain names. It just makes more sense, especially from a future maintainability perspective, and will make things easier for you in the future.

The IP address part is actually very efficient on its own, so there are no further changes I would make to this part:

^([0-9]{1,3}\.){3}[0-9]{1,3}$

The second part of the regex is the source of the slowdown. It also matches some strange things, like this:

a....a..................aa
aa
a.aa
a.-a.aa

I'm just going to assume that these are bugs, although I don't know given that you haven't made it clear what should match. That's why I will write up some rules for what I want to match:

Must start with an alphanumeric
There may be . or - characters, but they must be surrounded by alphanumerics.
Must end with . followed by two or more letters

Here is the regex that would be (with newlines added for emphasis):

^[a-zA-Z0-9]+
([-.][a-zA-Z0-9]+)*
\.[a-zA-Z]{2,}$

Note that - does not need to be escaped if it's the first thing in the character class. . is never a metacharacter in character classes.

Finally, I would use the i modifier to get rid of all the a-zA-Z redundancy:

/^[a-z0-9]+([-.][a-z0-9]+)*\.[a-z]{2,}$/i

Here are the tests:

a-a.com
1-1-1-1.com
1.1.1.com
aa.com
aa.cc
a.com
a.a.a.a.aa

a..a..................aa
aa
aaaaaaaaaaaaaa
a.-a.aa

aa.c
a--a.com
-aa.com
a-a-a-a-.com

The first group is matched by both, the second group is matched by only your regex, and the last group is matched by neither.

To show you the difference in performance, I tested against this variation of your original regex:

^([a-zA-Z0-9]+(([\-]?[a-zA-Z0-9]+)*\.)+)*[a-zA-Z]{2,}$

Using the PCRE option on Regex101 (with the gm options), your regex takes 66390 steps. The new regex I wrote took only 214 steps, and it's shorter and more legible too.


Javascript - Extract data from html table

Always use const to declare variables - only use let when you must reassign. This keeps code readable, because then a reader of the code doesn't have to constantly keep in mind that a variable might be reassigned later. (If you use let but then don't reassign, it can still be confusing - in professional code, one might think "Why is let being used here? Was this meant to be reassigned in a section of code that was later removed, or something?)

Array.from accepts an optional mapper function as a second parameter. Any time you have:

Array.from(arrayLike).map(mapper)

you may replace it with

Array.from(arrayLike, mapper)

(If all you're doing is converting an array-like object into an array, some prefer spread syntax because it's even more concise: [...arrayLike])

innerText is a weird property introduced by Internet Explorer (outside of web standards originally) that has a number of odd quirks. Unless you're deliberately looking to invoke those quirks, it would be a better idea to use textContent instead to retrieve text from an element.

You can easily distinguish the first tr from the other trs by using the query string #tab tr:first-child or #tab tr:nth-child(n + 2):

const headers = Array.from(
  document.querySelectorAll('#tab tr:first-child th'),
  th => th.textContent.trim()
);
// Make an empty array for every item in headers:
const data = Array.from(headers, () => []);
for (const tr of document.querySelectorAll('#tab tr:nth-child(n + 2)')) {
  [...tr.children].forEach((th, i) => {
    data[i].push(th.textContent.trim());
  });
}
console.log(headers);
console.log(data);

      Name

      Age

      Location

      Jason

      22

      Texas

      Lawson

      21

      Florida

      Jose

      25

      London

That's already quite simple, IMO. I think adding jQuery to the mix would make things unnecessarily more complicated, not less.

I refactored it out, but I don't think it's good idea to use reduce when the accumulator is going to be the same object every time. See: Is reduce bad? by Google devs. If it's always going to be the same object, it'll be a bit easier to read if that object is declared as a standalone variable in the outer scope.

The HTML is a bit weird. A  is a table header. It makes sense for the headers to be s, but the table data should probably be s instead.


Finished in the comments, can you post the part of the assignment question pertinent to your limited library usage and main method only code? Anyway, if you have been told that a RTE will signify the end of the reading (rather than the null) then this seems marginally better as it makes it clear why you are doing what you are doing:

while (true) {
    try {
        str = stdin.readString();
        if (str == null) {
            break;
        }
        int r = (int)(Math.random()*(i++ + 1));
        if(r < k){
            arr[r] = str;
        }
    }
    catch(RuntimeException rte) {
        //Log the crazy break condition
        break;
    }
}

It is always a discussion point whether while(true) is acceptable and is normally held up against using a running loop variable. In this case you can intialise a boolean to true and use it as your while condition - setting it to false where the breaks are. It involves extra checks, and makes for harder to read code in my eyes, but hey!

Now, give your variables meaningful names.

EDIT: Post clarification in the comments

So importantly you do not know the length of the input but you want to randomly sample it, and a RumtimeException is unexpected and can make your code blow up? In which case why anymore complex than this?:

public class Subset {

    public static void main(String[] args) {
        int sampleSize = Integer.parseInt(args[0]);
        String[] samples = new String[sampleSize];
        int current = 0;
        String str;

        //make sure you have at least sampleSize number of samples
        while((str=stdin.readString())!=null && current < sampleSize){
            samples[current++] = str;
        }

        //this will read until the end of the input
        while((str=stdin.readString())!=null) {
            int randomIndex = (int)(Math.random()*(current++));
            if(randomIndex < sampleSize){
                samples[randomIndex] = str;
            }
        }

        for(String sample : samples){
            System.out.print(sample + " ");
        }
        System.out.println("\n");
    }
}

EDIT post comments
In your original question on Reservoir Sampling it was important to add +1 to you possible random number as you wanted to pick a value from an array of known size without excluding the final element . However in this version you are using the random number to dictate (with decreasing probability of a hit) where to write the next value read from Stdin into your array of samples.

If you know that the Stdin implementation and that it will through an Exception and that is the only thing you have been told or advised will signify an end to the data (no special characters, no nulls, no maximum length, etc) then the only thing that you can do is catch and handle it and that is perfectly acceptable - in this instance it is not really an Exception condition, it is the norm! Maybe he exercise is in working with bad APIs (which is very common)!

In the latest (very different) version of your code you have this:

for(; ; i++){
    int randomIndex = (int)(Math.random()*(i + 1));
    if(randomIndex < sampleSize){
        sample[randomIndex] = StdIn.readString();
    }
 }

You should replace this with code more like my first example where the exception state is handled within the loop. So:

Replace for loop with while loop
Because you are using the randomIndex to determine whether to read a value at all into the array you are no longer random sampling, you are just random inserting. Every value read from the pool will get written to the array. It is also important that this could run for a long time or more likely run to an error state when i > MAX_INT, which would be possible with a large dataset and a small sample.


How to deal with some of the dependencies?

Your are using the observer-pattern in a right way. The pattern itself is probably one of the most important ones. In PHP it is not used that much though as applications have a rather quick life-cycle. Yet good to know the pattern of course. Some minor remarks about your code though:

All methods in an interface are public, no need to repeat yourself (in interfaces).
The name Observer and Subject indicate an use-case independent use. Yet your Observer are coupled to your Weather component. Better names would be WeatherDataObserver and WeatherDataSubject.
I'd consider it standard to have at least a whitespace between the function's header and the opening bracket: registerObserver(Observer $o) {
Not sure if your WeatherData is actually a service. For me the name WeatherData indicates a set of values, not necessarily the current values. I'd probably rename it to WeatherDataService.
notifyObserver should not be public. I don't want anyone else to trigger (possibly false positives) notifications. Better remove it from the interface too.
measurementsChanged serves no purpose yet. Replace it with notifyObserver.

Your concerns are right. If I remember the book correct, they present an alternative approach on this pattern: instead of pushing the changed values to the observers, just inform them something has changed and let them pull the new values from the subject. This changes the signature of your observer to function update(Subject $weatherData) and in the update method of your CurrentConditionsDisplay you get the new values by:

public function update(Subject $subject){
    $this->temperature = $subject->getTemperature();
    // and so on
}

Another approach would be some event'ed one. Send an event-object about what has changed. This pushed the changes to your observers and still avoids dependencies. This approach is a bit more work though.
This is a pattern from java user-interfaces (or any views that can update themselves). In PHP you usually don't use this pattern to update user interfaces as PHP can't update the UI :). You'd access the WeatherData directly when rendering the view. Nothing really bad about this here though.
Fine for me from what I can see.
No. Generally there are very very very few proper uses for static methods. Static methods are completely independent from others and object internal state. Successive calls to static methods should always have the same result (and should be side-effect free), no matter what happened in the application meanwhile. Usually this applies for simple helper functions only (e.g. abs or other math. helpers). Even this could be discussed though. CurrentConditionsDisplay has some internal state though, therefore static is a no-go.


So it looks like you're just printing out the vowels in your string, right?

System.out.println(answer.replaceAll("[^AaeEiIoOuU]", ""));

Removes the for loop and compacts your code a lot, making it a lot more readable IMO.  Also notice how easy it was for me to extend to capital letters.

It would be good to note that this is a replacement for your block of code as it is right now.  If you plan on printing the vowels in a variable amount of strings (perhaps input by the user), then you would want to make a method that would do this for you.  In this function you would pass in the precompiled regex and the string to print.

I wrote this up really quick in a text editor and didn't compile it, but here is a program that could handle multiple strings better:

class VowelPrinter
{
    static final Pattern p = Pattern.compile("[^AaeEiIoOuU]");

    public static String removePattern(Pattern p, String str)
    {
        return p.matcher(str).replaceAll("");
    }
    public static void main (String[] args)
    {
        int num = 0;

        System.out.print("How many names are you going to save: ");
        Scanner in = new Scanner(System.in);
        num = Integer.parseInt(in.nextLine().trim());

        String names[] = new String[num];
        for (int i = 0; i < names.length; i++)
        {
            System.out.print("Type a name: ");
            names[i] = removePattern(p, in.nextLine());
        }
        System.out.println("Names with consonants removed:");
        for (int i = 0; i < names.length; i++)
        {
            System.out.println(names[i]);
        }
    }
}


XPath does away with much of the very low-level w3c classes. And is within java SE: javax.xml.xpath.XPath.


Is it possible to make a generic method out of this, or at least separate them?

Well, it works well, but as you said, it can be optimized

Review:

You are commenting each line which is not good, only comment meaningful code sections. Commonly the names of the functions and variables you are calling is intuitive enough to know how the code is working. When it is insufficient then it is the moment to comment.

Refactoring:

You may create two class fields (ImageFactory,MemoryStream) and work with these.

private ImageFactory imageFactory;
private MemoryStream imageStream;

public [ClassName]() //the constructor if doubts
{
    imageFactory = ImageFactory(preserveExifData: true);
    imageStream = MemoryStream();
}

public Image Resize(Image image, int width, int height)
{
    imageFactory.Load(image).Resize(new Size(width, height)).Save(imageStream);
    return Image.FromStream(imageStream);
}

public Image Crop(Image image, float left, float top, float right, float bottom,
    bool isModePercentage = true)
{
    var cropMode = isModePercentage ? CropMode.Percentage : CropMode.Pixels;
    var cropLayer = new CropLayer(left, top, right, bottom, cropMode);

    imageFactory.Load(image).Crop(cropLayer).Save(imageStream);

    return Image.FromStream(imageStream);
}

Hope it has been helpful.


So, you were right about the "picosecond precision": the overly precise time is causing problems (GHC doesn't memoize but you can read more about what it does do here).

Basically, the solution is to use a library. I used the fast-logger library.

module Lib
    ( main
    , toBench
    , MessageLevel(..)
    ) where
import Control.Monad
import Data.Monoid
import System.Log.FastLogger.Date
import System.Log.FastLogger

main = toBench 10000000 OK "Hello, World!"

data MessageLevel = OK | INFO | WARN | FAIL | CRIT

instance Show MessageLevel where
    show OK     = "  OK  "
    show INFO   = " INFO "
    show WARN   = " WARN "
    show FAIL   = " FAIL "
    show CRIT   = " CRIT "

toBench :: Int -> MessageLevel -> String -> IO ()
toBench n lvl msg = do
    logFunction <- fst <$> fastLogger
    let toString msg t = toLogStr t <> toLogStr (mconcat msg)
    replicateM_ n $ (logFunction . toString) ([" || [", (show lvl), "] ", msg, "\n"])

fastLogger :: IO (TimedFastLogger, IO ())
fastLogger = do
    timeCache <- newTimeCache "%Y-%m-%d %H:%M:%S"
    let logType = LogFileNoRotate "test.log" 4096
    newTimedFastLogger timeCache logType

You can view the full code    here.

I did modify the code, but for good reason: loggers writing to file shouldn't include color. So I think this is a more apt benchmark.

With all that said and done, the Haskell version took around 3.6s on my computer and the python version took 2m35s to accomplish the same thing. This is pure Haskell code - no C whatsoever. I did use one of the suggestions from hlint - use the tools available to you!

As a parting shot: get comfortable using other people's code when writing Haskell. Your code is pretty good - you separate pure and impure code nicely, but on this front it is lacking. One of functional programming's strengths is that it enables easy code reuse. I suggest you take advantage of this - Haskell is not easy.


Project Euler #3 - checking only up to sqrt(input)

This implementation is not correct: for your example 14 it incorrectly returns 2 instead of 7.

You don't need to check == true of boolean expressions, you can use them directly, for example:

if (numbers[count]) { ... }

The formatting doesn't follow Java standards. In Eclipse, if you select your function and press Control-Shift-f it will change it this, which is more pleasant to read:

public static int largestPrime(long n) {
    int limit = (int) Math.sqrt(n) + 1;
    boolean[] numbers = new boolean[limit];
    Arrays.fill(numbers, true);
    int largestPrimeFactor = 1;
    for (int count = 2; count < limit; count++) {
        if (numbers[count]) {
            for (int i = 2; i * count < limit; i++) {
                numbers[count * i] = false;
            }
            if (n % count == 0) {
                largestPrimeFactor = count;
            }
        }
    }
    return largestPrimeFactor;
}

A simple way to fix the implementation is to re-implement following the same logic you do in math class:

Start from d = 2
Divide the target number by d as many times as possible without remainder
Increment d, return to step 2, until target is reduced to 1

This is pretty simple, correct, without using a boolean array:

public static long largestPrimeFactor(long n) {
    long d = 2;
    long target = n;
    while (target > 1) {
        while (target % d == 0) {
            target /= d;
        }
        ++d;
    }
    return d - 1;
}

To make this faster, after the ++d you can insert this check:

        if (d * d > target) {
            return target;
        }


Your link_to is set to POST but you are adding the statement_type on the url like if it were a GET request? Sounds like you are mixing things and you can resolve this using just a single form element without JS if I understand you right.

<%= form_tag new_statement_template_path, method: :post %>

    <%= select_tag :statement_type, options_for_select(Statement.statement_types.keys.to_a) %>

    <button type="button" class="btn btn-default" data-dismiss="modal">Cancel</button>
    <%= submit_tag "New Form" %>

<% end %>

Now your controller action will have a the selected option at params[:statement_type].


Determining function return types for std::function

I'm not sure why you need the typedef in the first place. Is there anything wrong with auto spectral_differential_current = ...?

On a similar note, why do you want to require that f is an std::function, disallowing reasonable calls like integrate(sin, 0, 2, 10)? Most code of this type can happily accept anything that can be called:

template <typename F, typename In>
auto integrate(F f, In xmin, In xmax, int nSteps)
-> decltype(f(xmin)  * xmin)

The compiler will tell you whether a given f is OK. Usually by not finding a suitable integrate function – SFINAE.

Also note the use of typename – that's just a stylistic thing of course, but e.g., double is not a class, so I prefer not to say class for template parameters that would accept a double or a plain function pointer.


Integer Power C++ Function with base and exponent parameters

Use the standard library
If you are going to give the result as a double, then this function isn't really a pure integer power calculator. And in that case, just use std::pow(). It has overloads for integer exponents that the compiler can optimize for.
A pure integer power function
If you just want to work purely on integers, then the typical algorithm to calculate a number raised to an arbitrary power efficiently is by recognizing that, for example, \$x^4 = ((x * x) * (x * x))\$, and so you can calculate \$y = x * x\$, and then \$x^4 = y * y\$. This only needs two multiplications instead of 3. So basically, you can divide and conquer the problem:
int integerPower(int base, int exponent) {
    if (exponent == 0)
        return 1;

    int result = integerPower(base, exponent / 2);
    int result *= result;

    if (exponent & 1)
            result *= base;

    return result;
}

The above doesn't work for negative exponents, but then again that is not very useful if the result is just an integer.


The at() method validates that the input is in the correct range. Since you have already done the validation there is no point in using this method you may as well use operator[]

Additionally there is a call to check for empty strings empty() so prfer to use that rather than test the length against zero.

if (line.empty() || (line.length() == 1 && isspace(line[0])))


I think you're confused here. The GCM algorithm has a 96 bit nonce, and fills the rest of the 128 bit block size with a 32 bit counter. So you have 2^32 full blocks at your disposal, or about 64 GiB. That's - uh - slightly more than 32 bits (well, OK, the counter starts at 2, so you'd have 48 bytes less than that, better keep it at 32 GiB or 48 GiB).


I took an elective last semester in which we had to do UVa questions.  Just dug up my old UVa 100 submission, and I have a few suggestions for speed (mine was accepted in 200 ms, though I suspect it could be optimized a ton more).

The main thing I see is that you shouldn't use recursion.  Due to the setup cost of calling functions, iteration is going to be way faster (in some situations the same stack frame can be reused, but I doubt that optimization is being done in your code).

Though that is my main suggestion, there are a few very minor algorithmic optimizations that can be noted mathematically.

t(n) = 1      if n is 1
     = 3n + 1 if n is odd
     = n / 2  if n is even

c(n) = number of times t(n) will happen
(terrible math syntax here, but erm... go with it)

Now, the approach I took was to build a table bottom up.  Basically an array like int m[1000001].

Then a loop like:

for (int i = 1; i <= 1000000; ++i) {
    if (i % 2 == 0) {
        //We know that i / 2 must already be set, so we can just do a constant time look up
        //(your code already does this implicitly through the recursion)
        m[i] = 1 + m[i / 2];
    } else {
        m[i] = cycleLength(i);
    }
}

Nothing special about this loop; it's just the iterative form of your code.

There is an odd little optimization you can make though:

if (i % 2 == 0) {
    m[i] = 1 + m[i/2];
} else if (i % 8 == 5) {
    m[i] = 4 + m[3 * ((i - 5) / 8) + 2];
} else {
    m[i] = cycleLength(i);
}

The theory here is that for any j < i, you know that m[j] is populated.  Thus, if you can do some manipulation to get m[i] = c + m[f(i)] such that f(i) < i you can do a constant time look up rather than the potentially enormously expensive cycleLength call.

It's the same concept in the 1 + m[i / 2] situation, just weirder.

If i mod 8 = 5, then by definition, i = 8j + 5 for some integer j.

i = 8j + 5 = 2k + 1 with k = 4j + 2 thus i is odd.

So we follow the pattern:

8j + 5 -> 24j + 16 -> 12j + 8 -> 6j + 4 -> 3j + 2

Note the crucial part here: 3j + 2 < 8j + 5.  This means that we can define m[i] in terms of 4 + m[3 * ((i - 5) / 8) + 2].

I'm too lazy to get out a pad of paper, but I suspect that more arithmetic trickery like this is possible (the weird situation of i mod 8 = 5 is something that the professor pointed out).  Even without it, an iterative approach is easily fast enough to be accepted by UVa's online judge, but it does make a surprising 48 ms difference with my program.


My first (simple) template that does more than storing a value of type T

Apart from cosmetic details, I see two things that could be improved IMHO.

The template comparator R<V>

As a template argument, you are asking for template <class> class R
which is then used so R<V> r;

This narrows the possibilities since you can't pass a non-template comparator.
For instance, if you have a custom type MyType and a custom comparator for that type MyTypeComp then you won't be able to use it with your Extremum struct because MyTypeComp doesn't accept template arguments.

The fix is easy:

template <
          typename V,
          class R,
          typename I = void
         >
struct Extremum {
    typedef V    value_type;
    typedef I    index_type;
    typedef R relation_type;
    V value;
    I index;
    R r;
    //...

And you would use it so:

Extremum<int, std::greater<int>, int> e(0,0);
Extremum<MyType, MyTypeComp, int> e(MyZero,0);

The index

You write almost the same code twice: once with index, once without.
You could avoid it by passing the index in a pair.
So you have only one class Extremum<V, R> and you set V = std::pair<Value, Index> and an appropriate comparator that compares two pairs according to their Value entry only.

template <class V, class R>
struct Extremum {
    typedef V    value_type;
    typedef R    relation_type;
    V value;
    R r;
    bool operator() (V v){
        bool result = r(v,value);
        if (result) {
            value = v; //you can use std::move here
        }
        return result;
    }
    Extremum(V v, R r) : value{v}, r{r} {} //you can use std::move here too
    std::string toString() const {
        std::stringstream s;
        s << "value= " << value;
        return s.str();
    }
};

template <class RV>
struct CompByFirstEntry {
    RV delegate;
    CompByFirstEntry(RV delegate);

    template <class V, class I>
    bool operator() (std::pair<V,I> const& lhs, std::pair<V,I> const& rhs) const {
        return delegate(lhs.first, rhs.first);
    }

    //or, even:

    template <class Pair>
    bool operator() (Pair const& lhs, Pair const& rhs) const {
        return delegate(lhs.first, rhs.first);
    }
}

EDIT : Here is a working example.

What changed: define the constructor CompByFirstEntry::CompByFirstEntry and operator<< for std::pair; choose one version of the operator().
Note: you can also create a class ExtremumIndexed<V, I, R> that extends Extremum<std::pair<V,I>, MyPairComp<R>> or simply a template using directive.

std::ostream& operator<<(std::ostream& out, std::pair<int, int> const& p)
{
    out << p.first << " " << p.second;
    return out;
}

template <class V, class R>
struct Extremum {
    typedef V    value_type;
    typedef R    relation_type;
    V value;
    R r;
    bool operator() (V v){
        bool result = r(v,value);
        if (result) {
            value = v; //you can use std::move here
        }
        return result;
    }
    Extremum(V v, R r) : value{v}, r{r} {} //you can use std::move here too
    std::string toString() const {
        std::stringstream s;
        s << "value = " << value;
        return s.str();
    }
};

template <class RV>
struct CompByFirstEntry {
    RV delegate;
    CompByFirstEntry(RV delegate) : delegate{delegate} {}

    template <class V, class I>
    bool operator() (std::pair<V,I> const& lhs, std::pair<V,I> const& rhs) const {
        return delegate(lhs.first, rhs.first);
    }
};

int main(int argc, const char * argv[]) {
    using MyPairComp = CompByFirstEntry<std::greater<int>>;
    MyPairComp comp(std::greater<int>{});
    int value = 1;
    int index = 0;

    Extremum<std::pair<int,int>, MyPairComp> e({value, index}, comp);
    std::cout << e.toString() << std::endl;

    e({value + 1, index - 1});
    std::cout << e.toString() << std::endl;

    return 0;
}


I just copied your code, pasted it in to a terminal, ran it, and it did exactly what it is supposed to do. Now, how to make it better?
You don't mention it in the question, but using ls, awk, find, and sort together somehow seems unlikely to be the answer your tutor expected. You don't mention what your course is in, but I would have expected there to be a less 'cobbled together' solution, using perhaps bash only, or perl, python, or something else.
I certainly would not have solved this problem this way (I would have used perl).
Still, assuming your code meets the expectations for tool use (as in, you are supposed to use awk), there are some things that are off.
Single-line multi-statements
You have a number of 1-liners that you should have expanded. You already have the massive awk code-block, why mix the compact 1-liner and block statements like you have done?

for ( iowner in usr_sum ) if ( usr_sum[iowner] > owner_sum ) {owner = iowner; owner_sum=usr_sum[iowner]}

The above should be:
for ( iowner in usr_sum )
{
    if ( usr_sum[iowner] > owner_sum )
    {
         owner = iowner
         owner_sum=usr_sum[iowner]
    }
}

Note, the use of block {} braces for the single if statement, and not using the semi-colon ; to separate statements on a single line.
These other statements should also be on their own lines (and the for statement should have a {} block):

for ( i=9; i<=NF; i++) name=name " " $i
...
sum=0;delete usr_sum;
...
sum+=$1;usr_sum[$2]+=$1;
sum_all+=$1;usr_sum_all[$2]+=$1;

Negative if
Your main if-block would be better if done as a positive check, not a negative check. your code is:

if (type!="d")
{
    ... do not a directory stuff
}
else
{
    ... do directory stuff
}

but would be more readable as:
if (type == "d")
{
    ... do directory stuff
}
else
{
    ... do not a directory stuff
}

Breathing space
Your code is suffocating since it has little breathing space. Many of your statements are compressed around the operators and other symbols. Consider lines like:

          print $5,$3,type,name
          sum_all+=$5
          usr_sum_all[$3]+=$5

Which should be written as:
            print $5, $3, type, name
            sum_all += $5
            usr_sum_all[$3] += $5

Performance
The multiple calls to find may be hurting your performance. In general I can't help but think there's a better way to do this without all the nested OS calls.
Still, it ran fast enough for me on my machine, so it is probably not a real problem.


Pixel bit-wise LSB extraction to byte array

I believe your basic algorithm is fine. Shifting only 4 bits on to each byte is not exactly filling the byte, but, there are ways I would consider neatening up the process. Note that this does not change the algorithm much at all.

Here are some tricks to note, though...

You should pass in the number of bits you want in a byte as a parameter value, or it should be a constant. The value 4 is a magic number that should not be there in the method.

private static final int BITS_IN_BYTE = 4;

You have: int numberOfBytes = (int) Math.ceil(pixelCount / 4.0);. This can be done "better" with:

int numberOfBytes = (pixelCount + BITS_IN_BYTE - 1) / BITS_IN_BYTE;

You declare convenience variables which really are unnecessary....
You are working with the data 'backwards'.... you build up the byte, then set the value in to the output, instead of just shifting the bits in to the output.

Here's how I would have written your code. It is more concise, but essentially the same.

public static byte[] extractLSBRL(final int perByte, final int[] pixels) {
    final int outSize = (pixels.length + perByte - 1) / perByte;
    final byte[] lsbs = new byte[outSize];
    for (int i = 0; i < pixels.length; i++) {
        // which output byte is the one for this pixel
        final int j = i / perByte;
        // make space for the new bit to arrive.
        lsbs[j] <<= 1;
        // copy the bit in to the output byte's position.
        lsbs[j] |= pixels[i] & 1;
    }
    return lsbs;

}

Note that I have a concern about the output here. are you sure the last byte should be 10, and not 1000 (i.e. shifted two bits to the left?). Just checking... different algorithms expect different things....


Python program to solve the "rod-cutting problem"

First of all, it's really a bad idea to change the value of the input parameter when this change has an outer effect in some other place in your code.

Also, You can get rid of recursive function calls, that makes your code so much faster.
So let's start from cut_rod_helper function:

def cut_rod_helper(p, n, r, s):
    q = 0
    for i in range(1, n + 1):
        temp = p[i] + r[n - i]
        if q < temp:
            q = temp
            s[n] = i
    return q

I removed comments for more clear view here. But it's good to have them in your code.

What I did here is removing recursion. I removed first if statement from your code because we don't do recursive call and don't need end of recursion statement. As you can see at the middle of the function, we get r value from that list directly.

Second change here is I removed second if statement in your code because we don't need this (At least when we only have prices more than zero). Fewer conditions mean better code. Because with fewer if and else, the code is more straightforward and less error-prone. Also in many cases, the if-less code is faster. So always try to minimize the number of if conditions in your code.

We can simply set q value to zero initially. If n be 0, range of for loop will be (1,1), when start and stop values of a for loop are same, that for will not execute. So you don't need to consider a different condition for that.

And the biggest trick is in the third line of the function. We really don't need recursive call. If we start calling helper function from 0 to n index of r, we always have previous values in the r and we can access them directly.

Now let's go to the primary function:

def cut_rod(p, n):
    r = [-1]*(n + 1)
    s = [-1]*(n + 1)
    r[0] = 0
    for i in range(1, n + 1):
        r[i] = cut_rod_hezerolper(p, i, r, s)
    return r, s

We have only two changes here. First, I have been set index 0 of r to zero. It's our initial state and is clear from problem description. length = 0 so price = 0!

Then, for avoiding the change of input parameters, We are looping through the r and set value of each index of it. Because for calculating ith value of r, we only need access to i - 1 values of it, and we have been set initial value r[0] and start our loop from i = 1, now our code works without any problem.

I did not a full performance test, but with few tries, the second version is about 3 times faster than the first version of your code.

In the end, I did not change variable names, because I think that may distract you. But I think you should change your variable names. s, r, q and ... are meaningless. If a variable is a list contains prices, you should name it something like prices_list. If you do that, you don't need many lines of those comments and everyone can find out what you did in your code at first glance. Spending some seconds on naming things in programming, saving minutes and hours later.

I hope this helps you.


Spotfire: Creating filtering relationships by passing filters between tables

from ... import * is discouraged by a large amount of people.

Currently it's easy to see what comes from Filters, as you only have one. But if you have two or more than it just becomes impossible to understand.
Your code doesn't look Pythonic, unfortunately this is because Spotfire has violated PEP 8 making anything that uses it look, to my eyes, ugly.

Whilst I'm not a fan of the style you are using, I would like to congratulate you on sticking to one style. Well done.
If the comments help you then keep them. However if you wrote them to help us then I would recommend that you remove them.
You have lots of un-needed variables; page, theFilter, theFilterPT.
Functions are the bread and butter of programming. And your code is in some dire need of functions.

In your code we see that you have the following definitions:

  #get information for the first filter
theFilter = filterPanel.TableGroups[0].GetFilter("Reason")
lbFilter = theFilter.FilterReference.As[ListBoxFilter]()
#get information for the second filter
theFilter2 = filterPanel.TableGroups[1].GetFilter("Reason")
lb2Filter = theFilter2.FilterReference.As[ListBoxFilter]()

  theFilterPT = filterPanel.TableGroups[0].GetFilter("Type")
lbFilterPT = theFilterPT.FilterReference.As[ListBoxFilter]()

theFilter2PT = filterPanel.TableGroups[1].GetFilter("Type")
lb2FilterPT = theFilter2PT.FilterReference.As[ListBoxFilter]()

We can see clearly that only two things change between them; [0] and [1], and "Reason" and "Type".
I personally think filterPanel.TableGroups[0] shouldn't be in the function and we can just pass the filter in the function. This can result in:

def getGroup(tableGroup, filter):
    return (
        tableGroup
            .getFilter(filter)
            .FilterReference
            .As[Filters.ListBoxFilter]()
    )

After this we can see you've written something else twice too.

  n = []
for value in lbFilter.SelectedValues:
    n.append(value)
if len(n)>0:
    lb2Filter.IncludeAllValues = False
    lb2Filter.SetSelection(lbFilter.SelectedValues)
#if no selections are made, reset the second filter
else:
    lb2Filter.Reset()

Firstly creating n can be simplified simply as list(lbFilter.SelectedValues).
You don't need to use len(n)>0 as just if n: does that check, with a list, for you.
Finally I find it easier to have the smaller code block first, and so flipped the if.

def filterGroups(tableGroups, filter):
    group1 = getGroup(tableGroups[0], filter)
    group2 = getGroup(tableGroups[1], filter)

    if not list(group1.SelectedValues):
        group2.Reset()
    else:
        group2.IncludeAllValues = False
        group2.SetSelection(group1.SelectedValues)

Finally I would wrap everything in a main function and you'd get the following:

import System
from Spotfire.Dxp.Application import Filters, Visuals

def getGroup(tableGroup, filter):
    return (
        tableGroup
            .getFilter(filter)
            .FilterReference
            .As[Filters.ListBoxFilter]()
    )

def filterGroups(tableGroups, filter):
    group1 = getGroup(tableGroups[0], filter)
    group2 = getGroup(tableGroups[1], filter)

    if not list(group1.SelectedValues):
        group2.Reset()
    else:
        group2.IncludeAllValues = False
        group2.SetSelection(group1.SelectedValues)

def main():
    tableGroups = (
        Filters
            .Application
            .Document
            .ActivePageReference
            .FilterPanel
            .TableGroups
    )
    filterGroups(tableGroups, "Reason")
    filterGroups(tableGroups, "Type")

if __name__ == '__main__':
    main()


Check for balanced parentheses in JavaScript

The algorithm looks fine to me, but I did not run it, as I do not have any stack implementation at hand. I expect most implementations to throw an exception when trying to pop from an empty stack, so your code may exhibit a weakness here.

Using a string as your preferred data structure to store and iterate parens looks a bit dubious. It implies repeated calls to indexOf() over the same values, and does not explicit well enough the nature of your parens (you need to track positions). Other approaches could be favored, e.g. maps. For instance, you could map opening tokens with their closing tokens, and conversely.

From a design point of view, your function mixes two things: the balance check itself and the parentheses which it should use to operate. I believe it would be cleaner to decouple these, for instance passing a list of Parens as argument to the function, each Parens indicating an opening token and a closing token. The design of your function would evolve in a good way, and you'd gain additional benefits as well (Open Closed Principle).


Accessing the Pixels

GetPixel() is notoriously slow for iterating through the whole bitmap. Consider using LockBits for accessing the pixels directly in memory. After making sure the alpha channel is the same on both bitmaps (should be automatically unless one of the images is transparent), simply compare integer values for each pixel instead of manually comparing R then G then B.

Improving the Search

I would also adopt a faster algorithm for doing the search: Take the first line of the second bitmap (needle) and check on which lines of the first bitmap (haystack) that line is matching. Afterwards, simply check if all the other lines of the needle are placed immediately below the first one => much less comparisons to perform.

Return value

Returning a bool only to request another value be passed as an out parameter is troublesome. Simply return the Point you found wrapped inside Nullable<> or null if the search returned nothing. Also, I would rather have the function return the top of bottom left corner of where the second image was found so that I can perform some operation using that coordinate instead of getting the center point with which I do little without extracting the corner anyway.

Sample Code

Here is an example code of the idea presented:

public Point? Find(Bitmap haystack, Bitmap needle)
{
    if (null == haystack || null == needle)
    {
        return null;
    }
    if (haystack.Width < needle.Width || haystack.Height < needle.Height)
    {
        return null;
    }

    var haystackArray = GetPixelArray(haystack);
    var needleArray = GetPixelArray(needle);

    foreach (var firstLineMatchPoint in FindMatch(haystackArray.Take(haystack.Height - needle.Height), needleArray[0]))
    {
        if (IsNeedlePresentAtLocation(haystackArray, needleArray, firstLineMatchPoint, 1))
        {
            return firstLineMatchPoint;
        }
    }

    return null;
}

private int[][] GetPixelArray(Bitmap bitmap)
{
    var result = new int[bitmap.Height][];
    var bitmapData = bitmap.LockBits(new Rectangle(0, 0, bitmap.Width, bitmap.Height), ImageLockMode.ReadOnly,
        PixelFormat.Format32bppArgb);

    for (int y = 0; y < bitmap.Height; ++y)
    {
        result[y] = new int[bitmap.Width];
        Marshal.Copy(bitmapData.Scan0 + y*bitmapData.Stride, result[y], 0, result[y].Length);
    }

    bitmap.UnlockBits(bitmapData);

    return result;
}

private IEnumerable<Point> FindMatch(IEnumerable<int[]> haystackLines, int[] needleLine)
{
    var y = 0;
    foreach (var haystackLine in haystackLines)
    {
        for (int x = 0, n = haystackLine.Length - needleLine.Length; x < n; ++x)
        {
            if (ContainSameElements(haystackLine, x, needleLine, 0, needleLine.Length))
            {
                yield return new Point(x, y);
            }
        }
        y += 1;
    }
}

private bool ContainSameElements(int[] first, int firstStart, int[] second, int secondStart, int length)
{
    for (int i = 0; i < length; ++i)
    {
        if (first[i + firstStart] != second[i + secondStart])
        {
            return false;
        }
    }
    return true;
}

private bool IsNeedlePresentAtLocation(int[][] haystack, int[][] needle, Point point, int alreadyVerified)
{
    //we already know that "alreadyVerified" lines already match, so skip them
    for (int y = alreadyVerified; y < needle.Length; ++y)
    {
        if ( ! ContainSameElements(haystack[y + point.Y], point.X, needle[y], 0, needle.Length))
        {
            return false;
        }
    }
    return true;
}

I've also done a benchmark for finding a 246x228 pixel needle located at (169,  281) in an 800x600 pixel haystack:

Optimized method: ~ 11 ms

Initial method:  ~ 261 ms


Finding the object and the URL associated with a notification

First, object_id is a built-in method of Object in Ruby, so I recommend renaming this field to model_id to avoid any clashes.

You want polymorphic classes for the views of different notification types. Since these classes will only be used by views, we'll make them all inherit from a common base class named NotificationView which will provide all the information the view needs:

class NotificationView
  attr_reader :action_description, :object_name, :url
end

Every subclass of NotificationView will initialize by setting these fields according to a given Notification model. For example:

class IssueCreatedNotificationView < NotificationView
  def initialize(notification)
    issue = Issue.find(notification.model_id)
    @action_description = "created issue"
    @object_name = issue.friendly_text
    @url = issue.show_url
  end
end

(I would put these classes in /app/models/notification_views/)

Now you need a method that constructs the appropriate NotificationView given a Notification. Since the type of a notification is determined by an integer field, you will need a case statement. But you'll only need a single case statement in your code because you can take advantage of polymorphism once you have a NotificationView instance.

I would put this method in the Notification class:

def view
  case action
  when 2
    ProjectForkedNotificationView.new(self)
  when 6
    IssueCreatedNotificationView.new(self)
  # etc...
  end
end

Then in your controller, you can get a NotificationView by calling Notification#view. For example:

@view = Notification.find(params[:id]).view

And you can use this object in your view:

= link_to(@view.action_description + @view.object_name, @view.url)

You may want to add some utility methods to the NotificationView base class, if you need them in your views, for example:

def message
  "#{action_description} #{object_name}"
end


The simpler option is to create a local function or two.

var appendText = function(fontSize) {
  return svg.selectAll('g')
    .append('text')
    .attr("text-anchor", "middle")
    .attr("alignment-baseline", "before-edge")
    .attr('font-family', 'FontAwesome')
    .attr('font-size', function(d) { return fontSize });
}

var appendTextSpan = function(fontSize) {
  return appendText(fontSize).
    .append('tspan')
    .style('fill', 'white')
    .style('display', function(d, i) {
      return (i < 5) ? 'block' : 'none';
    });
}

appendTextSpan('1.9em')
  .attr('x', '0')
  .attr('dy', '-0.2em')
  .text(function(d, i) {
    return circlesText[i].icon;
  });

appendTextSpan('0.9em')
  .attr('x', '0')
  .attr('dy', '0.9em')
  .text(function(d, i) {
      return circlesText[i].text[0];
  });

appendTextSpan('0.9em')
  .attr('x', '0')
  .attr('dy', '1.8em')
  .text(function(d, i) {
      return circlesText[i].text[1];
  });

appendTextSpan('1.9em')
  .attr('x', '0')
  .attr('dy', '1.8em')
  .text(function(d, i) {
      return '\uF056';
  });

The more complex one, but arguably better, would be to inject some CSS into the SVG such that you can just use

svg.selectAll('g')
  .append('text')
  .attr('class', 'label')
  .append('tspan')
  ...

How easy that is to do depends on how much control you have of the enclosing context.


Adding a timeout to an UDP socket server

I would instead use a single thread for both tasks and employ an epoll selector on the sockets with a certain timeout. To the best of my knowledge, this is more or less what popular networking frameworks (e.g., NodeJS, Nginx, Lighttpd, etc.) do. I always found waiting on a socket for an undetermined amount of time in a separate thread a waste of resources. That being said, YMMV.


Some quick remarks:

You should be using the with..as construct when opening a file to ensure it is always closed, even if some intermediate code raises an exception:

with open(path) as f:
    text = f.read()
...
with open(path, 'w') as f:
    f.write(cmd)

I would save the comment commands in a set and outside of the loop. in is O(1) for sets and O(n) for tuples (even though n=4 here).

cmds = {'+-', '-+', '<>', '><'}
found = True
while found:
    length = len(cmd)
    found = False
    for i in range(length-1):
        if cmd[i:i+2] in cmds:
            cmd = cmd[0:i] + cmd[i+2:length] # removing useless instruction pairs
            found = True

I would do the same with the longer command tuple before that:

comment_codes = {'+', '-', '<', '>', '.', ',', '[', ']'}
for c in text:
    if c in comment_codes:
        cmd += c

You could also use a generator comprehension and str.join here:

comment_codes = {'+', '-', '<', '>', '.', ',', '[', ']'}
cmd = "".join(c for c in text if c in comment_codes)

Your building of the output filename can be simplified using split and could use str.format:

name, ext = path.split('.')
path = '{}_opt.{}'.format(name, ext)


First of all, I really don't like to mix up routes of two different domains/apps in the same routes.php or web.php. So, I prefer and would suggest you as well to follow the below approach, which seems much cleaner and much authentic. :)

Also, I believe www.abc.com or www.abc.dev are not required to be mentioned here... Laravel is intelligent enough to work this through for routes without subdomains... (especially when it is installed in the root directory of that domain).... If required... you can still follow the procedure I used in mapAppRoutes() below.

Go to App\Providers\RouteServiceProvider

Now in the map() function... Add these method calls...

public function map(Router $router)
{
  $this->mapWebRoutes($router);     //You can comment out this if you want... Completely your choice

  $this->mapSiteRoutes($router);

  $this->mapAppRoutes($router);

  //
}

Now add these functions at the end...

protected function mapSiteRoutes(Router $router)
{
  $router->group([
    'namespace' => $this->namespace . '\Site', 'middleware' => 'web',
  ], function ($router) {
    require app_path('Http/routes.site.php');
  });
}

protected function mapAppRoutes(Router $router)
{
  $domains = ['my.xxxxxx.com', 'my.xxxxxx.dev'];    //Sub-domains you support

  foreach($domains as $domain) {
    $router->group([
      'namespace' => $this->namespace . '\MyApp',
      'middleware' => ['web', 'auth'],
      'domain' => $domain
    ], function ($router) {
      require app_path('Http/routes.myapp.php');
    });
  }
}

Now create, app/Http/routes.site.php and app/Http/routes.myapp.php

In routes.site.php, add this...

Route::get('/', 'SiteController@index');

In routes.myapp.php, add these....

Route::get('/', 'AdminController@index');

Route::get('profile/{user}','ProfileController@index');

Route::post('profile/{user}/update',array('as' => 'profile.update','uses' => 'ProfileController@update'));

Route::resource('posts','PostsController');

Route::resource('tags','TagsController');

Route::resource('health','HealthController');

Route::resource('health-categories','HealthCategoriesController');

You're good to go... :)

Please note that I have changed the namespace as well for the routes, so that you can make separate folders for your home site and subdomain sites and easily differentiate between the two... even by looking at the tree-view. This I believe is a much better way of structuring and dividing your code... Makes things simpler to read and navigate :)


I'll just comment on the cleanliness/readability aspect:

In spite of the given comments, you should give g and p better names.  It'll also allow you to remove those comments, which is good because they shouldn't need to be used as such.
This kind of initialization isn't really necessary:

vector<int> g = vector<int>({1, 2});

It could just be this:

vector<int> g = { 1, 2 };

If you're using C++11, you can also omit the =.
This loop statement may be a little hard to read:

for (int n = 1; n ; n++)

It's understood that having n alone means the loop will continue as long as it's nonzero, but you can still make that clear here anyway.  It's otherwise hard to read this statement at a glance.
This calculation is unclear as written:

g.push_back(k*(3*k - 1)/2);

You also reuse it, so consider making it a constant in terms of k.
Try to add more blank lines between code sections, such as the loops.  Having everything together like this can make it harder for the brain to parse out different sections easily.
The number 1000000 here is a "magic number" (ambiguous hard-coded number).  As such, it should be assigned to a constant with a relevant name.  This will also allow you to change this number in just one place.


State variables are bad. Keep the state explicit, along the lines of:

if (*p == '+' || *p == '-') p++;
if (!isdigit(*p)) return False;
while (isdigit(*p)) p++;
if (*p == 0) return True;
if (*p != '.') return False;
p++;
while (isdigit(*p)) p++;
return *p == 0;

Update: few fixes thanks to Edward


Diff Two Arrays - return the symmetric difference of the two arrays

As for performance: Array.indexOf in the worse case(element is not there), will have to check all values of the array to check if the value is in it. And doing this for all values of the second array, means O(n * m) where n is the arr1 size and m is the arr2 size. If n = m, we can consider that it takes O(n*n), which is quite expensive. You could improve this by sorting both arrays and doing a smarter check, but even that will have the cost of the sorting, which is O(n * log n) with a good implementation

Having said all that, what you really want is a structure to check if you have seen or not an element that can check it really quick. In javascript, we can use objets to do this. As its native code we don´t really know the complexity of access, but with testing a bit it is clear that it´s much better (in theory access should be O(1) or O(log n))

The pseudo code is like this:

var seen = {}
iterate all map1 values and mark is as seen
iterate all map2 values and unmark if already seen, mark if not
add all seen values to new array
return array

And here is a full fiddle:

function diffArray(arr1, arr2) {
  var newArr = [];

  arr1.map(function(val) {
    arr2.indexOf(val) < 0 ? newArr.push(val) : '';
  });

  arr2.map(function(val) {
    arr1.indexOf(val) < 0 ? newArr.push(val) : '';
  });

  return newArr;
}

function diffArray2(arr1, arr2) {
  seen = {}
  var newArr = []
  arr1.forEach(function(val) {
    seen[val] = 1 // we saw it on first array
  })
  arr2.forEach(function(val) {
    if (seen[val] == 1) { // we already saw it on the first one, unmark
      seen[val] = false
    } else if (seen.hasOwnProperty(seen[val]) == false) { // if we hadnt seen it earlier
      seen[val] = 2 // mark with a 2
    }
  })

  for (var val in seen) {
    if (seen[val]) { // if its a 1 or a 2, it was unique
      newArr.push(val)
    }
  }
  return newArr
}

var arr1 = []
var arr2 = []
var mx = 10000;
for (var i = 0; i < mx; i++) {
  arr1.push(i)
  arr2.push(i + Math.round(mx / 2))
}

console.time("diffArray with arrays and .index")
diffArray(arr1, arr2);
console.timeEnd("diffArray with arrays and .index")

console.time("diffArray with object")
diffArray2(arr1, arr2);
console.timeEnd("diffArray with object")


Find paths whose sum equals a target value in a binary tree

Simplification: remove temp vectors

Currently, your code spends a lot of effort merging partial return vectors with the final return vector.  You can eliminate the use of these temp vectors if you just pass in the final return vector as an argument to each recursive call.  Here is what your functions would look like with this change, which not only looks simpler but also speeds things up quite a bit:

void findPathsWithSumHelper(Tree * root, int target, int prev,
        vector<int> path, vector<vector<int>> &res)
{
    if (root == NULL)
        return;

    path.push_back(root->data);

    prev += root->data;
    if (prev == target)
        res.push_back(path);

    findPathsWithSumHelper(root->left,  target, prev, path, res);
    findPathsWithSumHelper(root->right, target, prev, path, res);
}

void findPathsWithSum1(Tree * root, int target, vector<vector<int>> &res)
{
    if(root == NULL)
        return;

    vector<int> path;
    findPathsWithSumHelper(root, target, 0, path, res);
    findPathsWithSum1(root->left,  target, res);
    findPathsWithSum1(root->right, target, res);
}

vector<vector<int>> findPathsWithSum(Tree * root, int target)
{
    vector<vector<int>> res;
    findPathsWithSum1(root, target, res);
    return res;
}

I tested with a complete binary tree of depth 16 (64K nodes) of all value 0, searching for value 0.  This returns 983041 paths (every path in the entire tree).  With the original code, this took 4 seconds.  With the simplified code, it took 0.78 seconds.

Simplification: Reduce recursions

Right now, your program recurses in two ways.  The first way recurses to the child, adding the child to an existing path.  The second way starts a new path at the current node and then recurses to the children.

You can reduce the amount of recursion by always adding to the existing path, but at each level, you must try each point in the path as a possible head of a path.  I rewrote your functions to do that and found that this change speeds up the program even further:

static void findPathsHelper(const Tree *node, int target, int depth,
        vector<int> &path, int pathSum, vector<vector<int>> &ret)
{
    if (node == NULL)
        return;

    path.push_back(node->data);
    pathSum += node->data;
    depth++;

    int sum = pathSum;
    // Try each point in the path as a new head of a path.
    for (int i=0; i<depth; i++) {
        if (sum == target) {
            vector<int> subPath(depth - i);
            for (int j=i; j < depth; j++) {
                subPath[j-i] = path[j];
            }
            ret.push_back(move(subPath));
        }
        sum -= path[i];
    }
    findPathsHelper(node->left,  target, depth, path, pathSum, ret);
    findPathsHelper(node->right, target, depth, path, pathSum, ret);
    path.pop_back();
}

vector<vector<int>> findPathsWithSum(const Tree *root, int target)
{
    vector<int> path;
    vector<vector<int>> ret;

    findPathsHelper(root, target, 0, path, 0, ret);
    return ret;
}

Test program and timings

Here is the code I used to test the various implementations.  I commented out the part that printed the results in order to reduce time spent on IO.  I mostly used complete binary trees with all nodes having value 0 and searching for value 0 in order to generate the worst case scenario, although I did test other cases including using randomized values (code not shown).

Tree *createCompleteBT(int depth, int value)
{
    if (depth == 0)
        return NULL;

    Tree *newNode = new Tree(value);
    newNode->left  = createCompleteBT(depth - 1, value);
    newNode->right = createCompleteBT(depth - 1, value);
    return newNode;
}

// Run with: depth value find
int main(int argc, char *argv[])
{
    int depth = 16;
    int value = 0;
    int find  = 0;

    if (argc > 1)
        depth = atoi(argv[1]);
    if (argc > 2)
        value = atoi(argv[2]);
    if (argc > 3)
        find  = atoi(argv[3]);

    Tree *root = createCompleteBT(depth, value);
    vector<vector<int>> res = findPathsWithSum(root, find);

    cout << res.size() << endl;
#if 0
    for (auto i : res){
        for (auto j : i) {
            cout << j << " ";
        }
        cout<<endl;
    }
#endif
    return 0;
}

Here are the timing results for depth 16 trees:

Original program    :  4.06 seconds
Removed temp vectors:  0.78 seconds
Reduced recursion   :  0.22 seconds

By the way, for a balanced tree, I believe the complexity is \$O(n \log^2 n)\$.  You need to visit each node once (\$n\$), and then for each node search a path from the root (\$\log n\$), and if the path matches the target, you need to add that path (\$\log n\$ again).  For a tree that is completely linear, the time would be \$O(n^3)\$.  For an arbitrary tree, the complexity should be \$O(n*d^2)\$ where \$n\$ is the number of nodes and \$d\$ is the depth of the tree.


Concatenating an already existing filename to create a new file

Prefer RAII-style opening the file in the constructor. You already done that with

    std::fstream fileIn( originalFilename, std::ios::in );

so why not follow the suit with

    std::fstream nextFile(nextFilename, std::ios::out);

There is no need to explicitly nextFile.close();. The destructor will take care of it at the end of each iteration.
There is nothing wrong with the early return. Instead of setting originalFileFound = true;, and testing it later on, quit immediately if opening fails. That would spare a boolean flag and a level of indentation.
Consolidate handling of nextFilename in one place. Consider

    if (!fileIn.is_open()) {
        print_error_message();
        return 1;
    }

    std::string prefix = originalFilename.substr(0, originalFilename.size() - extension.size());

    for (int i = 0, counter = 0; counter < 5; i++) {
        nextFilename = prefix + "_" + std::to_string(i) + extension;
        ....
    }

I would be very cautious to use this program, because it may overwrite existing files.


Temperature conversion program with a lot of repetition

The easiest way to remove repetition here is with nested for loops, going through all the combinations of units. In the code below I also use the fact that you can convert between any two scales in the following way:

subtract something then divide by something to get back to Celsius, then
multiply by something then add something to get to the desired unit

The "something"s can be stored in a single tuple (list of values), making the conversions simpler.

scales = ('Celsius', 'degrees ', 1, 0), ('Farenheit', 'degrees ', 1.8, 32), ('Kelvin', '', 1, 273.15)
value = get_temperature()
for from_unit, _, slope1, intercept1 in scales:
    print "This is what it would be if it was", from_unit
    celsius = (value - intercept1) / slope1
    for to_unit, prefix, slope2, intercept2 in scales:
        if to_unit != from_unit:
            print '{0} {1}{2}'.format(intercept2 + slope2 * celsius, degrees, to_unit)

Dividing code into functions is of course a good idea in general, but not that useful in this case. If he's interested in performing other operations on temperatures it might be interesting to go a step further and make a simple class like the following.

scales = {
            'Celsius': ('degrees ', 1, 0),
            'Farenheit': ('degrees ', 1.8, 32),
            'Kelvin': ('', 1, 273.15)
        }
class Temperature:
    def __init__(self, value, unit = 'Celsius'):
        self.original_unit = unit
        _, slope, intercept = scales[unit]
        self.celsius = (value - intercept) / slope
    def to_unit(self, unit):
        _, slope, intercept = scales[unit]
        return slope * self.celsius + intercept
    def print_hypothetical(self):
        print "This is what it would be if it was", self.original_unit
        for unit, (prefix, _, _) in scales.iteritems():
            if unit != self.original_unit:
                print "{0} {1}{2}".format(self.to_unit(unit), prefix, unit)
value = get_temperature()
for unit in scales:
    m = Temperature(value, unit)
    m.print_hypothetical()


Ruby - Sending commands to multiple servers using Net::SSH

I think this is interesting code. I admit I haven't dug much into Ruby's Net::SSH library, so I bet you've learned some things about it in writing this code. It sounds to me like your biggest concern now is about professionalism, stability, and dependability which is why you want feedback before offering to use this code in a production environment. If so, I feel like you're asking all the right questions.

Here are my suggestions:

Search for prior art
Don't touch passwords if you can help it, and other security practices
Add some tests

Prior Art

You haven't mentioned yet if you've looked into existing tools that have already done this work. For example, you could use Capistrano, which is written in Ruby and designed for the type of scenario you're describing. Capistrano is fairly well-maintained. You could put it on your jump box, if you wanted. If Capistrano looks too cumbersome for what you're doing, you may want to use its sshkit library, which handles some of the ssh concerns you're navigating manually here.

Security Practices

I see you have public/private keypairs for every server you're connecting to. Try to use them instead of getting user input for a password. I would lean on the side of requiring them for anyone using a tool like this. In fact, I might have a distinct identity for automation so that I can revoke or recreate its keys without affecting any other way I can log in to the system. You may also want to add logging so you can audit your script to make sure it did what you intended. This is also helpful as a changelog to look back on when your script worked fine and you're troubleshooting unexpected side-effects of the commands it ran.

The nice thing about this specific identity is you can also leverage it to avoid passing sudo passwords around. This identity could have a limited list of commands it can run either in your sudoers file or without needing sudo. For example, I have used Capistrano to log in as a user that does not have root permissions, but does have elevated permissions that my basic account does not have. This elevated user had file permissions so it could update the code for the servers' websites without requiring sudo, though it couldn't do too much else.

More generally, I get nervous whenever I'm writing any code that handles user input for security credentials. I'd say that if there's already an existing project where flaws in handling security are likely to result in the announcement of a public CVE, it's better to use those projects for than writing your own security implementations to use in production.

In summary

use something more likely to get security audits than you are
have a layered plan for what you do if something goes wrong or the system is abused

Add Some Tests

Do you know what errors SSH is likely to raise if something goes wrong? Should that stop the whole process without continuing? Does your code do that in a way you're happy with?

These are the kind of answers that tests are good at answering for you. One thing I like to keep in mind when writing separate test code feels like a waste of time is the saying, "Everyone tests their code. Some people then throw the tests away, and some people keep them."

I suspect, looking at your code, it would be hard to stub out the ssh library to simulate what happens when when several commands succeed and then one fails. (A discussion of how to stub and mock that sort of thing in Ruby tests feels to big to handle in this answer. Let me know if you have trouble finding guides for it online.) The fact that writing test you'd like to have feels difficult is an excellent sign that you should split up your code differently from how you've written it.

As an example, you could have one chunk of code that accepts some generic connection management object, and then worries about the sequencing of commands and handling failed commands. Then you could have another chunk of code that is the connection object, abstracting the SSH library down to the few parts of it you care about for this script. Maybe there's an even better way to make that split that you'll find once you sit down to it.

Either way, once you reorganize your code so the tests are easier to write, you'll also find it gets easier to make changes, fix bugs, add new features even if you were to never run the tests again. (But you should run the tests again — you already wrote them, and they're just sitting there. No need to waste the effort.)

Good Luck Out There

I hope this helps. Feel free to ask me for further clarification on any of this if you want.


I would recommend:

You're using a GridLayout(2, 3) -- good.
I'd make it actually a GridLayout(2, 0, GAP, GAP) where GAP is a constant that allows a gap between JLabels. The first and second parameters mean 2 rows with variable number of columns.
Then if you set the background color of the JPanel to yellow, make the JLabels opaque and white, give the background JPanel an empty border of GAP width, you've got your yellow borders
Use an enum to combine a Color with a String. An alternative to this could be to use a Map<String, Color>
The enum method values() would then return an array of all the items of that enum. If using the Map such as a HashMap, you'd get the key set and iterate through this.
You could then use a simple for loop to iterate through this array, creating JLabels that have the desired text and the desired foreground colors.
You risk painting yourself in a corner by having your class extend JFrame, forcing you to create and display JFrames, when often more flexibility is often called for. In fact, I would venture that most of the Swing GUI code that I've created and that I've seen does not extend JFrame, and in fact it is rare that you'll ever want to do this. More commonly your GUI classes will be geared towards creating JPanels, which can then be placed into JFrames or JDialogs, or JTabbedPanes, or swapped via CardLayouts, wherever needed. This can greatly increase the flexibility of your GUI coding.

For example,

import java.awt.Color;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.GridLayout;
import javax.swing.*;

@SuppressWarnings("serial")
public class SixLabels2 extends JPanel {
   private static final int GAP = 8;
   private static final int LABEL_WIDTH = 160;
   private static final Dimension LABEL_SIZE = new Dimension(LABEL_WIDTH, LABEL_WIDTH);
   private static final int ROWS = 2;
   private static final Font LABEL_FONT = new Font("Times New Roman", Font.BOLD, 20);

   public SixLabels2() {
      setBackground(Color.yellow);
      setLayout(new GridLayout(ROWS, 0, GAP, GAP));
      setBorder(BorderFactory.createEmptyBorder(GAP, GAP, GAP, GAP));

      // iterate through all the items in the MyColor enum
      for (MyColor myColor : MyColor.values()) {
         JLabel label = new JLabel(myColor.getText(), SwingConstants.CENTER);
         label.setForeground(myColor.getColor());
         label.setPreferredSize(LABEL_SIZE);
         label.setFont(LABEL_FONT);
         label.setOpaque(true);
         label.setBackground(Color.white);
         add(label);
      }
   }

   private static void createAndShowGui() {
      SixLabels2 mainPanel = new SixLabels2();

      JFrame frame = new JFrame("SixLabels2");
      frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
      frame.getContentPane().add(mainPanel);
      frame.pack();
      frame.setLocationByPlatform(true);
      frame.setVisible(true);
   }

   public static void main(String[] args) {
      SwingUtilities.invokeLater(new Runnable() {
         public void run() {
            createAndShowGui();
         }
      });
   }
}

enum MyColor {
   BLACK("Black", Color.black), BLUE("Blue", Color.blue),
   CYAN("Cyan", Color.cyan), GREEN("Green", Color.green),
   MAGENTA("Magenta", Color.magenta), ORANGE("Orange", Color.orange);

   private String text;
   private Color color;

   private MyColor(String text, Color color) {
      this.text = text;
      this.color = color;
   }

   public String getText() {
      return text;
   }

   public Color getColor() {
      return color;
   }

   @Override
   public String toString() {
      return text;
   }

}

Note, my code may not be actually shorter in length, but I'll wager that it's easier to debug and enhance. Consider adding a few more colors to your code and mine and see how many variables and statements need to be changed in both. For instance, if you wanted to add two more colors to mine, all you'd need to do would be to change the enum to include two more colors. The program would take care of the rest for you.

For example, if you changed the enum from this:

enum MyColor {
   BLACK("Black", Color.black), BLUE("Blue", Color.blue),
   CYAN("Cyan", Color.cyan), GREEN("Green", Color.green),
   MAGENTA("Magenta", Color.magenta), ORANGE("Orange", Color.orange);

to this:

enum MyColor {
   BLACK("Black", Color.black), BLUE("Blue", Color.blue),
   CYAN("Cyan", Color.cyan), GREEN("Green", Color.green),
   MAGENTA("Magenta", Color.magenta), ORANGE("Orange", Color.orange),
   GRAY("Gray", Color.gray), FOO("Foo", new Color(100, 250, 255));

The GUI would then automatcially accommodate the additional colors.


Good to see you're not doing a using namespace std ;).
Of course it doesn't really hurt in a source file (.cpp) (it does in a header file!),
but even then that is still considered bad practice. My personal reason to just always type the namespace is because it makes code easier to read (you know what is part of a namespace and which and don't have to guess that) and it is easier to search with regular expressions and the like.

While the standard does not use class names that begin with an upper case letter, many good coders do. It allows you - in fact - to have your own classes to stand out better, and using both upper case and lower case adds more state to your namespace (aka, less collisions). For example, you can call your class Range and a Range variable range.

Also for both, readability and regular expression searches, I strongly recommend only to use complete English words for any class, variable or function name and never resort to abbreviations. The only exceptions to this rule are local variables that are so local (used in a scope of only a few lines) that you can easily see their declaration and use on one screen and where their meaning is mostly irrelevant(!). For example a counter of a for loop can be called i if you must.

Also for ease of regular expression search and understandability (maintenance) of code it is a good idea to use the same name for variables that contain the same object (you copied a value from one to the other). Ok, that is normally not possible unless you pass them as function argument (you can't write range = range) but you'll surprised how often it is; for example use range_end for every variable that means "the end of a range". Why am I mentioning this? Well, this is the reason that class member variables often have a prefix (either M_ or m_ or something. I use m_. That way you avoid collisions with local variables with the same name, and member functions with the same name.

C++ provides classes with a reason: to write Object Oriented code. Encapsulation is part of that. In almost all cases your class member variables should be private!

For (ostream based) debugging output, it is a good idea to make every class
writable to an ostream.

std::move is just a cast, from an lvalue reference to an rvalue reference, and only useful when you pass the result to a function that takes an rvalue reference (aka, you pick the right overloaded function with it). Where you used std::move the argument isn't an lvalue reference, nor do you pass the result of the std::move to a function that accepts an rvalue reference, so using std::move there is nonsense. PS By convention a moved object (whose rvalue reference was passed to a function) should only be destructed afterwards (in all but exceptional cases). So if you cast an lvalue reference to an rvalue reference then that variable had better be passed in as an rvalue reference in the first place. The typical use case therefore looks as follows:

void bar(Bar&& bar);

void foo(Bar&& bar)
{
  // Here bar is an lvalue reference!
  bar(std::move(bar));
  // Now 'bar' may no longer used, only destructed (by convention:
  // it is like that because one would expect that bar() makes it so.
}

This would work fine when foo() took an lvalue reference (foo(Bar& bar)) but
then the caller of foo wouldn't be aware of the fact that bar was moved and could "accidently" use bar still after returning from foo(bar).

Combining what we learned so far, I arrive at the follow code for Range:

class Range
{
  private:
    int m_start_time = 0;
    int m_end_time = 0;

  public:
    Range() = default;
    Range(int start_time, int end_time) :
        m_start_time(start_time),
        m_end_time(end_time)
        { }

    int start_time() const { return m_start_time; }
    int end_time() const { return m_end_time; }

    friend std::ostream& operator<<(std::ostream& os, Range const& range);
};

Well, that's enough for today.
I'll finish with my own stab at celeb_density:

int celeb_density(std::vector<Range> const& schedule)
{
    // Do not pass an empty schedule.
    assert(!schedule.empty());          // #include <cassert>

    std::array<unsigned int, 24> hours; // #include <array>
    hours.fill(0);

    int best_time = schedule[0].start_time();
    int max_celebs = 1;
    for (auto range : schedule)
      for (int hour = range.start_time(); hour < range.end_time(); ++hour)
        if (++hours[hour] > max_celebs)
        {
          ++max_celebs;
          best_time = hour;
        }

    return best_time;
}


There's a lot about this method that makes it very difficult to follow... Let's start with the signature:

public List<Item> Spend(/* ... */)

So, based on the name and return type I'm going to "Spend" something(?) and get a list of items. To me, I find it had to believe this method is going to have a single responsibility straight away. The parameters only confirm the suspicion:

string[] text, List<Item> items, int currency, out int currencyLeft, string singular = null, string textZero = null

Wow. So I'm passing in some "text", a list of items, an amount of currency and optionally a "singular" string (no idea what that is) and a textZero string (again no idea) AND I'm getting an output of how much currency I have left. This is ringing alarm bells - why am I passing in text to a function that lets me spend money... Why is it an array? Why am I passing in items? At the very least I know this method is: showing info to the user, changing how much currency I have and probably changing what items I own. That's 3 separate responsibilities already and we haven't even got into the method yet.

Let's start on that:

var maxCurrencyLength=Math.Max(items.Aggregate(currency,(i,j)=>j.Value).Length()+text.Take(3).Aggregate(0,(i,j)=>i+j.Length)+
        Math.Max(0,(singular=singular??text[1].RemoveLast()).Length-text[1].Length),textZero?.Length??0);

That should be a method by itself! It's basically the most unreadable variable declaration I've ever seen (sorry if that sounds harsh).

Then there's this:

var totalItems=items.Select(i=>new Item(i)).ToList();

Okay, you're deep cloning the items (I would guess) - no indication of why and you've made me try to figure out what you're doing.

while (true) {
    switch (Input)

Most people writing C# prefer braces on new lines. What is Input? A property that gets input from the user?

I could go on but that's not very constructive. What you need to do, is go back to the drawing board and come up with some classes to encapsulate all the different things going on here.

public class Shop
{
    public IList<Item> GetAvailableItems()
    {
        // ..
    }

    public int GetSellItemPrice(Item item)
    {
        // ..
    }
}

You'll need other classes too. Like a Player with an ItemBag (for storing items) and a Wallet (for storing money). Potentially, Transaction objects to pass into Shops/Wallets to change the amount of money etc.

So to answer your question:  the method length is the least of your worries, you need to break the problem up into smaller pieces and make each piece responsible for a single thing.


Monthly change query with SQL Server 2012

The size of the horizontal scrollbar is the first smell. I'd start by breaking down the CASE WHEN's into multiple lines:

SELECT
    ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) Rn,
    CONVERT(varchar, effectivedate, 102) effectivedate,
    SUM(EuroCurrentBalance/1000000) TotalArrears,
    SUM(CASE WHEN Multiplier > 2 THEN EuroCurrentBalance/1000000
             ELSE 0
        END) NPLs,
    CASE WHEN ROW_NUMBER() OVER(ORDER BY (CONVERT(varchar, effectivedate, 102)) ASC) = 1 THEN 0
         ELSE SUM(EuroCurrentBalance/1000000) - (LAG(SUM(EuroCurrentBalance/1000000),1,0) OVER (ORDER BY (CONVERT(varchar, effectivedate, 102))))
    END MonthlyChange
FROM
    DataWarehouse.dwa.FinanceMonthEndData
GROUP BY
    effectivedate
GO

Note that you can group by effectivedate without doing the conversion again.

Then I'd look at the redundant pieces: EuroCurrentBalance/1000000 is repeated quite often, and CONVERT(varchar, effectivedate, 102) as well. Consider a subquery, or a CTE:

WITH PreSelectionCTE
AS
(
    SELECT
        CONVERT(varchar, effectivedate, 102) EffectiveDate,
        EuroCurrentBalance/1000000 Arrear,
        CASE WHEN Multiplier > 2 THEN EuroCurrentBalance/1000000
             ELSE 0
             END NPL
    FROM
        DataWarehouse.dwa.FinanceMonthEndData
)
SELECT
    ROW_NUMBER() OVER(ORDER BY EffectiveDate) Rn,
    SUM(Arrear) TotalArrears,
    SUM(NPL) NPLs,
    CASE WHEN ROW_NUMBER() OVER(ORDER BY (EffectiveDate)) = 1 THEN 0
         ELSE SUM(Arrear) - (LAG(Arrear,1,0) OVER(ORDER BY EffectiveDate))
         END MonthlyChange
FROM
    PreSelectionCTE
GROUP BY
    EffectiveDate

GO

This could probably be improved further, but at least it's much easier to read now! Note that ASC is implicit in an ORDER BY - I'd simply omit it, or specify it consistently: you have it explicit in 2 of your 3 ORDER BY's.