Standard Test methods for checking values in Lists, Maps etc

Map equals has same coverage and it is not attached to implementation: See map.equals javadoc.

So "better" (at least, more compact code) would be:

  assertEquals("something", providedMap, receivedMap);

Well, same for list. That means, you can use the same command as above.

  assertEquals("something", providedList, receivedList);

And even more...

  assertEquals("something", providedMapList, receivedMapList);

Sure, you will not see which key is missing.

So, you can create new static matchers, as assertEquals that could provide more information. But, assertEquals already prints the content of each container (executing toString of expected and received object), so you can figure out what is missing anyway.

Example:

java.lang.AssertionError:
Expected :[1, 2, 3, 4, 6]
Actual   :[1, 2, 3, 4, 5]

Edited

If the output is too big, you can use your method. There are some options to follow:

Create a class with static methods (similar with org.junit.Assert)

public class ContainerAssert {

public static <U, V >void assertEquals(Map<U, V> providedMap, Map<U, V> receivedMap) {
    Assert.assertNotNull("Provided Map is null;", providedMap);
    Assert.assertNotNull("Received Map is null;", receivedMap);
    Assert.assertEquals("Size mismatch for maps;", providedMap.size(), receivedMap.size());
    Assert.assertTrue("Missing keys in received map;", receivedMap.keySet().containsAll(providedMap.keySet()));

    providedMap.keySet().stream().forEach((key) -> {
        Assert.assertEquals("Value mismatch for key '" + key + "';", providedMap.get(key), receivedMap.get(key));
    });
  }
  // ...
}

Use Hamcrest

 // with strict order
 List<String> collection = Lists.newArrayList("ab", "cd", "ef");
 assertThat(collection, contains("ab", "cd", "ef"));

Use Hamcrest and create a specialized matcher


This is generally good code, with standards-conforming names and style.

It appears that size is an afterthought variable. It is not scoped as being 'private', and it is not decreased in the pop() method, so it is buggy in two ways.

Additionally, even in the inner private Node class, I would recommend declaring the item and next as private, and also adding getters/setters for them.

Otherwise, this is good code.


This is insanely nit-picky, but this is really just driving me nuts.

TResult Publish<TCommand, TResult>(TCommand command)

We read left to right from the return type to the name to the parameters, but you're ordered your generic types from param to result. I think this would be much easier to read as

TResult Publish<TResult, TCommand>(TCommand command)

It keeps the declaration closer to its use. Which is, admittedly, trivial in this case, but I think it makes the signature read more like a sentence.

On another note though, I really like that you're filtering the acceptable types. It reminds me that I need to do this more often myself. It's better than just trusting that you (or someone else most likely) knows what kind of types are okay to use generically.


As you might have noted, methods which do printing are of no use in a bigger piece of code. While your lengthy method tries to say something to the user, it fails to communicate the outcome to the caller.

You could add some return value to the printing... but that's like adding insult to injury. Try to write methods computing something, call them, and react by some printing.

Try to start at the top level like

while (true) {
    printQuestion();
    char c = readInput();
    if (!isLegalInput(c)) continue; // i.e., jump to the loop start to ask again
    if (isCorrectAnswer(c)) {
        print("Correct!\n\n");
    } else {
        print("Inorrect!\n\n");
    }
}

Now, there's a s*load of methods to write... but all of them are pretty trivial. You may also find out that they need more arguments or whatever... there are many ways to go. Just don't print in methods which may need to be reused. Every method should do a single thing.


You're mixing up data and logic. Separate your replaces into an associative array and iterate over that instead: (My JS is extremely bad, so this will be more pseudocode-ish)

var associations;
associations['/brun/i'] = "Brown";
associations['/sort/i'] = "Black";
//add all the other associations and then

associations.forEach(runReplace);

function runReplace(value, regex, array) {
   color.replace(regex, value);
}

Additionally I suggest you (just like me here) use the Array.prototype.forEach() when iterating over your colors


Code to remove columns in a (fasta) file with more than 90% '-' characters in that column

