Display captions based on video time I need to display a "caption" from a video but I cannot use built in captions provided by a vtt file for my situation. I created an array that stores the key,value pair (time, caption). I traverse that array every time the video time is updated and find which caption fits the current time. This solution works and I haven't had any issues (performance or otherwise) but it just feels brute force to me, I'm hoping someone can help me either refine what I have or guide me toward a more elegant solution. Any comments or criticism is appreciated.

 //this function is called whenever video time is updated
    this.onUpdateTime = function(currentTime, totalTime) {

        this.currentTime = currentTime;
        this.totalTime = totalTime;

        /*the for statement traverses the array chapters which contains
        [{"time": X,"caption": "XYZ"},...]*/

        for (var i = 0; i < $scope.chapters.length; i++) {

            /* the first if statement checks if (i is not the same as
            chapters.length (this was done to prevent an off by one error) and the time of
            the video is greater than or equal to time at chapters[i] and the time of the
            video is still less than time at the next chapter marker chapter[i+1]*/

            if (i != $scope.chapters.length - 1 && currentTime >= $scope.chapters[i].time && currentTime < $scope.chapters[i + 1].time) {

                /*set the caption to the value from chapters[i].caption*/

                $rootScope.caption = $scope.chapters[i].caption;

                /*I have never used $scope.$apply but it seemed to be needed to get the caption to display in my view*/

                $scope.$apply(); {

                    /*break the for loop so that it does not needlessly loop through the rest of the array after finding a match*/
                    break;
                }

                /*this if statement if for when i is equal to chapters.length -1, it is the final value in the array so it does
                not check against the "next value" like the previous if statement does because there is not next value*/

            } else if (currentTime >= $scope.chapters[i].time && i == $scope.chapters.length - 1) {

                /*set the caption to the value from chapters[i].caption*/

                $rootScope.caption = $scope.chapters[i].caption;

                /*I have never used $scope.$apply but it seemed to be needed to get the caption to display in my view*/

                $scope.$apply(); {

                    /*break the for loop so that it does not needlessly loop through the rest of the array after finding a match*/
                    break;
                }

                /*if there is not chapter marker at the current time*/
            } else {

                /*set the caption to a blank value because no match was found therefore there is no caption*/
                $rootScope.caption = "";
                $scope.$apply();
            }
        }
        // console.log("onUpdateTime function, currentTime is: " + currentTime);
        // console.log("onUpdateTime function, totalTime is: " + totalTime);
    };

This is an ideal case for bisect/binary search.
Provided you did the right thing and the caption array is sorted by time.

Additional optimization for sequential playback is to check the adjacent captions' time before bisect. Sometimes we might want to rewind a couple of seconds back so let's cache the previous caption as well.

// sequential playback uses the adjacent caption to avoid array enumeration
this.captionState = {prevStart:-1, start:-1, end:-1, nextEnd:-1};

this.onUpdateTime = function(currentTime, totalTime) {
    var state = this.captionState;
    if (currentTime >= state.prevStart && currentTime <= state.start) {
        return showCaption(state.index - 1);
    }
    if (currentTime >= state.end && currentTime <= state.nextEnd) {
        return showCaption(state.index + 1);
    }
    if (currentTime >= state.start && currentTime <= state.end) {
        return false;
    }

    // bisect the caption array sorted by time
    var a = 0, b = $scope.chapters.length - 1;
    while (a < b - 1) {
        var c = (a + b) / 2 |0;
        if ($scope.chapters[c].time > currentTime) {
            b = c;
        } else {
            a = c;
        }
    }

    // find a caption that starts before currentTime amidst [b], [b-1], [b-2]
    // or hide the current caption if none found e.g. the first one doesn't start at 0
    for (var i = 0; i <= 2; i++) {
        if (b - i < 0 || currentTime >= $scope.chapters[b - i].time) {
            return showCaption(b - i);
        }
    }

    function showCaption(index) {
        var current = $scope.chapters[index];
        if (!current) {
            $rootScope.caption = '';
            $scope.$apply();
            return false;
        }
        $rootScope.caption = current.caption;
        $scope.$apply();

        var prev = $scope.chapters[index - 1];
        var next = $scope.chapters[index + 1];
        var next2nd = $scope.chapters[index + 2];

        state.index = index;
        state.prevStart = prev ? prev.time : -1;
        state.start = current.time;
        state.end = next ? next.time : totalTime;
        state.nextEnd = next2nd ? next2nd.time : totalTime;
        return true;
    }
};


Welcome to programming! I'm new to code reviews (your StackOverflow post brought me here), but hopefully I have enough other experiences that something that follows will be of value to you.

As to your OOP question, since you're already learning it I would stick to Python rather than learning 2+ languages at once. It still has full support for objects and OOP principles, so there isn't a whole lot to lose. If you had to pick between Simula or SmallTalk, I'd recommend SmallTalk in a heartbeat (and I'd recommend temporarily dropping Python so that you're still only learning one language at a time).

Positives

I don't generally think critiques should only include the negatives, both for psychological reasons and since it's also important to keep doing the good things. At a high level, all of the following are nice. Keep it up.

You're breaking the code up into modular components. In your case, the components happen to be functions rather than objects, and that's fine. Functional code can express many problems more easily than OOP code.
The #--- comments you're using make the code surprisingly pleasant to read. I haven't seen that before, and I rather like it.
The if __name__ == '__main__' bit is a best practice in Python code, allowing different behavior if your program is called as a script or imported.
Your variable names are descriptive. This IS a big deal and makes everyone's lives easier (including yours). This is an important detail that I've noticed most new programmers leave out.

Needs Improvement

This looks like a lot in a big wall of text, but I promise there aren't that many things; I'm just trying to make sure that there's enough explanation that the comments are actually helpful to you.

Using global variables is almost universally recognized as a poor programming practice in every major language. The problem is that as the number of connections between different parts of your code increases, the number of code paths increases exponentially. It becomes very hard to reason about the behavior of your program. By having everything linked to even one (or in this case more than one) global variable, it is hard to analyze any component by itself. Even in your current code, if you tried to find the prime factors of several numbers at once you would likely have hard-to-track-down bugs due to the global variables.

One way to approach that option is to pass more arguments in your functions. As one example, instead of having look4factors() and find_prime_factors() both share the global variable list_of_factors, you could have look4factors() return the local variable list_of_factors, and then that could be an argument to find_prime_factors().
Capitalization schemes should typically be consistent in your code. The occasional use of Prime instead of prime could cause problems down the road.
Having hard-coded values should be avoided as much as possible. It's a lot of work for you, and it makes your code less robust too. With that few prime factors, it wouldn't work for numbers much bigger than 1000000 (approximately your biggest prime number squared).

It would be better to generate those prime numbers at run time. You could do that with trial division (for each new number, check to see if it's prime by checking if all the primes you've found up to the square root of that number so far are factors), or with more complicated algorithms like the Sieve of Eratosthenes. The key point is that by doing so you can handle any size of input instead of just small inputs.
Since you're using Python 2, the use of range() is discouraged when you're just iterating over those values. The range() function creates an object storing every integer you'll iterate over, whereas xrange() simply keeps yielding the new values. It saves a lot of RAM to use xrange(). This is one of the things that broke between Python 2 and Python 3. In Python 3, xrange() doesn't exist by default, and range() behaves like the old xrange() used to.
When you have a list of things (like list_of_Prime_factors) and need to do something special for just the beginning or just the end, rather than having an if statement inside your loop, it's better to have the special case either before or after the loop and have the loop only do the main operation. This matters because

if statements and other branches are more expensive for modern CPU's to deal with than other kinds of operations. Arguably that doesn't matter as much in Python since everything is so abstracted and since some of those branches are inlined by the bytecode compilation step, but maybe it's a tidbit you'll find useful one day.
Nested code is harder to reason about and debug than flat code.
Whether if statements are comparatively more expensive or not, you're still doing a lot of them that you don't need to.

Python has a concept of iterables that you might find helpful in simplifying your code. You can do other things than iterables, but one idea is that instead of writing

 for i in (0, len(thing)):
    # stuff with thing[i]

You can write something like

 for item in thing:
    # stuff with item

Extras

Most libraries, services, and security updates have been implemented in or completely switched to Python 3. If it were only for that, I'd recommend switching. There are some syntactical things that make your life easier too though, even in this code example. In Python 3.6+ you have so-called f-strings which allow arbitrary expressions inside them. Instead of '{}'.format(x) you can just write {x}, and the code runs faster too.

Note that many common things that you can do in a loop have already been implemented in the language. For example, it's possible to replace all of

for index in range (0, len(list_of_Prime_factors)):
    print "{}".format(list_of_Prime_factors[index]),
    if (index + 1 == len(list_of_Prime_factors)):
        pass
    else:
        print "X",

With the following line of code

print ' X '.join(map(str, list_of_Prime_factors))

Or if you'd rather learn about Python's list comprehensions, the following is roughly the same thing (and still works in Python 3 after modifying print to be a function, where the example I just gave would need a little tweaking).

print ' X '.join([str(p) for p in list_of_Prime_factors])


Optimizing variable order Markov Chain Implementation I've created a variable order Markov chain built on top of a tree, but I can't train on datasets >1MB worth of text without running out of memory. I'm sure the tree can be replaced by something else more efficient, but I'm struggling with figuring that out. I've heard a linked list might work, but I'm not sure how.

Below is the AddString method for variable order chains (of characters).

public void AddString(string s)
{
    // Construct the string that will be added.
    StringBuilder sb = new StringBuilder(s.Length + 2 * (MarkovOrder));

    sb.Append(StartChar, MarkovOrder);
    sb.Append(s);
    sb.Append(StopChar, MarkovOrder);

    for (int i = 0; i < sb.Length; ++i)
    {
        // Get the order 0 node
        Node parent = root.AddChild(sb[i]);

        //add N-grams
        for (int j = 1; j <= MarkovOrder && j + i < sb.Length; j++)
        {
            Node child = parent.AddChild(sb[j + i]);
            parent = child;
        }
    }
}

(code base found here)

This code bloats my memory with every order up to the defined order, and I'm not sure how I'd alter it to only store one order without it completely breaking down. I'd like to do something like

markov = new markovChain(order = 3);.

