Recursive comment function echoes HTML inside function To display comments recursively I'm using this:

function fetch_article_comments($article_id, $parent_id) {
$app = new Connection();

    if ($parent_id <= 0) {
        $parent_id = NULL;
    }
    $sql = "SELECT * FROM recursive WHERE article_id = :article_id AND comment_parent <=> :parent_id ORDER BY comment_timestamp ASC";
    $query = $app->getConnection()->prepare($sql);

    $query->execute(array(':article_id' => $article_id,
                          ':parent_id' => $parent_id));

    return $query->fetchAll();
}

And to display the comments:

function display_comments ($article_id, $parent_id=0, $level=0) {
$comments = fetch_article_comments($article_id, $parent_id);
  foreach($comments as $comment) {
        $comment_id = $comment->comment_id;
        $member_id = $comment->member_id;
        $comment_text = $comment->comment_text;
        $comment_timestamp = $comment->comment_timestamp;  //get timeAgo

        // RENDER
      ?>

              <?= timeAgoInWords($comment_timestamp); ?>

          <?= $comment_text ?>

              <a href="#" class="replyLink" data-comment-id="<?= $comment_id ?>"> reply
              <a href="recursive.php?delete=<?= $comment_id ?>"> delete

    <form action="recursive.php" method="post" class="uk-form comment_form" id="<?= $comment_id ?>">
    <fieldset data-uk-margin>
        <input type="hidden" name="id" value="<?= $article_id ?>" />
        <input type="hidden" name="parent" value="<?= $comment_id ?>" />
        <input type="text" placeholder="Comment" name="svar">
        <button class="uk-button">Comment</button>
    </fieldset>
</form>

<?php
        //Recurse
        display_comments($article_id, $comment_id, $level+1);
                echo '
 ';
    }

}
?>

And the comments is then echoed on the page calling the display_comments().

<?= display_comments(2); ?>

(the 2 is the post ID)

As you can see there's a lot of HTML inside the function, it's not ideal. How can the code be improved to remove the HTML and maybe foreach the results?

I would split up the function into 2 parts

fetch comments and recurse
render html for comment

To me it is easier to see the looping logic without all the html in place

You could then also look at templating engine if you are going to be doing lots of this sort of thing, there are various options available, smarty, twig, mustache, blades to name a few.

function display_comments($article_id, $parent_id = 0, $level = 0)
{
    // TODO handle sql connection error
    $comments = fetch_article_comments($article_id, $parent_id);

    foreach ($comments as $comment) {
        display_comment_html($comment);

        display_comments($article_id, $comment->comment_id, $level + 1);
        echo '
 ';
    }

}

function display_comment_html($comment)
{
    $comment_id = $comment->comment_id;
    $member_id = $comment->member_id;
    $comment_text = $comment->comment_text;
    $comment_timestamp = $comment->comment_timestamp;  //get timeAgo

// RENDER
    ?>

    <?= timeAgoInWords($comment_timestamp); ?>

    <?= $comment_text ?>

    <a href="#" class="replyLink" data-comment-id="<?= $comment_id ?>"> reply
    <a href="recursive.php?delete=<?= $comment_id ?>"> delete

    <form action="recursive.php" method="post" class="uk-form comment_form" id="<?= $comment_id ?>">
        <fieldset data-uk-margin>
            <input type="hidden" name="id" value="<?= $article_id ?>"/>
            <input type="hidden" name="parent" value="<?= $comment_id ?>"/>
            <input type="text" placeholder="Comment" name="svar">
            <button class="uk-button">Comment</button>
        </fieldset>
    </form>
    <?php
}


Restricted file upload to remote the Ruby way I am using a Rails web app on the machine webapp, from which I call an action, that pushes a file to a remote server backend. Due to security reasons, we are using SSH keys and put the public part into user@backend:~/.ssh/authorized_keys. The keys are restricted, so the Rails app does not have full control over user, like this:

from="webapp",command="touch datafile; cat > datafile" ssh-rsa ...

The Ruby method looks like this

def self.push_file
  errors = `cat datafile | ssh -i id_rsa user@backend 'true' 2>&1`
  raise errors unless errors.empty?
end

What the Ruby code does is, it cats the file and then connects to backend via SSH, where it executes the "command" true, which then executes the command specified in the user@backend:~/.ssh/authorized_keys file (create datafile, write content). Lastly, the stdout is overwritten by stderr, so I can notice errors, which may occurr.

The solution works (it might be noteworthy, that datafile is always <100KiB), but seems dirty in three ways:

I am not using SCP for doing what is essentially a copy.

Possible fix: Find a way to restrict SCP to allow exactly this operation, nothing else

I am using neither Net::SSH nor Net::SCP, but instead raw commands, the first is due to the need of pipes in order to execute the first command on webapp and the second one on backend; and the latter due to the non-restriction of SCP.
stdout is overwritten by stderr in order to capture errors (least problematic, I know about popen)

Any suggestions on how to make the solution cleaner?

I may be mistaken but I really do not see why using Net::SCP would be any different (more dangerous) than copying by piping through ssh. It is not like by using Net::SCP you would be giving somebody more rights, it is all contained within your code and under your control.

If somebody tries to misuse Net::SCP he can also misuse the cat solution copying anything else the user user has access to.

But - AFAIK 2>&1 doesn’t really overwrite stdout by stderr, it appends stderr to stdout, in which case you would be raising an error also on output and not only on error.

Try this instead:

def self.push_file
    output = `cat datafile | (ssh -i id_rsa user@backend 'true' | sed 's/^/O: /' >&9 ) 9>&2 2>&1 | sed 's/^/E: /'`
    errors = output.select{|e|e.match(/^E:/)}.map{|e|e.sub(/^E: /, "")}
    raise errors unless errors.empty?
end

Found in In the shell, what does “ 2>&1 ” mean?

If not using SCP you can still improve on this pipe solution by using rsync :

rsync -avz -e "ssh -i id_rsa" /home/user/datafile backend:/home/user/


Successful use of strtol() in C Attempted to use strtol() to parse a string into an integer and to output any appropriate error messages for failed conversions. How did I do? How could/should it be done? Code works as intended if I did not miss anything.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#include <limits.h>

#define MAXLENGTH 1024

void retrieveStringInput(char *str, size_t buffersize)
{
    for (;;) {
        if (fgets(str, buffersize, stdin) != NULL) {
            str[strcspn(str, "\n")] = '\0';
            return;
        }
        printf("No input detected please try again...\n");
    }
}

int stringToInt(char *str)
{
    char *end = NULL;
    long number = strtol(str, &end, 10);
    if (!*end && end != str) {
        if (number <= INT_MIN || number >= INT_MAX) {
            printf("ERROR INPUT IS OUT OF RANGE FOR INTEGER VALUE, %d < number < %d\n", INT_MIN, INT_MAX);
            return 0;
        }
        return (int)number;
    }
    printf("STRING FAIILED TO CONVERT...\n");
    return 0;
}

int main(void)
{
    char number[MAXLENGTH];
    int convertNumber = 0;
    retrieveStringInput(number, sizeof number);

    if (number[0] = '0') {
        printf("Your number is: %d\n", convertNumber);
    }

    if ((convertNumber = stringToInt(number)) != 0)
        printf("YOUR NUMBER IS: %d\n", convertNumber);

    printf("Press any key to continue...\n");
    getch();
    return 0;
}

Compiler errors

First of all, this code does not compile with the (IMO too lenient) command line of :

gcc -Wall -Werror main.c -o strtol

Instead it gives the message that @vnp showed in their answer, that you should change if (number[0] = '0') to either if ( (number[0] = '0') ) or to if (number[0] == '0').

Having said that, the command line I usually use (and the one I put in my Makefiles) is:

-Wall -Wextra -Wfloat-equal -Wundef -Werror -fverbose-asm -Wint-to-pointer-cast -Wshadow -Wpointer-arith -Wcast-align -Wstrict-prototypes -Wcast-qual -Wmissing-prototypes -Wstrict-overflow=5 -Wwrite-strings -Wconversion --pedantic-errors -std=gnu11 -ggdb

With these arguments I get the following from my IDE:

error: conversion to ‘int’ from ‘size_t {aka long unsigned int}’ may alter its value [-Werror=conversion] at line 15 col 24

error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses] at line 45 col 9

note: place parentheses around the assignment to silence this warning at line 45 col 19

note: use '==' to turn this assignment into an equality comparison at line 45 col 19

Without fixing these, the program does not work quite correctly:

$ ./strtol
4
Your number is: 0
Press any key to continue...

$ ./strtol
888
Your number is: 0
YOUR NUMBER IS: 88
Press any key to continue...

$ ./strtol
11
Your number is: 0
YOUR NUMBER IS: 1
Press any key to continue...

Upon further inspection, the first character is ignored in the output:

$ ./strtol
623642
Your number is: 0
YOUR NUMBER IS: 23642
Press any key to continue...

$ ./strtol
 435345345
Your number is: 0
YOUR NUMBER IS: 435345345
Press any key to continue...

Upon correcting the errors mentioned by the compiler flags, however, the bugs appear to have disappeared!