Disregarding using a better parser (biopython's SeqIO), here are some immediate speed boosts due to better use of vanilla Python:

You don't actually need the counterofsites at all. If you only want the fraction of '-', this is the same as the average of a sequence of 1 and 0, where the value is 1 (or equivalently, True) if the character is '-':

from statistics import mean

def gap_fraction(alignment, i):
    return mean(site == "-" for record in alignment for site in record.seq[i])

This uses a generator expression to flatten the sites.

The other improvement is using a set instead of a list for the to be filtered elements. This is needed since you later do if i not in tofilter, which needs to scan through the whole list in the worst case. With a set this is immediate, i.e. it is \$\mathcal{O}(1)\$ instead of \$\mathcal{O}(n)\$. You will only see a real difference if your number of columns to filter gets large (>>100), though.

to_filter = {i for i in range(length) if gap_fraction(alignment, i) > 0.9}

I also used a set comprehension to make this a lot shorter and followed Python's official style-guide, PEP8, which recommends using lower_case_with_underscores for variables and functions.

You should also keep your calling code under a if __name__ == "__main__": guard to allow importing from this script form another script without running the code.


Test for Array vs Range in my TEXTJOIN UDF

Okay, let's first eliminate the easy stuff Rubberduck points out:

Function is implicitly Public and implicitly returns a Variant - should be a String.
All parameters are implicitly passed ByRef (and semantically should be ByVal).
Parameter arr is implicitly Variant,
Local variables c, d, t, y, arr2 all have terrible, meaningless names (arr, matey! ..I like calling it Pirate Notation ;)
String-returning Left$ function should be used over Variant-returning Left function.
vbNullString should probably be preferred over "" empty string literal.

With default settings Rubberduck will also complain about Dim t As Long, y As Long, because having multiple declarations in a single instruction isn't ideal.

The variables are declared at the top of the function rather than as close as possible to their usage, which makes it hard to see what's used where.

Type-checking isn't type safe:

If TypeName(arr) = "Range" Then
    arr2 = arr.Value
Else
    arr2 = arr
End If

If the Microsoft Word object model is referenced, you can pass a Word.Range object and it will happily take it; same if I made my own Range class and gave it an instance. Use the TypeOf operator to perform compile-time type-safe type checks:

If TypeOf arr Is Excel.Range Then
    'definitely an Excel Range object
Else
    'could be anything
End If

Note, in the Else branch arr could literally be anything - but you're assuming it's an array. You could use the IsArray function to make sure of that, and then you can also assert on the upper bound using a rather ugly helper function, to make sure you're looking at the 2D array you're expecting.

Else
    Debug.Assert IsArray(arr)
    Debug.Assert GetArrayDimSize(arr) = 2
    arr2 = arr
End If

With the array dimension count known/asserted, the On Error Resume Next statement that follows can be removed... and t can be renamed sourceColumns or colCount or whatever, and y can be renamed sourceRows or rowCount or whatever - and arr2 could be renamed to sourceArray:

sourceColumns = UBound(sourceArray, 2)
sourceRows = UBound(sourceArray, 1)

And with that naming scheme I think I'd rename the arr parameter to source.

If t >= 0 And y >= 0 Then

Ah, ok, so that condition is leveraging the fact that VBA would have thrown an error given a 1D array, leaving t = 0. Not very obvious, let's improve that.

But first we need to tweak the assertions - we don't really want a 2D array, we just want any array with at most two dimensions. So... let's be less forgiving given anything other than that:

Dim dimensionCount As Long
If TypeOf(arr) Is Excel.Range Then
    sourceArray = source.Value
    dimensionCount = 2 'Range.Value is always a 2D array
Else
    Dim isValidArray As Boolean
    isValidArray = IsArray(source)
    If isValidArray Then dimensionCount = GetArrayDimSize(source)
    isValidArray = dimensionCount <> 0 And dimensionCount <= 2
    If Not isValidArray Then Err.Raise 5, "TEXTJOIN", "Expected: 1D or 2D array"
End If

And now that If statement can be much more explicit about what's going on and why:

If dimensionCount = 2 Then
    'handle 2D array
Else
    Debug.Assert dimensionCount = 1
    'handle 1D array
End If

So, c iterates dimension 1 / rows, d iterates dimension 2 / columns.

Now all these concatenations are inherently slow. I realize that's "just a worksheet function" and you'd like to share it as a single, simple, cohesive and focused little piece of code... but given thousands of iterations, VBA's string-handling will start becoming the performance bottleneck of the function.

There's a lightning-fast StringBuilder class right here on this site that you can use to address that.

As a bonus, using a StringBuilder makes the function assign to its return value only once - as opposed to what you have now, which is treating the return-value-identifier as if it were a local variable (which it technically is, ...I just don't like doing that).

Not sure why the d / column loop is inconsistent here:

For d = LBound(arr2, 1) To UBound(arr2, 2)

You're iterating the 2nd dimension, the LBound should be off the 2nd dimension too (yes, it should be the very same as that of the 1st dimension). Also, the upper bounds of both dimensions are already known and stored in local variables:

For currentRow = LBound(sourceArray, 1) To sourceRows
    For currentColumn = LBound(sourceArray, 2) To sourceColumns

This code will throw an error (by design?) if the array contains an Error value:

If arr2(c, d) <> "" Or Not skipblank Then

You could have a parameter that helps you decide how to treat errors - perhaps an Enum could be used:

Public Enum TEXTJOIN_ErrorValues
    ThrowOnError
    SkipError
    IncludeErrorText
End Enum

ThrowOnError would be the current/default behavior; SkipError would treat errors as blanks, and IncludeErrorText would include the e.g. #N/A error text into the result.

Except, once you have an Variant/Error value and not a Range, it's pretty much impossible to get the text back (unless you want to map CVErr(xlErrWhatever) values to a corresponding string... probably not worth it) - so scratch that enum value, and the behavior becomes either throw on error, or skip error values. And that's entirely possible, and quite easy to do - an additional optional Boolean parameter could be helpful for that.

Dim sb As StringBuilder
Set sb = New StringBuilder
'...

For currentRow = LBound(sourceArray, 1) To sourceRows
    For currentColumn = LBound(sourceArray, 2) To sourceColumns
        If Not IsError(sourceArray(currentRow, currentColumn)) Then
            If sourceArray(currentRow, currentColumn) <> vbNullString Or Not skipBlank Then
                sb.Append sourceArray(currentRow, currentColumn)
                sb.Append delim
            End If
        ElseIf Not skipErrors Then
            sb.Append delim
        End If
    Next
Next

TEXTJOIN = sb.ToString

Now, that's the 2D loop.. the 1D loop does essentially the same thing.. and that's annoying. I'd extract a method for that.

Private Sub ProcessValue(ByVal value As Variant, ByVal sb As StringBuilder, ByVal delim As String, ByVal skipBlanks As Boolean, ByVal skipErrors As Boolean)
    If Not IsError(value) Then
        If CStr(value) <> vbNullString Or Not skipBlanks Then
            sb.Append CStr(value)
            sb.Append delim
        End If
    ElseIf Not skipErrors Then
        sb.Append delim
    End If
End Sub

That turns the logic into:

If dimensionCount = 2 Then
    For currentRow = LBound(sourceArray, 1) To sourceRows
        For currentColumn = LBound(sourceArray, 2) To sourceColumns
            ProcessValue sourceArray(currentRow, currentColumn), sb, delim, skipBlanks, skipErrors
        Next
    Next
Else
    Debug.Assert dimensionCount = 1
    For currentRow = LBound(sourceArray, 1) To sourceRows
        ProcessValue sourceArray(currentRow), sb, delim, skipBlanks, skipErrors
    Next
End If

TEXTJOIN = sb.ToString

The order of the parameters strikes me as unintuitive - might be by design to match Microsoft's function, by I would have made the source array/range the first parameter, followed by an optional delimiter, followed by an optional flag to skip blanks (followed by an optional flag to skip errors).


Pivotting SQL Table Response to JSON in JavaScript

Whenever you have code that sets up an array, then has a loop to push items into that array, Array.map() could be used to condense that code. For example, these lines:

const jsonData = [];
data.forEach( (row) => {
  const json = {};
  row.forEach( (item, index) => {
    json[columns[index]] = item;
  });
  jsonData.push(json);
});
return jsonData;

Could be simplified to this:

return data.map( (row) => {
  const json = {};
  row.forEach( (item, index) => {
    json[columns[index]] = item;
  });
  return json;
});

You could optionally simplify the inner loop using Array.reduce()

return data.map( (row) => {
  return row.reduce( (json, item, index) => {
    json[columns[index]] = item;
    return json;
  }, {});
});

There are some good exercises in Functional Programming in Javascript where you practice implementing some of those methods.


PHP magic function for accessors and mutators

Exceptions are fine, but consider using less general Exception classes, like BaseException or whatever. This will allow for more flexible error reporting.

First thing that strikes me as odd about your code is that you're using underscore to name your properties, but CamelCase to name your methods. This is quite inconsistent and leads to unnecessary complexity here. Using naming convention that actually helps you is a good idea.

With that in mind, a little change in naming convention could turn into something like this:

function __call($method, $args)
{
    $action = substr($method, 0, 3);
    $prop = strtolower(substr($method, 3));
    $getter = "get".ucfirst($prop);
    $setter = "set".ucfirst($prop);

    switch ($action) {
        case "get" :
            if (isset($this->_fields[$prop])) {
                return $this->_fields[$prop];
            } else throw new EntityException("Unknown property {$prop}");
            break;
        case "set" :
            if (isset($this->_fields[$prop])) {
                $this->_fields[$prop] = $args[0];
                return null;
            } else
                throw new EntityException("Unknown property {$prop}");
            break;
        default : throw new EntityException("Unknown action {$action[0]}");
    }
}

Notice i use a an array $this->_fields to store all my fields. This goes for my second point - Safety and Reusability.

Consider this piece of code:

class User
{
    private $_name;

    function getName(){
        return $this->_name;
    }
}

We use our getName method everywhere, but later we need to add another field - second name. We can safely insert this second field inside our getName and the code would change without taking any further actions. This is kinda the whole point behind getters and setters - incapsulating logic away.

Now with your code you'll have complications with overriding your setters, and that doesn't make it safe in any way.

Now, reusability. In your base class, you write all your fields explicitly, which doesn't seem very reusable to me. Look this code (warning, large wall of text incoming).

abstract class Entity
{
    private $_fields;
    protected $schema = array();

    function __construct(array $data = array())
    {
        /** Check that our concrete implementation has its Schema */
        if (!$this->schema) {
            throw new EntityException("Class ".get_called_class()." Schema not defined");
        }
        /**
         * mapping fields value to our schema
         *
         * @TODO Move to separate method to do this not only on Construct;
         */

        $this->_fields = array_reduce(
            $this->schema,
            function ($fields, $element) use ($data) {
                $fields[$element] = $data[$element];
                return $fields;
            }
        );
    }

    function __call($method, $args)
    {
         /** Same call implementation here as above */
    }

    /**
     * Inner getter for getter/setter overrides
     */

    protected function getField($name)
    {
        if (!isset($this->_fields[$name])) {
            throw new EntityException ("Property {$name} not found");
        }
        return $this->_fields[$name];
    }

    /**
     * Inner setter for getter/setter overrides
     */

    protected function setField($name, $value)
    {
        if (!isset($this->_fields[$name])) {
            throw new EntityException ("Property {$name} not found");
        }
        $this->_fields[$name] = $value;
    }
}

/**
 * Class User
 *
 * Concrete implementation of Entity class
 *
 */
class User extends Entity
{
    /**
     * List of our entity fields
     * @var array
     */
    protected $schema = array("id", "name", "email", "phone");

    /**
     * method getName concrete implementation;
     *
     * @return string
     */

    function getName()
    {
        return "My name is ".$this->getField("name");
    }
}

try {
    $userData = array("id" => 1, "name" => "Andrew", "phone" => "000000000", "email" => "test@test.com");

    $user = new User($userData);

    echo $user->getName()."
";
    echo $user->getEmail()."
";
    $user->setPhone("111111111");
    echo $user->getPhone();

} catch (Exception $e) {
    echo $e->getMessage();
}

Now, if you want to create a News entity, you can simply write:

class News extends Entity
{
   protected $schema = array("id", "title", "text", "time");
}

And use it at your leisure. Later you can add active record methods to Entity, like save, delete, update and so on, and still use them on any entity you might want.

As to why i used array $_fields instead of simply listing fields - array is much more convenient when you have to map values to your fields and build queries from them.


I most cases the norm for complex numbers means the magnitude/absolute value, so if you want Gauss' definition you might think about a different name.

If you want it really fast and and with an error of at most one ulp it gets quite complicated. You might download a version of Cygwin's new-libc (e.g. from the git here: https://github.com/mirror/newlib-cygwin) and take a look at newlib/libm/math/e_hypot.c to get the gist of it.

What you have now is quite slow and imprecise, for two reasons:

the return of creal()/cimag() is a double, no need for the complex function cpow().
you have two squarings and one addition; all three together have a good chance of unnecessarily adding error.

There is the function cabs() which computes the Euclidean norm sqrt(a²+b²). I would propose to use that function and simply square the outcome if you do not want to go the long and complicated way. If cabs() is implemented well (error less than 1 ulp) you should get a good error value for your own function, too.


The first and most obvious improvement would be better variable naming:

IQueryable<Article> privateArticlesCreatedByUser =
    db.Articles.Include(article => article.Categories)
        .Include(article => article.Shared)
        .Where(article => article.Private && article.createdby == User.Identity.Name);

IQueryable<Article> articlesSharedWithUser =
    db.Articles.Include(article => article.Shared)
        .Where(sharedArticle => sharedArticle.Shared.Any(shareRecipient => shareRecipient.User == user));

IQueryable<Article> articlesCreatedByOrSharedWithUser = privateArticlesCreatedByUser.Concat(articlesSharedWithUser);

IQueryable<Category> categories = db.Categories
    .Where(category => articlesCreatedByOrSharedWithUser.Any(articleCategory => articleCategory.categoryID == category.Id));

Obviously these names may not perfectly align with what the data means (as they're basically educated guesses on my part), but already they're a lot easier for somebody else to understand.

I also changed your & to an &&, since it then saves an equality check sometimes.

I also notice that you are including shared articles occurs twice. This might be too trivial an improvement and it's debatable if it will improve readability at all, but you could try this:

var sharedArticles = db.Articles.Include(article => article.Shared);

IQueryable<Article> privateArticlesCreatedByUser =
    sharedArticles.Include(article => article.Categories)
        .Where(article => article.Private && article.createdby == User.Identity.Name);

IQueryable<Article> articlesSharedWithUser = sharedArticles
        .Where(sharedArticle => sharedArticle.Shared.Any(shareRecipient => shareRecipient.User == user));

IQueryable<Article> articlesCreatedByOrSharedWithUser = privateArticlesCreatedByUser.Concat(articlesSharedWithUser);

IQueryable<Category> categories = db.Categories
    .Where(category => articlesCreatedByOrSharedWithUser.Any(articleCategory => articleCategory.categoryID == category.Id));


Clojure programming exercise for calculating change

First, there is nothing wrong with your recursive call: recur is about as idiomatic as Clojure gets.

Now, let's try to improve your code.

If you look at it, smaller-than? is a synonym for >=. It just
passes it the same arguments in the same order.

You could define it as ...

(def smaller-than? >=)

... or, better, just use >=. If you want your own name, you'd have been better to call it not-smaller-than?.

There is no need to nest lets in change-calc. One let will do.
Successive bindings    in a let are evaluated in order.
Try to choose suggestive names. I'd replace the meaningless
calculated with something like candidate.

Thus we get

(defn max-smaller-than [input]
  (apply max (filter (partial >= input) in-bank)))

(defn change-calc [change-requested change-given]
  (let [candidate (max-smaller-than change-requested)
        upd-change-given (conj change-given candidate)
        sub-request (- change-requested candidate)]
          (if (= sub-request 0)
            upd-change-given (recur sub-request upd-change-given))))

There are a couple of worries here:

We'd better deal with a zero change-requested directly than work
out it's going to be zero next time round.
The test for sub-request is for exactly zero. What happens if we
can't give exact change? (With 1 in in-bank, this will not arise.
But if it ever does, max throws an arity exception).

These revisions get us to

(defn change-calc [change-requested change-given]
  (if (pos? change-requested)
    (let [candidate (max-smaller-than change-requested)
          upd-change-given (conj change-given candidate)
          sub-request (- change-requested candidate)]
      (recur sub-request upd-change-given))
    change-given))

But this doesn't deal with the exception throwing problem. What is perhaps a better solution to this follows.

Let's look at the problem afresh.

Your code returns

its second (sequence) argument with ...
the change from the in-bank denominations conj-ed onto it, so at
the front/back if the sequence is a list/vector.

In any programming language, the second function should be distinct: especially in Clojure, where it is easy to append to a vector.

How do we do it?

For the change, we can return a count of how many coins there are of
each denomination, using a map from denomination to number. If
Clojure had a standard multiset/bag, we'd use it instead.
And let's make the denomination set an explicit argument.

If we want to make up a sum from a number of denominations, we can do the following:

(defn change [sum denoms]
  (reduce
    (fn [[ans ch :as both] d]
      (let [c (quot ch d)]
        (if (zero? c) both [(assoc ans d c) (mod ch d)])))
     [{} sum]
     (sort > denoms)))

This is more or less the algorithm you wrote, with a few wrinkles:

We sort the denominations in decreasing order.
We deal with each once and for all through a reduce.
We use quot and mod to short-circuit a loop using subtraction.

The result is a pair:

a map representing a multiset of how many (non-zero) there are of
each denomination, and
a number for how much change is left over unreconciled.

Examples:

(change 8 in-bank)
;[{1 1, 2 1, 5 1} 0]

(change 8 #{2 5 10})
;[{2 1, 5 1} 1]

(change 13 #{1 5 10})
;[{1 3, 10 1} 0]

The idea of returning the residue too arose from the algorithm, where
it is part of the working.
If 1 is a denomination, the residue is always 0.
A development might be to know how many of each coin were present to
start with.


Factory object with a dependency on a bean

This looks like example code so I won't go into detail. If I had to choose between the two, I'd go with the first, because you should not be instantiating application contexts like this. There should be ONE application context and it should be created in main or ServletContextListener.contextInitialized or some such initialization method. In general service classes should be singleton scope (as opposed to prototype scope, not related to singleton pattern). Which means they should not be created using new in code.

@Component("finderFactory")
public class FinderFactory { // use standard java braces
    @Autowired private LocationCache locationCache;
    @Autowired private NorthFinderService northFinderService;
    @Autowired private SouthFinderService southFinderService; // use consistent names
    @Autowired private GeneralFinderService generalFinderService;

    public FinderService getFinderService(String location) {
        if (locationCache.isInNorth(location))
            return northFinderService;

        // you don't have to use `else`
        // when `if` is followed by a `return`.
        if (locationCache.isInSouth(location))
            return southFinderService;

        return generalFinderService;
    }
}


Python DictionaryTagger class takes too long to initialize

So far all I can see being called is the __init__ method, so I thought I would focus on that. It feels weird, so I wanted to first explain why it felt that way, and then address your questions a little.

Readability first

I think the core reason is that this code feels weird is that it appears to use list comprehensions "just because". It creates a temporary list called files; it creates a temporary list called dictionaries; it closes all the already-opened files, and then it iterates over all the dictionaries. This feels weird because these temporaries serve no obvious purpose. Instead the code could easily set its invariants (initializing self.dictionary and self.max_key_size) and then do a simple for loop over thedictionary_paths:

def __init__(self, dictionary_paths)
    self.dictionary = {}
    self.max_key_size = 0
    for path in dictionary_paths:
        dict_file = open(path, 'r')
        curr_dict = yaml.load(dict_file)
        dict_file.close()
        for key in curr_dict:
            : : :

This may have some performance benefits, but they are likely to be too small to measure. Namely it puts less file-based pressure on the system as it only opens a single file at a time. Similarly there's somewhat less memory pressure, as only one curr_dict is maintained at a time. One downside is if initialization should fail if it can't open one of the dictionary_paths, that now happens after it does more work. I'm fine with that trade-off.

This change also gives you the opportunity to use modern approaches for closing files. For example, the open/load/close could instead use with to automatically close at the end of the block:

    with open(path, 'r') as dict_file:
        curr_dict = yaml.load(dict_file)

All in all, this advice so far makes things slightly easier to read. There are fewer moving pieces to keep in your head at the same time, and the ones that are there, are ones that we are used to tracking.

Performance concerns

Once the code is easy to read, it becomes easier to consider performance. If you're used to optimizing a compiled language, you'll find that optimizing python can be counter-intuitive. Where in a compiled language it's sometimes helpful to do a larger number of smaller operations (building manual caches, doing shifts and additions to avoid a multiplication), in python it's almost always the reverse: the fewer operations the better, even if they each take longer. What's in common? Always profile. I don't have your data, so I'm going to shoot from the hip here and offer some general ideas. It's up to you to profile them and see if any of the ideas are useful.

Loops are a good first guess, and the loop of __init__ is almost just a conditional. At first it might seem like there's nowhere to go with this, but there are actually three main things I would test to see if they are worth changing. First, the code traverses the internals of curr_dict multiple times; once is in the for loop, and once is in the lookup curr_dict[key]. Do these together by using dict.items (or dict.iteritems in python 2.x if curr_dict might be large):

for key, value in curr_dict.items():         # or .iteritems() in 2.x
    if key in self.dictionary:
        self.dictionary[key].extend(value)
    else:
        self.dictionary[key] = value
        self.max_key_size = max(self.max_key_size, len(key))

Second, to make the if and else case more similar, pull the max_key_size calculation out of the conditional:

for key, value in curr_dict.items():         # or .iteritems() in 2.x
    if key in self.dictionary:
        self.dictionary[key].extend(value)
    else:
        self.dictionary[key] = value
    self.max_key_size = max(self.max_key_size, len(key))

Or better, per what I mentioned above, let's call it once outside of the loop, having it do more work the single time we call it:

for key, value in curr_dict.items():         # or .iteritems() in 2.x
    if key in self.dictionary:
        self.dictionary[key].extend(value)
    else:
        self.dictionary[key] = value
self.max_key_size = max(len(key) for key in self.dictionary)

Third, back to the conditional. Let's examine two ways to collapse this all into a single line of code. One way is to use dict.setdefault. It reads a little funny, but it does exactly the above in one line:

self.dictionary.setdefault(key, []).extend(value)

The upside is that this only traverses the dictionary internals once. What's the downside? Even if you don't need it, this creates an empty list. That's unlikely to be a big performance concern, but if it is, the second way avoids this. A specialized dictionary type, collections.defaultdict lets you choose a "factory" for missing items. So selecting list as that factory will make self.dictionary do something similar to that setdefault line on its own:

self.dictionary = collections.defaultdict(list)
: : :
    self.dictionary[key].extend(value) # creates and extend empty list if key not present

Of course there's a potential downside to this approach too: this dictionary will create empty lists rather than throwing exceptions if someone tries to query a missing key. If that's a problem, you can always convert it back to a regular dictionary later. For convenience, here's all of that together:

def __init__(self, dictionary_paths)
    self.dictionary = collections.defaultdict(list)

    for path in dictionary_paths:
        with open(path, 'r') as dict_file:
            curr_dict = yaml.load(dict_file)
        for key, value in curr_dict.items():
            self.dictionary[key].extend(value)

    # optionally: self.dictionary = dict(self.dictionary)
    self.max_key_size = max(len(key) for key in self.dictionary)

Note also at this point it may be simpler to combine the with and for blocks, as the name curr_dict exists solely to let you close a single dict_file a little earlier. That's much less important now that it's only a single file, and perhaps having one less name and one less line of code is worth keeping the file open a little longer.

What about your other ideas?

First I'd make the loading as fast as it can be without having to resort to ugly tricks. The next steps depend a lot on initialization patterns you don't show here.

Do you instantiate a lot of DictionaryTagger instances with the same set of dictionary_paths? If so, it may be worth caching the dictionary that is created for a set of paths, and just copying (or referencing) that dictionary in later instances.
Do you instantiate a lot of DictionaryTagger instances with different combinations of a few underlying dictionary files? If so, it may be worth caching the dictionary created for each file, and merging them instead of loading the yaml file from scratch.
Do you instantiate just a single DictionaryTagger or not share any dictionary files? If so, there's nothing I can see worth trying to share.
Can you reuse a DictionaryTagger multiple times? If so, and if it's expensive enough to create it, by all means cache and share the instance. If doing so adds more than a tiny bit of complexity to your code, try profiling to be certain that the benefit will be worth it. At the very least, do a simple test with timeit to verify that class initialization is the problem.

Finally, before you even consider a performance rewrite, write some unit tests. That's even a good idea before refactoring like I showed above. I nearly missed the difference between append and extend, and thought your code had a bug in the if/else condition. Had I refactored that while misunderstanding it, I would have introduced a bug. Unit tests can help you avoid such errors, or help you find others.

Other comments

I didn't read your other two functions very closely, but I did notice a comment in tag_sentence that I wanted to call out:

   j = min(i + self.max_key_size, n)  #avoid overflow

Since your code uses j in a slicing operation, this comment suggests a mindset I would expect in a different language. Slicing operations automatically limit to the bounds of the container. For example, the following three slices give the same result:

>>> hello = "world"
>>> hello[3:5]
'ld'
>>> hello[3:10]
'ld'
>>> hello[3:]
'ld'

Then again, your code does math on the value of j, so letting it start too large could result in extra iterations. Thus the min may be necessary. Or there may be a way to start with the expressions built from the whole sentence, then remove the last word each time through the loop until it's empty. When you can find algorithms in python that don't involve index arithmetic, you'll often be happier with the results.

Correspondingly it's usually a good idea to prefer constructs that look like

for i in range(1, n):
    : : :

over ones that look like

while i < n:
    : : :
    i += 1

however your actual case has some extra conditionals in there that might not allow this conversion, so this advice may not apply here.


I'm not familiar with CI, but from what I can tell the concept seems alright. At least, what I am able to glean from your code seems to be. There are some improvements you could make however.

The only reason to abstract the setting of the $_nav property with the _load_nav() method would be so that you could inject different navigation schemes to allow for further expansion. However, you do not inject your navigation into it and instead hardcode it, albeit with another method. If you are always going to use the _get_nav() method to "inject" your navigation then there is no need for the _load_nav() method as it just adds unnecessary abstraction.

//injection
protected function _load_nav( $nav ) {
    $this->_nav = $nav;
}

Another abstraction issue is with the $_nav property itself. Your current implementation creates an associative array container for the navigation array. This is redundant as well as unnecessarily abstract. The only reason to abstract the menu in this way is if you were abstracting submenus and injecting their contents into a main menu container. This is pretty similar to what you are currently doing with your _get_nav() method, though you have combined them all already.

protected function _load_nav( $header, array $nav ) {
    $this->_nav[ $header ] = $nav;
}

In your current implementation of your _render() method you are getting all of the function arguments into an array then, essentially, removing the first element after setting it to the TITLE_REGION. If you use array_shift() to physically remove that first element, then your array will no longer contain that element and you can loop over it more naturally. This also has the added benefit of removing these "magic" numbers from your code.

$title = array_shift( $args );
$this->template->write( self::TITLE_REGION, $title );

$n = count( $args );
for( $i = 0; $i < $n; $i++ ) {

There is no need to explicitly check the $active variable. If it is NULL, then it will not, or rather should not, be set in the $_nav array. Alternatively, if a variable is NULL then it is considered to be not set, so you can use isset() on both variables to simulate an AND comparison, though again, this is unnecessary in this context.

if( isset( $this->_nav[ 'nav' ] [ $active ] ) ) {
//Unnecessary Alternative
if( isset( $active, $this->_nav[ 'nav' ] [ $active ] ) ) {

You might want to think about saving your navigation to a separate file so that if you ever need to change it you wont have to change the code directly to do so. This follows the Separation of Concerns, which helps to make your code more extensible. Another benefit is that it will allow you to implement that abstract method in MY_Controller without losing functionality. One method would be to use JSON. There are many other methods you can use, but JSON is the one I'll show you. The method you choose is up to you.

//JSON file
{ "collection/view" : [
    {
        "href"  : "collection/view",
        "value" : "View collection",
        "class" : "icon-film"
    },
    //etc...
] }

//Importing the JSON
protected function _get_nav( $file ) {
    $json = file_get_contents( $file );
    return json_decode( $json, TRUE );
}

Your index() method is redundant with your view() method. Seeing as how they are both public, having them both is unnecessary. If one were public and the other private then one could be considered the public wrapper, but unless that wrapper was doing something else then it would still be considered redundant and better if you just changed the scope of the other method to public.

You should define your arrays before trying to append to them. Otherwise you could be editing a global. If you meant to edit said global, then you should know that globals are evil and you should never do such a thing. Defining the array before trying to use it, assuming you aren't using globals, ensures that you aren't accidentally manipulating another array defined earlier or accessing a string through the array syntax. The former may not be as noticeable, but the latter will have vastly different results. Another reason not to do this is to avoid PHP issuing warnings, which it should be doing. If you are not seeing these warnings then you should turn up your error reporting so that you can.

$data = array();
$data['list'] = $this->collection_model->get_list();

Instead of rendering your HTML with PHP, you might want to consider using the templating syntax and just rendering the PHP values you need into your HTML. This makes for cleaner, and thus easier read and edit, HTML. For example:

    <?php foreach( $nav AS $item ) : ?>

    <?php $class = isset( $item[ 'class' ] ) ? $item[ 'class' ] : ''; ?>
    <li class="<?php echo $class; ?>">
        <a href="<?php echo $item['href'];?>">

            <?php echo $item[ 'value' ]; ?>

    <?php endforeach; ?>


I don't really like the dependencies on all that static stuff. It effectively means your code becomes very hard to test.

Also this code:

if (Windows.Storage.ApplicationData.Current.RoamingSettings.Values.ContainsKey("Theme"))
{
    int value = (int)Windows.Storage.ApplicationData.Current.RoamingSettings.Values["Theme"];
    if (value >= 0 && value <= 2) { Theme = value; }
    else { Theme = 0; }
}
else { Theme = 0; }

seems repeated and should be consolidated into a single method.

I'd start cleaning it up by introducing an IThemeProvider which can be injected into your MainPage, MainPageVM and Settings classes. Would probably look like this:

class ThemeChangedEventArgs : EventArgs
{
    public readonly int NewTheme;

    public ThemeChangedEventArgs(int newTheme)
    {
        NewTheme = newTheme;
    }
}

interface IThemeProvider
{
    int GetCurrentTheme();
    void SetCurrentTheme(int theme);
    event EventHandler<ThemeChangedEventArgs> ThemeChanged;
}

Then your MainPage would look something like this:

private MainPageVM Data = new MainPageVM();
private IThemeProvider _ThemeProvider;

public MainPage(IThemeProvider themeProvider)
{
    _ThemeProvider = themeProvider;

    this.InitializeComponent();
    SettingsPane.GetForCurrentView().CommandsRequested += OnCommandsRequested;

    this.DataContext = Data;

    this.SizeChanged += (s, args) => PageSizeChanged()
    _ThemeProvider.ThemeChanged += (s, e) => Data.Theme = e.NewTheme;
}

MainPageVM:

private IThemeProvider _ThemeProvider;
public MainPageVM(IThemeProvider themeProvider)
{
    _ThemeProvider = themeProvider;
}

private void SetValues()
{
    Theme = _ThemeProvider.GetCurrentTheme();
}

and Settings:

private IThemeProvider _ThemeProvider;
public Settings(IThemeProvider themeProvider)
{
    _ThemeProvider = themeProvider;
}

private void InitSettings()
{
    Theme.SelectedIndex = _ThemeProvider.GetCurrentTheme();
}

private void ThemeChanged(object sender, SelectionChangedEventArgs e)
{
    _ThemeProvider.SetCurrentTheme(Theme.SelectedIndex);
}

Code looks a lot cleaner now I'd say and you can easily mock the IThemeProvider out and test it.

Implementation of the provider could be like this (based on what you currently do):

public class ApplicationDataThemeProvider : IThemeProvider
{
    public ApplicationDataThemeProvider()
    {
        Windows.Storage.ApplicationData.Current.DataChanged += (a, o) =>
        {
            CoreApplication.MainView.CoreWindow.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, () =>
            {
                OnThemeChanged((int)ApplicationData.Current.RoamingSettings.Values["Theme"]);
            });
        };
    }

    public int GetCurrentTheme()
    {
        if (Windows.Storage.ApplicationData.Current.RoamingSettings.Values.ContainsKey("Theme"))
        {
            int value = (int)Windows.Storage.ApplicationData.Current.RoamingSettings.Values["Theme"];
            return (value >= 0 && value <= 2) ? value : 0;
        }
        return 0;
    }

    public void SetCurrentTheme(int theme)
    {
        ApplicationData.Current.RoamingSettings.Values["Theme"] = theme;
        ApplicationData.Current.SignalDataChanged();
    }

    public event EventHandler<ThemeChangedEventArgs> ThemeChanged;
    private void OnThemeChanged(int newTheme)
    {
        var handler = ThemeChanged;
        if (handler != null)
        {
            handler(this, new ThemeChangedEventArgs(newTheme));
        }
    }
}

This encapsulates all the nasty global app data stuff in a single class where it should be a lot easier to handle and debug.


Filtering with multiple inclusion and exclusion patterns

You can use fnmatch.filter to simplify multiFilter:

for pattern in patterns:
    for name in fnmatch.filter(names, pattern):
        yield name

This still allows an element to be yielded several times, so instead you might want to do:

for name in names:
    if any(fnmatch.fnmatch(name, pattern) for pattern in patterns):
        yield name

You can also avoid creating a second set by using .difference:

return set(included).difference(excluded)

I would probably stick with the original, though. I only mention it because a fair number of people don't know about it and sometimes it does matter.

Your naming breaks PEP8. Don't let unittest fool you; unittest was written by heathens before the style guide was standardized.

A quick touch-up is

import fnmatch

def super_filter(names, inclusion_patterns=[], exclusion_patterns=[]):
    """
    Enhanced version of fnmatch.filter() that accepts multiple inclusion and exclusion patterns.

    If only inclusion_patterns is specified, only the names which match one or more patterns are returned.
    If only exclusion_patterns is specified, only the names which do not match any pattern are returned.
    If both are specified, the exclusion patterns take precedence.
    If neither is specified, the input is returned as-is.
    """
    included = multi_filter(names, inclusion_patterns) if inclusion_patterns else names
    excluded = multi_filter(names, exclusion_patterns) if exclusion_patterns else []
    return set(included) - set(excluded)

def multi_filter(names, patterns):
    """Generator function which yields the names that match one or more of the patterns."""
    for name in names:
        if any(fnmatch.fnmatch(name, pattern) for pattern in patterns):
            yield name

I would also consider changing the description; it's obvious from introspection that inclusion_patterns and exclusion_patterns default to empty, which is counter-intuitive. I would change it to

def super_filter(names, inclusion_patterns=('*',), exclusion_patterns=()):
    """
    Enhanced version of fnmatch.filter() that accepts multiple inclusion and exclusion patterns.

    Filter the input names by choosing only those that are matched by
    some pattern in inclusion_patterns _and_ not by any in exclusion_patterns.
    """
    included = multi_filter(names, inclusion_patterns)
    excluded = multi_filter(names, exclusion_patterns)
    return set(included) - set(excluded)

Your tests cover the general case fine but they don't check edge-cases; what happens with 0-length inputs? What about complicated patterns? Do you ever check precedence, despite having mentioned it?


In your first code you already have everything you need, you just don't use it. List2 and List3 are each lists of points to create. But they only contain one point, so your for loop does nothing exciting, it only runs once.

You can simply add more points to any of the lists, here I take the first as an example:

import simplekml

points = [['Placemark', 'old file', 51.500152, -0.126236],
          ['New placemark', 'new file', 51.600152, -0.136236]] # description, lat, lon

kml = simplekml.Kml()
for point in points:
    kml.newpoint(name=point[0], description=point[1],
                 coords=[(point[3], point[2])])  # lon, lat, optional height

kml.save("test2.kml")

You could make this a bit more readable by using a collections.namedtuple for the markers. This way your comments become superfluous:

from collections import namedtuple
import simplekml

Marker = namedtuple("Marker", ["name", "description", "lat", "lon"])

points = [Marker('Placemark', 'old file', 51.500152, -0.126236),
          Marker('New placemark', 'new file', 51.600152, -0.136236)]

kml = simplekml.Kml()
for point in points:
    kml.newpoint(name=point.name, description=point.description,
                 coords=[(point.lon, point.lat)])

kml.save("test2.kml")

Note that I followed Python's official style-guide, PEP8, when naming my variables. Variables and functions should be lower_case and only classes (such as the namedtuple class I create) should be in PascalCase. PEP8 also recommends putting a space after commas and not to use unnecessary whitespace.


for key in preliminary_key_list:
    key = list(key) # convert from immutable tuple to mutable list

You don't need to do this. Rather then creating a modifiable list, just use the tuple and construct a new tuple at the end

    i = 0
    translation = {}
    counts = [[j, key.count(j)] for j in set(key)]

This should be a list of tuples, not a list of lists.

    counts_right = [j[1] for j in counts]

    for count in set(key):

Use for i, count in enumerate(set(key)): to avoid having to manage i yourself.

You use set(key) twice, which will force python to do work twice. Also, there is no guarantee you'll get the same order both times.

        translation[counts[counts_right.index(max(counts_right))][0]] = i
        counts_right[counts_right.index(max(counts_right))] = 0

These lines are really expensive. max will scan the whole list checking for the highest value. .index will then rescan the list to find the index for the value.
Then you do the whole thing over again on the next line!

You'd do far better as @GlenRogers suggests to sort by the counts.

        i += 1

    for i in range(len(key)):
        key[i] = translation[key[i]]

Here using a comprehension would be better (I think):

 key = tuple(translation[item] for item in key)

Here is how I'd write the code:

ret = []
for key in preliminary_key_list:
    counts = sorted(set(key), key = lambda k: -key.count(k) )
    translation = { val: idx for idx, val in enumerate(counts) }
    ret.append([translation[k] for k in key])
return ret


UITableViewDataSource with different sections and SearchController

The numberOfRowsInSection: function looks good to me, changing this function to something better will depend on the possible logic you can apply in combination with the section parameter and your data. For example, if you have an array of dictionaries like this:

let data = [ 1: ["hola", "hey"], 2: ["adiós"] ]

The implementation of the method could be reduced to:

func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
    if self.searchController.isActive {
        let array = searchData[section]
        return array.count
    }
    let array = data[section]
    return array.count
}

That indeed is valid for five sections and for one millions sections.

For the cellForRowAt indexPath: function, I would put first the check for the searching status:

if self.searchController.isActive {
    // handle sections for searching
} else {
    // handle sections normaly
}

Doing it like this will allows you to, for example, group functionality shared across multiple sections. An implementation taking the example I put could be like:

func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
    let text = ""
    if self.searchController.isActive {
        let array = searchData[indexPath.section]
        text = array[indexPath.row]
    } else {
        let array = data[indexPath.section]
        text = array[indexPath.row]
    }

    configureCell(with: text, at: indexPath)
}

I did this because I prefer to not overload so much the implementation of the delegates. In configureCell:at: you must implement the logic needed for the creation of the cells based in sections and the data you received.

As I wrote before, the implementation of those methods will depends on the structure of your data, it could reduce or increase the amount of code you need to show the data in the intended way.


Excel sheet code is taking too much time

Excel interoperability is inherently slow - you have the .NET runtime talking to a COM object model through COM interop, there's a performance penalty just for doing that.

Hence, you should strive to reduce the number of times you're accessing the worksheet cells, be it to read or to write.

Before going into the algorithm, I'd like to point out the naming issues. Bad names make the code harder to read and longer to parse. Typing code is 20% of the job - the other 80% is spent reading code, so when the first 20% makes a good job at being readable, the 80% can be more productive.

Avoid single-letter identifiers (for-loop variable is ok though) and disemvoweling. Use comments - don't write code that assumes the reader is also looking at the worksheet in question: name things so that we know what columns C, E, J and L are, or comment to that effect.

Now. We have two loops, both iterating from 2 To i20 - 1. Round one would be to merge them:

For i As Integer = 2 To i20 - 1

    If (.range("E" & i).value = .range("L" & i).value) Then

        For j As Integer = 0 To 36
            bmhor(ij, j) = .cells(i, j + 1).value
        Next

        ij = ij + 1

    End If

    If (.range("C" & i).value = .range("J" & i).value) Then

        For j As Integer = 0 To 36 'I'd believe identifier j can be reused here
            bmver(ik, j) = .cells(i, j + 1).value
        Next

        ik = ik + 1

    End If

Next

Just doing that has reduced the number of iterations by half. Now these two blocks look awfully similar. Let's see if we can extract a method here:

Sub NameMe(Value1 As String, Value2 As String, Row As Long, TheWorksheet As Worksheet, TheArray As Object(,))

    Dim Counter As Integer
    If (Value1 = Value2) Then

        For i As Integer = 0 To 36
            TheArray(Counter, i) = TheWorksheet.Cells(Row, i + 1).Value
        Next

        Counter += 1

    End If

End Sub

Then the loop can look like this:

Dim ValueForColumnE As String
Dim ValueForColumnL As String
Dim ValueForColumnC As String
Dim ValueForColumnJ As String

For i As Integer = 2 To i20 - 1

    ValueForColumnE = .range("E" & i).Value.ToString()
    ValueForColumnL = .range("L" & i).Value.ToString()
    ValueForColumnC = .range("C" & i).Value.ToString()
    ValueForColumnJ = .range("J" & i).Value.ToString()

    NameMe ValueForColumnE, ValueForColumnL, i, ows, bmhor
    NameMe ValueForColumnC, ValueForColumnJ, i, ows, bmver

Next


Should I split code into smaller functions? Advice on layout

I would basically write a method wherever you've written a comment on what a block of code does. This allows you to hide the specifics of a particular operation and makes it clearer which code paths are taken and what they do at a high level:

private Service1 GetService()
{
    return new TestProjectService.Service1();
}

private CommandMessages GetCommand(Service1 service)
{
    var command = new SimpleWebServiceApp.TestProjectService.CommandMessages();

    switch (WebServiceRadio.SelectedIndex)
    {
        case 0:
            command = service.GetData();
            break;
        case 1:
            command = service.GetDataLINQ();
            break;
    }

    return command;
}

private void DisplayResult(string text)
{
    LatestCommandLabel.Text = text;
}

private string GetUsername()
{
    return System.Security.Principal.WindowsIdentity.GetCurrent().Name;
}

private void SubmitData(string username, Command command)
{
    var dc = new TestProjectLinqSQLDataContext();

    var msg = new TestProjectCommandMessage
    {
        Command_Type = command.CommandType;
        Command = command.Command;
        DateTimeSent = command.DateTimeSent;
        DateTimeCreated = command.DateTimeCreated;
        Created_User = userName;
        Created_Dttm = DateTime.Now;
    };

    dc.TestProjectCommandMessages.InsertOnSubmit(msg);
    dc.SubmitChanges();
}

private void ReturnValue()
{
    var service = GetService(); // We don't care how we construct the service, just get it
    var command = GetCommand(service); // We don't care what how we get the command, just get it

    if (command != null)
    {
        // Display the result however it needs to be displayed
        DisplayResult(String.Format("Last Command Called (Using {0}) : {1}", WebServiceRadio.SelectedItem.ToString(), command.Command));
        string userName = GetUsername(); // We don't care how we get the username, just get it
        SubmitData(username, command); // We don't care how we submit the data, just submit it
    }
    else
    {
        DisplayResult("No Commands Available");
    }
}

protected void GetCommandButton_Click(object sender, EventArgs e)
{
    ReturnValue();
}


Locating matching files with input folder and file prefix

The problem is almost certainly this line here:

t=$(echo $fn | perl -pe 's|.*?(\d+\.\d+)|\1|');

If you are invoking the Perl interpreter for each line, you will struggle.

A close second is that, for each file, you invoke 2 subshells, and two other program (sed and echo).

My recommendation is for you to actually rewrite the whole thing in Perl...

....

but, you may find it faster to use sed

t=$(echo $fn | sed -e '$s@.*[^0-9]\([0-9]\+\.[0-9]\+\).*@\1@g')

The above expression takes advantage of the fact that there has to be some non-digit character before the first date digit.

But, as I say, it is my suggestion that you do the whole thing in Perl, and avoid having to do all the execs.

EDIT: In Perl

#!/usr/bin/perl -w
#
#for cs in {47..52}; do
#  csdirnm="CASE$cs";
#  tsdirnm="eta_at_x2";
#  label='flow-time';
#  dir="$csdirnm/$tsdirnm/";
#  files=$(ls -tr $dir); # -tr sort on time created, reversed (newest last).
#  i=0;
#  for f in $files ; do
#    fn=$dir$f;
#    if [[ $i = 0 ]]; then
#      data="$(sed -n 1p $fn) $label";
#      ((i++));
#    fi
#    d=$(sed -n 2p $fn);
#    t=$(echo $fn | perl -pe 's|.*?(\d+\.\d+)|\1|');
#    data+="\n$d $t";
#  done;
#  echo -e "$data">"$csdirnm/${csdirnm}_${tsdirnm}_20T.time-series";
#done;

use strict;

foreach my $dirnum (47 .. 52) {
  my $csdirnm = "CASE$dirnum";
  print "Processing Dir $csdirnm\n";
  my $tsdirnm = "eta_at_x2";
  # Use ls to get the file ordering right.
  my $subdir = "$csdirnm/$tsdirnm";
  next unless -d $subdir; #Skiip non-existant dirs
  open DIRLIST, "-|", "ls -tr $subdir" or die "Unable to list files in $subdir";
  my $outfile = "${csdirnm}/${csdirnm}_${tsdirnm}_20T.time-series";
  open REPORTFILE, ">", $outfile or die "Unable to write to report file $outfile";

  my $file;
  my $cnt = 0;
  while ($file = <DIRLIST>) {
    chomp $file;
    print "Processing $file\n";
    my $datafile = "$subdir/$file";
    open DATA, "<", $datafile or die "Unable to read file $datafile";
    my $line1 = <DATA>;
    my $line2 = <DATA>;
    close DATA;
    print REPORTFILE $line1 unless $cnt;
    print REPORTFILE $line2;
    $cnt++;
  }

  close DIRLIST;
  close REPORTFILE;
}


Sorting and Filtering Maps Using Generics

Assuming the requirement was to maintain initial map as immutable object -- just a few concerns re those new statements in sorting. They violate SRP, at least. Either accept already created (mutable) to populate with results, or (I prefer) accept a factory to get one. Same apples to collection step in filtering.

If, however, it is given that input map can be mutated during the process, then whole solution should be written with this in mind and, perhaps, stream is not the right choice here.


regarding:

if (fgets(line, sizeof(line), stdin) != line || feof(stdin))

This is unnecessary messy.  Suggest:

if( fgets( line, sizeof( line ), stdin )

If any error occurs, the returned value is NULL so the body of the if() will not be entered.

regarding:

if (fgets(line, sizeof(line), stdin) != line || feof(stdin)) {
            exit(EXIT_FAILURE);
            break;

the break; will never be executed because the call to exit() will have already exited the program.

regarding:

while(!feof(stdin));

please read: while(!feof()) is always wrong

regarding:

    return n;
} while(true);

The return is always executed, so this loop will never iterate, looking for a valid input.

regarding:

char *cursor = line;
        while ((*cursor == ' ' || *cursor == '\t') && *cursor != '\0') {
            cursor++;
        }
        if (*cursor == '\n') {
            return n;
        }

This while() code block will iterate to the end of the array line[], most of the time.  the result will be no number will be extracted.  Suggest, starting at line[0] to check for isdigit( line[i] ) and if true, then extract the number, perhaps using something like: strtol()


Brand new to programming myself, but here's what I've got for you.

This is just different formatting to not call print so many times:

def printOutVictory():
    print('You won!')
    print('The word was ', theWord)
    print('Press enter to quit...')

Alternate format:

# snake_case over camelCase in python
def print_out_victory():
    message = "You won!\nThe word was " + theWord
    message += "\nPress enter to quit..."
    print(message)

You could clean up your main body by creating game_mode_1 and game_mode_2 outside of main and calling them for your if and elif and then just reduce your last elif to an else statement:

def game_mode_1():
    words = ['letters', 'sloppy', 'bedroom', 'jazzy', 'discovery', 'wistful', 'unadvised', 'help', 'line', 'shake', 'mend', 'time', 'attempt', 'dare', 'straw', 'destroy', 'health', 'shiny']
    theWord = random.choice(words)
    clear()
    game()
    break

def game_mode_2():
    theWord = str(input('Please write here the word: '))
    clear()
    game()
    break

You would then have:

#using game_mode in place of gameMode, snake_cases again
def main():
    welcome()
    while true:
        try:
            game_mode = int(input('INPUT: '))
        except ValueError:
            print('Write 1 or 2 please.')
            continue

        if game_mode == 1:
            game_mode_1()

        elif game_mode == 2:
            game_mode_2()

        else:
            print("Write 1 or 2 please.")

Furthermore you could reduce main by creating user_selection

def user_selection():
    while true:
        try:
            game_mode = int(input('INPUT: '))
        except ValueError:
            print('Write 1 or 2 please.')
            continue

        if game_mode == 1:
            game_mode_1()

        elif game_mode == 2:
            game_mode_2()

        else:
            print("Write 1 or 2 please.")

And then your main would look like this

def main():
    welcome()
    user_selection()

Then run main adding if __name__ == '__main__': guard, this will prevent main from running if you import this script into another script

if __name__ == '__main__':
    main()


Initializing characters in a visual novel game

Have you considered a data-driven approach? This would involve making a file format and having a file that contains the information for each character. You could then add or remove characters at will by simply updating the data file. You could make a routine which looks for a particular file or takes a path to a file and simply reads the data from it and creates characters as it reads them from the file. The file could be in any format that you find easy to work with - XML, JSON, INI, whatever.


Ruby function to surround a word with dashes

Well first your indention is a bit strange, it'd be better like this:

class Add_dash
  def dashesHere(word)
    attachTheDashes = "--" + word + "--"
    return attachTheDashes
  end
end

And you typically don't need return in Ruby, it'll return the last value evaluated in your function. So having that in mind you could write it like this:

class Add_dash
  def dashesHere(word)
    "--" + word + "--"
  end
end

But Ruby also offers a way to easily put any value into a string, without adding a bunch of strings together. It really allows you to put any Ruby code in a string, and whatever it returns, will be put into your string. That looks like this: "math: #{1 + 1}" which would be "math: 2", or "Hello #{name}" would greet who ever was in name.

So you could write your whole thing like this:

class Add_dash
  def dashesHere(word)
    "--#{word}--"
  end
end

And in Ruby class names (Add_dash in your case) are supposed to be written in CamelCase starting with a capital letter. Method names and everything else are written in snake_case.

This would be my final version:

class AddDash
  def dashes_here(word)
    "--#{word}--"
  end
end

I hope this helps!


JavaScript/ECMAscript 6 classes organization

Most interesting,

In BaseObject, you should really have a constructor that builds this.listeners, it would make your code much cleaner afterwards
I am not sure why your listeners are not private in BaseObject ?
I am not sure what is more important to you, learn EC6 or position divs efficiently, if the latter is more important, than this code is massive overkill ;)

If you want to avoid having a constructor for BaseObject, then I would re-write on like this:

on(eventname,func){
    this.listeners = this.listeners || [];
    this.listeners[eventname] = this.listeners[eventname] || [];
    this.listeners[eventname].push(func);
    return this;
}


Improving the load time of a commit list for a specific branch and tag from a SVN repo

First of all, please check whether it is possible to use a newer version of SVNKit. According to mvnrepository, the used version 1.2.3.5521 is dated of March 2009, which is quite dinosaurian in our days.

The obsolete version might explain the long delays you are experiencing (I underline might explain, because there might have been a lot of optimisations compared with the most recent version, but I don't have any precise idea about it).

Another explanation may be the network communication lags with the SVN server the app is connecting to. It hugely depends on your network conditions and it is possible that there is no remedy for that.

To prove that the problem is related with networks, roughly and manually, the calls of methods on  repository field can be surrounded with time measurements, as well as method entries and exit points, for example:

public List<Commit> getBranchCommits(String branch, Tag tag) throws SVNException {
  final long methodStart = System.nanoTime();
  // other code

  final long repoStart = System.nanoTime();
  Collection<SVNLogEntry> logEntries = repository.log(...);
  final long repoEnd = System.nanoTime();

  // other code

  final long methodEnd = System.nanoTime();

  System.out.println(String.format("Total time: %1$s ms.", (methodEnd - methodStart) / 1_000_000));
  System.out.println(String.format("Repo call time: %1$s ms.", (repoEnd - repoStart) / 1_000_000));
  return commitList;
}

If repository call times take most part of total execution times, the problem is definitely related with network communications.

As for the code you are trying to improve, I can say that there should not be problems with the code itself. Although the style is a bit oldish, it does not look problematic. Here are some suggestions, using Java 8 and functional style:

public List<Commit> getBranchCommits(String branch, Tag tag) throws SVNException {
  @SuppressWarnings("unchecked")
  Collection<SVNLogEntry> logEntries = repository.log(/* no changes in args */);
  return logEntries.stream()
      .filter(this::shouldSkipEntryByAuthor)
      .map(this::constructCommit)
      .collect(Collectors.toList());
}

private boolean shouldSkipEntryByAuthor(SVNLogEntry logEntry) {
  return !(logEntry.getAuthor() == null || "SVN_LOGON".equalsIgnoreCase(logEntry.getAuthor()));
}

private Commit constructCommit(SVNLogEntry logEntry) {

  final Commit commit = new Commit(logEntry.getRevision(), logEntry.getDate(), logEntry.getAuthor(), logEntry.getMessage());

  // the problem with this obsolete API is that getChangedPaths() returns an untyped Map, we need to cast it properly.
  final List<ChangedPath> changedPaths = ((Map<Object, SVNLogEntryPath>) logEntry.getChangedPaths()).values().stream()
                                                .map(entry -> extractChangedPath(entry))
                                                .filter(Optional::isPresent)
                                                .map(Optional::get)
                                                .sorted(Comparator.comparing(ChangedPath::getPath))
                                                .collect(Collectors.toList());
  commit.setChangedPathList(changedPaths);
  return commit;
}

private Optional<ChangedPath> extractChangedPath(SVNLogEntryPath entry) {
  // we need to wrap SVNException into an unchecked one in order to make it compile in lambdas
  try {
    SVNNodeKind nodeKind = repository.checkPath(entry.getPath(), -1);
    if (!SVNNodeKind.DIR.equals(nodeKind)) {
      return Optional.of(new ChangedPath(entry.getPath(), entry.getType(), entry.getCopyPath()));
    }
    return Optional.empty();
  } catch (SVNException ex) {
    throw new IllegalStateException(ex);
  }
}


Why

for module in [name for _, name, _ in pkgutil.iter_modules([package_dir])]:

and not

for _, module, _ in pkgutil.iter_modules([package_dir]):

? If it's about iterating eagerly, using list would work better than a list comprehension.

_ = __import__(module)

is also weird; why not just __import__(module)?

It makes sense for AutoRegClassRegistry to be an ABC, but by no means is this a requirement. It's worth noting that

def register(self):
    raise NotImplementedError("You must inherit from this class and override the register func")

has the wrong number of arguments, so will give an unhelpful error instead.

Don't use name mangling for self.__choices = [] unless there's good reason; _choices works just as well.

cls_short = getattr(cls, "SHORT", None)
if not cls_short:
    raise AttributeError("CLS {} doesn't have attribute 'SHORT' which is required to register to ClientStats")

seems like it breaks EAFTP;

cls_short = cls.SHORT

would work just as well and

try:
    cls_short = cls.SHORT
except AttributeError:
    raise AttributeError("CLS {} doesn't have attribute 'SHORT' which is required to register to ClientStats")

would be more descriptive of intent. The same happens immediately after.

The whole

if hasattr(self, cls_short):
    raise AttributeError("ClientStats already has a {} registered".format(cls_short))
setattr(self, cls_short, cls_short)
setattr(self, "{}_DESC".format(cls_short), cls_desc)
setattr(self, "{}_CLS".format(cls_short), cls)

looks really backwards, though; why not just put this in a simple data structure? Why does it have to be through messy attribute accesses.

This whole thing feels really dynamic, and dynamism should be a last resort. But I've never touched Django, so I can't really say whether what you're doing is justified or not.

As I said, I've never touched Django so I can't really say whether this could work, but I'd be tempted to do something more like

DailyStatExample = ClientStats.new("DSE", "Example stat class")

or

@ClientStats.register("DSE", "Example stat class")
class DailyStatExample:
    ...


I see several things up for improvements:

What problem are you solving? PDO on itself is an abstraction. Your abstraction over it is, on most cases, redundant.

A abstraction is a Mapper for example, which uses the PDO connection to map an object to the database (or whatever form of storage). See The DataMapper pattern

Registry is an antipattern. Why does your database class need all of the config? Just for the 4 details it needs? Just pass those in to the constructor/connect method.

public function __construct($dsn) {

Stay consistent with return types. What does your method return? Is it a resultset? A PDOStatement object? A boolean? Pick one and stick with it. Utilize Exceptions for errors.
Don't catch the (PDO)Exception inside the method. You don't know what you want to do yet! Maybe I want to try to connect, and fallback to files if it fails. Maybe I want to log? Maybe I want to send an email? Your Database object doesn't know the context in which it's called, and so it doesn't know the context in which to deal with errors.
Don't die(). When you die, the entire script dies, just like that. That can be very undesirable in cases where you instantiate a database connection in the middle of the application and it fails. Allow the exception to bubble up, breaking execution when needed, until it's being handled. (And if it isn't, it'll fatal the script anyway!)


Changing line colors and move text for menu navigation

Why do you need different navXXX and current_navXXX classes? Just have a single

var menus = {//you need some kind of mapping between elements clicked and menu items
  id1: '#home',
  id2: '#design',
  id3: '#seo',
  id4: '#galery',
  id5: '#about',
  id6: '#contact'
};

$('.nav a').click(function () {
    $('.sf_menu li').removeClass('current_nav');
    $(this).addClass("current_nav");

    var selected = menus[$(this).attr('id')];
    $('#content, ' + selected).slideDown(2000);

    for (var menu in menus) {
        if (menus[menu] != selected)
           $(menus[menu]).slideUp(2000);
    }
});


SICP - exercise 1.12 - pascal's triangle

You should memoise the recursive call in order to make it faster. Since each value has a computational sibling (its value is derived from (one or two) parents which also each has another child), you remove the double-computation that would occur.

Additionally, these double-computations would actually accumulate exponentially up the levels, so it's no longer just a micro-optimisation; it actually affects the your runtime complexity. So it's even more important to implement memoisation.


Prime factorization of numbers up to 1000

There are a few things which can improve this logic:

In order to find out whether a number n is prime, you only need to go up to sqrt(n), not n/2; that will speed up the prime validation considerably for large numbers.
Also, if you're verifying whether a sequence of numbers are primes, you should store the previous results instead of starting from scratch all the time. The function below calculates the primes from 2-1000, which is a good "pre-processing" step for your function (factorizing is now a matter of traversing only that prime numbers list)

Calculate prime numbers from 1..1000:

use strict;

my @primes;
my $i;

for my $i (2..1000) {
    my $isPrime = 1;
    my $sqrt = int(sqrt($i));
    for my $j (@primes) {
        if (($i % $j) == 0) {
            $isPrime = 0;
            last;
        }
        last if $j > $sqrt;
    }
    if ($isPrime) {
        push @primes, $i;
    }
}

print join(", ", @primes);


Possible improvements to Risk board game?

This code turned out to be a great exercise in refactoring, primarily using

Extract Method
Reduce Scope of Variable
Extract Class
Introduce Explaining Variable
Self Encapsulate Field

Here's my goal:

public class RiskDieRoller
{
    //////////////////////////////////////////////////////////////////////

    private static class Strength
    {
        public int attack;
        public int defence;
        public int attackMod = 6;

        public Strength() {}

        public Strength(Strength other)
        {
            this.attack = other.attack;
            this.defence = other.defence;
            this.attackMod = other.attackMod;
        }

        public void blitz()
        {
            this.attackMod = 7;
        }

        public boolean isBlitz()
        {
            return this.attackMod == 7;
        }

        public String toString()
        {
            return "Attack " + this.attack +
                   " Defence " + this.defence +
                   " AttackMod " + this.attackMod;
        }
    }

    //////////////////////////////////////////////////////////////////////

    // …

    public static void main(String[] args)
    {
        RiskDieRoller roller = new RiskDieRoller();

        Strength before = new Strength();
        while (null != (before = roller.prompt(before)))
        {
            Strength after = roller.play(before);
            boolean isDecisive = roller.report(before, after);
            before = isDecisive ? new Strength() : after;
        }
    }
}

I've renamed the class to RiskDieRoller and introduced a Strength class to help tame the proliferation of variables.  The main() function just outlines how the program flows.

It's now just a simple matter of filling in the blanks. ☺

Prompt

You failed to handle the "Cancel" button — a NullPointerException occurs.  You handle "0 0" as a special case, but what if there is gratuitous extra whitespace?  Also, if the exclamation mark is not preceded by a space, then Double.parseDouble() barfs.  Anyway, you should be calling Integer.parseInt() instead.

In any case, analyzing the string character by character is tedious, error prone, and unforgiving of variations in the user-provided input.   Just use a regular expression.

private static final Pattern INPUT_RE =
    Pattern.compile("^\\s*(\\d+)\\s+(\\d+)\\s*(!)?\\s*$");

public Strength prompt(Strength before)
{
    do
    {
        String input = JOptionPane.showInputDialog("Please enter the "
                + "number of attackers then the number of\ndefenders "
                + "separated by a space.",
                before.attack + " " + before.defence);
        if (null == input)
        {
            return null;
        }

        Matcher matcher = INPUT_RE.matcher(input);
        if (matcher.matches())
        {
            Strength s = new Strength();
            s.attack = Integer.parseInt(matcher.group(1));
            s.defence = Integer.parseInt(matcher.group(2));
            if (null != matcher.group(3))
            {
                s.blitz();
            }

            if (s.attack == 0 && s.defence == 0)
            {
                return null;
            }
            if (s.attack <= 1)
            {
                output("INVALID ATTACK! You need"
                        + " at least two armies to make an attack!");
            }
            else if (s.defence <= 0)
            {
                output("INVALID INPUT! There must"
                        + " be at least one defending army!");
            }
            else
            {
                return s;
            }
        }
    } while (true); // Repeat until input passes validation
}

private void output(String message)
{
    JOptionPane.showMessageDialog(null, message);
}

It's not necessary to have a null Component instance variable; you can just pass a literal null to .showMessageDialog().

Play

This part of the code is more or less the same as before, but with nicer variable names.  I've changed (int)Math.ceil(Math.random() * …) to Random.nextInt().

private Random random = new Random();

public Strength play(Strength before)
{
    Strength s = new Strength(before); // Return a copy
    Integer[] attackRolls = new Integer[3],
              defenceRolls = new Integer[2];

    do
    {
        for (int i = 0; i < 3; i++)
        {
            attackRolls[i] = 1 + random.nextInt(s.attackMod);
        }
        for (int i = 0; i < 2; i++)
        {
            defenceRolls[i] = 1 + random.nextInt(6);
        }
        Arrays.sort(attackRolls, Collections.reverseOrder());
        Arrays.sort(defenceRolls, Collections.reverseOrder());

        for (int rollCnt = 0; rollCnt < 2; rollCnt++)
        {
            if (s.defence <= 0 || s.attack <= 1)
            {
                return s;
            }
            if (defenceRolls[rollCnt] >= attackRolls[rollCnt])
            {
                s.attack--;
            }
            else
            {
                s.defence--;
            }
        }
    } while (s.isBlitz())
    return s;
}

Strictly speaking, you don't have to add 1 to .nextInt(), but it's more human-friendly to do so.

Report

Again, the main difference is in the management of variables.  This function now has to return true if the battle was decisive.  Arguably, that violates the single-responsibility principle, and could be improved further.

In your original code, you checked for dfnc2 > 0 to continue the battle and dfnc2 < 1 to report annihilation of the defenders.  I've changed this to s.defence <= 0 to check for termination of the battle and after.defence <= 0 to report annihilation, so that the similarity is apparent.

I think String.format() is more readable than concatenation.

public boolean report(Strength before, Strength after)
{
    String verdict = (after.defence <= 0) ?
                        "\n\nDefenders have lost!!" :
                     (after.attack <= 1)  ?
                        "\n\nAttackers have been repelled!!!" : "";

    output(String.format("Attacking force now at %d (Lost %d)\n" +
                         "Defence force now at %d (Lost %d)" +
                         "%s",
                         after.attack,
                         before.attack - after.attack,
                         after.defence,
                         before.defence - after.defence,
                         verdict));

    return !verdict.isEmpty();
}

Nitpicks

"armys" → "armies"
"a" + "mount of attackers" → "number of attackers" (because it's a countable noun)


First of all, the convention in ruby is to use snake_case, not camelCase for variable and method names. It's generally a good idea to adhere to a language's naming conventions - if only so that the code looks consistent when you're calling standard library methods as well as your own.

In your xrandrPairs method, you mention that you're using an index to iterate because you have to go through the array in pairs. You can avoid this by using each_cons(2) which will yield each item together with the item after it (e.g. [1,2,3].each_cons(2) will yield 1,2 in the first iteration and 2,3 in the second).

However there's a better way to do this then to iterate through the lines. You can use scan to find all the lines that start without spaces and extract the information you want in one go:

def xrandr_pairs (xrandr_output)
## Returns [[<display name>, <max-resolution>] ...]
  display_re = /^(\S+)/
  option_re = /^\s+(\S+)/
  xrandr_output.scan(/#{display_re}.*\n#{option_re}/)
end

Since scan returns an array containing one subarray per match where each item in the subarray corresponds to one capturing group in the regex, this will produce the output you want. Note that xrandr_pairs now takes xrandr's output as a string, not an array of lines.

In addition to using scan I also changed the regexen a bit: I replaced [^\s] with \S, which is equivalent but shorter, and used + instead of *, so it does not match empty strings.

The xrandr_string method can also be rewritten to be much nicer by using the each_cons method like this:

def xrandr_string (x_pairs)
## Takes [[<display name>, <max-resolution>] ...] and returns an xrandr command string
  s = "xrandr --output #{x_pairs[0][0]} --mode #{x_pairs[0][1]}"
  x_pairs.each_cons(2) do |(previous_output, previous_mode), (output, mode)|
      s += " --output #{output} --mode #{mode} --right-of #{previous_output}"
    end
  end
  s
end

You don't need to check that the size is at least 2 because each_cons simply doesn't do anything if the array is smaller than the given chunk-size.

I also used the destructuring bind of block arguments to assign the elements of the subarrays to variables directly.

Instead of building up the string imperatively you could also use map and join like this:

def xrandr_string (x_pairs)
## Takes [[<display name>, <max-resolution>] ...] and returns an xrandr command string
  cmd = "xrandr --output #{x_pairs[0][0]} --mode #{x_pairs[0][1]}"
  args = x_pairs.each_cons(2).map do |(previous_output, previous_mode), (output, mode)|
    "--output #{output} --mode #{mode} --right-of #{previous_output}"
  end
  [cmd, *args].join(" ")
end


Creating a specific distribution of random numbers in Powershell

Since you tagged your question with Performance, my example is not totally idiomatic PowerShell.

You could build a lookup array with each number as many times as you have defined the probability for them.
Afterwards you can easily index into it using one or an array of random (index) numbers.
$propabilityDefinition = @{
    0 = 10
    1 = 11
    2 = 19
    3 = 12
    4 = 3
    5 = 10
    6 = 6
    7 = 7
    8 = 4
    9 = 18
}

# build a lookup array with each number as many times as defined as probability
$numberLookup = foreach ($entry in $propabilityDefinition.GetEnumerator()) {
    # using foreach with Linq.Enumerable.Repeat is the fastest way I found so far
    [System.Linq.Enumerable]::Repeat($entry.Key, $entry.Value)
}

# get x random numbers within the scope of the array indexes.
# with PS7+ we can also use the Count parameter of Get-Random, but don't use "Get-Random" within a loop as it hits the performance quite hard
#   PS7 variant: "$randomIndex = (Get-Random -Minimum 0 -Maximum $numberLookup.Count -Count 1000)"
# if you only want to get one random number, you can always use "$randomIndex = Get-Random -Minimum 0 -Maximum $numberLookup.Count"
$random = [System.Random]::new()
$randomIndex = foreach ($null in 0..10000) {
    $random.Next(0, $numberLookup.Count)
}

# we can use the entiry array of random numbers at once for the index operator
$numberLookup[$randomIndex] | Group-Object -NoElement | Sort-Object Count -Descending

Output:
Count Name
----- ----
 1896 2
 1821 9
 1194 3
 1141 1
  983 0
  979 5
  694 7
  596 6
  393 8
  304 4


Thanks for sharing your code.

what I like

you follow the Java Naming conventions
you resist to solve the problem based on an array

what I dislike

violation of encapsulation / information hiding

You access directly properties of an object and (even worse) their properties too like this:

x == ship.shipGeographic.x && ship.shipGeographic.y <= y

But the code using an object should not know anything about the internal implementation pf this object. The using code should only call (public) methods on on an object. This enables both: polymorphism and improving the internal implementation without affecting the calling code. The getters and setters you mention are not a solution to this problem.

The names for this "code smells" is feature envy: the calling code does something what should better be done inside the object by applying the "tell, don't ask" principle.

separation of concerns - intermix of user IO and business logic

In your code the Board class is responsible for the user IO and also does game logic. This reduces the  re-usability of the game logic. I.e.: you have to do a complete rewrite if you want to change the User interface to  graphics instead of a command line. You should learn about the Model-View-Controller concept (MVC) and its relatives MCP, MVVC and MVVM.

unnecessary mutability

Your ship objects are mutable, this means their properties could be changed at any time during the game. But in practice the properties never change after initialization. Therefore your should pass all the properties as constructor parameters and store them in final declared properties.

incomplete OO approach.

IMHO you stopped doing an OO approach half way.

Holding a list of ships which know their positions is a good first step. But you could walked that path further.

E.g. The list of ships could only hold unsunk ones. Then the check for the end of the game would change from a loop to a simple statement:

boolean allShipsSank() {
    return listOfShips.isEmpty();
}

Next possibility could be the "ship hit" logik.

For once the ship should know itself if being hit:

class Ship {
  //...
  boolean isHitBy(int x, int y) {
    // logic here
  }
  //...
}

but instead of doing a calculation here I'd go the "collection" approach again.

First we need to introduce another class:

class BordPosition{ // this is a DTO and has no business logik
   final int x;
   final int y;
   BordPosition(int x, int y){
     this.x = x;
     this.y = y;
   }
   // implement equals() and hascode()
}

and we need to enhance the Orientation enum:

enum Orientation {
    HORIZONTAL{
        public BordPosition translate(BordPosition startPoint, int stepsToMove){
           return new BordPosition(startPoint.x+stepsToMove, startPoint.y);
        }
    },
    VERTICAL{
        public BordPosition translate(BordPosition startPoint, int stepsToMove){
           return new BordPosition(startPoint.x, startPoint.y+stepsToMove);
        }
    };
    abstract public BordPosition translate(BordPosition startPoint, int stepsToMove);
}

then class Ship would change to this

class Ship {
  private final Collection<BordPosition> occupiedFields = new HashSet<>();
  private final Collection<BordPosition> hitFields = new HashSet<>();
  Ship(BordPosition startPoint, Orientation orientation, int size){
    for (int stepsToMove = 0; stepsToMove<size; stepsToMove++) {
        occupiedFields.add(orientation.translate(startPoint,stepsToMove));
    }
  }

  boolean isHitBy(BordPosition missleTarget) {
    if(occupiedFields.contains(missleTarget)){
      hitFields.add(missleTarget);
      if (hitFields.size()==occupiedFields.size()){
       // deregister from ship list via Listener pattern
      }
      return true;
    } else {
      return false;
    }
  }
  //...
}


PreWarmCache for MVC Application with Stopwatch Infinite Loop

There are some things that you could have done better certainly.

My first pick is the class name DoStuff. Please try pick a more meaningful name like Action or Task.

In this class you seem to be appending the current time to the property DoStuffName (which should also be renamed accordingly). You could pass the name in the constructor and append the current time instead, like:

public class Task{
   public Task(string name){
      Name = name + " " + DateTime.Now;
   }
   public string Name{
     get; private set;
   }
}

You also seem to have one of the most evil bad practises of programming - the empty catch block. Be sure to, at least, log the exception somewhere.


Finding a pair of words from two files with a particular MD5

In general, no, this would not be the fastest approach. However, assuming that all the lines are quite small, I think you won't do better by the 'general' fast approach.

You could use hashlib.md5().copy().

import hashlib
with open('1.txt') as f:
    a= [line.rstrip() for line in f]
with open('2.txt') as f:
    b= [line.rstrip() for line in f]

for y in a:
    prefix_hash = hashlib.md5(y)
    for z in b:
        if prefix_hash.copy().update(z).hexdigest() == 'XXXTHEXXXHASHXXX':
            print y + z
            break

Again, assuming len(y) is not that large, this is unlikely to do any better, and might even do worse because of the .copy() now used. But you're welcome to benchmark it.


Data Structures C# Dictionary and user has a property that is a list

You neither need TryGetValue nor ContainsKey if you don't care about the existence of the User in the Dictionary<TKey, TValue> which seems to be the case, because you want to either add  or update the User in the dict.

You can simply use the Item property setter to achieve this and after changing the method name to AddOrUpdate (based on the NET naming guidelines) your former method will look like so

public static void AddOrUpdate(Dictionary<int, User> dic, int key, User user)
{
    //sets a new user data and just replaces it in the dictionary
    //var used = new User { ID = "id1", Name = "Harry" };
    var used = new User{ ID = "id3", Name = "Henry" ,AddressBook = new List<ContactNumber>(){new ContactNumber(3111),new ContactNumber(4444)}};

    dic[key] = used;

}

This is because internally the Dictionary.Add() method calls Insert() with true for the add parameter like so

public void Add(TKey key, TValue value) {
    Insert(key, value, true);
}

and the Item property looks like so

public TValue this[TKey key] {
    get {
        int i = FindEntry(key);
        if (i >= 0) return entries[i].value;
        ThrowHelper.ThrowKeyNotFoundException();
        return default(TValue);
    }
    set {
        Insert(key, value, false);
    }
}

where the add parameter of the Insert() method call is false.


Calculation of image offsets for performing cropping

int width = 40;
int height = 34;
int cellSize = 3600;
int xOffset = 0;
int yOffset = 0;
int pixelWidth = cellSize * width;
int pixelHeight = cellSize * height;

I would find this easier to follow in a different order:

final int CELL_SIZE = 3600;

final int WIDTH = 40;
final int HEIGHT = 34;

final int PIXEL_WIDTH = CELL_SIZE * WIDTH;
final int PIXEL_HEIGHT = CELL_SIZE * HEIGHT;

int xOffset = 0;
int yOffset = 0;

Now  I see a natural progression of values and have the variables separate from the constants.  The variables are also closest to where they are changed.

I changed the values that never change to final to reflect that.

The ALL_CAPS notation is common for constants.

for (int i = 0 ; i < width; i++)
    {
    for (int j = 0; j < height; j++)
        {
        yOffset = pixelHeight - cellSize *(j+1) ;
        Rectangle r = new Rectangle(xOffset,
                                    yOffset,
                                    cellSize,
                                    cellSize);
        crop(r);
        }
      xOffset = pixelWidth - cellSize * (i+1);
     }

You can write this as

for (int xOffset = PIXEL_WIDTH - CELL_SIZE; xOffset >= 0; xOffset -= CELL_SIZE)
{
    for (int yOffset = PIXEL_HEIGHT - CELL_SIZE; yOffset >= 0; yOffset -= CELL_SIZE)
    {
        Rectangle r = new Rectangle(xOffset,
                                    yOffset,
                                    CELL_SIZE,
                                    CELL_SIZE);
        crop(r);
    }
}

Then you don't need i or j at all and you don't have to calculate the offsets further.

I also changed the indent of the brackets to match the more common practice of lining up with the for declarations rather than the block contents.

You may want to do this in the other order:

for (int xOffset = 0; xOffset < PIXEL_WIDTH; xOffset += CELL_SIZE)
{
    for (int yOffset = 0; yOffset < PIXEL_HEIGHT; yOffset += CELL_SIZE)
    {

As that is easier to read.  Not sure how much order matters.

If you need the original order exactly, you may need an extra loop.  The original order started with an xOffset of 0 and then went to PIXEL_WIDTH - CELL_SIZE.

If you are committed to the i and j method, consider starting them at 1, as so

for (int i = 1 ; i <= width; i++)
    {
    for (int j = 1; j <= height; j++)
        {
        yOffset = pixelHeight - cellSize * j;

Then you don't have to add 1 to i and j each time.


I have 2 concerns with your code

Mixing PHP and HTML works, but it's difficult to follow (which means bug-prone, not scalable).  It's much better to separate your business logic from the display as much as possible.
Having a default login of admin:admin is definitely not a good idea. It's probably the first thing anyone without credentials would try.

As @fracz suggested, HTTP Basic Authentication would fit your needs well, and it's really easy to use. I would probably do something like:

<?php
$LOGIN_INFORMATION = [
    'joe' => 'fkwoeiwsfrj3', 'jill' => '9ioDdoODidosD'
];

/**
 * Returns TRUE if authentication is successful. FALSE otherwise.
 * @param array $registeredLogins All acceptable logins
 * @return bool
 */
function isAuthenticated($registeredLogins){

    $authenticated = null; //make true if authentication succeeds

    //Server vars below will contain what user entered in the login prompt
    $login = isset($_SERVER['PHP_AUTH_USER'])? $_SERVER['PHP_AUTH_USER']: null;
    $pass = isset($_SERVER['PHP_AUTH_PW'])? $_SERVER['PHP_AUTH_PW']: null;

    if (!$login || !$pass):
        //required username or password is missing
        $authenticated = false;
    else:
        //username and password were provided. Are they accurate?
        $authenticated = array_key_exists($login, $registeredLogins)
                            && $registeredLogins[$login] === $pass;
    endif;

    return $authenticated;
}

/**
 * Exits and causes the browser to show user a login prompt
 */
function requestCredentials(){
    header('WWW-Authenticate: Basic realm="Login to Analytics Dashboard"');
    http_response_code(401);
    die("Invalid username or password");
}

//check for authentication. Require it if missing.
if(!isAuthenticated($LOGIN_INFORMATION)) requestCredentials();
?>

<html>
<!-- your page goes here. Will only display if user is authenticated -->
</html>

I have simplified things a bit, so you will need to adjust it to your specific situation. With this scheme, once the user enters username and password that is accepted, the browser will keep automatically sending credentials with new requests made to the same domain. Here are the details.

A couple of important things to keep in mind:

Whatever your security logic, if you don't use TLS (HTTPS), the client and the site will be vulnerable.  Any attacker can just read your user's verify cookie, or the username and password transmitted. The attacker can then use this info to impersonate the user and gain access. This affects both your code and what I suggested. Use TLS if you can. It's free.
Basic Authentication doesn't have a clean logout system (a way for the server to tell the browser to stop sending credentials and to delete them).  This can be worked  around easily enough though. The reference I linked above goes over this. Have a logout function that redirects the user (by setting the Location header and then exiting) to a page on the same domain with bad credentials hard-coded; eg: header("Location: http://badusername:badpass@www.example.com/logout.html");exit. Call the logout function before you echo anything to the browser and the browser will forget the old credentials, remember the bad credentials and redirect the user to logout.html.  User will be forced to enter proper credentials again if he wants access.
Because browsers will automatically attach credentials to all requests made to the domain, even the pages where you do not include password_protect.php will be queried with these credentials. This means that with this solution, TLS should be used not just on the protected pages, but on all domain's pages.


As a very basic application this is fine, however, if the program becomes more complicated and you want to add a data base and asynchronous programming you will run into conflicts with some of the class names. For instance the Task class is supplied by the C# library for asynchronous programming.
If I was going to implement a task list application I would have a few more properties for the task class, such as:

Priority
Status (not started, started, in progress, completed).
Due date
Name
Description

These are the minimum I would use, some additional properties might be

Assigned to
Assigned by

if this was a multi-user task list.


parsing log file and get time difference between two timestamp actions in python

In general, the indentation should be 4 spaces for each indentation level in Python. See PEP-0008 (the Python style guide). Loops and if statements should count as a level. The whole script could be shifted to the left, as well. Some of the variable names are a bit confusing because 'files' and 'file' can become easy to mix up as loop variables. One other thing of note is the spacing between things. I'd consider making fewer new lines between the bits of code. Some of the indentations seem like they should throw exceptions, so if you see IndentationErrors, that's what it's chirping about, I'd look at indenting after the blocks (for loops, and such).