I've been playing around with algorithms that can store a chain of order (i.e.) 4 without going through the other orders. These implementations aren't performing as well, and I keep resorting to several lists that make node creation complex for me.
(https://gist.github.com/mtbarta/8127895)

I'm not sure what structure to use so that I can generate a chain at a given order without bloating memory use. Can I implement a linked list that stores a list of following nodes? Does that ruin the point of a linked list while bloating my memory anyway?

I'm sure the tree can be replaced by something else more efficient, but I'm struggling with figuring that out. I've heard a linked list might work, but I'm not sure how.

Perhaps use the SortedList class instead of the Dictionary class.

For example, this blog entry says,

  Best memory footprint we see in the SortedList, followed by Hashtable, SortedDictionary and the Dictionary has highest memory usage. Despite all that, we have to note, that the differences are not significant and unless your solution requires extreme sensitivity about the memory usage you should consider the other two parameters: time taken for the insert operations and time taken for searching a key as more important.

Other optimizations might include:

Don't allocate Node.Children until/unless the AddChild method is called.
Use the Dictionary constructor which lets you specify the initial capacity (and specify a very low initial capacity)
Add a KeyValuePair<char,Node> OnlyChild member to Node, and use it (instead of creating Dictionary) when the first child is added (delete it and create a dictionary when the second child is added) to optimize the case where a Node has only exactly one child

Beware that the above are 'micro-optimizations' which don't change the algorithm; changing the algorithm (I don't understand Artur Mustafin's comment) might result in bigger savings.


Converting UNIX time I have crated the following small script.  I just need to improve this complicated code if possible.

def convUnixTime(t):
    expr_date = int(t) * 86400
    fmt = '%Y-%m-%d %H:%M:%S'
    d1 = datetime.date.today().strftime('%Y-%m-%d %H:%M:%S')
    d2 = datetime.datetime.fromtimestamp(int(expr_date)).strftime('%Y-%m-%d %H:%M:%S')
    d1 = datetime.datetime.strptime(d1, fmt)
    d2 = datetime.datetime.strptime(d2, fmt)
    return (d2-d1).days

I think the main issue here is that you are trying to reuse snippets from Google without trying to understand what is doing what.

Magic numbers

This might be purely personal but I find 60*60*24 much easier to understand than 86400.

Do not repeat yourself

What is the point of having fmt = '%Y-%m-%d %H:%M:%S' if later on you rewrite strftime('%Y-%m-%d %H:%M:%S') ? You could write it : d1 = datetime.date.today().strftime(fmt) making obvious to everyone that the same format is used.

Useless conversion between strings and dates

You are converting dates to string and string to date in a convoluted and probably not required way.

Usess conversion to int

You are using int(expr_date)) but expr_date is defined as int(t) * 86400 which is an integer anyway. Also, I am not sure you would need the conversion of t as an int.

Taking into account the different comments, my resulting not-tested code looks like :

def convUnixTime(t):
        return (datetime.datetime.fromtimestamp(t*60*60*24)
              - datetime.datetime.today()).days

Edit : I was using datetime.date.today() instead of datetime.datetime.today(). Also fixed the sign issue.


Using/storing server-side data on the client side I've a slot booking script which is as follows:

My Current Algorithm

A calendar is shown to the user
The user clicks on a date
I make an ajax call to determine if at least 1 slot is booked on the
clicked date
I return back a form from the ajax page ("Time To", "Time
From", "Event Name" etc; form along with a hidden input whose value
is "1" if at least 1 slot is booked or "0" if no booking is done for
that day. This hidden input will be used a flag at a later stage)
The user then enters/selects values in the form and clicks "Book"
Upon the click of the "Book" button, I check the value of the hidden
input via javascript
If it is "0", I return true so that the form goes for submission (As there is not even a single slot booked on that day, there is no need to validate if the chosen time slot is booked or not)
If the value is "1", I run another ajax call to validate whether the time slot selected by the user is free, and if not I return false and display message to user stating the slot is already booked

Now this is where my problem arises. Since the value of the hidden input can be easily manipulated through Firebug before the Book button is clicked, I'm wondering what other way can I set the flag which will be tamper proof by the user.

HTML

JavaScript

// executed upon clicking on a date
$("#calendar .day").on("click", function () {
    var clicked_date = $(this).find("input").val();
    $.ajax({
        type: 'post',
        url: 'set-flag.php',
        data: {
            dt: clicked_date
        },
        success: function (data) {
            $("#booking-form").html(data);
        }
    });
});

//executed upon clicking "Book Date"
$("form#booking-form").on("submit", function ()
{
    var flag = $(this).find("input:hidden").val();
    if (flag == 0) // form can be submitted
    {
        return true;
    }
    else
    {
        $.ajax({
            type: 'post',
            url: 'check-slot-availability.php',
            context:this,
            data: {
                // parameters containing date clicked, time from, and time to values
            },
            success: function (data) {
                if($.trim(data) == 0) // slot already booked
                {
                    // show message to user to select another time slot
                }
                else // form can be submitted
                {
                    this.submit();
                }
            }
        });
    }
    return false;
});

set-flag.php

$clicked_date = safe($_POST['dt']);
/* perform db operations
.
.
.
*/
if($db->num_rows() > 0) // at least 1 slot is booked on selected day
{
   $flag = 1;
}
else
{
   flag = 0;
}

$form = '<form id="booking-form.php" action="some-page.php" method="post">';
$form .= 'Time From : <select></select>';
$form .= 'Time To : <select></select>';
$form .= '<input type="hidden" value="'.$flag.'" />';
$form .= '<input type="submit" value="Book" />';
$form .= '</form>';
echo $form;
exit;

Right now, you have two separate AJAX calls: one to determine if a day has bookings, and one to determine whether a specific time slot can be booked.

I would suggest one of two things:

Send a request only once that returns all booking data for the specified date.
Send only a validation request upon clicking Book; don't bother checking if a date has bookings.

Option 1 is the ideal approach, since it minimises requests. However, if for some reason you don't want a user to know when you're booked, you'll need to go with option 2. Whichever option you go with, you should provide feedback on its success (or failure) after submission, in case somebody else booked it sometime between validation and submission.

If you're adamant about continuing with your approach, then instead of using 0 and 1, consider using a salted hash involving the date. For illustrative purposes, I'm only using one round (since this type of thing doesn't need to be cryptographically secure).

define("BOOKING_DATE_SALT", "asprin111")
$flag = md5($clicked_date . ($db->num_rows() ? "yes" : "no") . BOOKING_DATE_SALT);
...

Of course, you won't want to repeat this code in check-slot-availability.php so it would probably belong better as a function in some shared library.


Optimize/Refactor Javascript Unique Array function This is one idea of implementation:

 Array.prototype.unique = function () {
  unique_array = [];
  for (var i = 0, l = this.length; i < l; i++) {
    if(unique_array.indexOf(this[i]) == -1){
      unique_array.push(this[i]);
    }
  }
  return unique_array;
 }

This version uses Object.keys which is a ECMAScript 5 only feature, as you can see on this website http://kangax.github.com/es5-compat-table/

Array.prototype.unique_e5 = function () {
  unique_object = {};
  for (var i = 0, l = this.length; i < l; i++) {
    unique_object[this[i]] = 1;
  }
  return Object.keys(unique_object);
}

And this is how is done in prototype.js
https://github.com/sstephenson/prototype/blob/master/src/prototype/lang/array.js

 /**
   *  Array#uniq([sorted = false]) -> Array
   *  - sorted (Boolean): Whether the array has already been sorted. If `true`,
   *    a less-costly algorithm will be used.
   *
   *  Produces a duplicate-free version of an array. If no duplicates are
   *  found, the original array is returned.
   *
   *  On large arrays when `sorted` is `false`, this method has a potentially
   *  large performance cost.
   *
   *  ##### Examples
   *
   *      [1, 3, 2, 1].uniq();
   *      // -> [1, 2, 3]
   *
   *      ['A', 'a'].uniq();
   *      // -> ['A', 'a'] (because String comparison is case-sensitive)
  **/
  function uniq(sorted) {
    return this.inject([], function(array, value, index) {
      if (0 == index || (sorted ? array.last() != value : !array.include(value)))
        array.push(value);
      return array;
    });
  }

Also not that the prototype version uses the Prototype enumerable method include, which is:

 /**
   *  Enumerable#include(object) -> Boolean
   *  - object (?): The object to look for.
   *
   *  Determines whether a given object is in the enumerable or not,
   *  based on the `==` comparison operator (equality with implicit type
   *  conversion).
   *
   *  ##### Examples
   *
   *      $R(1, 15).include(10);
   *      // -> true
   *
   *      ['hello', 'world'].include('HELLO');
   *      // -> false ('hello' != 'HELLO')
   *
   *      [1, 2, '3', '4', '5'].include(3);
   *      // -> true ('3' == 3)
  **/
  function include(object) {
    if (Object.isFunction(this.indexOf))
      if (this.indexOf(object) != -1) return true;

    var found = false;
    this.each(function(value) {
      if (value == object) {
        found = true;
        throw $break;
      }
    });
    return found;
  }

Is there a better way to do it? faster or "better" cross browser compatible?

For me, I'd always avoid methods that require lots of includes to things get working - and in my mind, the more code used.. the slower things will be (unless some form of caching is used). Which is why I would opt for a simple JavaScript solution. Your idea will work, but I think this one is faster:

Array.prototype.unique = function () {
  var a = this, b = [], c, i = a.length;
  again: while ( i-- ) {
    c = a[i];
    k = i; while( k-- ){ if (a[k] == c){ continue again; } }
    b.unshift( a[i] );
  }
  return b;
}

There are probably other improvements that can be made, for example it might be faster to find a way to use .push() rather than .unshift().

I haven't tested the above excessively, but it seems to work in all my tests so far. The reason why it gets a speed increase is because it reduces the area it is checking each time; it is also using subtle other speed increases like a decrementing while loop (means there are less conditional statements to check on each iteration), and creating shortcut vars that cut down access time.

As proof here is a jsPerf... albeit only tested on my set-up so far ;)

http://jsperf.com/compare-array-unique-versions