$ ./strtol
4545
YOUR NUMBER IS: 4545
Press any key to continue...

$ ./strtol
 90
YOUR NUMBER IS: 90
Press any key to continue...

$ ./strtol
asdasdasdadasdasd
STRING FAIILED TO CONVERT...
Press any key to continue...

$ ./strtol
9999999
YOUR NUMBER IS: 9999999
Press any key to continue...

Let this be a lesson to ye -- listen to the C compiler, for it knows many things :)

API

When I write in C, I try to make my functions applicable to a wide variety of uses because C is verbose, simplistic and lacks polymorphism.

Having said that, I'm not sure how I feel about the interface I find when I read your code. Because the point of this program is hardly an end unto itself, surely this has applications in other projects' #includes.

To this end, it would likely be best and most testable if functions didn't have rather unrelated side effects, like printing, which could be handled by the caller.

That is, however, quite subjective and your use-case may differ entirely.

Organisation / Style

I know you're using Windows because you call getch() without an #include <curses.h>, or perhaps that was a typo and you meant getchar() (from <stdio.h>).

So even though all the functions in windows.h are laughable with their longCamelCaseCOMNames, "idiomatic" C uses snake_case_ndrscrs_and_abbrs, not camelCase.

Moreover, in "idiomatic" modern C:

#include <stdbool.h> gives us the much cleaner while ( true ) { ...
a function purporting to call strtol (string to long int) should have type long, not int
code uses the bool or ssize_t return type, or writes error to a pointer passed as an argument,  to indicate error rather than 0

this way, I don't have to have two if statements every time I want to use your function -- this is almost as bad as atoi.

always use braces around if statement bodies -- okay, maybe this is personal preference but I think it misleads me and other people reading your code.

How I would write it

I've been writing a lot of C lately, so here's how I'd reimplement your idea.

Mine writes a lot less STDOUT, but you could make it do more with the information the functions give you:

#include <stdbool.h>
#include <stddef.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <limits.h>

#define MAXLENGTH 1024

bool    readln (char** buf, const size_t len);
char* conv_str (const char* const str, long* out);
void str_chomp (char* const str);

// read a line of STDIN
bool readln (char** buf, const size_t len) {

  if ( fgets(*buf, len > INT_MAX ? INT_MAX : (int) len, stdin) != NULL) {
    str_chomp(*buf);
    return true;
  }
  return false;

}

// try to convert a string to a number or return the erroneous part
char* conv_str (const char* const str, long* out) {
  char* end   = NULL;
  long number = strtol(str, &end, 10);

  if ( end != str && !(*end) ) {
    *out = number;
    end = NULL;
  }

  return end;
}

// cut the last newline or don't
void str_chomp (char* const str) {

  if (str && (strchr(str, '\n') != NULL) ) {
    str[ strcspn(str, "\n") ] = 0;
  }
}

int main(void) {

  char* in_buf = malloc(sizeof (char) * MAXLENGTH);

  printf("Enter a number! ");

  while ( ! readln(&in_buf, MAXLENGTH) ) {
    printf("\nEnter another! ");
  }

  long out;
  char* err = conv_str(in_buf, &out);
  if ( err == NULL) {
    printf("Your number was: %ld\n", out);

  } else {
    fprintf(stderr, "That number '%s' was junk!\n", err);
  }

  free(in_buf);
  return EXIT_SUCCESS;
}


Implementing heap in Python This is basically a straightforward heap implementation. I am just moving from C to Python and I wanted to make sure that I follow Python's best practices in general. This heap is supposed to support data from any data type with any comparing function.

class Heap(object):
    """"
    Attributes:
        heap: List representation of the heap
        compar(p, c): comparator function, returns true if the relation between p and c is parent-chield
    """
    def __init__(self, compar):
        self.heap = []
        self.compar = compar
    def is_empty(self):
        return len(self.heap) == 0
    def _inv_heapify(self, element_id):
        """
        Do heapifying starting from bottom till it reaches the root.
        """
        while element_id > 0:
            if self.compar(self.heap[element_id / 2], self.heap[element_id]):
                return
            self.heap[element_id / 2], self.heap[element_id] = self.heap[element_id], self.heap[element_id / 2]
            element_id /=2
    def _heapify(self, element_id):
        """
        Do heepifying starting from the root.
        """
        l = len(self.heap)
        if l == 1:
            return
        while 2 * element_id < l:
            el_id = 2 * element_id
            if 2 * element_id + 1 < l and self.compar(self.heap[element_id * 2 + 1], self.heap[element_id * 2]):
                el_id += 1
            if self.compar(self.heap[element_id], self.heap[el_id]):
                return
            self.heap[element_id], self.heap[el_id] = self.heap[el_id], self.heap[element_id]
            element_id = el_id
    def del_min(self):
        if self.is_empty():
            return None
        x = self.heap.pop(0)
        if not self.is_empty():
            self.heap = [self.heap[-1]] + self.heap[0:-1]
            self._heapify (0)
        return x
    def min(self):
        if self.is_empty():
            return None
        return self.heap[0]
    def add(self, element):
        self.heap +=[element]
        self._inv_heapify (len (self.heap) - 1)

You have a couple of PEP8 style problems:

Functions should have an empty line above and below them.
Function calls should have their bracket immediately after the name. So fn( not fn (.
Assignment operators should have a space either side of them.
Try to keep code less than 80 characters long.

You also have a couple of naming problems:

compar should be compare or comparer, since compar is not a word. If you wanted to shorten it comp would be the best shortened version, but is worse than both the written out versions.
Lists and arrays don't have IDs, they have indexes. And so element_id is confusing. At first I used item_index or element_index, but decided to instead use parent and child to better describe the parent child relationship.
In heapify I'd change el_id to child and element_id to parent. (And you should use child rather than parent * 2)
For better readability, and for a minor performance boost, I used heap = self.heap.

Other things I'd change:

Your constructor shouldn't need to be passed compare, and so it could default to operator.lt. You may also want to take a heap as input, but you may need to add more code so it works correctly.
Add a __repr__, so that you can more easily tell what the object is.
In del_min when you add the two lists, it runs in \$O(n)\$ time. Where you can do the same with heap.pop(), which runs in \$O(1)\$ time.
You may want to look at heapq's source code to find other things you can do. It for example uses; _siftup, and _siftdown, and; _siftup_max, and _siftdown_max. Where you only write two of these.

Combining the above together gets you:

import operator

class Heap(object):
    """"
    Attributes:
        heap: List representation of the heap
        compare(p, c): comparator function, returns true if the relation between p and c is parent-chield
    """
    def __init__(self, heap=None, compare=operator.lt):
        self.heap = [] if heap is None else heap
        self.compare = compare

    def __repr__(self):
        return 'Heap({!r}, {!r})'.format(self.heap, self.compare)

    def _inv_heapify(self, child_index):
        """
        Do heapifying starting from bottom till it reaches the root.
        """
        heap, compare = self.heap, self.compare
        child = child_index
        while child > 0:
            parent = child // 2
            if compare(heap[parent], heap[child]):
                return
            heap[parent], heap[child] = heap[child], heap[parent]
            child = parent

    def _heapify(self, parent_index):
        """
        Do heepifying starting from the root.
        """
        heap, compare = self.heap, self.compare
        length = len(heap)
        if length == 1:
            return
        parent = parent_index
        while 2 * parent < length:
            child = 2 * parent
            if child + 1 < length and compare(heap[child + 1], heap[child]):
                child += 1
            if compare(heap[parent], heap[child]):
                return
            heap[parent], heap[child] = heap[child], heap[parent]
            parent = child

    def del_min(self):
        heap = self.heap
        last_element = heap.pop()
        if not heap:
            return last_element
        item = heap[0]
        heap[0] = last_element
        self._heapify(0)
        return item

    def min(self):
        if not self.heap:
            return None
        return self.heap[0]

    def add(self, element):
        self.heap.append(element)
        self._inv_heapify(len(self.heap) - 1)

Rather than implementing this yourself, you can use Pythons heapq, which may be written in C.
Since it's not a class you can easily make it one by wrapping it in one. But it doesn't have your custom comparisons, it is instead always a min heap.
If you need the custom comparisons, you could instead look into writing your own comparison object that does what you want, and use the heap class.

class Heap(list):
    def __init__(self, heap=None):
        if heap is None:
            heap = []
        heapq.heapify(heap)
        super(Heap, self).__init__(heap)

    def __repr__(self):
        return 'Heap({})'.format(super(Heap, self).__repr__())

    def push(self, item):
        return heapq.heappush(self, item)

    def heappop(self):
        return heapq.heappop(self)

    def pushpop(self, item):
        return heapq.heappushpop(self, item)

    def replace(self, item):
        return heapq.heapreplace(self, item)

    def nlargest(self, n, *args, **kwargs):
        return heapq.nlargest(n, self, *args, **kwargs)

    def nsmallest(self, n, *args, **kwargs):
        return heapq.nsmallest(n, self, *args, **kwargs)


Java Tic Tac Toe console everyone!I have created a bit ugly looking Tic Tac Toe console game using Java. I'm learning how to code and I would love to hear some comments about my code. Because it's quite streatforward, I'm not using classes and objects ( my assignment was not to use any ). Thank you in advance!
package com.company;

import java.util.ArrayList;
import java.util.Scanner;

public class Main {
    public static Scanner scan = new Scanner(System.in);

    public static char[][] table = {
        {'1', '2', '3'},
        {'4', '5', '6'},
        {'7', '8', '9'}
    };

    public static ArrayList<Character> picksTillNow = new ArrayList<Character>();
    public static char playerOne = ' ';
    public static char playerTwo = ' ';

    public static void main(String[] args) {

    for(char i = '1'; i <= '9'; i++){
        picksTillNow.add(i);
    }

    printTable();

    while (true){
        playerOneChoice();
        endGame();
        playerTwoChoice();
        endGame();
    }
}

/**
 * Prints the table with no players choices made ( 1 - 9 )
 */
public static void printTable() {

    for (int i = 0; i < table.length; i++) {
        for (int j = 0; j < table[i].length; j++) {
            System.out.print(table[i][j] + " ");
        }
        if (table[i][2] != 9) {
            System.out.println();
        }
    }
}

/**
 * Prints the table with the players choice included
 * @param player - player 1 or player 2
 * @param choice - char from 1 - 9
 */
public static void printTable(int player, char choice){

    for (int i = 0; i < table.length ; i++) {
        for (int j = 0; j < table[i].length; j++) {
            if(choice == table[i][j] && player == 1){
                //X
                table[i][j] = 'X';
            }else if(choice == table[i][j] && player == 2){
                //O
                table[i][j] = 'O';
            }
            System.out.print(table[i][j] + " ");
        }
        if(table[i][2] != 9){
            System.out.println();
        }
    }
}

/**
 * Asks for player one choice method and prints the table, checks if that position has been chosen
 */
public static void playerOneChoice(){
    System.out.print("Играч 1: ");
    playerOne = scan.next().charAt(0);
    if(picksTillNow.contains(playerOne)){
        picksTillNow.remove(picksTillNow.indexOf(playerOne));
        printTable(1, playerOne);
    }else{
        playerOneChoice();
    }
}

/**
 * Asks for player one choice method and prints the table, checks if that position has been chosen
 */
public static void playerTwoChoice(){
    System.out.print("Играч 2: ");
    playerTwo = scan.next().charAt(0);
    if(picksTillNow.contains(playerTwo)){
        picksTillNow.remove(picksTillNow.indexOf(playerTwo));
        printTable(2, playerTwo);
    }else{
        playerTwoChoice();
    }
}

/**
 * Checks for a winner. If yes - exits program
 */
public static void endGame(){
    for(int i = 0; i < table.length; i++){
        if(table[i][0] == table[i][1] && table[i][1] == table[i][2] && table[0][i] != 'О'){
            System.out.println("Победа!");
            System.exit(0);
        }
    }

    for(int j = 0; j < table.length; j++){
        if(table[0][j] == table[1][j] && table[1][j] == table[2][j] && table[j][0] != 'O'){
            System.out.println("Победа!");
            System.exit(0);
        }
    }

    if(table[0][0] == table[1][1] && table[1][1] == table[2][2] && table[0][0] != 'O'){
        System.out.println("Победа!");
        System.exit(0);
    }

    if(table[0][2] == table[1][1] && table[1][1] == table[2][0] && table[0][2] != 'O'){
        System.out.println("Победа!");
        System.exit(0);
    }

    if(picksTillNow.isEmpty()) System.exit(0);
 }
}

Adding to Bobby's very good answer, you could also explore alternative ways for checking the winning game.
You can think of noughts and crosses as 1s and 0s (true and false):
boolean[][] board = new boolean[][]{
     {false, false, false},
     {false, true, false},
     {false, false, true}
};

and therefore transform the board to an integer number:
int intboard = 0;
int exponent = 0;
for (int i = 0; i < board.length; i++) {
    for (int j = 0; j < board[0].length; j++) {
        intboard += board[i][j] ? Math.pow(2, exponent) : 0;
        exponent++;
    }
}

Then you can simply check intboard against a set of pre-calculated integers for all winning boards:
List<Integer> winningBoards = List.of(
        7, // Horizontal, row-0 --> 000000111
        56, // Horizontal, row-1 --> 7 << 3
        448, // Horizontal, row-2 --> 7 << 6
        73, // Vertical, column-0 --> 001001001
        146, // Vertical, column-1 --> 73 << 1
        292, // Vertical, column-2 --> 73 << 2
        273, // Diagonal-0 --> 100010001
        84 // Diagonal-1 --> (273 >> 2) | 16
);

Note that you can calculate some of these winning boards using bitwise operations too!

Finally checking for the winning cases is a matter of applying the winningBoards masks on intboard:
final int boardToCheck = intboard;
boolean youHaveWon = winningBoards.stream()
        .anyMatch(winningboard -> (boardToCheck & winningboard) == winningboard);

System.out.println("Nought has won: " + youHaveWon);

If you want to check for crosses you simply need to flip the bits of the winning boards using the complement operator ~:
youHaveWon = winningBoards.stream()
        .anyMatch(winningboard -> (~boardToCheck & winningboard) == winningboard);

System.out.println("Cross has won: " + youHaveWon);

The advantage of this technique is not clear for a game as simple as noughts and crosses, but if you are working on more complex games and need to check many many boards (even millions or billions!) when performing Minimax or equivalent, you can get a very nice speed up by changing the representation of your game from using objects to bits.


Using $q in Angular JS to pass navigator geo JSON from factory to controller This code is used to get the users location and deliver it as a resource to the controller. The aim is to have this data shared across multiple controllers in the application. So far this does deliver the data as a parameter of the then() method on the promise.  Is this pattern a suitable way of sharing a resource which is delivered through a promise? Any feedback would be greatly appreciated.

angular.module('geolocation', [])
.factory('geolocation', function ($q, $window) {
    return {
        getLocation: function () {
            var deferred = $q.defer();

            $window.navigator.geolocation.getCurrentPosition(function(position){
                deferred.resolve(position);
            });

            return deferred.promise;
        }
    };
});

angular.module('nearestLocationCtrl', [
    'geolocation'
])
.controller('NearestLocationCtrl', ['$scope', 'geolocation', function ($scope, geolocation) {
    geolocation.getLocation().then(function (data) {
        console.log(data);
    });
}]);

You can use resolve feature of ui-router (https://github.com/angular-ui/ui-router/wiki) or ngRoute.
Promise will be resolved and injected into controller automatically, service and controller will be nicely decoupled.
Using ui-router code would look like this:

app.controller('NearestLocationController', function($scope, location) {
}

$stateProvider.state('locationState', {
      resolve: {
         location: function(geolocation){
            return geolocation.getLocation();
         }
      },
      controller: 'NearestLocationController'
})


Dictionary to generate control names I have found myself referencing controls on a form (think Windows forms but more proprietary) by hard-typing their names in over and over again. Often I'll have a number of similar controls which could be named coolControl1department, coolControl2department, coolControl1jobTitle, coolControl2jobTitle, etc. This becomes tedious and a nightmare to maintain. Renaming the controls is definitely an option but I would still need some way of generating their names to assign values and whatnot.

To make this less reliant on me typing in the names I've created a dictionary method to concatenate the various parts of the control names, including unique reference numbers, and then call the final values elsewhere.

While it's more centralised than it was previously, there is still room for improvement and I'd appreciate any suggestions to streamline this.

Note that I'm only able to use C# version 4.0 in this application. Code below.

    public void DoStuffWithControls()
    {
        Dictionary<string, string> myControls1 = GeneratedControlNames(1);
        Dictionary<string, string> myControls2 = GeneratedControlNames(2);
        string genericControl1;
        string genericControl2;
        string departmentControl1;
        string departmentControl2;
        myControls1.TryGetValue("generic", out genericControl1);
        myControls1.TryGetValue("department", out departmentControl1);
        myControls2.TryGetValue("generic", out genericControl2);
        myControls2.TryGetValue("department", out departmentControl2);
        EnquiryForm.GetEnquiryControl(genericControl1, EnquiryControlMissing.Exception).Value = "some value";
        EnquiryForm.GetEnquiryControl(departmentControl1, EnquiryControlMissing.Exception).Value = "some value";
        EnquiryForm.GetEnquiryControl(genericControl1, EnquiryControlMissing.Exception).AnotherProperty = "some other value";
        EnquiryForm.GetEnquiryControl(genericControl2, EnquiryControlMissing.Exception).Value = "some value";
        EnquiryForm.GetEnquiryControl(genericControl2, EnquiryControlMissing.Exception).AnotherProperty = "some other value";
        EnquiryForm.GetEnquiryControl(departmentControl2, EnquiryControlMissing.Exception).Value = "some value";
    }

    private static Dictionary<string, string> GeneratedControlNames(int refNumber)
    {
        var genericPrefix = "coolControl";
        var departmentControlSuffix = "Department";
        var jobTitleControlSuffix = "Title";
        var directDialControlSuffix = "DDI";
        var emailDialControlSuffix = "Email";

        var dict = new Dictionary<string, string>();
        dict.Add("generic", string.Concat(genericPrefix, refNumber));
        dict.Add("department", string.Concat(genericPrefix, refNumber, departmentControlSuffix));
        dict.Add("jobTitle", string.Concat(genericPrefix, refNumber, jobTitleControlSuffix));
        dict.Add("directDial", string.Concat(genericPrefix, refNumber, directDialControlSuffix));
        dict.Add("email", string.Concat(genericPrefix, refNumber, emailDialControlSuffix));
        return dict;
    }

I think the dictionary is overkill.  Plus adds a lot more code with the TryGet.  I would make a class to help and hide the magic strings.

As a side note magic strings should be made into constants so they are maintained in one place and if a typo you just need to fix it in one place.    Also having a 1 or 2 being passed in also seems magic to me. I would make that into an enum.  If we left it as an int then when coming back I have no idea what values I can pass in.  Is negative numbers allowed?  What about 9999?

public enum RefNumber
{
    One = 1,
    Two = 2,
}

I'd create a class to hold the magic strings

public static class ControlsNames
{
    private const string GenericPrefix = "coolControl";
    private const string DepartmentSufix = "Department";
    private const string JobTitleControlSuffix = "JobTitle";
    private const string DirectDialControlSuffix = "DDI";
    private const string EmailDialControlSuffix = "Email";

Looking at your example you always format with the Prefix then refnumber then optional suffix.  I'll make a method in the class to handle that for us

private static string FormatName(string suffix, RefNumber refNumber)
{
     return string.Concat(GenericPrefix, (int)refNumber, suffix);
}

Now we just need to make an easy way to know what suffix we need. I'm going to make some static methods that will do that for us and return back the string.  The only exception is Generic we don't want a suffix but we can just pass in string.empty

public static string Generic(RefNumber refNumber)
{
    return FormatName(string.Empty, refNumber);
}

public static string Department(RefNumber refNumber)
{
    return FormatName(DepartmentSufix, refNumber);
}

public static string JobTitle(RefNumber refNumber)
{
    return FormatName(JobTitleControlSuffix, refNumber);
}

public static string DirectDial(RefNumber refNumber)
{
    return FormatName(DirectDialControlSuffix, refNumber);
}

public static string Email(RefNumber refNumber)
{
    return FormatName(EmailDialControlSuffix, refNumber);
}

Now we don't have any dictionary or TryGets and the code looks like this now

public void DoStuffWithControls()
{
    string genericControl1 = ControlsNames.Generic(RefNumber.One);
    string genericControl2 = ControlsNames.Generic(RefNumber.Two);
    string departmentControl1 = ControlsNames.Department(RefNumber.One);;
    string departmentControl2 = ControlsNames.Department(RefNumber.Two); ;

    EnquiryForm.GetEnquiryControl(genericControl1, EnquiryControlMissing.Exception).Value = "some value";
    EnquiryForm.GetEnquiryControl(departmentControl1, EnquiryControlMissing.Exception).Value = "some value";
    EnquiryForm.GetEnquiryControl(genericControl1, EnquiryControlMissing.Exception).AnotherProperty = "some other value";
    EnquiryForm.GetEnquiryControl(genericControl2, EnquiryControlMissing.Exception).Value = "some value";
    EnquiryForm.GetEnquiryControl(genericControl2, EnquiryControlMissing.Exception).AnotherProperty = "some other value";
    EnquiryForm.GetEnquiryControl(departmentControl2, EnquiryControlMissing.Exception).Value = "some value";
}


Refactoring If clause I have a Customer class that has the code below. I can refactor the two if condition clauses into the Memebership class. Apart from that, I am wondering how I should refactor the if clause within the Benefits set property, without over-complicating thing.

Does any one of these below apply?

Patterns of refacotring if to subclass/strategy/state/polymorphism.

  public class Customer
    {

            public Membership Membership { get; private set; }
            public bool Hasbenefits { get; private set; }

            private int benefits;

            public int Benefits
            {
                get
                {
                    return benefits;
                }

                set
                {
                    if (Membership.Name == "MembershipA")
                    {
                        this.Hasbenefits = false;
                    }
                    else if (Membership.Name == "MembershipB")
                    {
                        this.Hasbenefits = true;
                        this.benefits = value * 4 + 2;
                    }
                    else
                    {
                        this.Hasbenefits = true;
                        this.benefits = value;
                    }
                }
            }
    }

Any advice would be very much appreciated!

Personally here's what I would do :

Use the Membership type to create derived class; this will remove the if chain completely.
Avoid side effects in setters; bad practice in my opinion.
Avoid using properties with private sets; bad practice in my opinion.

This results in the following code :

public abstract class Customer
{
     protected int benefits;

     public abstract void UpdateBenefits(int newBenefits);

     public bool HasBenefits()
     {
         return benefits != 0;
     }

     public int GetBenefits()
     {
         return benefits;
     }
 }

 public class MembershipACustomer : Customer
 {
     public override void UpdateBenefits(int newBenefits)
     {
     }
 }

 public class MembershipBCustomer : Customer
 {
     public override void UpdateBenefits(int newBenefits)
     {
         benefits = newBenefits * (4 + 2);
     }
 }

 public class MembershipSomethingElseCustomer : Customer
 {
     public override void UpdateBenefits(int newBenefits)
     {
         benefits = newBenefits;
     }
 }

IMO it would also be a good idea to make a hierarchy with a new Benefit class, instead of customers. You can pass the benefit type to the customer by DI and that way it is the Benefit class that is responsible for calculating the benefit, not the customer. This would respect SRP a little more.


Function to return percentages in Python I created a function for giving me percentages from two integers.  I want two decimal places in the result.

def percent(num1, num2):
    num1 = float(num1)
    num2 = float(num2)
    percentage = '{0:.2f}'.format((num1 / num2 * 100))
    return percentage

It works, but I doubt it's very pythonic (since I have no idea what I'm doing)

>> print percent(1234, 5678)
21.73

In Python 3.0 or later, you do not need to explicitly convert your numbers to float. This is because the / operator always does floating point division (the // operator does "floor" division).

For older versions of Python (2.2 or later), you can use:

from __future__ import division

which changes the old meaning of / to the above. This makes the operation of / more predictable as the result no longer depends on the type of the inputs.


Check Three Dom Elements to See if they are empty or not and return detailed result I am checking three elements on the DOM to see if they are empty or not so that I can run only the Ajax calls that I should run and save on overall bandwidth. The code works but it "smells bad". Here is my source.
const processValues = (area, city, text) => {
    if (area.val() && city.val() && text.val()) {
        return 'all';
    } else if (area.val() && city.val() && !text.val()) {
        return 'areaCity';
    } else if (area.val() && !city.val() && text.val()) {
        return 'areaText';
    } else if (area.val() && !city.val() && !text.val()) {
        return 'city';
    } else if (!area.val() && city.val() && text.val()) {
        return 'cityText';
    } else if (!area.val() && !city.val() && text.val()) {
        return 'text';
    } else if (!area.val() && city.val() && !text.val()) {
        return 'city';
    } else {
        return 'none';
    }
};

I will be repeating this check in a few methods and I will be using a switch statement in those methods with the returned string values. If there is a better way to do this, I would appreciate it.

After speaking with some friends, this is seen as the best solution. One adjustment to the return values was required however; change the casing from camel case to title case. I am also using constants to make sure that return values can be consistently used properly.
/**
 * Check the three dom elements to see if they are empty or not and return detailed result.
 * @param {jQuery|HTMLElement} area Area input field.
 * @param {jQuery|HTMLElement} city City input field.
 * @param {jQuery|HTMLElement} text Text input field.
 * @return {string} Detailed result of which input fields are empty.
 * If all are filled, returns "all".
 * If none are filled, returns empty string.
 */
export const processValues = (area, city, text) => {
    const a = (area.val()) ? 'Area' : '';
    const c = (city.val()) ? 'City' : '';
    const t = (text.val()) ? 'Text' : '';
    if (a !== "" && c !== "" && t !== "") {
        return "all";
    } else {
        return a + c + t;
    }
};

export const ALL = "all";
export const AREA_CITY = "AreaCity";
export const AREA_TEXT = "AreaText";
export const AREA = "Area";
export const CITY_TEXT = "CityText";
export const CITY = "City";
export const TEXT = "Text";
```


Scan similar files in different directories and report differences I wrote a program in Python that scans all files in a directory, makes dictionaries with stuff like date of creation and a hash of the contents of a file. Then i scan another directory which is just a copy of the original dir, but with some files added, deleted and changed. My program reports these changes.

import os
import stat
import hashlib
import json

class DirInfo:
    def __init__(self):
        self._dirInfo = {}

    def probe(self, dirname):
        fname = os.path.join(dirname, "data.txt")
        blocksize = 512000000
        for file in os.listdir(dirname):
            if file != "data.txt":
                fileWithPath = os.path.join(dirname, file)
                fileInfo = {}
                fileInfo['name'] = file
                if os.path.isfile(fileWithPath):
                    sbuf = os.fstat(os.open(fileWithPath, os.O_RDONLY))
                    fileInfo['type'] = stat.S_IFMT(sbuf.st_mode)
                    fileInfo['mode'] = stat.S_IMODE(sbuf.st_mode)
                    fileInfo['mtime'] = sbuf.st_mtime
                    fileInfo['size'] = sbuf.st_size
                    hasher = hashlib.sha256()
                    with open(fileWithPath, 'rb') as x:
                        for chunk in iter(lambda: x.read(blocksize), b""):
                            hasher.update(chunk)
                    fileInfo['sha256'] = hasher.hexdigest()
                if os.path.islink(fileWithPath):
                    fileInfo['symlink'] = os.readlink(fileWithPath)
                self._dirInfo[file] = fileInfo
                del fileInfo
        DirInfo.save(self._dirInfo, fname)
        pass

    def compare(self, other):
        pad = "D:\\Testmapje met random files\\data.txt"
        pad2 = "D:\\Testmapje met random files 2\\data.txt"
        dct = DirInfo.load(self, pad)
        dct2 = DirInfo.load(other, pad2)
        assert isinstance(other, DirInfo)
        for key in dct:
            if key not in dct2:
                print (key, "is verwijderd")
        for key in dct2:
            fileHuidig = key
            if key not in dct:
                print (key, "is toegevoegd")
            else:
                allKeysPerFileMeasurement1 = (dct.get(key))
                allKeysPerFileMeasurement2 = (dct2.get(key))
                for key in allKeysPerFileMeasurement2:
                    Measurement1Value = (allKeysPerFileMeasurement1.get(key))
                    Measurement2Value = (allKeysPerFileMeasurement2.get(key))
                    if Measurement1Value != Measurement2Value:
                        print("In bestand:", fileHuidig, "was", key, Measurement1Value, "nu is", key, Measurement2Value)
        return []

    def save(self, fname):
        json.dump(self, open(fname, 'w'))
        pass

    def load(self, fname):
        return json.load(open(fname, 'r'))

if __name__ == '__main__':
    dirname = "D:\\Testmapje met random files"
    dirnameanders = "D:\\Testmapje met random files 2"
    dirInfo1 = DirInfo()
    dirInfo1.probe(dirname)
    # change some files
    dirInfo2 = DirInfo()
    dirInfo2.probe(dirnameanders)
    for err in dirInfo1.compare(dirInfo2):
        print(err)

My program gives the following output:

LinkNaarTestFolder2 is verwijderd (deleted)

In bestand: Lief dagboek.txt was mtime 1530555920.17531 nu is mtime 1530723663.3718975

In bestand: Lief dagboek.txt was size 30 nu is size 22

In bestand: Lief dagboek.txt was sha256 c33b32ad0a34316c970febc149199ca59946e75753ddc6438f09d1204835d0e4 nu is sha256 4728379b3f491004319de08ca6161d0b47d3f7921d79d7a9b95e1ca2c458ed48

In bestand: LinkNaarTestFolder was symlink C:\Test voor python nu is symlink C:\Users\Kevin\Documents\Battlefield 3

lol.txt is toegevoegd (is added)

Any ways to make my code shorter?

There is no reason here for this to be a class. You don't even use any class features and even have to work around it being a class by doing e.g. DirInfo.save(self._dirInfo, fname) instead of being able to do self.save(fname).

I would also add more functions, for example for hashing the file content.

import os
import stat
import hashlib
import json

STAT_FILE = "data.txt"

def get_stat_info(file_name):
    sbuf = os.fstat(os.open(file_name, os.O_RDONLY))
    return {'type': stat.S_IFMT(sbuf.st_mode),
            'mode': stat.S_IMODE(sbuf.st_mode),
            'mtime': sbuf.st_mtime,
            'size': sbuf.st_size}

def hash_file(file_name):
    hasher = hashlib.sha256()
    blocksize = 512000000
    with open(file_name, 'rb') as f:
        for chunk in iter(lambda: f.read(blocksize), b""):
            hasher.update(chunk)
    return hasher.hexdigest()

def probe(dir_name):
    dir_info = {}
    for file in os.listdir(dir_name):
        if file == STAT_FILE:
            continue
        file_with_path = os.path.join(dir_name, file)
        file_info = {'name': file}
        if os.path.isfile(file_with_path):
            file_info.update(get_stat_info(file_with_path))
            file_info['sha256'] = hash_file(file_with_path)
        if os.path.islink(file_with_path):
            file_info['symlink'] = os.readlink(file_with_path)
        dir_info[file] = file_info
    return dir_info

def save(dir_info, file_name):
    with open(file_name, 'w') as f:
        json.dump(dir_info, f)

def load(file_name):
    with open(file_name, 'r') as f:
        return json.load(f)

def compare(dir_info1, dir_info2):
    files1, files2 = dir_info1.keys(), dir_info2.keys()

    for f in files1 - files2:
        print (f, "is verwijderd")
    for f in files2 - files1:
        print (f, "is toegevoegd")
    for f in files1 & files2:
        file_info1, file_info2 = dir_info1[f], dir_info2[f]
        for key in file_info1:
            if file_info1[key] != file_info2[key]:
                print("In bestand:", f, "was", key, file_info1[key], "nu is", key, file_info2[key])

if __name__ == '__main__':
    dir_name1 = "D:\\Testmapje met random files"
    dir_info1 = probe(dir_name1)
    save(dir_info1, os.path.join(dir_name1, STAT_FILE))

    dir_name2 = "D:\\Testmapje met random files 2"
    dir_info2 = probe(dir_name2)
    save(dir_info2, os.path.join(dir_name2, STAT_FILE))

    compare(dir_info1, dir_info2)

Note that the save and load functions are not actually necessary, but I left them in because you might want to look at the results. In the compare function it makes a lot more sense to directly compare the two dictionaries, instead of saving them to a file and reading them back again.

I also changed all variables to conform to Python's official style-guide, PEP8, which recommends lower_case.


Select values from two collections I have a model:

class ColumnItsValues
{
    public int PropertyID { get; set; }
    public string ColumnName { get; set; }
    public string ValueOfColumn { get; set; }
}

two collections:

List<ColumnItsValues> listColumns = new List<ColumnItsValues>()
{
   new ColumnItsValues() {ColumnName="GUID", PropertyID=0, ValueOfColumn=null },
   new ColumnItsValues() {ColumnName="Name", PropertyID=1, ValueOfColumn=null },
   new ColumnItsValues() {ColumnName="Reference", PropertyID=2, ValueOfColumn=null },
   new ColumnItsValues() {ColumnName="Group", PropertyID=3, ValueOfColumn=null },
};

List<ColumnItsValues> listValues = new List<ColumnItsValues>()
{
    new ColumnItsValues() {ColumnName=null, PropertyID=1, ValueOfColumn="An user" },
    new ColumnItsValues() {ColumnName=null, PropertyID=2, ValueOfColumn="LIST of USERS" },
    new ColumnItsValues() {ColumnName=null, PropertyID=3, ValueOfColumn="False" },
    new ColumnItsValues() {ColumnName=null, PropertyID=0, ValueOfColumn=" " },
};

That's my linq query:

var fooColl = from columns in listColumns join values in listValues
    on columns.PropertyID equals values.PropertyID
    select new { AColumn = columns.ColumnName, AValue = values.ValueOfColumn };

My question are:

in my view, I should use where operator, but I use equals. Is it correct?
Could you advice me more high-performance query?

I would convert your fooColl to this:

IEnumerable<ColumnItsValues> foolCols = listColumns.Join(listValues,
            columns => columns.PropertyID,
            values => values.PropertyID,
            (columns, values) => new ColumnItsValues
                                 {
                                     ColumnName = columns.ColumnName,
                                     ValueOfColumn = values.ValueOfColumn
                                  });

in this way you're using a type you already know (ColumnItsValues), which make it more extendable/testable/readable.
About where == vs equals, I suggest to take a read to this blog post.

Alternatively, if as required by comments, you can change fooCols in this way:

IList<ColumnItsValues> foolCols = listColumns.Join(listValues,
            columns => columns.PropertyID,
            values => values.PropertyID,
            (columns, values) => new ColumnItsValues
                                 {
                                     ColumnName = columns.ColumnName,
                                     ValueOfColumn = values.ValueOfColumn
                                  }).ToList();

or

ColumnItsValues[] foolCols = listColumns.Join(listValues,
            columns => columns.PropertyID,
            values => values.PropertyID,
            (columns, values) => new ColumnItsValues
                                 {
                                     ColumnName = columns.ColumnName,
                                     ValueOfColumn = values.ValueOfColumn
                                  }).ToArray();

Depends on which additional functionalities you need and on your final purpose. My choice usually is try to keep my objects as simple and lightweight as possible. Both the array and the list extends IEnumerable<T>: IList<T> gives the capability to insert/remove at a given index and, since it is an extension of ICollection<T> permits you Add, Clear or check if it Contains an element, on the other hand if you keep using arrays (ColumnValues[]) the lenght would be immutable.


Basic math interpreter Essentially what I've done here is written an Interpreter that takes in Python math, such as this, 23 ** 374748, and prints out the result. This program relies on the fact that in Python 2.7 input() is "broken". This means it can take in Python math.

# Main parser class
class Parser(object):
    def __init__(self):
        self.PROMPT = "MaTh @ ==> "
        self.ERR_MSG = "Error: Invalid math!"

    # Parse inputted math
    def parse_math(self):
        while True:
            try:
                math = input(self.PROMPT)
                print "{}\n".format(math)
            except:
                print self.ERR_MSG

# Run the program
if __name__ == "__main__":
    Parser().parse_math()

Can I improve anything here? Is it possible for this to be Python3 compatible?

For 2.x and 3.x compatibility, I would explicitly use eval(raw_input(...)); you can then use:

import sys

if sys.version_info[0] > 2:
    raw_input = input

This makes more sense if you're writing for 2.x first, and is slightly easier than shoehorning eval onto 3.x's input everywhere you need it (which also makes life more difficult everywhere you don't).

Also for compatibility, print is a function in 3.x. You can import from __future__, or just use

print("{}\n".format(math))

in both.

You could make the constants class attributes:

class Parser(object):

    PROMPT = "..."
    ERR_MSG = "..."

    def parse_math(self):
        ...

These are still accessible via self, so your other code doesn't change. Note that now there are no instance attributes, you don't need __init__. In fact, you could take this further and make parse_math a class method, then you don't need to create an instance to call it.

Try giving the user a way to escape the loop. Perhaps:

while True:
    math = raw_input(self.PROMPT)
    if math.lower() == "done":
        break
    try:
        math = eval(math)
    except (NameError, SyntaxError):
        print(self.ERR_MSG)
    else:
        print("{}\n".format(math))

Note here also

Splitting input and evaluation to allow pre-evaluation checking;
Catching specific errors (see e.g. here); and
Using the else to minimise the code in the try block.

Finally, your code has no documentation. This is simple enough to not really need it, but it's a habit worth getting into. See e.g. PEP-257.


Connecting to EC2 instances using the instance name instead of IP I'm an intensive user of AWS EC2 instances, many instances are launched, stopped, repurposed, etc.

To connect to any instance using SSH I must keep track of their IPs.

The bash script I wrote (following the question I asked on SO) uses aws-cli to do the heavy lifting for me, leaving me to remember only the logical names I've given to my instances.

Here is the main code, including the auto-completion code:

# connect to machine
function sash {
  if [ -z $1 ]; then
    echo "Please enter machine name"
    return 1
  fi
  local instance ip pem
  instance=$(aws ec2 describe-instances --filters "Name=tag:Name,Values=$1" "Name=instance-state-name,Values=running" --query 'Reservations[*].Instances[].[KeyName,PublicIpAddress]' --output text)

  if [ -z "${instance}" ]; then
    echo Could not find an instance named $1
    return 1
  else
    ip=$(echo $instance | awk '{print $2}')
    pem=$(echo $instance | awk '{print $1}')

    echo "Connecting to $1 ($ip)"
    ssh -i ~/.aws/$pem.pem ubuntu@$ip
  fi
}

function clear_sash {
  unset -v _sash_instances
}

# completion command
function _sash {
  if [ -z "${_sash_instances}" ]; then
    _sash_instances="$( aws ec2 describe-tags --filter Name=key,Values=Name Name=resource-type,Values=instance --query Tags[].Value --output text )"
  fi

  local curw
  COMPREPLY=()
  curw=${COMP_WORDS[COMP_CWORD]}
  COMPREPLY=($(compgen -W "${_sash_instances}" -- $curw))
  return 0
}

complete -F _sash sash

The main function (sash) takes the first parameter, and queries aws ec2 for a machine with a 'Name' tag with that value, extracts its public ip and pem file, and calls the proper ssh command.

The auto-completion command (_sash) enumerates the names of all EC2 machines, and keeps them in a cache in the scope of the shell.

A small helper command (clear_sash) is used to clear the cache for the auto-complete.

Since this is the first function I've written in bash, I would love to hear some opinions on the code, style, caching decisions, etc.

It's mostly fine, but I would suggest some minor improvements.

Give a proper name to $1 early on, for example:

host=$1
if [ -z $host ]; then
  echo "Please enter machine name"
  return 1
fi

You use it in several places later and it will make the code more readable.

Instead of this:

if [ -z "${instance}" ]; then

you can use [[ ... ]] instead and drop the double quotes:

if [[ -z $instance ]]; then

This is a bit ugly:

ip=$(echo $instance | awk '{print $2}')
pem=$(echo $instance | awk '{print $1}')

It's ugly because you're spawning two awk processes for the same input. You could use a single read instead:

read pem ip junk <<< $instance

Or perhaps slightly cleaner to read into an array:

read -a arr <<< $instance
ip=${arr[1]}
pem=${arr[0]}

The array solution is especially good if the indexes of ip and pem are dynamic, for example if they come from variables:

read -a arr <<< $instance
# given: ip_idx=2 and pem_idx=1
ip=${arr[$ip_idx - 1]}
pem=${arr[$pem_idx - 1]}

(It would be cleaner to have the *_idx 0-based, but I used this example for the sake of illustrating simple arithmetics in array indexes.)

For the record, here's my earlier uglier awk solution:

set -- $(awk '{print $1, $2}' <<< $instance)
pem=$1
ip=$2

This is not as good as read, because it spawns an awk process, and it overwrites the positional parameters $1, $2, ... Other variants using $pem_idx and $ip_idx:

set -- $(awk "{print \$$pem_idx, \$$ip_idx}" <<< $instance)
# or:
set -- $(awk -v ip_idx=$ip_idx -v pem_idx=$pem_idx '{print $(pem_idx), $(ip_idx)}' <<< $instance)

In this code:

local curw
COMPREPLY=()
curw=${COMP_WORDS[COMP_CWORD]}
COMPREPLY=($(compgen -W "${_sash_instances}" -- $curw))

The first COMPREPLY=() is unnecessary because you overwrite it soon after anyway.

Also, I think it's better to write local on the same line as the assignment:

local curw=${COMP_WORDS[COMP_CWORD]}

Actually, since the line fits within 80 characters, I'm not sure I would use the local curw at all:

COMPREPLY=($(compgen -W "${_sash_instances}" -- ${COMP_WORDS[COMP_CWORD]}))

The final return 0 is unnecessary if the last operation is successful. If the last operation is not successful, you probably want to let the function return failure anyway.


Implementing a Yearmonth class Recently I wrote a program that was required to handle year and month data and so I wrote this class to encapsulate that handling.  What I needed was a way to initialize the Yearmonth object based on the current local time, and allow a simple method of calculating future Yearmonth values based on a duration in months.  This sample code illustrates how I use it:

ymtest.cpp

#include <iostream>
#include "Yearmonth.h"

int main()
{
    YM::Yearmonth ym;    // today
    std::cout << ym << '\n';
    ym += 2;         // 2 months from now
    std::cout << ym << '\n';
    ym += 14;        // test year increment
    std::cout << ym << '\n';
}

Yearmonth.h

#ifndef YEARMONTH_H
#define YEARMONTH_H
#include <iostream>

namespace YM {
class Yearmonth
{
public:
    // construct with today's year and month
    Yearmonth();
    // construct with year, month (1=Jan, 12=Dec)
    Yearmonth(unsigned ayear, unsigned amonth);
    // increment by given number of months
    Yearmonth &operator+=(const unsigned mon);
    // return year
    unsigned year() const;
    // return month (1=Jan, 12=Dec)
    unsigned month() const;
    // prints to ostream.  E.g. 2014 Dec ==> "201412"
    friend std::ostream& operator<<(std::ostream &out, const Yearmonth &ym);
private:
    unsigned myyear;
    unsigned mymonth;
};
}
#endif //YEARMONTH_H

Yearmonth.cpp

#include <ctime>
#include "Yearmonth.h"

namespace YM {
Yearmonth::Yearmonth()
{
    time_t tt;
    time(&tt);
    tm *t = localtime(&tt);
    myyear = t->tm_year + 1900;
    mymonth = t->tm_mon;
}

Yearmonth::Yearmonth(unsigned ayear, unsigned amonth)
    : myyear(ayear), mymonth(amonth-1)
{
    myyear += mymonth/12;
    mymonth %= 12;
}

unsigned Yearmonth::year() const
{
    return myyear;
}

unsigned Yearmonth::month() const
{
    return mymonth+1;
}

Yearmonth &Yearmonth::operator+=(const unsigned mon)
{
    mymonth += mon;
    myyear += mymonth/12;
    mymonth %= 12;
    return *this;
}

std::ostream& operator<<(std::ostream &out, const Yearmonth &ym)
{
    return out << ym.myyear * 100 + ym.mymonth+1;
}
}

The class seems sufficient for my needs and everything works.  Have I missed anything important?

I only have a few small remarks to make:

In your header file, don't include <iostream>: include <iosfwd> instead which contains the forward declarations for every type in <iostream>.
Also, you only use std::ostream in your source file, so you could simply include <ostream> there.
Several times, you compute mymonth / 12 and mymonth % 12. If your class is designed to be used intensively, you could consider using std::div(mymonth, 12) which will compute both values at once and may therefore be slightly faster (if you really need it).
You may want to prefix time_t, time, tm and localtime with std::. Them coming from the C standard library doesn't prevent you to use std::.
Using a const unsigned parameter seems pretty useless. I don't have strong opinions on the const on value parameters, but you could safely drop it since it adds little value.


Palindrome checking function I recently did a little task for fun that required me to create a class to determine if a word is a palindrome or not. I know this is quite simple, but I'd be interested to know if a real developer (I'm not a programmer) would approach the task in the same way, or if I make any amateur mistakes/do anything stupid.

My code passed the automated tests on the website, but I understand those aren't perfect.

class Palindrome
{
    public static function isPalindrome($word)
    {
        $word = strtolower($word);
        $wordLength = strlen($word);
        $wordSplitPoint = ceil($wordLength / 2);

        if ($wordLength % 2 == 0) {
            $firstHalf = substr($word, 0, $wordSplitPoint);
            $secondHalf = substr($word, $wordSplitPoint, $wordSplitPoint);
        } else {
            $firstHalf = substr($word, 0, $wordSplitPoint-1);
            $secondHalf = substr($word, $wordSplitPoint, $wordSplitPoint);
        }

        $secondHalfReversed = strrev($secondHalf);

        if ($firstHalf == $secondHalfReversed) {
            return TRUE;
        } else {
            return FALSE;
        }
    }
}

echo Palindrome::isPalindrome('Deleveled');

Others have pointed out a fix you could make to your implementation.  But there is a way to write this which is much simpler.  That takes advantage of a simplified definition of palindrome: "a word which reads the same forwards and backwards".

Now I haven't done PHP in years, and don't have a test environment, so this might have issues, but the basic idea should be clear.

public static function isPalindrome($word)
{
    return $word == strrev($word);
}

Update

Based on suggestions in the comments, this would be nicer:

function isPalindrome($word) {
    $word = preg_replace('/[^a-zA-Z]/', '', $word);
    $word = strtolower($word);
    return $word == strrev($word);
}

That will now correctly identify "Level" (ignoring capitalization) and "Madam, I'm Adam." (ignoring spaces and punctuation.)


Generate a list of the last 100 modified folders This is a subroutine I have written as a part of an Excel VBA script. I usually write vb.net or Python but I need the final results for this project in Excel so I'm using VBA.

My initial task is to get the latest 5-100 folders by date last modified (user selects the number of folders). Looking at this question on SO, I found out how to get a list of folders and store it in the cells in excel with the date last modified. This can then be sorted by date modified and I can then work my way down the list for however many folders the user selected.

The code puts this list into a new sheet called Folders, clearing out any old versions if needed.

I can't help but feel that storing the list in cells isn't the best way to do it. I would have thought it would be better to have it in some sort of dictionary or list but I could be mistaken.

Currently, there are around 1000 subfolders in the directory to search and this could potentially grow over 10,000 in the future. Will this approach still work well with that many folders?

Are there any other general improvements I could make?

Sub Get_Sorted_Folder_List(results_dir As String)

    ' Var Declarations
    Dim fso As New FileSystemObject
    Dim fs_folder As Folder
    Dim folder_sheet As Worksheet

    ' Setup "Folders" worksheet
    ' Detect if it already exists, delete if it does
    Application.DisplayAlerts = False
    Err.Clear
    On Error Resume Next
    Set folder_sheet = Sheets("Folders")
    If Err = 0 Then folder_sheet.Delete
    On Error GoTo 0

    ' Create new Folders sheet
    Set folder_sheet = ThisWorkbook.Sheets.Add(After:= _
                            ThisWorkbook.Sheets(ThisWorkbook.Sheets.Count))
    folder_sheet.Name = "Folders"
    folder_sheet.Select
    Range("A1").Select
    Columns(1).ColumnWidth = 10
    Columns(2).ColumnWidth = 18

    ' Get list of files
    Set fs_folder = fso.GetFolder(results_dir)
    Dim counter As Integer
    counter = 0

    For Each Folder In fs_folder.SubFolders
        counter = counter + 1
        Cells(counter, 1).Value = Folder.Name
        Cells(counter, 2).Value = Folder.DateLastModified
    Next

    ' Sort newest to oldest
    folder_sheet.Sort.SortFields.Clear
    folder_sheet.Sort.SortFields.Add Key:= _
        Range(Cells(1, 2), Cells(counter, 2)), SortOn:=xlSortOnValues, _
        Order:=xlDescending, DataOption:=xlSortNormal

    With folder_sheet.Sort
        .SetRange Range(Cells(1, 1), Cells(counter, 2))
        .Header = xlGuess
        .MatchCase = False
        .Orientation = xlTopToBottom
        .SortMethod = xlPinYin
        .Apply
    End With

End Sub

A couple of general things (in addition to what Vogel said about using explicit references):

Consider avoiding the "On Error" statements and using a short/reusable "sheetExists" function instead. Something like this:

Function sheetExists(wb As Workbook, wsName As String) As Boolean
    Dim tempBool As Boolean
    tempBool = False
    Dim ws As Worksheet
    For Each ws In wb.Sheets
        If ws.Name = wsName Then
            tempBool = True
            Exit For
        End If
    Next
    sheetExists = tempBool
End Function

Avoid select statements, which slow down your code and make it less readable. For example:

'This section...
folder_sheet.Select
Range("A1").Select
Columns(1).ColumnWidth = 10
Columns(2).ColumnWidth = 18
'...could be rewritten as:
With folder_sheet
    .Columns(1).ColumnWidth = 10
    .Columns(2).ColumnWidth = 18
End With

Read the folder information into an array and write it to the workbook all at once, instead of writing one value at a time. This will greatly improve performance, especially once you start getting into the thousands of folders.

'Old version
Set fs_folder = fso.GetFolder(results_dir)
Dim counter As Integer
counter = 0

For Each Folder In fs_folder.SubFolders
    counter = counter + 1
    Cells(counter, 1).Value = Folder.Name
    Cells(counter, 2).Value = Folder.DateLastModified
Next

'New version
Set fs_folder = fso.GetFolder(results_dir)
ReDim subFolderArr(1 To fs_folder.SubFolders.Count, 1 To 2) As Variant

Dim counter As Long
counter = 0
For Each Folder In fs_folder.SubFolders
    counter = counter + 1
    subFolderArr(counter, 1) = Folder.Name
    subFolderArr(counter, 2) = Folder.DateLastModified
Next

With folder_sheet
    .Range(.Cells(1, 1),.Cells(counter, 2)) = subFolderArr
End With

Overall, here's what the finished product would look like (it took 36.5 seconds to complete in a directory with 100k test subfolders):

Sub Create_Sorted_Folder_List(results_dir As String)

    'Create/clear data from results sheet
    Dim folder_sheet As Worksheet
    If sheetExists(ThisWorkbook, "Folders") Then
        Set folder_sheet = ThisWorkbook.Sheets("Folders")
        folder_sheet.Cells.Clear
    Else
        Set folder_sheet = ThisWorkbook.Worksheets.Add
        folder_sheet.Name = "Folders"
    End If

    'Get subfolder list, add data to sheet, and create table
    Dim fileArr As Variant
    fileArr = getSubfolderList(results_dir)

    With folder_sheet
        .Range("A1") = "File Name"
        .Range("B1") = "Date Modified"
        .Range(.Cells(2, 1), .Cells(UBound(fileArr, 1) + 1, 2)) = fileArr
        .ListObjects.Add(xlSrcRange, .Range(.Cells(1, 1), _
                                            .Cells(UBound(fileArr, 1) + 1, 2)), , xlYes).Name = "ResultsTable"
    End With

    Dim resultsTbl As ListObject
    Set resultsTbl = folder_sheet.ListObjects("ResultsTable")

    'Sort and format table
    With resultsTbl
        .Sort.SortFields.Clear
        .Sort.SortFields.Add Key:=.ListColumns("Date Modified").Range, _
                        SortOn:=xlSortOnValues, _
                        Order:=xlDescending, _
                        DataOption:=xlSortNormal
        .Sort.Apply
        'Set column widths manually...
        .ListColumns("File Name").Range.ColumnWidth = 10
        .ListColumns("Date Modified").Range.ColumnWidth = 18
        '...or just autofit instead
        '.Range.Columns.Autofit
    End With

End Sub
Function getSubfolderList(sourcePath As String) As Variant
'Creates array of subfolders in the first level of a directory (sourcePath)
'Stores name/date modified of each subfolder

    Dim fso As Object
    Set fso = CreateObject("Scripting.Filesystemobject")

    ReDim subfolderArr(1 To fso.GetFolder(sourcePath).Subfolders.Count, 1 To 2) As Variant
    Dim i As Long
    i = 0
    Dim subF As Object
    For Each subF In fso.GetFolder(sourcePath).Subfolders
        i = i + 1
        subfolderArr(i, 1) = subF.Name
        subfolderArr(i, 2) = subF.DateLastModified
    Next

    getSubfolderList = subfolderArr

End Function
Function sheetExists(wb As Workbook, wsName As String) As Boolean
'Determines whether a sheet with a given name (wsName) exists in a workbook (wb)

    Dim ws As Worksheet
    Dim tempBool As Boolean
    tempBool = False
    For Each ws In wb.Sheets
        If ws.Name = wsName Then
            tempBool = True
            Exit For
        End If
    Next
    sheetExists = tempBool

End Function


'Better' way to handle adding a record Scenario: I am using a helper class called 'OutputHelper' to add a new record to the table 'Output'. My 'Output' object contains the following:

ShiftID
Model
WorksOrder
LotNo
Quantity
ShiftHour

I do not want a record to be added if a record already exists with the ShiftID and ShiftHour of the record that I am trying to add.

The way I am achieving this currently is having the property 'RecordOutput' in the helper class try and add the record then return a string which I handle with a switch statement and provide feedback to the user accordingly.

This seems to be working fine but I can't help but think it's a terrible way of doing things. Can anyone provide any feedback and suggestions for improvement?

Output helper class:

public class OutputHelper
{
    public string RecordOutput(Models.Output output)
    {
        var shiftId = output.ShiftID;
        var shiftHour = output.ShiftHour;

        using (var db = new TestContext())
        {
            try
            {
                if (db.Outputs.Any(x => x.ShiftID == shiftId && x.ShiftHour == shiftHour))
                {
                    return "Exists";
                }
                db.Outputs.Add(output);
                db.SaveChanges();
            }
            catch (Exception)
            {
                return "NotSaved";
            }
            return "Success";
        }
    }
}

Code from button click on form

 var outputHelper = new OutputHelper();

        switch (outputHelper.RecordOutput(output))
        {
            case "Exists":
                lblErrorMessage.Text = "Output already recorded for this timeframe";
                failMessage.Visible = true;
                break;
            case "Not Saved":
                lblErrorMessage.Text = "There was a problem saving this record. Please try again";
                failMessage.Visible = true;
                break;
            default:
                lblSuccess.Text = "Output record added to database";
                successMessage.Visible = true;
                ResetForm();
                break;
        }

String literals vs. constants vs. enums
I think that the biggest terrible thing that you are doing here is 1) using magic string literals that are not defined as constants. 2) using a string when you can use an enum instead.
public enum SaveResult
{
    Exists, NotSaved, Success
}

In fact, your current code returns "NotSaved", while you check for case "Not Saved":. That's why you should use constants or an enum! Also, having success as the "default" case is not something I would recommend. It'd be better to assume failure unless success has been explicitly confirmed. Or even better, default is for "Oops, I forgot to check this case explicitly!", which would help you notice that you had written "Not Saved" instead of "NotSaved".
In the end, an enum is the choice I would recommend here.
Catching
That said and done, I really question the catch (Exception). In my experience, you should only catch the exceptions that you really need to catch. If the code would throw a NullReferenceException for example, that should propagate upwards and tell you that there is a serious bug in the code that needs to be fixed. Some Exceptions are just not meant to be caught.
Your question, and the database aspect
Besides this, I think that what you are doing here is reasonable. I think that it is  However, I would also like to point out that if you are using a RDMS here, then you might want to make your table have a primary key - or another unique index - over two columns: Both ShiftId and ShiftHour. Technically, that's the only way to really make sure that your table will not have duplicates over these values. Personally, I think it is perfectly OK to do a select-before-insert though. I think that is better than inserting without knowing if things go wrong or not, and catching the appropriate exception if things actually have gone wrong - i.e. a duplicate existed.
Be aware though, that after your SELECT query checking for a duplicate and before your INSERT query, there can theoretically be another query that just inserted a duplicate, effectively causing a race-condition. If such a situation would occur, the catch would still catch it of course (assuming you have the appropriate indexes in your table set up).


Noughts and crosses game for 2 players This is my first program in Python that I have coded and I am relatively happy with how it turned out. I would like to know how I could make my code shorter and more efficient.

def one_round(player):
    #Distinguish player 1 from player 2
    if player == "p1":
       p = "Player 1"
    elif player == "p2":
         p = "Player 2"

    pin = raw_input("%s move (a/b/c for row | 1/2/3 for column):" % (p)) #receive input for player from console
    #separating row from column
    row = pin[0].lower()
    col = int(pin[1]) - 1

    if row == "a": #changing the row letter into a digit for the index of board
       row = 0
    elif row =="b":
         row = 1
    elif row == "c":
         row = 2

    while (row > 2 or col > 2) or (board[row][col] == 'X' or board[row][col] == 'O'):
          if row > 2 or col > 2:
             print "Oops, these co-ordinates are off of the board, please try again"

          elif board[row][col] == 'X' or board[row][col] == 'O':
               print "Oops this spot has been taken, please pick another"

          pin = raw_input("%s move (a/b/c for row | 1/2/3 for column):" % (p)) #receive input for player from console
          #separating row from column
          row = pin[0].lower()
          col = int(pin[1]) - 1
          if row == "a": #changing the row letter into a digit for the index of board
             row = 0
          elif row =="b":
               row = 1
          elif row == "c":
               row = 2

    if player == "p1": #Determining whether to use X or O
       x = 'X'
    elif player == "p2":
         x = 'O'

    del board[row][col] #Swapping out the dashes for a symbol
    board[row].insert(col, x)

    print str(" ".join(board[0])) + '\n' + str(" ".join(board[1])) + '\n' + str(" ".join(board[2])) #Printing board

    if board[0] == [x, x, x] or board[1] == [x, x, x] or board[2] == [x, x, x] or (board[0][0] == x and board[1][0] == x and board[2][0] == x) or (board[0][1] == x and board[1][1] == x and board[2][1] == x) or (board[0][2] == x and board[1][2] == x and board[2][2] == x) or (board[0][0] == x and board[1][1] == x and board[2][2] == x) or (board[0][2] == x and board[1][1] == x and board[2][0] == x): #Checking for winner
          print '%s wins!' % (p) + '\n' + 'Game Over!'
          return True
    elif board[0][0] != '-' and board[0][1] != '-' and board[0][2] != '-' and board[1][0] != '-' and board[1][1] != '-' and board[1][2] != '-' and board[2][0] != '-' and board[2][1] != '-' and board[2][2] != '-': #Checking for draw
          print 'Draw!' + '\n' + 'Game Over!'
          return True
    else:
          return False
#---------------------------------------------------------------------------------------------------------------------
"""Tic tac toe game. (Noughts and crosses)"""
print "Welcome to Tic Tac Toe!" + "\n" + " Find a friend and start the game by typing coordinates for a 3x3 grid labelled with the letters a,b,c from top to bottom along the vertical axis and with 1,2,3 from left to right along the horizontal axis." + "\n" + "Have Fun!"
#initialising board and players
board = [["-", "-", "-"], ["-", "-", "-"], ["-", "-", "-"]]
p1 = False
p2 = False
row = 0
col = 0
while (p1 == False and p2 == False): #running game
      p1 = one_round("p1")
      if p1 == True:
         break
      p2 = one_round("p2")

Welcome to Python xD!  You did a great job on it ^^

Here are my suggestions

Don‘t code everything all in one function

there are two parts in your one_round

place new piece on board
check if there is a winner or game over

so it is better to divide to 2 functions, or more, it is best that one function just do one thing

Clean up lots if else in it

For example:

if row == "a":
      row = 0
elif row =="b":
      row = 1
elif row == "c":
      row = 2

You can use dictionary instead, board_row = {"a" : 0, "b": 1, "c": 2} or code like row = ord(row) - ord("a")

if player == "p1":
    p = "Player 1"
elif player == "p2":
    p = "Player 2"

AND

if player == "p1":
    x = 'X'
elif player == "p2":
    x = 'O'

Can be replaced by

player_info = {"p1": ["Player 1", "X"], "p2": ["Player 2", "O"]}
...
p, x = player_info[player]

Check win

You did this in a quit straight forward code, so it is a bit long, also in python it is best to just with 79 chars in one line.

You can list all position of lines and use a for loop to check if there is a winner(if the current player is the winner)

positions = (((0,0),(0,1),(0,2)), ((1,0),(1,1),(1,2)), ...)

for p in positions:
    if len(set([board[x][y] for x, y in p])) == 1:
        ...
        # then we have a winner, and the winner can only be the current player

Or just check the lines around current piece position

directions = (((0,1),(0,-1)), ((1,0),(-1,0)), ((1,1),(1,-1)), ((-1,1),(-1,-1)))

for d in directions:
    for x, y in d:
        line = set()
        current_row, current_col = row, col
        while 0 <= current_row < 3 and 0 <= current_col < 3:
            line.add(board[current_row][current_col])
            current_row += x
            current_col += y
        if len(line) == 1:
            # Win

check if it is a draw just need if all('-' not in b for b in board)

Other

del board[row][col]
board[row].insert(col, x)

You just need do board[row][col] = x