side note: -- the downside to my method is that it will only include the last found occurance of a duplicate (not the first as your's will). So if that ordering is important to you, then you'll have to refactor the code.

revision: -- after a few jsPerfs it seems clear that the while(i--) no longer holds a speed difference (at least not for FireFox 16 Mac OSX). Whilst on Chrome Mac OSX i--; seems slower than i++;

http://jsperf.com/compare-a-dec-while-against-a-for-loop

So taking in to account BillyBarry's comments the improved version should be:

Array.prototype.unique8 = function () {
  var a = this, b = [], c, i, l = a.length, j, k = 0;
  again: for ( i = 0; i < l; i++ ) {
    c = a[i];
    for ( j = 0; j < k; j++ ) { if (b[j] === c){ continue again; } }
    b[k++] = c;
  }
  return b;
}

Working from b, rather than a improves things quite a lot. Plus using k++; rather than .length for the internal loop makes quite a bit of difference for FireFox (Mac OSX) but has no affect on Chrome.


Dynamic String Alignment Code My code works for user output alignment to the right side by maximum length of 4 different user input.
I have 2 question.

Is there a simplier way for append method in my code ?
Is there a simplier way for right alignment by maximum legth of 4 different user input.

Thank You for Your Time...

"""
Main purpose of this code is alignment of the user output to the right
"""
# Take Input from User
name = input("Name: ")
surname = input("Surname: ")
age = input("Age: ")
occupation = input("Occupation: ")
person_dict = {"Name": name, "Surname": surname, "Age": age, "Occupation": occupation}

# Calculate Maximum Character Length
len_list1 = []
len_name = len(name)
len_list1.append(len_name)
len_surname = len(surname)
len_list1.append(len_surname)
len_age = len(age)
len_list1.append(len_age)
len_occupation = len(occupation)
len_list1.append(len_occupation)
a = max(len_list1)

# Output
print(f'Name:       {person_dict["Name"]:>{a}}\n'
      f'Surname:    {person_dict["Surname"]:>{a}}\n'
      f'Age:        {person_dict["Age"]:>{a}}\n'
      f'Occupation: {person_dict["Occupation"]:>{a}}'
      )

All the code from len_list1 = [] to a = max(len_list1) can be replaced with:

a = len(max(person_dict.values(), key=len))

max's key parameter is really helpful here. key is called on each element before max checks it. Dictionary's values method is useful here too. If you already have the data in a dictionary, you might as well make use of that fact.

You could also use a generator expression here to have the same effect:

a = max(len(val) for val in person_dict.values())

Or, since we're just mapping using an existing function, map would be clean here as well:

a = max(map(len, person_dict.values()))

The point is, use iteration constructs to avoid needing to repeat the same code over and over again for different objects.


A function that checks if a string subnet mask is valid I've written a small helper function that receives a string and checks if it translates to a valid subnet mask value (255.255.255.0 for example).

I would appreciate some peer review!

static bool isValidSubnetMask(IN char *subNetMask)
{
    char *str = NULL;
    char *endptr;
    int counter = 0;
    long int nChainInstance = -1;

    /*Check if string is valid*/
    if (!subNetMask || subNetMask[0] == '\0' || strlen(subNetMask) > 15) {
        return false;
    }

    /*Check string validity*/
    str = strtok(subNetMask, ".");
    while (str != NULL) {
        if (!is_numeric(str)) {
            return false;
        } else {
            /*Save number to be used as entry*/
            nChainInstance = strtol(str, &endptr, 10);
            if (str == endptr) {
                /*Fail to translate string to number*/
                return false;
            }
            if (nChainInstance > 255 || nChainInstance < 0) {
                return false;
            }
        }
        counter++;
        str = strtok(NULL, ".");
    }

    if (counter == 4) {
        return true;
    }
    return false;
}

Missing includes

#include <stdbool.h>
#include <string.h>

Other test may be needed

@Wingblade

Non-standard function

is_numeric() is not standard C library nor did I find it in Linux.

Questionable signature

I'd expect a function that checks a string's validity to cope with a const char * subNetMask.  So using strtok() on the supplied subNetMask is not possible.  Code needs to make a copy or otherwise assess the string.

// isValidSubnetMask(IN char *subNetMask)
isValidSubnetMask(IN const char *subNetMask)

Weak functionality

Below are 7 test cases that failed OP's code.  Sample working code provided.  sscanf() is a bit ponderous, yet useful as a test competitor.

bool is_numeric() {  // Added undefined Op's missing function.
  return true;
}

static bool isValidSubnetMask2(const char *subNetMask) {
  int n = 0;
  sscanf(subNetMask, "%*3[0-9].%*3[0-9].%*3[0-9].%*3[0-9]%n", &n);
  if (n == 0 || subNetMask[n]) {
    return false;
  }
  int i[4];
  sscanf(subNetMask, "%d.%d.%d.%d", &i[0], &i[1], &i[2], &i[3]);
  return i[0] <= 255 && i[1] <= 255 && i[2] <= 255 && i[3] <= 255;
}

void test(bool expect, const char *s) {
  char buff[99];
  strcpy(buff, s);
  bool y = isValidSubnetMask(buff);
  if (y != expect) {
    printf("Failed %d <%s>\n", expect, s);
  }
  y = isValidSubnetMask2(s);
  if (y != expect) {
    printf("Oops %d <%s>\n", expect, s);
  }
}

int main() {
  test(1, "255.255.255.0");
  test(1, "255.255.255.123");
  test(1, "0.0.0.0");
  test(0, "455.255.255.0");
  test(0, "255.255.255.-0");
  test(0, "255.255.255. 0");
  test(0, "2.2.5.2.3");
  test(0, "2.2.3");
  test(0, "+2.5.2.3");
  test(0, "A.5.2.3");
  test(0, "2.2.5.2.");
  test(0, "2..2.5.2");
  return 0;
}

Output

Failed 0 <255.255.255.-0>
Failed 0 <255.255.255. 0>
Failed 0 <2.2.5.2.3>
Failed 0 <2.2.3>
Failed 0 <+2.5.2.3>
Failed 0 <2.2.5.2.>
Failed 0 <2..2.5.2>


Geolocation check and error management for Google Maps I've written this code that I've been using for a while now to check for browser geolocation services for Google Maps applications. I have to say that I am not very happy with it as I've had to hack it quite a bit to work in IE9.

Could I please get some critical assessment of the following and possible suggestions for an elegant solution?  I need to stress that I do not want to use Modernizr or Gears. However, if there are good arguments, I'm willing to listen.

Notes: All the vars and functions in this script do pretty much as the function or var describes. All variables are declared as jQuery objects. start() launches Google Maps once the latitudes/longitudes are set.

function checkGeolocate()
{
    return 'geolocation' in navigator;
}

if (checkGeolocate())
{
    // Note that the if condition does not start until the end of the
    // entire geolocation procedure.

    if(navigator.geolocation.getCurrentPosition(function(position)
    {
        userLat.val(position.coords.latitude);
        userLng.val(position.coords.longitude);
        userDist.val(25);

        start();

        mapControls.fadeIn(200);
        mapOverlay.fadeOut(100);
    },
    function(error)
    {
        console.log(error);

        displayPostcodeSelect();
    },
    {
        timeout : 5000,
        maximumAge : 90000,
        enableHighAccuracy : true
    }))
    {
        // Do nothing as script has started
        // This if clause is to force IE9 to error out
        // instead of hanging when geolocation throws a PERMISSION_DENIED error.
    }
    else {
        displayPostcodeSelect();
    }
}
else
{
    console.log('no geo support');

    displayPostcodeSelect();
}

A tricky question,

navigator.geolocation.getCurrentPosition will always return undefined, regardless of browser or whether or not the the user allowed the location to be retrieved.

That means that //Do nothing as script has started, This if clause is to force IE9 to error out instead of hanging when geolocation throws a PERMISSION_DENIED error. sounds like a superstitious approach. I believe you that it works, but it's not because you provided an empty if.

Also please read this, if you must provide callbacks, then that means that //Note that the if condition does not start until the end of the entire geolocation procedure. is wrong.

From the little code in your question, I believe you should always call displayPostcodeSelect first, and if there is a geoLocation success you trigger your function. This means you no longer need to catch failures anymore since you want displayPostcodeSelect in that case anyway.

Furthermore, production code should not have console.log, it just looks bad.

Finally, 1 liner function that are used only once do not make sense, you should inline the checkGeolocate function.

All in all, I would counter propose:

//Start by showing the PostcodeSelect
displayPostcodeSelect();
if ('geolocation' in navigator)
{
  var geoOptions =
  {
        timeout            : 5000,  //5 seconds
        maximumAge         : 90000, //1.5 minutes
        enableHighAccuracy : true
  };
  navigator.geolocation.getCurrentPosition(function(position)
  {
        userLat.val(position.coords.latitude);
        userLng.val(position.coords.longitude);
        userDist.val(25);

        start();

        mapControls.fadeIn(200);
        mapOverlay.fadeOut(100);
  }, function(){}, geoOptions );
}


optimizing conversion between string and date I was wondering if there was a more efficient way of taking strings that represent dates with the pattern being mmddyy or mmd yy or m d yy and converting them into a DateTime object?

111110 would be 11 11 10

111 10 would be 11 1 10

1 1 10 would be 1 1 10

The spaces are either ' ' or '\0' (the input I am given is not very clean)

This is what I have so far, which works for all cases.

//Converts a given input string into a valid date
private DateTime convertToDateFromString(string dateString)
{
  int length = dateString.Length;
  int month = 1;
  int day = 1;
  int year = 1;
  bool gotMonth = false;
  bool gotDay = false;
  bool gotYear = false;
  char c = ' ';
  char peek = ' ';
  string buffer = "";
  DateTime bufferDate;
  int count = 0;

  try
  {
    for (int i = 0; i < dateString.Length; i++)
    {
      c = dateString[i];
      if ((i + 1) < dateString.Length)
        peek = dateString[i + 1];
      else
        peek = '\0';

      if (c != ' ' && c != '\0')
      {
        buffer += c;
        count++;
        if ((peek == ' ' || peek == '\0' || count == 2) && gotMonth == false)
        {
          count = 0;
          gotMonth = true;
          month = int.Parse(buffer);
          buffer = null;
        }
        else if ((peek == ' ' || peek == '\0' || count == 2) && gotDay == false && gotMonth == true)
        {
          count = 0;
          gotDay = true;
          day = int.Parse(buffer);
          buffer = null;
        }
        else if ((peek == ' ' || peek == '\0' || count == 2) && gotYear == false && gotMonth == true && gotDay == true)
        {
          count = 0;
          gotYear = true;
          year = int.Parse(buffer);
          buffer = null;

          if (year >= 80 && year <= 99)
            year += 1900;
          else if (year >= 0 && year <= 79)
            year += 2000;
        }
      }
    }
    bufferDate = new DateTime(year, month, day);
  }
  catch (System.Exception ex)
  {
    bufferDate = new DateTime(1, 1, 1);
  }

  return bufferDate;
}

Here's a one-liner for you:

    private DateTime convertToDateFromString(string dateString)
    {
        DateTime bufferDate;

        return DateTime.TryParseExact(
            dateString,
            new[] { "MMddyy", "MMd yy", "M d yy" },
            CultureInfo.InvariantCulture,
            DateTimeStyles.None,
            out bufferDate)
            ? bufferDate
            : new DateTime(1, 1, 1);
    }

With more than one line:

    private DateTime convertToDateFromString(string dateString)
    {
        var allowedFormats = new[] { "MMddyy", "MMd yy", "M d yy" };
        DateTime parsedDate;
        var couldParse = DateTime.TryParseExact(
            dateString,
            allowedFormats,
            CultureInfo.InvariantCulture,
            DateTimeStyles.None,
            out parsedDate);

        if (couldParse)
        {
            return parsedDate;
        }

        return new DateTime(1, 1, 1); // or throw an exception
    }


Rhythm notation converter (task #1) About a year ago, I was given a C++ assignment as a preliminary job-interview test.

The assignment included 4 different tasks, from which I had to choose one.

I actually chose and completed 3 out of 4 tasks, but I did not pass the test.

I was not given any specific rejections, so I would like to get an unbiased opinion.

My answer:

File main.cpp:

// --------------------------------
// Input (command-line):
// - Number of beats per bar
// - Number of notes per beat
// - A list of raw beat times
// --------------------------------
// Output (standard):
// - A list of formatted beat times
// --------------------------------

#include <sstream>
#include <iostream>
using namespace std;

static string Compute(int numOfBeatsPerBar,int numOfNotesPerBeat,double beatTime);

int main(int argc,char* argv[])
{
    if (argc < 3)
    {
        cout<<"Invalid Input";
        return -1;
    }

    int numOfBeatsPerBar  = stoi(argv[1]);
    int numOfNotesPerBeat = stoi(argv[2]);

    for (int i=3; i<argc; i++)
    {
        double beatTime = stod(argv[i]);
        string sampTime = Compute(numOfBeatsPerBar,numOfNotesPerBeat,beatTime);
        cout<<beatTime<<' '<<sampTime<<endl;
    }

    return 0;
}

static string Compute(int numOfBeatsPerBar,int numOfNotesPerBeat,double beatTime)
{
    int barId  = (int)beatTime;
    beatTime   = (beatTime-barId)*numOfBeatsPerBar;
    int beatId = (int)beatTime;
    beatTime   = (beatTime-beatId)*numOfNotesPerBeat;
    int noteId = (int)beatTime;

    ostringstream oss;
    oss<<barId+1<<'.'<<beatId+1<<'.'<<noteId+1;
    if (beatTime > 0)
        oss<<'+';
    return oss.str();
}

I think your condition for putting the '+' isn't right: it should be beatTime-noteId > 0 instead of beatTime > 0.

For the given example there will be no difference in the output, but test it with  e.g. 4 4 1.125 as command line arguments: 1.125 matches a full 16th but your code prints 1.125 2.1.3+ instead of 1.125 2.1.3.


Given a String, return a boolean if it is a number Yesterday in an online interview, the interviewer asked me this question:

  If I give you a string, you have to return if a string is number or
  not. You are not allowed to use any parse function available.

This was my first online interview after a long long time, so I took way too much time in this question. I want to gather feedback about my solution and better ideas for the solution to this problem. You can ignore complex numbers for now.

public static boolean isNumber(String toTest) {
    boolean isNegativeFoundAlready = false;
    boolean isDecimalPointFoundAlready = false;
    for (int i=0; i < toTest.length(); i++) {
        if (!"0123456789-.".contains(new String(new char[]{toTest.charAt(i)}))) {
            return false;
        } else {
            if ('-' == toTest.charAt(i) && i != 0) {
                return false;
            }
            if ('-' == toTest.charAt(i) && (i == toTest.length() - 1)) {
                return false;
            }

            if ('-' == toTest.charAt(i) && isNegativeFoundAlready) {
                return false;
            }
            if ('-' == toTest.charAt(i)) {
                isNegativeFoundAlready = true;
            }
            if ('.' == toTest.charAt(i) && isDecimalPointFoundAlready) {
                return false;
            }
            if ('.' == toTest.charAt(i)) {
                isDecimalPointFoundAlready = true;
            }
            if ('.' == toTest.charAt(i) && (i == toTest.length() - 1)) {
                return false;
            }
        }
    }
    return true;
}

Test cases I ran:

    System.out.println("123 isNumber? " + isNumber("123"));
    System.out.println(".123 isNumber? " + isNumber(".123"));
    System.out.println("-.123 isNumber? " + isNumber("-.123"));
    System.out.println("-. isNumber? " + isNumber("-."));
    System.out.println(".- isNumber? " + isNumber(".-"));
    System.out.println("23.34.545 isNumber? " + isNumber("23.34.545"));
    System.out.println("- isNumber? " + isNumber("-"));
    System.out.println("-0 isNumber? " + isNumber("-0"));
    System.out.println("12$%^&*# isNumber? " + isNumber("12$%^&*#"));

Instead of reading the output of those print statements, it's better write proper unit tests to verify your implementation:

@Test
public void testValidInteger() {
    assertTrue(isNumber("123"));
}

@Test
public void testValidStartsWithDecimal() {
    assertTrue(isNumber(".123"));
}

@Test
public void testValidNegativeStartsWithDecimal() {
    assertTrue(isNumber("-.123"));
}

@Test
public void testValidDecimal() {
    assertTrue(isNumber("1.23"));
}

@Test
public void testValidMinusZero() {
    assertTrue(isNumber("-0"));
}

Your implementation passes all these, which is good. I would add at least two more:

@Test
public void testValidEndsWithDecimal() {
    assertTrue(isNumber("123."));
}

@Test
public void testValidDecimalLooooong() {
    assertTrue(isNumber("1.2311111111111111111111111111111111111111111"));
}

The second passes but the first doesn't and maybe it should, no?

For testing cases of non-numbers:

@Test
public void testNotValid() {
    assertFalse(isNumber("-."));
    assertFalse(isNumber(".-"));
    assertFalse(isNumber("12.34.56"));
    assertFalse(isNumber("-"));
    assertFalse(isNumber("12$%^&*#"));
    assertFalse(isNumber("hello"));
    assertFalse(isNumber("   "));
    //assertFalse(isNumber(""));
}

It would be good to split these to individual cases as I did for the valid cases. I leave that for you as an exercise. I added some more cases here too, and unfortunately the one with empty string doesn't pass. I think that's a bug.

As per your implementation, it seems more complicated than it needs to be.
Most importantly, I'm wondering if the interviewer would consider regular expressions as "parse function". If not, then the implementation can be simplified:

public static boolean isNumber(String toTest) {
    return toTest.matches("[+-]?(\\d*\\.?\\d+)|(\\d+\\.?\\d*)");
}

This passes all the above tests. To correct and simplify your implementation, it should not be terribly difficult to implement the logic of the regular expression.


Compressing string in Scala, how to do this immutably? I have a string "abbbbccdddd" and the function should return "a1b4c2d4".

This is what I have written in Scala.

Iterative-version

def compress(str: String) = {
  val chars = List(str).flatten.map(_.toString) ++ List(null)
  var result: String = ""

  var lookBack: String = chars.head
  var occurance: Int = 0

  chars.foreach { c =>
    if (c != lookBack) {
      result = result + lookBack.toString + occurance
      occurance = 0
    }

    occurance = occurance + 1
    lookBack = c
  }

  result
}

Recursive-version

def compress(str: String): String = {
  def compressHandler(str: String, lookBack: String, occurance: Int, result: String): String = {
    if(str.isEmpty) {
      result
    } else if(str.head.toString == lookBack) {
      compressHandler(str.drop(1), str.head.toString, occurance + 1, result)
    } else {
      compressHandler(str, str.head.toString, 0, result + lookBack + occurance)
    }
  }

  compressHandler(str + "0", str.head.toString, 0, "")
}

Scala — being a functional language should have much better solutions!

How to improve second (by somehow using map/reduce/fold) and how to do the first following concept of immutability (purely functional)?

Here's my take using foldLeft:

def compress(s: String) = {
    val a : List[(Char,Int)] = List()
    s.toCharArray.foldLeft(a)((acc, elem) => acc match {
        case Nil => (elem, 1) :: Nil
        case (a, b) :: tail =>
            if (a == elem) (elem, b + 1) :: tail else (elem, 1) :: acc
    }).reverse
    .map{ case (a, b) => a.toString + b }
    .mkString("")
}

Note: I have assumed that order matters. That is, aabbbaa will reduce to a2b3a2


Slow Erlang code to find Pythagorean triplets Why does this Erlang code take about one minute on my machine?

On Pascal something like this took less then second on my machine.

What I can do to speed it up?

-module(slow).
-export([slow/0]).

pyth(N) ->
    [{A, B, C} ||
        A <- lists:seq(1, N - 2),
        B <- lists:seq(A + 1, N - 1),
        C <- lists:seq(B + 1, N),
        A + B + C =< N,
        A * A + B * B == C * C].

slow() -> io:format("~p\n", [pyth(1000)]).

Erlang runs on a VM, so you will get better performance on such a pure arithmetic computation by compiling it into native code using HIPE.

From the command line:

erlc +native slow.erl

Or from the Erlang shell:

1> hipe:c(slow).

Or:

1> c(slow, [native]).


Comparison of 2 values based on custom operator I am working on a library that parses query strings and evaluates the result for a given context. The query has the format FIELD OPERATOR VALUE (AND|OR) .... The list of operators is constant, but the field's type can be customized (default primary types are supported right now - other types can be added).

To enable the user of the library to overwrite the behavior of the operators, there is a FieldComparer class that evaluates 2 values for a given operator. It uses the default comparer of the type, but can be extended to provide specialized behavior.

The StringFieldComparer is such a specialized comparer for fields of type string. It adds support for wildcards and regex.

Operators

public enum Operator
{
    Equals,
    LessThen,
    LessThenOrEquals,
    GraterThan,
    GraterThanOrEquals,
    NotEquals,
    Matches,
    NotMatches,
}

FieldComparer

public class FieldComparer<TFieldValue>
{
    protected FieldComparer()
    { }

    public virtual bool Compare(Operator op, TFieldValue left, TFieldValue right)
    {
        var comparer = Comparer<TFieldValue>.Default;
        switch (op)
        {
            case Operator.Equals:
                return comparer.Compare(left, right) == 0;
            case Operator.NotEquals:
                return comparer.Compare(left, right) != 0;
            case Operator.GraterThan:
                return comparer.Compare(left, right) < 0;
            case Operator.GraterThanOrEquals:
                return comparer.Compare(left, right) <= 0;
            case Operator.LessThen:
                return comparer.Compare(left, right) > 0;
            case Operator.LessThenOrEquals:
                return comparer.Compare(left, right) >= 0;
            default:
                throw new NotSupportedException($"Operator '{op}' is not supported for type '{typeof(TFieldValue)}'");
        }
    }

    internal static FieldComparer<TFieldValue> Default { get; } = new FieldComparer<TFieldValue>();
}

StringFieldComparer

public class StringFieldComparer : FieldComparer<string>
{
    private readonly Dictionary<string, Regex> myRegexes = new Dictionary<string, Regex>();

    public override bool Compare(Operator op, string left, string right)
    {
        switch (op)
        {
            case Operator.Matches:
            case Operator.NotMatches:
                var regex = GetRegex(left);
                var match = regex.IsMatch(right);
                return op == Operator.Matches ? match : !match;
            case Operator.Equals:
            case Operator.NotEquals:
                regex = GetWildcardRegex(left);
                match = regex.IsMatch(right);
                return op == Operator.Equals ? match : !match;
            default:
                return base.Compare(op, left, right);
        }
    }

    private Regex GetWildcardRegex(string left)
    {
        var pattern = Regex.Escape(left)
            .Replace(@"\*", ".*")
            .Replace(@"\?", ".");

        pattern = "^" + pattern + "$";

        return GetRegex(pattern);
    }

    private Regex GetRegex(string left)
    {
        Regex regex;
        if (!myRegexes.TryGetValue(left, out regex))
        {
            regex = new Regex(left, RegexOptions.Compiled | RegexOptions.IgnoreCase);
            myRegexes.Add(left, regex);
        }
        return regex;
    }
}

Usage

var comparerStr = new StringFieldComparer();
comparerStr.Compare(Operator.Equals, "Hello*", "Hello World"); // true
comparerStr.Compare(Operator.Matches, "He.lo.*", "Hello World"); // true

var comparerInt = FieldComparer<int>.Default;
comparerInt.Compare(Operator.Equals, 1, 1); // true
comparerInt.Compare(Operator.LessThan, 1, 4); // false

I am not really happy with the method StringFieldComparer.Compare because the op will be checked twice for Equals/NotEquals and Match/NotMatch. Is there a way to realize it with only one check without creating the regex unnecessarily?

As always, any feedback is welcome! :)

public class FieldComparer<TFieldValue>
{
  protected FieldComparer()
  { }
}

The right way to prevent a type from being created and allowing only derived types to do it is... to use an abstract class. By making the FieldComparer abstract you won't need to hide the default constructor.

An example implementation could look like this where you have a private class DefaultFieldComparer that implements the FieldComparer:

public abstract class FieldComparer<TFieldValue>
{
    ..

    internal static FieldComparer<TFieldValue> Default { get; } = new DefaultFieldComparer();

    private class DefaultFieldComparer : FieldComparer<TFieldValue> { }
}

I find this is a cleaner solution becasue it informs me that the FieldComparer is a base class and intended be subclassed becasue on its own it has no use but if it has, then it should be instantiable.

private readonly Dictionary<string, Regex> myRegexes = new Dictionary<string, Regex>();

private Regex GetRegex(string left)
{
  Regex regex;
  if (!myRegexes.TryGetValue(left, out regex))
  {
      regex = new Regex(left, RegexOptions.Compiled | RegexOptions.IgnoreCase);
      myRegexes.Add(left, regex);
  }
  return regex;
}

I'm not sure whether it's really necessary to use a dictionary for the regexes. In most cases the static Regex.IsMatch is sufficient and it already does the caching thing:

  The static IsMatch(String, String) method is equivalent to constructing a Regex object with the regular expression pattern specified by pattern and calling the IsMatch(String) instance method. This regular expression pattern is cached for rapid retrieval by the regular expression engine.


Multiple return statements smells like what? Here's some code of mine that I don't like (this was my very first attempt at DI/IoC!). It's a small C# application that runs scheduled on one of our servers and is responsible for fetching the latest version of a VB6 codebase, compiling it and packaging an installer for it.

There's an Execute method in there that smells like... well you tell me what it smells like:

    /// <summary>
    /// Executes the build process.
    /// </summary>
    public override void Execute()
    {
        var errors = new List<string>();
        var logger = _logProvider.GetLogger(GetType().Name);

        DoArchiveOldInstallers(errors, logger);
        if (Properties.Settings.Default.StopOnBackupCurrentInstallerFailure && !DoBackupCurrentVersion(errors, logger))
        {
            OnExecutedCompleted(false);
            return;
        }

        if (Properties.Settings.Default.StopOnUpdateCodeBaseFailure && !DoGetLatestVersion(errors, logger))
        {
            OnExecutedCompleted(false);
            return;
        }

        if (Properties.Settings.Default.StopOnCompileFailure && !DoCompileCodeBase(errors, logger))
        {
            OnExecutedCompleted(false);
            return;
        }

        if (Properties.Settings.Default.StopOnBuildInstallerFailure && !DoBuildInstaller(errors, logger))
        {
            OnExecutedCompleted(false);
            return;
        }

        OnExecutedCompleted(true);
    }

    private void OnExecutedCompleted(bool success)
    {
        if (Completed != null) Completed(this, new CompletedEventArgs(success));
    }

Mea Culpa

Sorry this was posted like this. The conditions should read as follows:

if (!DoXXXXX(errors, logger) && !settings.StopOnXXXXXFailure)

This was a last-minute untested change I made just minutes prior to posting, without realizing it would totally mess up the method's logic.

First of all, I would question whether this code works correctly. Based on the names in your code, I assume that for example DoCompileCodeBase() should execute even when StopOnCompileFailure is false, but its result should be ignored. But that's not what happens: when StopOnCompileFailure is false, DoCompileCodeBase() doesn't even execute, because of short-circuiting evaluation of &&.

If what the code does is really what it should do (which is what I'm going to assume in the following text), then you should probably rename the settings properties.

Second, you can extract the checks and the Do method calls into a separate method (Simon's answer already suggested something similar):

// may need a better name
private bool ExecuteInternal()
{
    var errors = new List<string>();
    var logger = _logProvider.GetLogger(GetType().Name);
    var settings = Properties.Settings.Default;

    DoArchiveOldInstallers(errors, logger);

    if (settings.StopOnBackupCurrentInstallerFailure && !DoBackupCurrentVersion(errors, logger))
        return false;

    if (settings.StopOnUpdateCodeBaseFailure && !DoGetLatestVersion(errors, logger))
        return false;

    if (settings.StopOnCompileFailure && !DoCompileCodeBase(errors, logger))
        return false;

    if (settings.StopOnBuildInstallerFailure && !DoBuildInstaller(errors, logger))
        return false;

    return true;
}

public override void Execute()
{
    bool success = ExecuteInternal();
    OnExecutedCompleted(success);
}

Third, since all the Do methods are on the same object, you could change errors and logger into fields, so that you don't have to pass them around. Though I'm not completely convinced this is actually better, especially if Execute() is a method that's called over and over.

Fourth, you might consider associating all the settings properties with the right Do method using some kind of data structure:

private readonly Tuple<bool, Func<bool>>[] action;

// in constructor:
actions = new Tuple<bool, Func<bool>>[]
{
    new Tuple<bool, Func<bool>>(settings.StopOnBackupCurrentInstallerFailure, DoBackupCurrentVersion),
    new Tuple<bool, Func<bool>>(settings.StopOnUpdateCodeBaseFailure, DoGetLatestVersion),
    new Tuple<bool, Func<bool>>(settings.StopOnCompileFailure, DoCompileCodeBase),
    new Tuple<bool, Func<bool>>(settings.StopOnBuildInstallerFailure, DoBuildInstaller)
}

private bool ExecuteInternal()
{
    var errors = new List<string>();
    var logger = _logProvider.GetLogger(GetType().Name);
    var settings = Properties.Settings.Default;

    DoArchiveOldInstallers();

    foreach (var action in actions)
    {
        if (action.Item1 && !action.Item2())
            return false;
    }

    return true;
}

The syntax to initialize the list of Tuples is not great. You could improve that by having a special type TupleList<T1, T2> : List<Tuple<T1, T2>>, which would have a method like void Add(T1 item1, T2 item2). That way, the initialization simplifies to:

actions = new TupleList<bool, Func<bool>>
{
    { settings.StopOnBackupCurrentInstallerFailure, DoBackupCurrentVersion },
    { settings.StopOnUpdateCodeBaseFailure, DoGetLatestVersion },
    { settings.StopOnCompileFailure, DoCompileCodeBase },
    { settings.StopOnBuildInstallerFailure, DoBuildInstaller }
}

The next step after this could be encapsulating each action into its own object, so that you could do something like:

foreach (var action in actions)
{
    if (!action.Execute())
        return false;
}

And each action would be responsible for checking the right setting, if appropriate. (Actions that don't do any checks, like DoArchiveOldInstallers could be represented by a different type that inherits from the same base, or something like that.) But I'm not sure doing that would be worth it in this case.


Passing parameter to singleton I wrote this factory class in order to pass parameter to Singlton class, is it a good design in term of design for multithreading environment?

public static class LoggingServiceFactory
{
    private static string _connectionstring;
    private static readonly Lazy<LoggingService> _INSTANCE = new Lazy<LoggingService>(() => new LoggingService(_connectionstring));

    public static ILoggingService GetService(string connectionString)
    {
        _connectionstring = connectionString;
        return _INSTANCE.Value;
    }

    private class LoggingService : ILoggingService
    {
        private string _connectionstring;
        internal LoggingService(string connectionString)
        {
            _connectionstring = connectionString;
        }

        public void LogMessage(string msg)
        {
            // do the logging work
        }

    }
}

public interface ILoggingService
{
    void LogMessage(string msg);
}

I would not personally put LoggingService implementation inside factory, as it reduces testability and usually needed to play some tricks with generics type parameters only.

To have a predictable behavior according to your design it might look like:

public static class LoggingServiceFactory
{
    static ConcurrentDictionary<string, ILoggingService> Services { get; } =
        new ConcurrentDictionary<string, ILoggingService>();

    public static ILoggingService GetService(string connectionString) =>
        Services.GetOrAdd(connectionString, cs => new LoggingService(cs));
}

Anyway, it makes sense to think about delegating it to IoC container...

I don't think that it is a responsibility of this class to verify if connection string is the same. This interface delivers different perception.


HaxeFlixel 2 player game: make input code cleaner I'm makeing two-player game. player1 can play with the WASD keys, and player2 can play with the arrow keys.

PlayState.hx

package;

import flixel.FlxState;

class PlayState extends FlxState
{
    //basic global varible
    var WindowWidth:Int = 640;
    var WindowHeight:Int = 480;

    //sprites
    var player1:Player;
    var player2:Player;

    override public function create():Void
    {

        player1 = new Player(50, WindowHeight / 2 - 50);
        add(player1);

        player2 = new Player(590, WindowHeight / 2 - 50);
        add(player2);

        super.create();
    }

    override public function update(elapsed:Float):Void
    {
        player1.update();
        player2.update();
        super.update(elapsed);
    }
}

Player.hx

package;

import flixel.FlxSprite;
import flixel.FlxG;
import flixel.util.FlxColor;

class Player extends FlxSprite
{
    public function new(X:Float,Y:Float)
    {
        super(X,Y);
        makeGraphic(20,100,FlxColor.WHITE);
    }

    override public function update(elapsed:Float):Void
    {

        super.update(elapsed);
    }
}

In this code, I made a player class. This class is used by both player1 and player2 instances.

Currently both players are moved by the arrow keys. But I want to make two players using different keys (arrow keys and WASD keys).

I'm considering two solutions, but I think the both ways are not perfect. It smells of bad code.

Solution 1:
Make player1 and player2 classes. They extend the Player class and override the update() method.

class Player1 extends Player
{
    override public function update(elapsed:Float):Void
    {
        //make player1 move by wasd keys.
        super.update(elapsed);
    }
}
class Player1 extends Player
{
    override public function update(elapsed:Float):Void
    {
        //make player2 move by arrow keys.
        super.update(elapsed);
    }
}

But if I add the code in update(), there will be duplicated code. I want to make two player is completely identical, only different location and keyboard.

Solution 2:
Receive argument in update() function which key will use, and use it.

override public function update(elapsed:Float, upkey, downkey, rightkey, leftkey):Void
{
    //I will write it psuedo code
    if(upkey.pressed):
        move up
    if(downkey.pressed):
        move down
    if(leftkey.pressed):
        move left
    if(rightkey.pressed):
        move right
    super.update(elapsed);
}

But it have to additional argument in the update() function. And I think it makes the code more complex.

To me, both seem like bad code. What can I do?

Parametrizing the keys seems like the correct solution to me. The subclassing approach leads to code duplication and inflexibility. However, you should not do that by adding parameters to update() - in fact, you can't, because the function signature of an overriden method has to match. You would get a compiler error:

Field update overloads parent class with different or incomplete type
Different number of function arguments

Since it's 4 parameters that are closely related to each other, you might want to bundle them in a structure. By using the FlxG.keys.anyPressed() API, which takes an array of keys, we could in theory also allow multiple keys per input:
package;

import flixel.FlxG;
import flixel.FlxSprite;
import flixel.util.FlxColor;
import flixel.input.keyboard.FlxKey;

class Player extends FlxSprite
{
    var keys:PlayerKeys;

    public function new(x:Float, y:Float, keys:PlayerKeys)
    {
        super(x, y);
        makeGraphic(20, 100, FlxColor.WHITE);
    }

    override public function update(elapsed:Float):Void
    {
        if (FlxG.keys.anyPressed(keys.up)) { /* move up */ }
        if (FlxG.keys.anyPressed(keys.down)) { /* move down */ }
        if (FlxG.keys.anyPressed(keys.left)) { /* move left */ }
        if (FlxG.keys.anyPressed(keys.right)) { /* move right */ }

        super.update(elapsed);
    }
}

typedef PlayerKeys = {
    up:Array<FlxKey>,
    down:Array<FlxKey>,
    left:Array<FlxKey>,
    right:Array<FlxKey>
}

And to construct the player instances:
player1 = new Player(x, y, {up: [UP], down: [DOWN], left: [LEFT], right: [RIGHT]});
player2 = new Player(x, y, {up: [W], down: [S], left: [A], right: [D]});

There's two other issues with your code:

You update() your players manually in PlayState - Flixel already does that when you call super.update(), since you've add()ed the players to the state. This means the players end up being updated twice per frame.
You manually added variables for WindowWidth and WindowHeight, which are already available globally via FlxG.width and FlxG.height.


Getting point on a rectangle border To get points which lie on the border of a rectangle, I use 4 for loops. At first I thought about creating Rectangle and using PathItterator, but it confused me very much. Is there a better way than mine?

int r = 5;
int x = 0;
int y = 0;
int length = r * 2 + 1;
for (int i = 0; i < length; i++) {
    System.out.print(String.format("(%d,%d) ", x - r + i, y - r));
}
System.out.println();
for (int i = 0; i < length; i++) {
    System.out.print(String.format("(%d,%d) ", x - r + i, y + r));
}        System.out.println();

for (int i = 0; i < length; i++) {
    System.out.print(String.format("(%d,%d) ", x - r , y - r+i));
}        System.out.println();

for (int i = 0; i < length; i++) {
    System.out.print(String.format("(%d,%d) ", x + r , y - r+i));
}
System.out.println();

Most rectangles have a length and a width.  I’ve never heard of a rectangle with a radius, but it would be known as a square.

More constructively:

System.out.print(String.format("(%d,%d) ", x - r + i, y - r));

Could be written as:

System.out.format("(%d,%d) ", x - r + i, y - r);


Conditionally displaying a notice about free shipping in Woocommerce I wrote PHP code to display a Woocommerce notice - "free shipping for $40 & under" only once on shop page and once on cart page, but also in cart should not be more than 40$.  The code works, but i am not sure that it is the best way to do it:

$actual_link = (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'on' ? "https" : "http") . "://$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";

$shop_link = (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'on' ? "https" : "http") ."://$_SERVER[HTTP_HOST]/shop/";

$cart_link = (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'on' ? "https" : "http") ."://$_SERVER[HTTP_HOST]/cart/";

$first_time_shop = false;

if ((!isset($_COOKIE["mynoticeshop"]))&&($actual_link == $shop_link))
{
    $first_time_shop = true;
    setcookie("mynoticeshop", "mynoticesaleshop", time() + 600, $shop_link);
}
if ($first_time_shop) {
   function sp_custom_notice() {
        $subtotal = WC()->cart->subtotal;
        $free_shipping_threshold = 40;
        if ($subtotal < $free_shipping_threshold) {
        wc_add_notice( 'free shipping for $40 & under', 'notice' );
    }
}

add_filter( 'woocommerce_init', 'sp_custom_notice' );
}

$first_time_cart = false;
if ((!isset($_COOKIE["mynoticecart"]))&&($actual_link == $cart_link)) {
    $first_time_cart = true;
    setcookie("mynoticecart", "mynoticesalecart", time() + 600, $cart_link);
}
if ($first_time_cart) {
   function sp_custom_notice() {
        $subtotal = WC()->cart->subtotal;
        $free_shipping_threshold = 40;
        if ($subtotal < $free_shipping_threshold) {
        wc_add_notice( ' free shipping for $40 & under', 'notice' );
    }
}

add_filter( 'woocommerce_init', 'sp_custom_notice' );
}

I am new to Programming and I am interested to know if there is a better and shorter solution of this code?

Don't Repeat Yourself
The D.R.Y. principle is a fundamental principle in programming. Anytime logic is repeated more than once, it should be abstracted. For example, the first three variable assignments contain the same ternary operation:

(isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'on' ? "https" : "http")

That could be put into a function:
function GetProtocol() {
    return isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'on' ? "https" : "http";
}

That way if any of that logic needed to be updated, it could be changed in once place instead of three. The HTTP_HOST could also be included in that function - in that case, a name of GetProtocolAndDomain would be appropriate.
Like Peter's answer on SO mentions- the function sp_custom_notice() can be declared once, since the only difference between the two definitions appears to be the last line - where the first argument passed is 'free shipping for $40 & under' in the first case and the first argument is ' free shipping for $40 & under' in the second case. There isn't really a need for a space in the second case?
Peter also claims the first three variables are not necessary:

Firstly, you don't need the 3 $*_link variables as WooCommerce has 2 conditionals - is_shop() and is_cart() to check if you're on a shop page or a cart page. Also, the , $shop_link & , $cart_link in the setcookies aren't required, so you can safely get rid of them.1

This makes sense, since you can have the cookies available for all pages on the domain instead of just a single page. In the event that is_shop() and is_cart() weren't available, you could compare the last 5 characters of $_SERVER[REQUEST_URI] with cart/ or shop/ using substr() with -5 as the start parameter.
Threshold constant
It appears that the value for $free_shipping_threshold doesn't change. Because of this, it can be made a constant at the top of the file:
const FREE_SHIPPING_CONSTANT = 40;

Then that constant can be used in the sp_custom_notice function, and if that value needed to be updated, you could just look at the top of the file for the line to change.
Erroneous Line
I see the following line in your post:

$first_time_shop = true;

Did you manually add the HTML break tag (i.e.
) when posting here? I would expect that if that was actually in your code then the PHP interpreter would thrown an error.


Rolling z-score function in R I have written a rolling z-score function myself. Is my implementation of rolling z-score function correct?

#x is an xts object and y duration eg rollz(x,25)
rollz<-function(x,y){
  avg=rollapply(x, y, mean)
  std=rollapply(x, y, sd)
  z=(x-avg)/std
  return(z)
}

EDIT

Here is the description of z-score. A rolling (moving) z-score calculates z-score at a particular width like how we calculate simple moving averages.

This is very old, but I'd like to give a "modern way" of implementing this.

Everything should be a project, e.g. usethis::create_package("roll_util")

Every function should be in its own R-file, e.g. usethis::use_r("roll_zscore")

First version:
roll_zscore <- function(x, ...) {
avg <- RcppRoll::roll_mean(x, ...)
std <- RcppRoll::roll_sd(x, ...)

(x - avg) / std
}

Here, instead of y, we are inheriting all the rolling-operation behaviour
that is provided by {RcppRoll} (remember to add it as a dependency,
usethis::use_package("RcppRoll"))

Documentation, second version
#' Rolling Z-scores
#'
#'
#' @param x a time-series `xts` object
#' @inheritDotParams RcppRoll::roll_mean
#'
roll_zscore <- function(x, ...) {
#omitted

Next, example/test/etc.

Testing framework: usethis::use_testthat()
Unit-test for this function: usethis::use_test("roll_zscore")
We want it to work for {xts} objects, so we add it as a suggestion, because really this function could work with other stuff. usethis::use_package("xts", "Suggests")

The test looks like this:
test_that("standard example of roll_zscore", {
    requireNamespace("xts", quietly = TRUE)

    data(sample_matrix, package = "xts")
    sample.xts <- xts::as.xts(sample_matrix, descr='my new xts object')

    roll_zscore(sample.xts$Open, n = 2, fill = NA)
    roll_zscore(sample.xts$Open, n = 25, NA)
})

From this test, we discover some crucial problems with this implementation.

It doesn't work for n = 1. This is probably due to sd.
attached to it.
In order, to do the last calculation, the fill argument must be set to
something, otherwise the vectors won't be of equal length.

Either decide to handle these in the function, in the documentation, or elsewhere.

To properly handle ellipsis arguments, I've made use of do.call. Honestly, if I
wasn't trying to make this as simple as possible, I'd have used rlang and purrr::exec.
Final version of the code:
#' Rolling Z-scores
#'
#'
#' @param x a time-series `xts` object
#' @inheritDotParams RcppRoll::roll_mean
#'
roll_zscore <- function(x, ...) {
  args <- list(...)

  # either use provided `fill` or `NA`
  args$fill <- if (is.null(args$fill)) NA else args$fill
  args <- append(args, list(x = x))

  avg <- do.call(RcppRoll::roll_mean, args)
  std <- do.call(RcppRoll::roll_sd, args)

  (x - avg) / std
}

And final test file:
test_that("standard example of roll_zscore", {
  requireNamespace("xts", quietly = TRUE)

  data(sample_matrix, package = "xts")
  sample.xts <- xts::as.xts(sample_matrix, descr='my new xts object')

  roll_zscore(sample.xts, n = 3)
  roll_zscore(sample.xts$Open, n = 25)
  roll_zscore(sample.xts, n = 3, fill = 0)
  roll_zscore(sample.xts, n = 3, fill = NA)
  roll_zscore(sample.xts$Open, n = 2, fill = NA)
  roll_zscore(sample.xts$Open, n = 25, fill = NA)
})


(Improved) get-release npm module Improved version of the original question
I've created a simple npm module and CLI program to get the latest GitHub and Bitbucket release from their APIs using Node.js. Please tell me if there's anything to improve!
GitHub
npm
RunKit
Improved module code
#!/usr/bin/env node
const fetch = require("node-fetch");

/**
 * @param {string} string
 * @return {string}
 */
const normalize = (string) => {
    try {
        return string.toUpperCase().trim()
    } catch (e) {
        return string
    }
}

/**
 * @param {string} url
 * @return {string}
 */
const getJSON = async (url) => {
    return (await (await fetch(url)).json())
}

module.exports.providerMethods = {
    GITHUB: async ({user, repo, part = ""}) => {
        let json = await getJSON(`https://api.github.com/repos/${user}/${repo}/releases/latest`)
        if (json.message === "Not Found") throw "Invalid repository"
        if (!("assets" in json)) throw "Rate limit exceeded"
        let browser_download_urls = json.assets.map(asset => asset.browser_download_url)
        return browser_download_urls.filter(url => url.includes(part))
    },
    BITBUCKET: async ({user, repo, part = ""}) => {
        let json = await getJSON(`https://api.bitbucket.org/2.0/repositories/${user}/${repo}/downloads/`)
        if (json.type === "error") throw "Invalid repository"
        let links = json.values.map(value => value.links.self.href)
        return links.filter(url => url.includes(part))
    }
}

/**
 * @param {string} provider
 * @param {string} user
 * @param {string} repo
 * @param {string} part
 */
module.exports.getRelease = async ({provider, user, repo, part = ""}) => {
    if (!(module.exports.providerMethods[normalize(provider)])) {
        throw "Invalid provider"
    }
    return module.exports.providerMethods[normalize(provider)]({user, repo, part})
}

const usage = _ => {
    console.log(
        `Usage: get-release (github|bitbucket) user repo [partofreleasefile]
   Ex: get-release github phhusson treble_experimentations
       get-release github phhusson treble_experimentations arm64-ab-gapps
       get-release bitbucket JesusFreke smali
       get-release bitbucket JesusFreke smali baksmali`
    )
    process.exit(1)
}

// If via CLI
if (require.main === module) {
    let args = process.argv.slice(2)
    if (args.length !== 3 && args.length !== 4) {
        usage()
    }
    module.exports.getRelease({
        provider: args[0],
        user: args[1],
        repo: args[2],
        part: args[3]
    }).then(result => {
        if (result.length !== 1) {
            console.log(result)
        } else {
            console.log(result[0])
        }
    }).catch(error => {
        console.log(error)
        usage()
        process.exit(1)
    })
}

To be called using
/**
 * Options:
 * {
 *     provider: (github | bitbucket),
 *     user: username,
 *     repo: repository,
 *     part: part of release file name - optional
 * }
 *
 */

const { getRelease, providerMethods } = require("get-release")
;(async _ => {
  let url = await getRelease(
      {
          provider: "github",
          user: "phhusson",
          repo: "treble_experimentations",
          part: "arm64-ab-gapps"
      }
  )
  console.log(url[0])
})()

Custom providers can be defined using
const { getRelease, providerMethods } = require("get-release")
providerMethods.GITLAB = async ({user, repo, part = ""}) => {
  // Custom code, should return a string
}

Now that you've allowed the users to add custom providers, how about hiding the providerMethods object itself, and only providing a function which allows them to add to it. This would allow you to also validate if the users' want to override behaviour for predefined providers.
You have a process.exit(1) inside the call to usage(). Butm when used from cli, you catch all exceptions, then call the usage function, and against have another process.exit(1).
The call to usage should not be exiting from the program entirely. It is already handled in case of errors. You can then have a --help commandline parameter, which spits out the usage string (without raising an error for the shell).

Most of your if- conditions use the negated equality check, which is somewhat harder to follow for a developer (at least in my experience). When checking for args.length, you have double negation checks. It took me almost a minute to figure out what it is supposed to do. How about applying de Morgan's laws:
if (!(args.length === 3 || args.length === 4))

Or, maybe simply adding a comment?
Also consider:
console.log((result.length === 1) ? result[0] : result)


Django delete user from userprofile I'm creating a Django project where users can decide to delete their account and everything we know about them (solved riddles, votes ...).
I think you can compress this code but I don't know how.

I have a User related to Comment, Report, Vote, .... And I've got a Userprofile with a user relation:

class UserProfile(models.Model):
    User = models.OneToOneField(User, on_delete=models.CASCADE, verbose_name="Benutzer", unique=True, primary_key=True, default=None)
    # Some other fields

Comment-User relation:

class Comment(models.Model):
    User = models.ForeignKey(User, on_delete=models.CASCADE, default=None, verbose_name="Kommentierender")

Report, Vote, UserClicked, UserVoted, SolvedRiddle have the same relation like Comment.

Article-User relation:

class Article(models.Model):
    authors = models.ManyToManyField(User, verbose_name="Autor", blank=True, default=None)

that's my del method in userprofile to delete everything from a user:

def __del__(self):
    user = self.User
    if user is None:
        return False
    Comment.objects.filter(User=user).delete()
    Report.objects.filter(User=user).delete()
    Vote.objects.filter(User=user).delete()

    userclicked = UserClicked.objects.filter(User=user)
    if userclicked.exists():
        for object in userclicked:
            del object

    uservoted = UserVoted.objects.filter(User=user)
    if uservoted.exists():
        for object in uservoted:
            del object

    author_article = Article.objects.filter(authors__in=[user, ])
    if author_article.exists():
        for object in author_article:
            object.remove_author(user)

    solvedriddle_user = SolvedSolution.objects.filter(User=user)
    if solvedriddle_user.exists():
        for object in solvedriddle_user:
            del object

    self.User.delete()
    self.delete()
    return True

This is not how deleting objects work in Django.

The __del__ method is a Python mechanism and is called when an object is garbage collected. This would be a huge mess if database rows were dropped anytime an object goes out of scope. This is also why calling del object doesn't seem to have any side effect: Django still caches the value somewhere, you just got rid of your variable.

Instead, you should use the delete() method on a model instance or on a queryset. But in fact none of this code is required as the on_delete=models.CASCADE will take care of that for you: as soon as a User object is deleted, the other linked objects will be deleted as well.

The only thing missing is to automatically delete the User instance when a UserProfile instance is deleted. This is better implemented using signals as it will be called for both usages discussed earlier.

Alternatively, you can provide a method on your UserProfile model that will delete the underlying User and, thus, everything related to it:

def delete_user(self):
    self.User.delete()

But you won't be able to use it for bulk deletions.


Simple log rotation script I created a simple script that implements log rotation. It checks the log file size (say, out.log) if it exceeds 5MB limit. If it does then the script copies it to out1.log and clears out.log. But it also copies out1.log to out2.log, etc. So log files are kind of 'shifted' and contents of the last one is discarded. In this implementation I keep only 5 files (initial out.log and out[1-4].log).

I am not a Python programmer, although I find this language extremely useful for such kinds of tasks. I would like my code to be as much idiomatic as possible, so what can I change/improve in my script for this?

from os import listdir
from os.path import getsize, exists
from shutil import copyfile
from argparse import ArgumentParser

SIZE_5MB = 5e6
MAX_LOG_FILES_COUNT = 5

class LogRotator(object):
    def __init__(self, prefix, suffix):
        self.prefix = prefix
        self.suffix = suffix

    def __str__(self):
        return  "{}[x].{}".format(self.suffix, self.prefix)

    def __touch(self, file_name):
        open(file_name, 'w').close()

    def rotate(self):
        files = ["{}{}.{}".format(self.prefix, x + 1, self.suffix) for x in range(MAX_LOG_FILES_COUNT)]

        [self.__touch(f) for f in files if not exists(f)]

        current_log = "{}.{}".format(self.prefix, self.suffix)
        if (getsize(current_log) < SIZE_5MB):
            return

        files.insert(0, current_log)

        for i in range(len(files) - 1, 0, -1):
            copyfile(files[i-1], files[i])

        self.__touch(files[0])

if __name__ == '__main__':
    parser = ArgumentParser(description="Rotate log files")
    parser.add_argument("-prefix", help="log file prefix")
    parser.add_argument("-suffix", help="log file suffix")

    args = parser.parse_args()

    logRotator = LogRotator(args.prefix, args.suffix)
    logRotator.rotate()

Return early, there's no need to touch files if the current file is <5mb.
DRY your code, move the "{}{}.{}".format into it's own function.
Don't use comprehensions for mutations. Use an explicit for loop for that.
As you want idiomatic code, you should be aware that some people discourage the use of __ as it performs name mangling.

If the function is intended to be private, not protected, then it should be ok.
You can use the pairwise recipe to make the reversed pairwise loop. I think it's more readable, however you may not.
It's unidiomatic to put brackets on if statements. Unless the statement spans multiple lines.
It's idiomatic to put two newlines around top level classes and functions.
I'm a bit confused why you want to touch everything.
You may want to split all the different aspects of rotate into smaller functions to follow SRP.

Your code's pretty idiomatic otherwise, your naming conventions are good, you've used a main guard and you've got white space where it should be.

from os import listdir
from os.path import getsize, exists
from shutil import copyfile
from argparse import ArgumentParser
import itertools

def pairwise(iterable):
    "s -> (s0,s1), (s1,s2), (s2, s3), ..."
    a, b = itertools.tee(iterable)
    next(b, None)
    return zip(a, b)

SIZE_5MB = 5e6
MAX_LOG_FILES_COUNT = 5

class LogRotator(object):
    def __init__(self, prefix, suffix):
        self.prefix = prefix
        self.suffix = suffix

    def __str__(self):
        return  "{}[x].{}".format(self.suffix, self.prefix)

    def __touch(self, file_name):
        open(file_name, 'w').close()

    def _gen_file_name(self, name):
        return "{}{}.{}".format(self.prefix, name, self.suffix)

    def rotate(self):
        current_log = self._gen_file_name('')
        if getsize(current_log) < SIZE_5MB:
            return

        files = [
            self._gen_file_name(i)
            for i in range(1, MAX_LOG_FILES_COUNT + 1)
        ]

        for file in files:
            if not exests(file):
                self.__touch(f)

        for older, newer in pairwise(reversed([current_log] + files)):
            copyfile(newer, older)

        self.__touch(current_log)

if __name__ == '__main__':
    parser = ArgumentParser(description="Rotate log files")
    parser.add_argument("-prefix", help="log file prefix")
    parser.add_argument("-suffix", help="log file suffix")

    args = parser.parse_args()

    log_rotator = LogRotator(args.prefix, args.suffix)
    log_rotator.rotate()


Using sed to implement the cp command I have a script for school that mimics the cp command. Part of the assignment is to modify the script so that the while loop (that assigns stdin to the variable filelist) is instead done with sed.

The last argument needs to be saved to the variable to because that could be a directory depending on other user input in the script.

while [ "$#" -gt 1 ]
do
        filelist="$filelist $1"
        shift
done

to="$1"

Is this finished product good? How could I polish it up?

filelist=$(echo $* | sed 's/ [^ ]*$//')
to=$(echo $* | sed 's/^.* \([^ ]*\)$/\1/')

filelist="$(
   IFS='
'
   echo "$*" | sed -n '$!H;${x;s/.//;s/\n/ /g;p;}'
   )"

to="$(
   IFS='
'
   echo "$*" | sed '$!d'
   )"

This should do, assuming there are no new lines in the parameter/s.

Your code assume there is only 1 word per argument. This have nearly no impact unless the last argument is a multi word (miss interpreted by your code taking the very last word and not argument).

For this reason:

Use another separator than standard white space/tab for passing the parameter to sed.

Use $* (and not $@) to have the shell separation on argument and not word.
Using IFS Internal Field Separator of the shell.
Using a sub shell to limit IFS scope to this function (hopefully already the case with "$(...)" used for assignation.

Adapt sed to separate argument with new line, and readapt the new line to white space for final value (sed by default work per line so need to load the lines in buffer before working on it at the end).


Android layout with equal spacing between elements I am trying to make a nice set of items, which are equally spaced from each other.
On example below, I set buttons between empty textview. Every button and text view has height = 0dip and weight=1. And it looks pretty nice. However, is there a way to clean up the layout file?

<LinearLayout
            android:layout_width="wrap_content"
            android:layout_height="match_parent"
            android:layout_centerHorizontal="true"
            android:layout_centerVertical="true"
            android:orientation="vertical" >

            <TextView
                android:id="@+id/textView2"
                android:layout_width="wrap_content"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:text="" />

            <Button
                android:id="@+id/btn_distance_walked"
                android:layout_width="match_parent"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:clickable="false"
                android:text="Distance walked = " />

            <TextView
                android:id="@+id/textView3"
                android:layout_width="wrap_content"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:text="" />

            <Button
                android:id="@+id/btn_exersise_time"
                android:layout_width="match_parent"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:clickable="false"
                android:text="Exercise time = " />

            <TextView
                android:id="@+id/textView4"
                android:layout_width="wrap_content"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:text="" />

            <Button
                android:id="@+id/btn_average_speed"
                android:layout_width="match_parent"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:clickable="false"
                android:text="Average speed = " />

            <TextView
                android:id="@+id/textView5"
                android:layout_width="wrap_content"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:text="" />

            <Button
                android:id="@+id/btn_calories_burned"
                android:layout_width="match_parent"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:clickable="false"
                android:text="Callories burned = " />

            <TextView
                android:id="@+id/textView6"
                android:layout_width="wrap_content"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:clickable="false"
                android:text="" />

            <Button
                android:id="@+id/btn_back"
                android:layout_width="match_parent"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:text="Back" />

            <TextView
                android:id="@+id/textView1"
                android:layout_width="wrap_content"
                android:layout_height="0dip"
                android:layout_weight="1"
                android:text="" />
        </LinearLayout>

I don't know what the standards are for Layout Naming on Android, but I can tell you that these names are not very good.

textView2
btn_distance_walked
textView3

none of these accurately describes what they do.

you should probably look for naming schemes on Google.

btn_distance_walked doesn't tell us what the button is going to do. What is it going to do with the distance walked? It should be something like calculate_Distance or calculateDistance (my preferred naming scheme camelCasing)

Update:

I recently answered a question similar to this one and came up with a name scheme for buttons, action_ObjectActedUpon  this may be more specific to the naming issue in your code as well.

Your indentation seems to be off as well, it should look like this

<LinearLayout
    android:layout_width="wrap_content"
    android:layout_height="match_parent"
    android:layout_centerHorizontal="true"
    android:layout_centerVertical="true"
    android:orientation="vertical" >

    <TextView
        android:id="@+id/textView2"
        android:layout_width="wrap_content"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:text="" />

    <Button
        android:id="@+id/btn_distance_walked"
        android:layout_width="match_parent"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:clickable="false"
        android:text="Distance walked = " />

    <TextView
        android:id="@+id/textView3"
        android:layout_width="wrap_content"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:text="" />

    <Button
        android:id="@+id/btn_exersise_time"
        android:layout_width="match_parent"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:clickable="false"
        android:text="Exercise time = " />

    <TextView
        android:id="@+id/textView4"
        android:layout_width="wrap_content"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:text="" />

    <Button
        android:id="@+id/btn_average_speed"
        android:layout_width="match_parent"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:clickable="false"
        android:text="Average speed = " />

    <TextView
        android:id="@+id/textView5"
        android:layout_width="wrap_content"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:text="" />

    <Button
        android:id="@+id/btn_calories_burned"
        android:layout_width="match_parent"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:clickable="false"
        android:text="Callories burned = " />

    <TextView
        android:id="@+id/textView6"
        android:layout_width="wrap_content"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:clickable="false"
        android:text="" />

    <Button
        android:id="@+id/btn_back"
        android:layout_width="match_parent"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:text="Back" />

    <TextView
        android:id="@+id/textView1"
        android:layout_width="wrap_content"
        android:layout_height="0dip"
        android:layout_weight="1"
        android:text="" />
</LinearLayout>

I just answered a similar question and I think that I should add a little bit of that answer to this one..

Another way that you can reduce clutter in the layout is to define some styles by creating an xml file in the res/values/ folder of your project and do this

<?xml version="1.0" encoding="utf-8"?>
<resources>
    <style name="MyStyle1">
        <item name="android:layout_width">match_parent</item>
        <item name="android:layout_height">0dip</item>
        <item name="android:layout_weight">1</item>
        <!-- etc. -->
    </style>
</resources>

and then in your Layout XML you add the style to the layout element like this

<LinearLayout
    android:layout_width="wrap_content"
    android:layout_height="match_parent"
    android:layout_centerHorizontal="true"
    android:layout_centerVertical="true"
    android:orientation="vertical" >

    <TextView
        style="@style/MyStyle1"
        android:id="@+id/textView2"
        android:text="" />

    <Button
        style="@style/MyStyle1"
        android:id="@+id/btn_distance_walked"
        android:clickable="false"
        android:text="Distance walked = " />

    <TextView
        style="@style/MyStyle1"
        android:id="@+id/textView3"
        android:text="" />

    <Button
        style="@style/MyStyle1"
        android:id="@+id/btn_exersise_time"
        android:clickable="false"
        android:text="Exercise time = " />

    <TextView
        style="@style/MyStyle1"
        android:id="@+id/textView4"
        android:text="" />

    <Button
        style="@style/MyStyle1"
        android:id="@+id/btn_average_speed"
        android:clickable="false"
        android:text="Average speed = " />

    <TextView
        style="@style/MyStyle1"
        android:id="@+id/textView5"
        android:text="" />

    <Button
        style="@style/MyStyle1"
        android:id="@+id/btn_calories_burned"
        android:clickable="false"
        android:text="Callories burned = " />

    <TextView
        style="@style/MyStyle1"
        android:id="@+id/textView6"
        android:clickable="false"
        android:text="" />

    <Button
        style="@style/MyStyle1"
        android:id="@+id/btn_back"
        android:text="Back" />

    <TextView
        style="@style/MyStyle1"
        android:id="@+id/textView1"
        android:text="" />
</LinearLayout>

note that I am not a professional, but I found Styles and Themes from doing a quick search for styles.xml android example

Don't forget about Developer.Android.com it can be very helpful.