Can my responsive-tables JS code be made more performant? I was using dataTables.net's responsive plugin before this, and am happy to say that my (much simpler) code runs about 5 times quicker than that did.  On my machine the fiddle averages about 50ms versus 250ms if I use DT responsive.

Is there anything I should do to improve performance of my code? Can it be made better?  I'm FAR from a JS expert, as I'm sure it shows.  I'm very happy to drop jQuery if it can be done better without!

Here's a working JS fiddle.

function initResponsiveTables() {
    //does this enhance performance at all?
    'use strict';

    //get an array of <th> elements
    var th = $('th');

    //initialise in top scope
    var priorities = [];

    //iterate over the <th>s, extracting their column priorities
    th.each(function (index) {
        var priority = th[index].getAttribute('data-priority');

        //TODO: this seems very messy, I feel there must be a cleaner way?
        if (priorities[priority] === undefined) {
            priorities[priority] = index;
        } else {
            priorities[priority] += ',' + index;
        }
    });

    //clean up the array so we have consecutive indices
    priorities = priorities.filter(function (x) {
        return x !== undefined && x !== null;
    });

    //initialise all of these outside of the loop
    var sCols = '';
    var aCols = [];
    var aSelectors = [];
    var sSelectors = '';
    var i;

    //while the table is wider than the window, and there's still columns to remove
    while ($('#table').width() > $(window).width() - 40 && priorities.length > 0) {
        //get the lowest priority
        sCols = (priorities.pop() + '');

        //split it, in case we have more than one column on the same priority
        //in this case we prefer to remove them as a group, rather than one at a time
        aCols = sCols.split(',');

        //clear the array from previous iteration
        aSelectors = [];
        for (i = 0; i < aCols.length; i++) {
            //build up the jQuery selectors
            aSelectors.push('td:nth-child(' + aCols[i] + '), th:nth-child(' + aCols[i] + ')');
        }

        //make jQuery selector string
        sSelectors = aSelectors.join(', ');

        //hide relevant columns
        $(sSelectors).css('display', 'none');
    }
}

var start = performance.now();;

initResponsiveTables();

alert(performance.now() - start);

I know the people on here can do a way better job, but here it is without jQ and those array function things. Hopefully someone here will give you some real advise.  I just wanted to get rid of the jQ.

One note, you don't need to array.join strings any more.

...only found that out about a year ago myself.

Your main bottle necks will be in how you alter/touch the dom, but I'm no good at those points... google I guess.

All this does give you more speed, but I bet theres people here that would hate me not using array function stuff.  But you seemed to be into speed so that's what I gave you.  Me I can go either way...speed this, comfort that.

function initResponsiveTables() {
  //does this enhance performance at all?
  'use strict';

  //get an array of <th> elements
  var th = document.getElementsByTagName('th');

  //initialise in top scope
  var priorities = [];

  //iterate over the <th>s, extracting their column priorities
  for (var i = 0, end = th.length; i < end; i += 1) {

    var priority = th[i].getAttribute('data-priority');

    //TODO: this seems very messy, I feel there must be a cleaner way?
    if (priorities[priority] === undefined) {
      priorities[priority] = i;
    } else {
      priorities[priority] += ',' + i;
    }
  }

  for (var spot = 0, i = 0, end = priorities.length, x; i < end; i += 1) {
    x = priorities[i];
    if (x !== undefined && x !== null) {
      priorities[spot] = x;
      spot += 1;
    }

  }
  priorities.length = spot;

  //initialise all of these outside of the loop
  var sCols = '';
  var aCols = [];
  var aSelectors = [];
  var sSelectors = '';
  var i;

  var table = document.getElementById('table');

  while (table.getBoundingClientRect().width > window.document.documentElement["clientWidth"] - 40 && priorities.length > 0) {

    //get the lowest priority
    sCols = (priorities.pop() + '');

    //split it, in case we have more than one column on the same priority
    //in this case we prefer to remove them as a group, rather than one at a time
    aCols = sCols.split(',');

    //clear the string from previous iteration
    aSelectors = '';
    for (i = 0; i < aCols.length - 1; i += 1) {
      //build up the selectors
      aSelectors += ('td:nth-child(' + aCols[i] + '), th:nth-child(' + aCols[i] + ')') + ', ';
    }
    aSelectors += ('td:nth-child(' + aCols[i] + '), th:nth-child(' + aCols[i] + ')');

    var els = table.querySelectorAll(aSelectors);
    for (var i = 0, end = els.length; i < end; i += 1) {
      els[i].style.display = 'none';
    }
  }
}

var start = performance.now();;

initResponsiveTables();

alert(performance.now() - start);

EDIT:
Couldn't sleep its like 40 degrees C here right now so thought Id make you an example to show the difference when you dont thrash the dom.  I think the technical term (bad memory) would be trying to avoid reflows.

Where yours is is pretty obvious, this....
while (table.getBoundingClientRect().width > window.document.documentElement["clientWidth"] - 40 && priorities.length > 0) {
...specificly....
table.getBoundingClientRect().width for sure and maybe window.document.documentElement["clientWidth"]
...combined with...
els[i].style.display = 'none';

Your, in a loop, changing the dom and then asking for a prop that definitely causes a reflow/repaint/wotevaitscalled if the dom has changed since last call and that stuff is real costly.

So what if we knew the width of the cells and only had to get the width of the table and window once?  Well youd end up with code like below and if you test it youll see it is faster, ESPECIALLY in FireFox (didnt test IE).

I pre picked the width of the cells, but you could look into maybe getting it from the already rendered table (hate that by the way) or other options, or just stick with a set width which the browser would like anyway (easier to paint).

http://plnkr.co/edit/oeJpGK?p=preview      I prfr plnkr ;)

function initResponsiveTables() {
  //does this enhance performance at all?
  'use strict';

  var th = document.getElementsByTagName('th');
  var priorities = [];
  for (var end = 0, i = th.length - 1; i >= end; i -= 1) {

    var priority = th[i].getAttribute('data-priority');

    //TODO: this seems very messy, I feel there must be a cleaner way?
    if (priorities[priority] === undefined) {
      priorities[priority] = [
        i
      ];
    } else {
      priorities[priority].push(i);
    }
  }

  var removeCols = [];
  // Looking at the idea of having a set width
  // Im not great at html, depending on how tables work you might be able to get the width after the table is created instead of using a set width. worse come to worse scan each row for biggest cell
  // its also set in the css
  var colWidth = 85;

  var table = document.getElementById('table');
  var tableWidth = table.getBoundingClientRect().width;
  var targetDiff = table.getBoundingClientRect().width - window.document.documentElement["clientWidth"] - 40;
  var currentDiff = 0;
  var colIndex = priorities.length - 1;
  var cols;

  if (targetDiff > 0) {
    // var atLeast = Math.ceil(diff / colWidth); // how many cols to remove
    while (currentDiff < targetDiff && colIndex >= 0) {

      cols = priorities[colIndex];

      if (!cols) {
        //skip sparse bits
        while (cols === undefined && colIndex >= 0) {
          cols = priorities[colIndex--];
        }
      }

      if (cols) {
        cols.forEach(function(index) {
          table.tHead.rows[0].children[index].style.display = 'none';
          for (var i = 0, end = table.children[1].children.length; i < end; i += 1) {
            var row = table.children[1].children[i];
            row.children[index].style.display = 'none';
          }
        });
        currentDiff += colWidth * cols.length;
      }
      colIndex--;
    }
  }
}

var start = performance.now();;

initResponsiveTables();

alert(performance.now() -

start);


readable validation with python regex and simple logic I have come with such a code for checking IP addresses. But would like to know how clean and good it is from 1 to 10. What I care about are readability and simplicity. Please give me any feedback.
Should I go here with unittests?
import re
import logging

logging.basicConfig(level=logging.ERROR)

pattern = re.compile(r'^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$')

def is_in_bounds(s):
    return int(s) >= 0 and int(s) <= 255

def is_leading_zeros(s):
    if s is not '0':
        return s[0] is not '0'
    return True

def validate_ipv4(ipv4: str, expectFail: bool = False) -> bool:
    logging.info(f"Validating IP '{ipv4}' START.")

    try:
        match = pattern.match(ipv4)
        assert(match)

        groups = match.groups()
        assert(len(groups)==4)

        for s in match.groups():
            assert(is_leading_zeros(s))
            assert(is_in_bounds(s))

        logging.info(f"Validating IP '{ipv4}' SUCCESS.")

    except AssertionError:
        logging.info(f"Validating IP '{ipv4}' FAILED.", exc_info=True)
        if not expectFail:
            raise

if __name__ == '__main__':
    octets = []
    octets.extend(range(0,3))
    octets.extend(range(9,12))
    octets.extend(range(98,102))
    octets.extend(range(198,202))
    octets.extend(range(250,256))

    for i in octets:
        for j in octets:
            for k in octets:
                for l in octets:
                    validate_ipv4(f'{i}.{j}.{k}.{l}')

    octets = []
    octets.extend(range(-3,0))
    octets.extend(range(256, 260))

    exceptions = []

    for i in octets:
        for j in octets:
            for k in octets:
                for l in octets:
                    validate_ipv4(f'{i}.{j}.{k}.{l}', True)

When to use is not
Your usage of is not is incorrect. You want to use == when you're testing equality, like when you check if s doesn't equal a '0'. You want to use is not when checking for identity, like when you're ensuring two variables are of the same object.
def is_leading_zeros(s: str) -> bool:
    if s != '0':
        return s[0] != '0'
    return True

Clearer comparisons
Python has a syntax that allows you to do this:
def is_in_bounds(s: str) -> int:
    return 0 <= int(s) <= 255

Type Hints
You should add types hints to your functions, which display what types are accepted and returned from your functions. See above for an example of this.


Stable-sorting all even numbers before odd numbers in a linked list Given a Linked List of integers, write a function to modify the linked
  list such that all even numbers appear before all the odd numbers in
  the modified linked list. Also, keep the order of even and odd
  numbers same. - (from geeksforgeeks: Segregate even and odd nodes in a Linked List)

  Example:

  Input:

3
7
17 15 8 9 2 4 6
4
1 3 5 7
7
8 12 10 5 4 1 6

  Output:

8 2 4 6 17 15 9
1 3 5 7
8 12 10 4 6 5 1

My Approach: get pointer to the last node of list. And then traverse the list starting from the head node and move the odd valued nodes from their current position to end of the list.Also if all the node's data is even or odd then list remains unmodified so first I check if this is the case then I simply return(len is total number of nodes & count is total number of even data nodes).

I am getting right answer on an IDE, but on other IDE where the code needs to be submitted, it's showing time limit exceeded.

How do I optimize my code?

#include <iostream>
using namespace std;
int flag=0;
struct Node{
    int data;
    Node* next;
};
void addkey(struct Node** head_ref,int key)
{struct Node* temp=(struct Node*)malloc(sizeof(Node));
temp->next=NULL;
temp->data=key;
    if(*head_ref==NULL)
    {
      *head_ref=temp;
    }
    else
    {
        struct Node* ptr=*head_ref;
        while(ptr->next!=NULL)
        {
            ptr=ptr->next;
        }
        ptr->next=temp;
    }
}
void segregate(struct Node**head_ref)
{   int len=0,count=0;
    struct Node* ptr1=*head_ref;
    struct Node* ptr2=*head_ref;
    struct Node* prev=NULL;
    while(ptr2->next!=NULL)
    {
        if((ptr2->data)%2==0) count++;
        ptr2=ptr2->next;
        len++;
    }
    if(ptr2->data%2==0)  count++;
    if(count==len+1||count==0)  return;
    struct Node* ptr5=ptr2;
    while(ptr1!=ptr5->next)
    {
        if((ptr1->data%2)==0)
        { if(flag==0) {
        *head_ref=ptr1;
        flag=1;}
            prev=ptr1;
            ptr1=ptr1->next;
        }
        else
        {
          if(prev!=NULL)  prev->next=ptr1->next;
            ptr2->next=ptr1;
            ptr2=ptr2->next;
            ptr1=ptr1->next;
            ptr2->next=NULL;
        }
    }
}
void printlist(struct Node* head)
{
    if(head==NULL) return;
    else
    {
        while(head!=NULL)
        {
            cout<<head->data<<" ";
            head=head->next;
        }
    }
}
int main() {
    int T ;
    cin>>T;
    while(T--)
    {
        int n;
        cin>>n;
        struct Node* head=(struct Node*)malloc(sizeof(Node));
        head=NULL;
        for(int i=0;i<n;i++)
        {int key;
         cin>>key;
         addkey(&head,key);
        }
        segregate(&head);
        printlist(head);cout<<"\n";
    }
    return 0;
}

The best way to accelerate your code is to defenestrate it (9.82 m/s2) and use the correct algorithm.

You don't need to do anything fancy. Just go through the list once, print all the even numbers, go through once more and print the odd ones.


Using prototypes creating a Hangman game I have a Hangman object constructor with a couple of methods connected to it.

I would like to know how I should encapsulate the logic.

Can I have logic outside the methods (as I'm doing, like hangman.letters.push(e.key);) on document.onkeydown or should I only call the methods here and they should strictly run the game?

Any logic improvements are welcome!

Thank you very much!

var hangman;

function Hangman() {
  this.words = ["sweden", "brazil", "spain", "germany"];
  this.secretWord = "";
  this.letters = [];
  this.guessedLetter = "";
  this.errorsLeft = 5;
  this.messages = {
    win: "You won!",
    loose: "Game over!",
    guessed: "You already guessed this letter, please try again..",
    notValidLetter: "Please enter a valid letter from a-z",
  };
}

Hangman.prototype._getWord = function() {
  return this.words[Math.floor(Math.random() * this.words.length)];
};

Hangman.prototype._checkIfLetter = function(keyCode) {
  return /^[a-zA-Z]*$/.test(String.fromCharCode(keyCode));
};

Hangman.prototype._checkClickedLetters = function(key) {
  return !this.letters.includes(key);
};

Hangman.prototype._addCorrectLetter = function(key) {
  if (this.secretWord.includes(key)) {
    console.log(`${key} is part of the secret word`);
    for (let ix = 0; ix < hangman.secretWord.length; ix++) {
      if (hangman.secretWord[ix] === key) {
        this.guessedLetter += key;
      }
    }
    if (this._checkWinner()) {
      console.log(this.messages.win);
    }
  } else {
    console.log(`${key} is NOT part of the secret word`);
    this._addWrongLetter();
    if (this._checkGameOver()) {
      console.log(this.messages.loose);
    }
  }
};

Hangman.prototype._addWrongLetter = function(letter) {
  return this.secretWord.includes(letter)
    ? this.errorsLeft
    : (this.errorsLeft -= 1);
};

Hangman.prototype._checkGameOver = function() {
  return this.errorsLeft === 0;
};

Hangman.prototype._checkWinner = function() {
  return (
    this.secretWord
      .split("")
      .sort()
      .join("") ===
    this.guessedLetter
      .split("")
      .sort()
      .join("")
  );
};

document.getElementById("start-game-button").onclick = function() {
  hangman = new Hangman();
  hangman.secretWord = hangman._getWord();
  console.log(hangman.secretWord);
};

document.onkeydown = function(e) {
  if (hangman != undefined) {
    // Check if char is a valid alpha char.
    if (hangman._checkIfLetter(e.keyCode)) {
      // Check if we already
      if (hangman._checkClickedLetters(e.key)) {
        hangman._addCorrectLetter(e.key);
        hangman.letters.push(e.key);
      } else {
        console.log(hangman.messages.guessed);
      }
    }
  } else {
    console.log(hangman.messages.notValidLetter);
  }
};
.game-logo {

}

#start-game-button {
  border: 0;
  box-shadow: 0;
  background: linear-gradient(to bottom, #9DA400, #BAC600);
  color: #fff;
  padding: 20px 40px;
  font-size: 30px;
  border-radius: 5px;
}

#hangman {
  text-align: center;
}
<!DOCTYPE html>

  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <meta http-equiv="X-UA-Compatible" content="ie=edge">
  <link rel="stylesheet" href="stylesheets/styles.css">
  <title>The Hangman</title>

    <button id="start-game-button">START GAME</button>

    <canvas id="hangman" height="800px" width="1200px"></canvas>

Encapsulation

  I would like to know how I should encapsulate the logic.

  Can I have logic outside the methods (as I'm doing, like
  hangman.letters.push(e.key); in document.onkeydown or should I only
  call the methods here and they should strictly run the game?

Ideally no internals should be exposed.
Using the hangman object should go more like this:

document.onkeydown = function(e) {
  if (hangman.guessLetter(e.keyCode)) {
    if (hangman.won()) {
      // congrats!
      // -> display complete word
      // -> clean up UI
    } else {
      // good guess!
    }
  } else {
    if (hangman.lost()) {
      // ouch, you lost
      // -> clean up UI
    } else {
      // ouch, you missed, try again
    }
  }
  // -> display word status, for example _we_e_ when "w" and "e" were already found in "sweden"
};

No properties of hangman should be accessed, only methods called.

Usability

The game looks incomplete:
the UI shows a large image and a button,
but expects input from the keyboard and writes messages to console.
It's hard to use like this.

Performance

The implementation of _checkWinner is inefficient.
It splits the secret word and the guess to their letters and joins them to compare to each other.
This operation is unnecessary for the secret word,
because once it's generated at the beginning of the game,
it never changes.
So it would be better to generate the word with sorted letters up front.


Intersection of N lists, or of an N-d array I often find myself having to do np.intersect1d() multiple times because I need to find the intersection of a number of list like or array like things. So I ended up creating a function for it. Though, if possible I'd like to simplify / and or speed up the code for when the input becomes rather large.

def overlap(splits):
    uniques = {item: 0 for item in list(set([i for x in splits for i in x]))}
    for idx in range(len(splits)):
        for val in list(set(splits[idx])):
            if val in uniques.keys():
                uniques[val] += 1
    intersection = np.array([x for x in uniques.keys() if uniques[x] == len(splits)])
    return intersection

You can loop through a set, there is no need to wrap them in lists.
You can loop through splits, using for i in range(len(splits)): splits[i] is just long winded.
Just use for spilt in splits:.
When checking if a key is in a dictionary, don't compare to it's .keys(), just do 'a' in dict.
You should notice that you're forcing the dictionary to be pre-populated, alternately you could use collections.defaultdict.
After changing the code to use a defaultdict you should be able to notice that all you're doing is counting this:

(item for split in splits for item in set(split))

And so you could use collections.Counter instead.
In your final loop you use .keys() and then use uniques[x]. Instead you should use .items(), with tuple unpacking.
Finally you should aim at improving the readability, so I generate all the items on one line. And build the np array on another.

This can result in:

from collections import Counter

def overlap(splits):
    items = (item for split in splits for item in set(split))
    return np.array([k for k, v in Counter(items).items() if v == len(splits)])

However this does the same as np.intersect1d with two arrays as input. The docs also say, that if you wish to intersect more than two arrays you can use:

>>> from functools import reduce
>>> reduce(np.intersect1d, ([1, 3, 4, 3], [3, 1, 2, 1], [6, 3, 4, 2]))
array([3])


Finding the states with the three most populous counties I just started to use Python and Pandas. My current solution to a problem looks ugly and inefficient. I would like to know how to improve it. Data file is Census 2010 can be viewed here

Question:

  Only looking at the three most populous counties for each state, what are the three most populous states (in order of highest population to lowest population)? Use CENSUS2010POP.
  This function should return a list of string values.

Current code

def answer_six():
    cdf = census_df[census_df['SUMLEV'] == 50]
    cdf = cdf.groupby('STNAME')
    cdf = cdf.apply(lambda x:x.sort_values('CENSUS2010POP', ascending=False)).reset_index(drop=True)
    cdf = cdf.groupby('STNAME').head(3)
    cdf = cdf.groupby('STNAME').sum()
    cdf = cdf.sort_values('CENSUS2010POP', axis=0, ascending=False).head(3)
    return list(cdf.index)

nlargest could help, it finds the maximum n values in pandas series.

In this line of code, groupby groups the frame according to state name, then apply finds the 3 largest values in column CENSUS2010POP and sums them up. The resulting series has the unique state names as index and the corresponding top 3 counties sum, applying nlargest gets a series with the top 3 states as required

return census_df[census_df['SUMLEV'] == 50].groupby(
    'STNAME')['CENSUS2010POP'].apply(
    lambda x: x.nlargest(3).sum()).nlargest(
    3).index.values.tolist()


PHP code to make a WHOIS request and return the received data I have written some PHP to make WHOIS queries and return the retrieved WHOIS record.

The code is an excerpt from a much larger program used for monitoring domain name expiry. It's a command-line program designed to be run in a shell such as Bash or PowerShell, and has no web services involved.

Firstly, there is a function to identify the first-level TLD for the domain:

//Return the first-level TLD for a domain
function getTld($domain) {
    //If domain contains dot, find first-level TLD and return it
    if(strpos($domain, ".")) {
        return(pathinfo($domain, PATHINFO_EXTENSION));
    //If domain doesn't contain a dot, just return domain as-is
    } else {
        return($domain);
    }
}

Then, the first-level TLD is prepended to .whois-servers.net, and a WHOIS lookup is made to it over TCP port 43:

//Return raw WHOIS data for domain
function whois($domain) {
    $whoisServer = getTld($domain) . ".whois-servers.net";
    set_error_handler("whoisWarningHandler", E_WARNING);
    $whois = fsockopen($whoisServer, 43);
    restore_error_handler();
    stream_set_timeout($whois, 3);
    stream_set_blocking($whois, false);
    fwrite($whois, $domain . "\r\n");
    $read = [$whois]; $retries = 1; $null = null;
    while(stream_select($read, $null, $null, 5) === false) {
        if($retries++ >= 5) {
            return(0);
        }
        print("Retrying... (attempt " . $retries . ")");
        sleep(1);
    }
    sleep(1);
    return(filter_var(fread($whois, 65535), FILTER_DEFAULT, FILTER_FLAG_STRIP_HIGH));
}

Finally, there is a custom warning handler to terminate the program if a connection to the WHOIS server cannot be established:

//Custom warning handler for WHOIS fsock/stream
function whoisWarningHandler($errno, $errstr) {
    print("An error occurred while connecting to the WHOIS server.\n\nError output:\n\n" . $errstr . "\n");
    exit(2);
}

The code may then be called using e.g. print(whois("example.com"));, which would print the WHOIS record for example.com.

Please could anyone advise as to whether best practise for code quality, efficiency and security has been followed, and any recommended adjustments/improvements that could be made?

I have accepted the answer from @ShapeOfMatter, however I have documented the actions/comments for each of their findings below.

This was my first question on Code Review SE, and I realise I should have been more clear about the usage context of the code and the fact that it's an excerpt from a larger application.

Efficiency

  You should close the socket when you're done with it.

Thanks, implemented in the full program.

  What are the sleep(1) lines for? You're already waiting for data to become available on the socket.

The sleeps are to add a delay between WHOIS lookups if whois() is called from a loop (which it is in the full program). This is to help prevent hitting WHOIS server rate limits.

Having the sleep inside the whois() function isn't ideal, so I've moved this to the loop that calls the function instead. In the full program the sleep time can also be manually overridden via a command-line argument.

  I would be tempted to make your timeouts and retry count optional arguments with default values, but that's not a huge deal.

The hard-coded timeout and retries are fine for now, but I have added a backlog item to reconsider in future.

Security

  Most of the security concerns are probably outside your control.

  How do you know if the server you're opening the socket to is the one you actually wanted? Do you care if a 3rd party is reading these messages? Could a 3rd party alter the messages in transit?

  Very likely these aren't problems you need to worry about, but depending on the application they might be.

The program is designed with the idea that all WHOIS data is untrusted, as it is a plaintext protocol that has no native encryption or integrity checking.

The attacks that I am primarily concerned about are RCE, local information disclosure and DoS caused by reading malformed/malicious data from a [spoofed/compromised] WHOIS server. In the full program, the WHOIS data is parsed and certain content extracted out of it, however this is only string manipulation - the content is never executed, used to write arbitrary files, used to connect to arbitrary network locations, etc.

The WHOIS lookups and responses do not need to be confidential in this case.

I have conducted around 12 hours of fuzz testing against the WHOIS function using Radamsa, where fuzzed WHOIS responses were repeatedly served in order to test the resilience and robustness of the code. No crashes, hangs or other issues were identified, and all errors were handled as expected.

Code Quality

  Calling exit() from an error handler probably isn't a great idea. If this is going to be used in a web-service of any kind, then this is actually a security issue: Anyone who can cause problems with your connection to the whois server can now break your site.

This is part of a command-line application with no associated web services. I should have clarified this in the original question.

Outputting the error and exiting is the intended action for the error handler.

  Format the comment at the top as a PHPDoc to help IDEs and intellisense.

Good idea, thanks. Added this as a backlog item.

  Don't do multiple assignments on one line.

The full program doesn't have this, I had actually added these manually when writing the Stack Exchange question, to reduce the number of code lines in the question box.

  Don't name a variable $null. You could name it $dummy or $temp or have separate $read and $except. You may also have the option of passing anonymous empty arrays in-line.

Thanks, adjusted in the full program.

  return isn't typically written like a function; it's a language command on the level of try or class.

Thanks, adjusted all returns in the full program.

  When you're writing a loop, your first thought should be a for loop (or foreach), which will serve just fine here.

A while loop is acceptable for now, however I have added this as a backlog item to reconsider in future.

  Use type-signatures for your functions.

Thanks, adjusted all functions in the full program.


Project Euler #2 Even Fibonacci numbers Problem:

  Each new term in the Fibonacci sequence is generated by adding the
  previous two terms. By starting with 1 and 2, the first 10 terms will
  be:

  1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...

  By considering the terms in the Fibonacci sequence whose values do not
  exceed four million, find the sum of the even-valued terms.

Solution:

#include <stdio.h>
#include <stdlib.h>

void print_answer(int limit){

    int a = 1; //1st term
    int b = 2; //2nd term
    int even_fibbonaci = 0; //start as zero,because of the loop
    int sum = 2;//sum of even up to 2nd term
    int steps_taken = 0;
    do{
        sum += even_fibbonaci;
        even_fibbonaci = 2*a + 3*b; //even fibbonaci
        a += 2*b; //fibbonaci just before even fibbonaci
        b = even_fibbonaci;
        steps_taken++;
    }while(even_fibbonaci<limit);

    printf("Sum of the even-valued fibbonaci below %d\n",limit);
    printf("Answer = %d, Steps Taken = %d\n",sum,steps_taken);

}

int main(int argc, char ** argv){

    if(argc!=2){
        printf("Invalid number of arguments\n");
        printf("Usage a.exe [limit]\n");
        return -1;
    }

    int limit = atoi(argv[1]);

    if(limit < 3){
        printf("Invalid input\n");
        printf("Enter a limit of 3 or more\n");
        return -1;
    }

    print_answer(limit);

    return 0;
}

For limit = 4000000

Execute as {executable} [limit]

  Sum of the even-valued fibbonaci below 4000000

  answer = 4613732, steps taken = 11

Compiler:

I'd suggest a couple of things.

Firstly, this is a typical Euler problem example in that Euler usually provides an example test case. Build that into your code so that you confidently refactor.

Secondly, I recommend separating the algorithm from the reporting of its results; this again supports writing built in tests without generating unwanted output.

Taking this approach, you remove the need for a command line or inputs; just keep adding test cases to the code.

I've also taken the liberty of removing/renaming  a and b from the loop, but some may consider your original more readable in this respect. What is worth doing is making sure all your variable names are meaningful.

int sumOfEvenFibonaccis(int limit, int* steps)
{
    int prior = 1;
    int even_fibbonaci = 2;
    int sum = 0;//sum of even up to 2nd term
    int steps_taken = 0;
    do{
        sum += even_fibbonaci;
        prior += 2 * even_fibbonaci;
        even_fibbonaci = 2 * prior - even_fibbonaci;
        steps_taken++;
    } while (even_fibbonaci<limit);
    if (steps != NULL)
        *steps = steps_taken;
    return sum;
}

struct TestCase
{
    int limit;
    int result;
};

struct TestCase testCases[] =
{
    { 89, 2 + 8 + 34 }
};

int numTestCases = sizeof(testCases) / sizeof(testCases[0]);

void runTests()
{
    for (unsigned i = 0; i != numTestCases; ++i)
    {
        int testResult = sumOfEvenFibonaccis(testCases[i].limit, 0);
        int expectedResult = testCases[i].result;
        assert(testResult == expectedResult);
    }
}

int main(int argc, char ** argv)
{

    runTests();

    int steps = 0;
    int result = sumOfEvenFibonaccis(4000000, &steps);
    printf("Result: %d using %d steps", result, steps);

    return 0;
}


Compile-time string hash A while back this question proposed a constexpr compile-time Sieve of Eratosthenes. It also linked to another SO question about compile-time computation of CRCs. I've found a place where I'd like to precompute the hash of a few C-strings, so I used those previous questions as a base.

I'm implementing the one-at-a-time hash algorithm for C-style strings. I'm stuck to using C++11, so the function has to be recursive and without any local state:

namespace ct
{

constexpr std::uint32_t stringLength(const char * cstr)
{
    return (*cstr != '\0') ? (stringLength(cstr + 1) + 1) : 0;
}

constexpr std::uint32_t sumSHL(std::uint32_t h, std::uint32_t shift) { return h + (h << shift); }
constexpr std::uint32_t sumSHR(std::uint32_t h, std::uint32_t shift) { return h + (h >> shift); }
constexpr std::uint32_t xorSHR(std::uint32_t h, std::uint32_t shift) { return h ^ (h >> shift); }

constexpr std::uint32_t hashFinishImpl(std::uint32_t h)
{
    // h += (h <<  3)
    // h ^= (h >> 11)
    // h += (h << 15)
    return sumSHL(xorSHR(sumSHL(h, 3), 11), 15);
}

constexpr std::uint32_t hashStepImpl(std::uint32_t h, std::uint32_t c)
{
    // h += c
    // h += (h << 10)
    // h ^= (h >>  6)
    return xorSHR(sumSHL(h + c, 10), 6);
}

constexpr std::uint32_t hashImpl(const char * cstr, std::uint32_t length, std::uint32_t h)
{
    return (length != 0) ? hashImpl(cstr + 1, length - 1, hashStepImpl(h, *cstr)) : hashFinishImpl(h);
}

constexpr std::uint32_t hashCString(const char * cstr)
{
    return hashImpl(cstr, stringLength(cstr), 0);
}

} // namespace ct {}

I'm thinking it looks a bit convoluted, but couldn't think of a better way of handling it... Any suggestions are appreciated.

Your code looks very clean to me and I already like it very much. A few minor remarks.

Assuming that you'll probably want to provide this functionality in a header file, you should declare all your functions as inline. Even if they're not in a header file, it doesn't hurt to do so.
Consider declaring your functions as noexcept, too.
I'd prefer to see the implementation details hidden in a “detail” namespace to make it clear that only hashCString is supposed to be used directly. (Or is this assumption incorrect?)
ct::hashCString is not the most descriptive name. I'd prefer if the name somehow indicated the implemented hash algorithm.
You don't actually need the length of the string. Just replace length != 0 with *cstr != '\0' in hashImpl and figure out the length as you go. This will save you one function.
It seems to me that “plus” or “add” would be more consistent with “xor” than “sum” as the emphasis is on the operation.
Your code assumes that char is an 8 bit integer. It would also be nice if you could hash arrays of signed and unsigned chars alike. Something like the following might do.

template <typename CharT>
constexpr typename std::enable_if<std::is_integral<CharT>::value && (CHAR_BIT * sizeof(CharT) == 8), std::uint8_t>::type
hashCString(const CharT *const s) noexcept;

Instead of requiring CHAR_BIT * sizeof(CharT) == 8, you could relax it to (CHAR_BIT * sizeof(CharT)) % 8 == 0 and manually loop over each octet in wider types. Smells like overkill, though.
Another possible generalization would be to provide the classical interface that takes a pair of “begin” and “end” iterators. This might be handy for hashing sub-strings, strings with embedded zeros or strings that are not terminated with a 0 byte. You can still provide your single-argument version as a convenience overload.


Bash script to validate a MySQL database I am fairly new to bash script programming so I would love some feedback.
I am trying to write a series of migration scripts for our DBs (please don't direct me to other tools; I spent a lot of time evaluating them and am forced to write custom code) and this is script that contains validation logic.

Basic requirements:

Validate file format.
Make sure the script exists in at least 1 descriptor. A descriptor dictates the scripts to execute and in which order. If a release file isn't in a descriptor it's inert so validate it's present
ensure a corresponding rollback exists
ensure a validation script exists
execute the validation script and verify results.

how I intend it (for now) to be used is

source ./validation.sh
validateFormat scripts/release/DBA-1234 || echo "fail"

as an example. Or this, which would apply all:

validate

ideally, I would love to be able to chain them together functionally but when I try the below, it doesn't seem to know my symbols:

  xargs: validateFormat: No such file or directory

find scripts/releases -name '*.sql' | xargs validateFormat | validateDescriptor | cat | mysql

or just

find scripts/releases -name '*.sql' | xargs validate | cat | mysql

release-format (DBA-1234_some_message.sql)

([A-Z]{2,5}[_-][0-9]{1,5})[_-]?(.*)

validation.sh

#!/usr/bin/env bash

user=root
password=my-secret-pw
port=3306
host=127.0.0.1
release_format=$(cat .utils/release-format)
releases=${scripts}/releases/
validators=${scripts}/validators/
descriptors=${scripts}/descriptors/
rollbacks=${scripts}/rollbacks/

validateDescriptor() {
  [[ -z $1 ]] && return -1
  local script=$(basename $1)
  grep -rq ${script} ${descriptors};
  return $?
}

ensureRollback() {
  [[ -z $1 ]] && return -1
  ls ${rollbacks}/$(toTicket ${1})* 2>&1 >/dev/null
  return $?
}

ensureValidator() {
  [[ -z $1 ]] && return -1
  ls ${validators}/$(toTicket ${1})* 2>&1 >/dev/null
  return $?
}

validateFormat() {
  [[ -z $1 ]] && return -1
  [[ $(basename -s .sql $1) =~ ${release_format} ]] && return 0 || return -1
}

toTicket() {
  [[ -n $1 ]] && basename -s .sql ${1} | sed -E "s/$release_format/\1/"
}

toValidator() {
  [[ -z $1 ]] && return -1
  local ticket=$(toTicket ${1})
  local validator=$(ls ${validators}/${ticket}* | head -1)
  [[ $? != 0 ]] && return $? || echo ${validator}
}

applyValidationScripts() {
  [[ -z $1 ]] && return -1
  local validator=$(toValidator ${1})

  if [[ ! -e ${validator} ]]; then
    return -1
  fi

  result=$(mysql -sN -h ${host} -u ${user} -p${password} -P ${port} <${validator})
  if [[ $? != 0 ]]; then
    return $?
  fi

  if [[ $(echo ${result} | wc -l | tr -d ' ') != 1 ]]; then
    echo "validation result count was more than 1 row. Invalid results"
    return -1
  else
    return $((result - 1))
  fi
}

validate() {
  for release in $(find ${releases} -type f -exec basename -s .sql {} \;); do
    #  applyValidationScripts ${release}
    validateFormat ${release}  && ensureRollback ${release} && ensureValidator ${release} && validateDescriptor ${release}
    [[ $? != 0 ]] && echo "${release} failed validation"
  done;
}

Prefer portable shell

There's no reason to use Bash [[ where [ will do, unless you already depend on other Bash features (such as arrays).  This will of course increase the number of places where you must:

Quote parameter expansions

In expansions like $(basename -s .sql $1), don't forget the quotes:

[[ $(basename -s .sql "$1") =~ ${release_format} ]]
# I removed `&& return 0 || return -1` - see next item

Let exit status flow naturally

The Bash manual says:

  When executed, the exit status of a function is the exit status of the last command executed in the body.

This means that adding return $? to the end of every function adds no value, as do constructs such as && return 0 || return -1 - just let success/fail flow through.

Put credentials in a separate file

Don't put user and password in the script (particularly if you want to put it into a version control server).  Instead, require each user to provide their credentials in a (non-world-readable) file.  If you're particularly diligent, you could fail the script if the permissions on the credentials file are too permissive.

Don't invoke ls just to check existence

# true if the first argument exists
one_exists()
{
    test -e "$1"
}

ensureRollback() {
     test -n "$1" && one_exists "${rollbacks}/$(toTicket ${1})"*
}


Change filename of Excel-files based on the content of a cell I have hundreds of Excel-sheets looking something like this:

 I can't use the actual sheet, and not the exact document numbers, since they are confidential.

The name of the files are something like

2301 Item 1.xlsx
2014 Item 2.xlsx
5321 Item 3.xlsx
3212 Item 200.xlsx

I want to change the names of these files, based on the document name that's given in cell C5 in each Excel-sheet. Note: the cells C5 - E5 are merged.

I've written the following Python 3 script:

import os
import xlrd

def rename_excel_files():

    path = r"C:\Users\Stewie\Documents\excel_folder\\"

    for filename in os.listdir(path):

        excel_file = xlrd.open_workbook(path + filename)
        work_sheet = excel_file.sheet_by_index(0)
        new_filename = work_sheet.cell_value(4,2)
        os.rename(path + '\\' + filename, path + new_filename + '.xlsx')

rename_excel_files()

It works perfectly fine... But I guess it's not perfect..?

Any tips?

Note, this finishes almost instantly, so I don't really want to improve the execution time if that results in code that's harder to read, write and understand.

Also note: It's always cell C5. I don't need the cell-number to be dynamic. This is a fixed template that's used on literally thousands of files. I will only use it on the files on that template.

The good practice is to finish your code with

if __name__ == '__main__':
     sys.exit(main())

which has the advantage of returning an error code to the system (for example for batch processing of related programs).

It means 3 things for changing your code:

import sys
moving your command rename_excel_files(r"C:\Users\Stewie\Documents\excel_folder") to the new created main() function.
adding an return statement to the main() function with value 0 in the case of the success (and optionally something else for the failure):

So the result will be something as

import os
import sys
import xlrd
    ...
    ...
    ...

def main():
    rename_excel_files(r"C:\Users\Stewie\Documents\excel_folder")
    return 0

if __name__ == '__main__':
    sys.exit(main())

(See "3.3.1 Use sys.exit in your script to return proper error codes" in the book Writing Idiomatic Python by Jeff Knupp, 2013 - or directly from our BDFL - in All Things Pythonic - Python main() functions by Guido van van Rossum
May 15, 2003)


Object-oriented student library This library management program can do following functions:

Add a book to the library
Display all available books
Lend books to students
Return books to the library
Display students who have yet to return a book

Question: How do you refactor this code so that it follows OOP, reads better, is manageable and is pythonic? How can I write name functions and classes better? How do you know which data structure you need to use so as to manage data effectively?

Library class that contains a list of books

class Library:
    def __init__(self):
        self._books = []

    def add_book(self, new_book):
        self._books.append(new_book)

    def display_books(self):
        if self._books:  # if list of books is not empty
            print("The books we have made available in our library are:\n")
            for book in self._books:
                print(book)
        else:
            print("Sorry, we have no books available in the library at the moment")

    def lend_book(self, requested_book):
        if requested_book in self._books:
            print("You have now borrowed \"%s\" " % requested_book)
            self._books.remove(requested_book)

        else:
            print("Sorry, \"%s\" is not there in our library at the moment" % requested_book)

StudentDatabase class (container for all students)

class StudentDatabase:
    def __init__(self):
        self._books = {}

    def get_student(self, name):
        if name not in self._books:
            return "Not Found"

        return self._books[name]

    def borrow_book(self, name, book):
        if name not in self._books:
            self._books[name] = list(book)
        else:
            self._books[name].append(book)

    def return_book(self, name, book):
        if book not in self._books[name]:
            print("You don't seem to have borrowed \"%s\"" % book)
        else:
            self._books[name].remove(book)

    def display_students_with_books(self):
        for name, books in self._books.items():
            if books:
                print("%s: %s" % (name, books))

Follow-up : Object-oriented student library

In addition to the below, I recommend reading Maarten Fabré's answer.

Edge case

Consider the following scenario:

book_tracking = StudentDatabase()
book_tracking.return_book("Student A", "Moby Dick")
# KeyError: 'Student A'

However, we don't need to do a try-except block to catch the student who never handled books before trying to return a book; Python provides defaultdict in the collections module to help us with this very problem:

from collections import defaultdict

class StudentDatabase:
    def __init__(self):
        self._books = defaultdict(list)

    # ...

Now, if anything tries to access an unassigned key of _books, it will simply return an empty list. This can subsequently simplify borrow_book():

    def borrow_book(self, name, book):
        self._books[name].append(book)

Interclass operability

Library and StudentDatabase don't seem to interact at all. If they're related, the student database listing book borrows should inform the library when a book is borrowed or returned. The most versatile way to do this at this stage is to add a library parameter to relevant methods.

class StudentDatabase:
    # ...

    def borrow_book(self, name, book, library):
        if library.lend_book(book):
            self._books[name].append(book)

    def return_book(self, name, book, library):
        if book not in self._books[name]:
            print("You don't seem to have borrowed \"%s\"" % book)
        else:
            library.add_book(book)
            self._books[name].remove(book)

This will require returning False when the book is not found:

class Library:

    # ...

    def lend_book(self, requested_book):
        if requested_book in self._books:
            print("You have now borrowed \"%s\" " % requested_book)
            self._books.remove(requested_book)
            return True

        else:
            print("Sorry, \"%s\" is not there in our library at the moment" % requested_book)
            return False

The implementation you currently have is very simple, so there's not much else to discuss in terms of design: it becomes more relevant when you have more moving parts. I do find it somewhat odd that it's only tracking by student, and not by book (i.e. Library could know how many copies of each book it had (int), to whom it's checked (a list perhaps with a custom CheckedOut class to store the person and the due date), and any other information pertaining to that book. A BookStatus class could store everything related to each book; the library would then contain a list of BookStatus objects.

Checking if a sequence is empty.

(Note I previously advocated using len(self._books) > 0 until Maarten Fabré (along with three people upvoting his comment) pointed out this suggestion contradicted PEP-8) The relevant passage is excerpted below.

  For sequences, (strings, lists, tuples), use the fact that empty sequences are false.

Format string literals

You seem to be using printf-style string formatting. In Python 3, format strings are more flexible and would probably be considered more Pythonic. Python 3.6 adds format string literals, which are very compact. Here are some example uses:

Original: "You have now borrowed \"%s\" " % requested_book

With f-string: f"You have now borrowed \"{requested_book}\"."

Original: "Sorry, \"%s\" is not there in our library at the moment" % requested_book

With f-string: f"Sorry, \"{requested_book}\" is not there in our library at the moment"

Original: "You don't seem to have borrowed \"%s\"" % book

With f-string: f"You don't seem to have borrowed \"{book}\""

Original: "%s: %s" % (name, books)

With f-string: f"{name}: {books}"

Result

from collections import defaultdict

class Library:
    def __init__(self):
        self._books = []

    def add_book(self, new_book):
        self._books.append(new_book)

    def display_books(self):
        if len(self._books) > 0:
            print("The books we have made available in our library are:\n")
            for book in self._books:
                print(book)
        else:
            print("Sorry, we have no books available in the library at the moment")

    def lend_book(self, requested_book):
        if requested_book in self._books:
            print(f"You have now borrowed \"{requested_book}\".")
            self._books.remove(requested_book)
            return True

        else:
            print(f"Sorry, \"{requested_book}\" is not there in our library at the moment")
            return False

class StudentDatabase:
    def __init__(self):
        self._books = defaultdict(list)

    def get_student(self, name):
        if name not in self._books:
            return "Not Found"

        return self._books[name]

    def borrow_book(self, name, book, library):
        if library.lend_book(book):
            self._books[name].append(book)

    def return_book(self, name, book, library):
        if book not in self._books[name]:
            print(f"You don't seem to have borrowed \"{book}\"")
        else:
            library.add_book(book)
            self._books[name].remove(book)

    def display_students_with_books(self):
        for name, books in self._books.items():
            if books:
                print(f"{name}: {books}")


Extended Euclidean algorithm and modular multiplicative inverse element After my first exercise here is another. Again, this is exactly that: Exercise, which means that the output doesn't necessarily need to be beautifully formatted, etc. I probably have some variable naming issues around and also more general issues.

# coding=utf-8

"""Basic usage:
$ python xggt.py 3343 77
(1, -12, 521)
65
"""

import argparse

###############################################################################

def main():
    """Prints out the results of xggt().
    xggt() takes two non-optional integers as arguments.
    """

    parser = argparse.ArgumentParser()
    parser.add_argument('num_1', type=int)
    parser.add_argument('num_2', type=int)
    args = parser.parse_args()

    if (args.num_1 == 0) and (args.num_2 == 0):
        print("Please enter two positive integers.")
    else:
        print(xggt(args.num_1, args.num_2))
        print(modinv(args.num_1, args.num_2))

###############################################################################

def xggt(num_1, num_2):
    """Implements the Extended Euclidean algorithm.
    Returns the greatest common divisor of two integers > 0.
    Also returns the coefficients of Bézout’s identity.
    """

    if num_1 % num_2 == 0:
        return(num_2, 0, 1)
    else:
        gcd, lin_fact_1, lin_fact_2 = xggt(num_2, num_1 % num_2)

        lin_fact_1 = lin_fact_1 - num_1 // num_2 * lin_fact_2

        return(gcd, lin_fact_2, lin_fact_1)

###############################################################################

def modinv(num_1, num_2):
    """Returns the modular multiplicative inverse of two integers > 0
    """

    gcd, lin_fact_1, _ = xggt(num_1, num_2)

    if gcd != 1:
        return None
    else:
        return lin_fact_1 % num_2

###############################################################################

if __name__ == '__main__':
    main()

This looks like a bug in the input validation:

if (args.num_1 == 0) and (args.num_2 == 0):
    print("Please enter two positive integers.")
else:
    print(xggt(args.num_1, args.num_2))
    print(modinv(args.num_1, args.num_2))

The message "Please enter two positive integers." contradicts with the condition:
if both numbers are 0. I think you meant more like this:

if args.num_1 > 0 and args.num_2 > 0:
    print(xggt(args.num_1, args.num_2))
    print(modinv(args.num_1, args.num_2))
else:
    print("Please enter two positive integers.")

I also removed redundant parentheses.

You had redundant parentheses in other places too:

    return(num_2, 0, 1)
    # ...
    return(gcd, lin_fact_2, lin_fact_1)

These are all unnecessary, so I recommend to remove them.

You can simplify x = x - ... with x -= ..., for example here:

# instead of this:
# lin_fact_1 = lin_fact_1 - num_1 // num_2 * lin_fact_2
lin_fact_1 -= num_1 // num_2 * lin_fact_2

Other than these minor issues, I think this is good and clean code! The names seem quite fine to me.


Return specific variable of Enum, determined by variable with the same name of another Enum? I wonder if there is any way to write the followig in a few lines instead of using all cases. Both CategoryType and AnimalSpecies are Enums.

private AnimalSpecies GetAnimalSpeciesBasedOnCategoryType(CategoryType categoryType)
{
    switch (categoryType)
    {
        case CategoryType.Reptile :
            return AnimalSpecies.Reptile;

        case CategoryType.Bird:
            return AnimalSpecies.Bird;
        case CategoryType.Mammal:
            return AnimalSpecies.Mammal;
        case CategoryType.Insect:
            return AnimalSpecies.Insect;
        case CategoryType.Marine:
            return AnimalSpecies.Marine;

        default:
            MessageBox.Show("Category doesn't exist.Something wrong in programming code");
            return AnimalSpecies.Marine;
    }
}

How about (untested not near a computer at the moment)

   return (AnimalSpecies )Enum.Parse(typeof(AnimalSpecies ), categoryType.ToString());


Web-based text editor (#2) var appname = " - simple notepad",
    textarea = document.getElementById("textarea"),
    untitled = "untitled.txt" + appname,
    filename = "*.txt",
    isModified;

document.title = untitled;

textarea.onpaste = textarea.onkeypress = function() {
    isModified = true;
};

function confirmNav() { // Confirm navigation
    if (isModified) {
        var a = window.confirm("You have unsaved changes that will be lost.");
        if (a) {
            isModified = false;
        }
    }
}

function New() { // New
    confirmNav();
    if (!isModified) {
        textarea.value = "";
        document.title = untitled;
    }
    textarea.focus();
}

function Open() { // Open
    confirmNav();
    if (!isModified) {
        document.getElementById("selected_file").click();
    }
}

function loadFileAsText() { // load file as text
    var selected_file = document.getElementById("selected_file").files[0];
    var fileReader = new FileReader();
    fileReader.onloadend = function(e) {
        if (e.target.readyState === FileReader.DONE) {
            filename = selected_file.name;
            document.title = filename + appname;
            textarea.value = e.target.result;
            textarea.focus();
        }
    };
    fileReader.readAsText(selected_file);
}

function rename() { // Rename
    filename = window.prompt("Name this note:", filename);
    if (filename) {
        filename = (filename.lastIndexOf(".txt") === -1) ? filename + ".txt" : filename;
        document.title = filename + appname;
    }
}

function Save() { // Save
    rename();
    if (filename) {
        var blob = new Blob([textarea.value], {
            type: "text/plain;charset=utf-8"
        });
        window.saveAs(blob, filename);
        isModified = false;
    }
    textarea.focus();
}

function Print() { // Print
    var prnt_helper = document.getElementById("prnt_helper");
    prnt_helper.innerHTML = textarea.value;
    window.print();
    prnt_helper.innerHTML = "";
    textarea.focus();
}

function Help() { // Help
    var help = document.getElementById("help_content"),
        overlay = document.getElementById("overlay");

    function closeHelpAndOverlay() {
        help.setAttribute("aria-hidden", "true");
        overlay.setAttribute("aria-hidden", "true");
        textarea.focus();
    }

    if (help["aria-hidden"] === "true") {
        closeHelpAndOverlay();
    } else {
        help.setAttribute("aria-hidden", "false");
        overlay.setAttribute("aria-hidden", "false");
        textarea.blur();
        document.getElementById("overlay").onclick = function() {
            closeHelpAndOverlay();
        };
        // document.onkeydown = function(e) { // esc to close help
        //     if (e.keyCode === 27 || e.which === 27) {
        //         closeHelpAndOverlay();
        //     }
        // };
    }
}

// Confirm close
window.onbeforeunload = function() {
    if (isModified) {
        return "You have unsaved changes that will be lost.";
    }
};

// Keyboard shortcuts
document.onkeydown = function(e) {
    var key = e.keyCode || e.which;
    if (e.ctrlKey) {
        if (e.altKey) {
            if (key === 78) {
                // Ctrl+Alt+N
                New();
            }
        }
        if (key === 79) {
            // Ctrl+O
            e.preventDefault();
            Open();
        }
        if (key === 83) {
            // Ctrl+S
            e.preventDefault();
            Save();
        }
        if (key === 80) {
            // Ctrl+P
            e.preventDefault();
            Print();
        }
        if (key === 191) {
            // Ctrl+/
            e.preventDefault();
            Help();
        }
    }
    if (e.keyCode === 9 || e.which === 9) { // tab
        e.preventDefault();
        var s = textarea.selectionStart;
        textarea.value = textarea.value.substring(0, textarea.selectionStart) + "\t" + textarea.value.substring(textarea.selectionEnd);
        textarea.selectionEnd = s + 1;
    }
};

You can optimize some queries.

var selected_file = document.getElementById("selected_file")

Unless your HTML changes considerably (replacing nodes periodically), every call to getElementById() will return the same node. You can access the node more efficiently if you cache the node in a local variable, and use the same local variable throughout your code, instead of looking it up multiple times.

Same with:

// Help
var help = document.getElementById("help_content"),
    overlay = document.getElementById("overlay");

As long as all these elements exist when your code initializes, you can lookup and store them in variables once, instead of each time Help() (etc) is called. In your code, the performance increase will be very minor, but it can add up in other situations.

In your keydown block:

if (key === 79) {
...
if (key === 83) {
...

// function rename()
(filename.lastIndexOf(".txt") === -1)

You can shave a few bytes by using == instead of ===. Since you're comparing primitives, not objects, == and === function equivalently.

Using === (instead of ==) is a good practice when you're not sure, but it is unnecessary in the places you are using it.

It looks like you didn't update your last check in the keydown callback

if (e.keyCode === 9 || e.which === 9) { // tab

Can be rewritten as:

if (key == 9) { // tab

function confirmNav() { // Confirm navigation
  if (isModified) {
      var a = window.confirm("You have unsaved changes that will be lost.");
      if (a) {
          isModified = false;
      ...

While nothing is technically wrong here, I'd either like to see the confirm inlined into the if() or a more descriptive variable name. a is meaningless.

 if (window.confirm("You have unsaved changes that will be lost.")) {
 // or
 var leavePage = window.confirm("You have unsaved changes that will be lost.");
    if (leavePage) {

Your code only has three long lines. The longest, at 136 chars:

var s = textarea.selectionStart
textarea.value = textarea.value.substring(0, textarea.selectionStart) + "\t" + textarea.value.substring(textarea.selectionEnd);
textarea.selectionEnd = s + 1;

I compacted it be using the local you already declared, and by adding a second. Now 97 chars:

var sStart = textarea.selectionStart,
    txt = textarea.value;
textarea.value = txt.substring(0, sStart) + "\t" + txt.substring(textarea.selectionEnd);

I don't think s was a bad variable name (s for selection or start is meaningful), but sStart is more clear.

You wrote some lovely, readable, and coherent JavaScript.

Here is everything:

var appname = " - simple notepad",
    textarea = document.getElementById("textarea"),
    help = document.getElementById("help_content"),
    overlay = document.getElementById("overlay"),
    selected_file = document.getElementById("selected_file"),
    prnt_helper = document.getElementById("prnt_helper"),
    untitled = "untitled.txt" + appname,
    filename = "*.txt",
    isModified;

document.title = untitled;

textarea.onpaste = textarea.onkeypress = function() {
    isModified = true;
};

function confirmNav() { // Confirm navigation
    if (isModified) {
        var leavePage = window.confirm("You have unsaved changes that will be lost.");
        if (leavePage) {
            isModified = false;
        }
    }
}

function New() { // New
    confirmNav();
    if (!isModified) {
        textarea.value = "";
        document.title = untitled;
    }
    textarea.focus();
}

function Open() { // Open
    confirmNav();
    if (!isModified) {
        selected_file.click();
    }
}

function loadFileAsText() { // load file as text
    var file = selected_file.files[0],
        fileReader = new FileReader();
    fileReader.onloadend = function(e) {
        if (e.target.readyState == FileReader.DONE) {
            filename = file.name;
            document.title = filename + appname;
            textarea.value = e.target.result;
            textarea.focus();
        }
    };
    fileReader.readAsText(file);
}

function rename() { // Rename
    filename = window.prompt("Name this note:", filename);
    if (filename) {
        filename = (filename.lastIndexOf(".txt") == -1) ? filename + ".txt" : filename;
        document.title = filename + appname;
    }
}

function Save() { // Save
    rename();
    if (filename) {
        var blob = new Blob([textarea.value], {
            type: "text/plain;charset=utf-8"
        });
        window.saveAs(blob, filename);
        isModified = false;
    }
    textarea.focus();
}

function Print() { // Print
    prnt_helper.innerHTML = textarea.value;
    window.print();
    prnt_helper.innerHTML = "";
    textarea.focus();
}

function Help() { // Help
    function closeHelpAndOverlay() {
        help.setAttribute("aria-hidden", "true");
        overlay.setAttribute("aria-hidden", "true");
        textarea.focus();
    }

    if (help["aria-hidden"] == "true") {
        closeHelpAndOverlay();
    } else {
        help.setAttribute("aria-hidden", "false");
        overlay.setAttribute("aria-hidden", "false");
        textarea.blur();
        overlay.onclick = function() {
            closeHelpAndOverlay();
        };
        // document.onkeydown = function(e) { // esc to close help
        //     if (e.keyCode == 27 || e.which == 27) {
        //         closeHelpAndOverlay();
        //     }
        // };
    }
}

// Confirm close
window.onbeforeunload = function() {
    if (isModified) {
        return "You have unsaved changes that will be lost.";
    }
};

// Keyboard shortcuts
document.onkeydown = function(e) {
    var key = e.keyCode || e.which;
    if (e.ctrlKey) {
        if (e.altKey) {
            if (key == 78) {
                // Ctrl+Alt+N
                New();
            }
        }
        if (key == 79) {
            // Ctrl+O
            e.preventDefault();
            Open();
        }
        if (key == 83) {
            // Ctrl+S
            e.preventDefault();
            Save();
        }
        if (key == 80) {
            // Ctrl+P
            e.preventDefault();
            Print();
        }
        if (key == 191) {
            // Ctrl+/
            e.preventDefault();
            Help();
        }
    }
    if (key == 9) { // tab
        e.preventDefault();
        var sStart = textarea.selectionStart,
            txt = textarea.value;
        textarea.value = txt.substring(0, sStart) + "\t" + txt.substring(textarea.selectionEnd);
        textarea.selectionEnd = sStart + 1;
    }
};


Z^m number system in Rust A \$Z^m\$ number system includes integers in the interval \$[0, m)\$ when \$m > 0\$ or \$(m, 0]\$ when \$m < 0\$. The code defines a trait Mod to represent numbers in this system and the +, -, and * operator for it.

use std::ops::Add;
use std::ops::Mul;
use std::ops::Sub;

struct Mod<T: Modulo<T>> {
    modulo: T,
    i: T,
}

trait Modulo<T> {
    fn modulo(&self, n: T) -> T;
}

impl Modulo<i32> for i32 {
    fn modulo(&self, n: i32) -> i32 {
        let mut x: i32 = *self;
        while x.signum() != n.signum() {
            x += n;
        }
        x % n
    }
}

impl Mod<i32> {
    fn new(modulo: i32, i: i32) -> Mod<i32> {
        let n = i.modulo(modulo);
        Mod {
            modulo: modulo,
            i: n,
        }
    }
}

impl Add for Mod<i32> {
    type Output = Mod<i32>;
    fn add(self, other: Mod<i32>) -> Mod<i32> {
        Mod::new(self.modulo, self.i + other.i)
    }
}

impl Sub for Mod<i32> {
    type Output = Mod<i32>;
    fn sub(self, other: Mod<i32>) -> Mod<i32> {
        Mod::new(self.modulo, self.i - other.i)
    }
}

impl Mul for Mod<i32> {
    type Output = Mod<i32>;
    fn mul(self, other: Mod<i32>) -> Mod<i32> {
        Mod::new(self.modulo, self.i * other.i)
    }
}

fn main() {
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    println!("{}", (x + y).i);
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    println!("{}", (x - y).i);
    let x = Mod::new(-5, 3);
    let y = Mod::new(-5, 8);
    println!("{}", (x * y).i);
}

All suggestions are welcome, but I am particularly interested in:

Extending to u32, u16, u8, i16, i8 etc without too much code duplication.
Increasing performance.

I would hesitate to implement Mod<T> * Mod<T>, and instead implement Mod<T> * T (and vice-versa). This is because you make no attempt to preserve the rhs' modulo, so you probably shouldn't have one there.

Consider the code

let mut x: i32 = *self;
while x.signum() != n.signum() {
    x += n;
}
x % n

That while loop makes this operation \$\mathcal{O}(n)\$, whereas a simpler implementation

((x % n) + n) % n

is \$\mathcal{O}(1)\$.

Performance elsewhere is probably uninteresting, since it devolves to basic integer operations.

Extending to other T should probably be done by being generic over the Mod and Add traits, as well as whatever particular operation is needed to implement. Something like

impl<T> Sub<T> for Mod<T>
    where T: Add<T> + Mod<T> + Sub<T>
{
    type Output = Mod<T>;
    fn sub(self, other: T) -> Mod<T> {
        Mod::new(self.modulo, self.i - i)
    }
}

The Add and Mod are needed by Mod::new, the Sub is needed inside the sub call.


Rock/Paper/Scissors using C# I created a Rock/Paper/Scissors game in C# that asks the player the number of rounds he wants to play, and then calculates the points and determines the winner.
Is there a way I can fix my code? Is it appropriate to use switch and if/else together?

Console.Write("How many rounds do you want to play ? : ");
int numberOfRounds = Convert.ToInt32(Console.ReadLine());

Random rnd = new Random();
string[] hands = { "Rock", "Paper", "Sci" };
    //               0       1        2

int playerPoints = 0;
int computerPoints = 0;

for (int counter = 1; counter <= numberOfRounds; counter++)
{
    Console.Write("Player Hand : ");
    string playerHand = Console.ReadLine();
    int randomNumber = rnd.Next(0, 3);
    string computerHand = hands[randomNumber];
    Console.WriteLine("Computer Hand : " + computerHand);

    switch (playerHand)
    {
        case "Rock":
            if (computerHand == "Rock")
            {
                playerPoints++;
                computerPoints++;
                Console.WriteLine("Player And Computer Win a Point");
                Console.WriteLine();
            }
            if (computerHand == "Paper")
            {
                computerPoints++;
                Console.WriteLine("Computer wins a point");
                Console.WriteLine();
            }
            if (computerHand == "Sci")
            {
                playerPoints++;
                Console.WriteLine("Player wins a point");
                Console.WriteLine();
            }
            break;

        case "Paper":
            if (computerHand == "Paper")
            {
                playerPoints++;
                computerPoints++;
                Console.WriteLine("Player and Computer Win a Point");
                Console.WriteLine();
            }
            if (computerHand == "Sci")
            {
                computerPoints++;
                Console.WriteLine("Computer wins a point");
                Console.WriteLine();
            }
            if (computerHand == "Rock")
            {
                playerPoints++;
                Console.WriteLine("Player wins a point");
                Console.WriteLine();
            }
            break;

        case "Sci":
            if (computerHand == "Sci")
            {
                playerPoints++;
                computerPoints++;
                Console.WriteLine("Player and Computer Win a Point");
                Console.WriteLine();
            }
            if (computerHand == "Rock")
            {
                computerPoints++;
                Console.WriteLine("Computer wins a point");
                Console.WriteLine();
            }
            if (computerHand == "Paper")
            {
                playerPoints++;
                Console.WriteLine("Player wins a point");
                Console.WriteLine();
            }
            break;
    }
}

Console.WriteLine("*** RESULTS ***");
Console.WriteLine("Player Points : " + playerPoints);
Console.WriteLine("Computer Points : " + computerPoints);

if (playerPoints > computerPoints)
{
    Console.WriteLine("Player Wins");
}
else
{
    if (computerPoints > playerPoints)
    {
        Console.WriteLine("Computer Wins");
    }
    else
    {
        Console.WriteLine("Equal None Win");
    }
}

Console.ReadLine();

My take on this:

For console apps, I like to use a more OOP approach.

A Message class to hold a string array to handle a multiline message, and if the message requires a response a string of the characters allowed, will allow you to replace all those string literals with named variables:

class Message
{
    public String[] message { get; }
    public String choices { get; }
    public Message()
    {
        message = new String[0];
        choices = "";
    }
    public Message(String choices, params String[] message)
    {
        this.message = message;
        this.choices = choices.ToLower();
    }
}

To compliment this class, a static storage class(Messages) to put names to the various messages that will be required.  I put a simple print method in there as well:

static class Messages
{
    public static Message startMessage = new Message("", "Welcome to RockScissorPaper");
    public static Message instructions = new Message("", "When you're ready to play, press the desired number.",
            "The computer will reveal its choice and who won.");
    public static Message rockWins = new Message("", "Rock breaks Scissors.", "Rock wins.");
    public static Message scissorsWins = new Message("", "Scissors cuts Paper.", "Scissors wins.");
    public static Message paperWins = new Message("", "Paper covers Rock.", "Paper wins.");
    public static Message playAgain = new Message("yn\n", "Would you like to play again?(y/n)");
    public static Message continuePlay = new Message("", "Press any key to continue.");
    public static Message choices = new Message("0123", "1 - Rock", "2 - Scissors", "3 - Paper", "0 - Quit", "\nWhich do you choose?");
    public static Message goodbye = new Message("", "Thank you for playing.  Goodbye.");
    public static Message tie = new Message("", "A tie.  You both chose the same.");
    public static Message wrongChoice = new Message("", "You made a wrong choice.  Try again.");

    public static void PrintMessage(TextWriter tOut, Message message)
    {
        tOut.WriteLine();
        foreach (var s in message.message)
        {
            tOut.WriteLine(s);
        }
    }
}

This approach makes it very easy, with minimal refactoring, to create language choices for your app.

For classes that require I/O, I like to keeps things flexible and use base classes instead of hard coding standard I/O.  This makes it very easy to leverage networking at some point.

For the game class(RockScissorPaper), a method to get the user's input, clears a lot of code from the PlayGame method.

private static char GetChoice(TextWriter tOut, TextReader tIn, Message message)
{
    char choice = '\0';
    bool done = false;
    do
    {
        Messages.PrintMessage(tOut, message);
        string response = tIn.ReadLine().ToLower();
        choice = response.Length > 0 ? response[0] : '\n';
        if (message.choices.Contains(choice))
        {
            return choice;
        }
        Messages.PrintMessage(tOut, Messages.wrongChoice);
    } while (!done);
    return choice;
}

I find an enum for the different states of the game, helps with the decision process to decide a winner:

private enum State
{
    None = 0,
    Tie = 1,
    Rock = 3,
    Scissors,
    Paper
}

Notice the Rock constant starts at 3. This allows simple math and one conditional to decide which player is the winner. Once the winner is decided the switch block to select which message to display is very simple:

private static Message GetWinner(State comp, State player)
{
    if (comp == State.None || player == State.None)
    {
        throw new ArgumentOutOfRangeException("Both 'comp' and 'player' must be 'Rock', 'Scissors' or 'Paper'.");
    }
    int cmptr = (int)comp;
    int plyr = (int)player;
    State winner = State.None;
    if (player == comp)
    {
        winner = State.Tie;
        plyrScore++;
        compScore++;
    }
    else if(plyr % 3 == (cmptr - 1) % 3)
    {
        winner = player;
        plyrScore += 2;
    }
    else
    {
        winner = comp;
        compScore += 2;
    }
    switch (winner)
    {
        case State.Tie:
            return Messages.tie;
        case State.Rock:
            return Messages.rockWins;
        case State.Scissors:
            return Messages.scissorsWins;
        case State.Paper:
            return Messages.paperWins;
        default:
            return new Message("", "");
    }
}

I've incorporated the score calculation here, as well.  I noticed you scored a tie  the same a win.  Typically a tie is considered half a win.  I would prefer to see a win at 2 points and a tie at 1.

With these helper methods done, the game play is almost trivial to code:

static class RockScissorsPaper
{
    private static RandomEngine rnd = new RandomEngine();
    private enum State
    {
        None = 0,
        Tie = 1,
        Rock = 3,
        Scissors,
        Paper
    }
    static int plyrScore = 0;
    static int compScore = 0;
    public static void PlayGame(TextWriter tOut, TextReader tIn, String name, int rounds)
    {
        plyrScore = 0;
        compScore = 0;
        Messages.PrintMessage(tOut, Messages.startMessage);
        for(int round = 0; round < rounds; ++round)
        {
            Messages.PrintMessage(tOut, Messages.instructions);
            int choice = GetChoice(tOut, tIn, Messages.choices) - '0';
            State compChoice = (State)Enum.GetValues(typeof(State)).GetValue((rnd.Next(0, 300) % 3) + 2);
            State playerChoice = (State)(choice + 2);
            tOut.WriteLine($"The player chose {playerChoice}.\nThe computer chose {compChoice}.");
            Messages.PrintMessage(tOut, GetWinner(compChoice, playerChoice));
            tOut.WriteLine($"The score is:\n{name} - {plyrScore}\nComputer - {compScore}");
            Messages.PrintMessage(tOut, Messages.continuePlay);
            tIn.Read();
        }

    }

    private static char GetChoice(TextWriter tOut, TextReader tIn, Message message)
    {
        char choice = '\0';
        bool done = false;
        do
        {
            Messages.PrintMessage(tOut, message);
            string response = tIn.ReadLine().ToLower();
            choice = response.Length > 0 ? response[0] : '\n';
            if (message.choices.Contains(choice))
            {
                return choice;
            }
            Messages.PrintMessage(tOut, Messages.wrongChoice);
        } while (!done);
        return choice;
    }
    private static Message GetWinner(State comp, State player)
    {
        if (comp == State.None || player == State.None)
        {
            throw new ArgumentOutOfRangeException("Both 'comp' and 'player' must be 'Rock', 'Scissors' or 'Paper'.");
        }
        int cmptr = (int)comp;
        int plyr = (int)player;
        State winner = State.None;
        if (player == comp)
        {
            winner = State.Tie;
            plyrScore++;
            compScore++;
        }
        else if(plyr % 3 == (cmptr - 1) % 3)
        {
            winner = player;
            plyrScore += 2;
        }
        else
        {
            winner = comp;
            compScore += 2;
        }
        switch (winner)
        {
            case State.Tie:
                return Messages.tie;
            case State.Rock:
                return Messages.rockWins;
            case State.Scissors:
                return Messages.scissorsWins;
            case State.Paper:
                return Messages.paperWins;
            default:
                return new Message("", "");
        }
    }
}


Minimal, very simple, implementation of an HTTP server for static content, using only core node.js modules To practice the very basics of node back-end, I'm implementing the most basic server possible for static content, using only the core node modules:

http
fs
path

With the following server requirements:

Can serve any http file inside the public directory.
Accepts only GET requests, 404 for anything else.
Accepts only .html extensions, 404 for anything else.
Use public/not-found.html for 404s.

These requirements are arbitrary, just to include some very basic logic in the request and response handling.
The working solution I'm presenting for code review is the following:
const fs = require('fs')
const http = require('http')
const path = require('path')

const hostname = 'localhost'
const port = 3000
const runLog = `Server running ar http://${hostname}:${port}`
const rootPath = '../public'
const errorPath = `${rootPath}/not-found.html`

const server = http.createServer((req, res) => {
  const fileUrl = req.url === '/' ? '/index.html' : req.url
  const filePath = path.resolve(`${rootPath}${fileUrl}`)
  res.setHeader('Content-Type', 'text/html')

  if (reqIsOk(req, filePath)) {
    res.statusCode = 200
    fs.createReadStream(filePath).pipe(res)
  } else {
    res.statusCode = 404
    fs.createReadStream(errorPath).pipe(res)
  }
})

function reqIsOk (req, filePath) {
  const fileExt = path.extname(filePath)
  return req.method === 'GET' && fileExt === '.html' && fs.existsSync(filePath)
}

server.listen(port, hostname, () => {
  console.log(runLog)
})

I'm looking for all kind of feedback, from style, code smells, anti-patterns, anything that can be improved is very welcome.

Are the fs and path packages needed? Do they simplify things enough to justify the inclusion?
Can this be written more concisely?
Is the ternary operator justified? Would you keep it here, or is an if construct preferred?

Are the fs and path packages needed? Do they simplify things enough to justify the inclusion?

fs is needed. How else would you get your data down the line without reading it in? path could be replaced with your own path utility library, but I wouldn't waste time reinventing that when a built-in one already exists.

Can this be written more concisely?

This is probably as concise as you can get it. Any further would probably make it unreadable.

Is the ternary operator justified? Would you keep it here, or is an if construct preferred?

Yep, ternary is fine. It's the check that might need improvement. That's because you're only tacking on index.html if the path is /. In other web servers, any path that ends in / assumes that index.html will be tacked on.
const filePath = path.resolve(`${rootPath}${fileUrl}`)

So one of the dangers when writing a web server is directory traversal. It's when your path resolver (in your case, this line) resolves to a path outside the web directory, and your server just willingly serves it. You'll need to check if the path resolved is still in your web directory.
res.setHeader('Content-Type', 'text/html')

function reqIsOk (req, filePath) {
  const fileExt = path.extname(filePath)
  return req.method === 'GET' && fileExt === '.html' && fs.existsSync(filePath)
}

A static server can be more than just HTML. Replace the hardcoded text/html with a value coming from a map of file extension to mimetype.


Multiple types of contacts I have a list of contacts, classified into categories. Each category is represented with a different class.

Base class:

class Contact
{
    public ushort Id { get; private set; }
    public Phone Phone { get; private set; }

    public Contact(ushort id, Phone phone)
    {
        Id = id;
        Phone = phone;
    }
}

And here are the categories:

PrivateServiceProvider (meaning - private person):

class PrivateServiceProvider : Contact
{
    public string FirstName { get; private set; }
    public string LastName { get; private set; }
    public ServiceField ServiceField { get; private set; }

    public PrivateServiceProvider(ushort id, string firstName, string lastName,
                                  ServiceField serviceField, Phone phone)
        : base(id, phone)
    {
        FirstName = firstName;
        LastName = lastName;
        ServiceField = serviceField;
    }
}

BusinessServiceProvider:

class BusinessServiceProvider : Contact
{
    public string Name { get; private set; }
    public ServiceField ServiceField { get; private set; }
    public Email Email { get; private set; }

    public BusinessServiceProvider(ushort id, string name,
                                   ServiceField serviceField, Phone phone, Email email)
        : base(id, phone)
    {
        Name = name;
        ServiceField = serviceField;
        Email = email;
    }
}

PrivateGenericContact:

class PrivateGenericContact : Contact
{
    public string FirstName { get; private set; }
    public string LastName { get; private set; }

    public PrivateGenericContact(ushort id, string firstName, string lastName, Phone phone)
        : base(id, phone)
    {
        FirstName = firstName;
        LastName = lastName;
    }
}

BusinessGenericContact:

class BusinessGenericContact : Contact
{
    public string Name { get; private set; }
    public Email Email { get; private set; }

    public BusinessServiceProvider(ushort id, string name, Phone phone, Email email)
        : base(id, phone)
    {
        Name = name;
        Email = email;
    }
}

It works, but how could I improve it1?

Also, it drives me CRAZY that the names of the classes are so lengthy. I think it's a smell but I am not sure what to do about it. If some day I decide to create another specification of Contact, let's say - for people who lives in the city versus in the country2, will I call the class CountryPrivateServiceProvider? It's NUTS.

An example of use is when the user creates a contact. So he chooses the type of contact and inserts all of the information into a form (a View - I am using MVP). Then the View notify the Presenter that the user pressed the "Save" and creates an object of the selected type. Then the Presenter ask the Model to insert the information into the database.

How can I improve my code, both generally and specifically to this naming problem?

1. They are deliberately immutable.

2. Apparently, I am not good with examples.

I find those classes should but just simple POCO classes with get/set properties. They could sit either inside a DTO or Model namespace or could have a Dto suffix and simple names like Person or PrivatePerson or BusinessPerson. They are not service providers as you call them.

However, you do want to differentiate between a PrivateCountry and a BusinessCountry. You could do this by adding the type of operation you need them for, like PrivateCountryForAddingDto or BusinessCountryForUpdatingDto. With this pattern you will have a few more models but the good thing is that each command will have its own independent model that can have its own validation logic. A model for adding does not necessarily have to have the same requirements as a model for updating. Some fields might be optional in one case and mandatory in another case.


Simple slack bot in python I wrote a simple slack bot, which is able to save links (command: "save name link") and print them (command: "get name"). Optionally, a delay in minutes and seconds can be specified (command: "get name XmXs"). I would appreciate any feedback.

import os
import re
from time import sleep
from threading import Thread
from slackclient import SlackClient

BOT_NAME = 'klausbot'
BOT_ID = ''
slack_client = SlackClient(os.environ['SLACK_BOT_TOKEN'])

links = {}
save_pattern = re.compile('^save\s(?P<name>[a-zA-Z]+)\s(?P<link>\S+)$')
get_pattern = re.compile('^get\s(?P<name>[a-zA-Z]+)\s?((?P<minutes>\d+)m)?\s?((?P<seconds>\d+)s)?$')

def get_bot_id():
    api_call = slack_client.api_call('users.list')
    if api_call.get('ok'):
        users = api_call.get('members')
        for user in users:
            if 'name' in user and user.get('name') == BOT_NAME:
                global BOT_ID
                BOT_ID = '<@{}>'.format(user.get('id'))

def parse_message(message):
    text = message['text']
    if text.startswith(BOT_ID):
        text = text.split(BOT_ID)[1].strip().lower()
        if save_pattern.match(text):
            handle_save_command(message, save_pattern.match(text))
        elif get_pattern.match(text):
            handle_get_command(message, get_pattern.match(text))
        else:
            post_message('Command not found', message['channel'])

def handle_save_command(message, match):
    name = match.group('name')
    link = match.group('link')
    links[name] = link
    response = '{} saved to {}'.format(link, name)
    post_message(response, message['channel'])

def calculate_time(match):
    minutes = match.group('minutes') or 0
    seconds = match.group('seconds') or 0
    return int(minutes) * 60 + int(seconds)

def handle_get_command(message, match):
    name = match.group('name')
    if name not in links:
        error_message = '{} not found'.format(name)
        post_message(error_message, message['channel'])
    else:
        sleep_time = calculate_time(match)
        sleep(sleep_time)
        post_message(links[name], message['channel'])

def handle_messages_read(messages):
    if messages and len(messages) > 0:
        for message in messages:
            if message and 'text' in message:
                parse_message(message)

def post_message(message, channel='#general'):
    slack_client.api_call(
        'chat.postMessage',
        channel=channel,
        text=message,
        as_user=True
    )

if __name__ == '__main__':
    if slack_client.rtm_connect():
        print('KlausBot started and running')
        get_bot_id()
        while True:
            messages = slack_client.rtm_read()
            handle_message_thread = Thread(target=handle_messages_read, args=(messages,))
            handle_message_thread.start()
            sleep(0.1)
    else:
        print('Could not connect')

In general this code looks pretty good.  I do have a couple of small pythonism comments:

dict.get() returns None if key not present:

This:

if 'name' in user and user.get('name') == BOT_NAME:

Can simply be this:

if user.get('name') == BOT_NAME:

... since dict.get() will return None if the key is not present and I presume BOT_NAME will never be None.

Iterating over 0 length item:

This:

if messages and len(messages) > 0:
    for message in messages:

Can simply be this:

if messages:
    for message in messages:

... since the for loop will not have anything to do if the length is zero.

Also, I am not sure what this if is testing for:

if messages:
    for message in messages:

If guarding against None, then it would be more pythonic as:

if messages is not None:
    for message in messages:

And if the check is a surrogate length check, then it isn't necessary at all, and the if could be removed.


MVC flash card application I am trying to learn how to build a very basic MVC structure before jumping on rails and other frameworks. I am working from a .txt file, and I organized a project into four parts: a model, a view, a controller, and a runner.

.txt file pattern:

line1 -> "question"
line2 -> "answer"
line3 -> "" #blank line
line4 -> "question"
line5 -> "answer"
line6 -> ""

and so on.

runner.rb:

require_relative 'view'
require_relative 'model'
require_relative 'controller'

file = 'flashcard_samples.txt'
go = Controller.new(file)

view.rb

require_relative 'model'

class View
  def initialize(question)
    @question = question
    puts "Question: " + question
  end
end

controller.rb

require_relative 'model'

class Controller
  def initialize(filename)
    a = Model.new(filename)
    a.play
  end
end

model.rb

require_relative 'view'

class Model
  attr_reader :data, :questions, :answers, :punchline

  def initialize(filename)
    @data = []
    @questions = []
    @answers = []
    @score = 0
    @file = File.open(filename)
  end
  def play
    welcome_message
    parse
    startquestions
  end
  def parse
    @file.each_line do |line|
      @data << line
    end
  end
  def checkanswer
    @goodanswer = false
    @oneanswer == @useranswer ? @goodanswer = true && @score += 1: @goodanswer = false && score += 0
  end
  def displaypunchline
    if @goodanswer == true
      puts "well done"
    else
      puts "the good answer was #{@oneanswer}"
    end
  end
  def displayscore
    puts "Your score is now: " + @score.to_s
  end
  def display_separation
    puts "--------------------------------------------------------------------------"
  end
  def getuserinput
    @useranswer = gets.chomp
  end
  def crazygameloop
    @counter = 1
    loop do
      break if @counter >= @data.length - 1
        @questions = @data.unshift[@counter-1].tr("\n","")
        @oneanswer = @data.unshift[@counter].tr("\n","")
        @onequestion = View.new(@questions)
        @data.unshift[@counter+1]
        @counter += 3
      getuserinput
      checkanswer
      displaypunchline
      displayscore
      display_separation
    end
  end
  def startquestions
    crazygameloop
  end
  def welcome_message
    puts "welcome in my shitty game"
    puts "|||||||||||||||||||||||||"
  end
end

So far it's working but it's un-DRY, not very readable and probably a bad/incomplete MVC implementation. I'm looking for advice on how I should re-organize my model in order to increase my view responsibilities and optimize my controller in case I would add other features. Any input appreciated.

All the parts are here, but your model is taking on too much work. First, let's tackle one of the three hardest things in computer programming: Naming things.

Naming Classes in an MVC Application

You have three classes, called aptly Controller, View and Model. I know you are trying to learn the MVC pattern, but an important aspect or learning this pattern is that names should make sense beyond telling you which layer in MVC they go. A convention for naming things is a must.

The "Rails" way of naming things all starts with the "model". In your case, your Model class contains data about a "quiz", so Quiz is a perfect name for that class. The controller that handles user interaction for Quiz objects should be called QuizesController -- note that "Quiz" is pluralized, because the controller object handles all quizes, not just one.

After that, a QuizView seems like a good name for the view object.

Really if you think about it, a "quiz" is composed of two other things: quiz questions and quiz answers. We need to properly model a "quiz" before anything else. The Model layer in MVC is the foundation -- the bedrock of your application.

Know Your "Domain:" Properly Modeling Your Quiz

We actually have a need for 3 model classes:

Quiz
QuizQuestion
QuizAnswer

A quiz has one or more questions. An answer requires a question. We need to model this in Ruby.

class Quiz
  attr_reader :questions

  def initialize(questions)
    @questions = questions
    @grade = 0
  end

  def answer_question(question, answer_text)
    question = questions[question_number - 1]
    question.answer answer_text
  end

  def grade
    return @grade if @grade > 0

    number_correct = 0

    questions.each do |question|
      number_correct += 1 if question.answered_correctly?
    end

    @grade = calculate_grade number_correct
  end

  def retake
    @grade = 0

    questions.each do |question|
      question.remove_answer
    end
  end

  def quesion_count
    questions.count
  end

private

  def calculate_grade(number_correct)
    number_correct / questions.count
  end

end

A Quiz is a tad more complex, but it just needs a list of QuizQuestions in its constructor. All other operations on the quiz are public methods, some of which are delegated to the QuizQuestion class.

class QuizQuestion
  attr_reader :text, :answer, :expected_answer

  def initialize(text, expected_answer)
    @text = text
    @expected_answer = expected_answer
  end

  def answer(answer_text)
    @answer = QuizAnswer.new self, answer_text
  end

  def answered_correctly?
    return @answer.nil? ? false : @answer.correct?
  end

  def remove_answer
    @answer = nil
  end
end

The QuizQuestion class glues the quiz together with the answer, which we will see in a moment. Again, the constructor doesn't do much, except take in the data it needs in order to exist: The question text and the expected answer. The answer method creates the QuizAnswer object and returns it. The logic of "retaking" a quiz is also delegated to the remove_answer method.

class QuizAnswer
  attr_reader :question, :text

  def initialize(question, text)
    @question = question
    @text = text
  end

  def correct?
    return question.expected_answer == text
  end

  def question_text
    question.text
  end
end

Lastly the QuizAnswer class glues together the quiz question and the user's answer, along with the logic for testing the answer.

Now that we've properly modeled our "domain", the Quiz, QuizQuestion and QuizAnswer classes become our Domain Models combining data with business logic (taking a quiz). Next, we will take a step up from the "foundation" of our application to explore the data access layer, or "Repository".

The Data Access Layer (Repository)

The Repository Pattern decouples the data access from your model and controller. What your controller needs is a QuizRepository object that does all the data access.

Since you are going with flat file storage, we want to extract this behavior into its own layer.

class QuizRepository
  def initialize(filename)
    @filename = filename
  end

  def all
    return @quizes unless @quizes.nil?
    load
    @quizes
  end

  def find(index)
    return nil if index < 0 || index > all.count

    return all[index]
  end

  def reload
    @quizes = nil
  end

private
  def filename
    @filename
  end

  def load
    questions = []
    index = 0
    question_text = ""
    expected_answer = ""
    file = File.open filename

    file.each_line do |line|
      if index % 2 == 0
        question_text = line
      else
        expected_answer = line
        questions << QuizQuestion.new question_text, expected_answer
      end

      index += 1
    end

    @quizes << Quiz.new questions
  end

end

The QuizRepository class needs a filename, and it lazy loads the quiz objects only when you need to get them.

Notice that there is no console interaction. This should be encapsulated by the "Controller".

Handling User Interaction and Displaying Information

Knowledge of the console, and what to display is the realm of the controller and views.

class QuizesController
  def initialize(quiz_repository)
    @quiz_repository = quiz_repository
  end

  def run
    counter = 1
    quiz = @quiz_repository.all.first

    puts "Welcome to the Quiz!"
    puts "--------------------"

    quiz.questions.each do |question|
      puts QuizQuestionView.new question, counter
      answer_text = gets.chomp
      answer = quiz.answer_question question, answer_text
      puts QuizAnswerView.new quiz, answer
      counter += 1
    end
  end
end

The main loop is inside the controller, since the controller in MVC responds to user input. The controller needs a "quiz repository" which is the only argument in its constructor. You'll also notice there are 2 views: QuizQuestionView and QuizAnswerView.

class QuizQuestionView
  def initialize(question, question_number)
    @question = question
    @question_number = question_number
  end

  def to_s
    "Question \##{@question_number}: #{@question.text}"
  end
end

class QuizAnswerView
  def initialize(quiz, answer)
    @quiz = quiz
    @answer = answer
  end

  def to_s
    text = if answer.correct?
             "well done"
           else
             "the correct answer is #{answer.question_text}"
           end

    text + "\nYour score is now #{@quiz.grade}"
         + "\n--------------------------------------------------------------------------"
  end
end

Also notice that there are no calls to gets or puts in the "view" classes. These classes are responsible for output only. The fact that you are outputting to a console is the responsibility of the Controller, since the Controller knows you are interacting with the user via the console. The to_s method is defined, which returns a string that the Controller then prints to the standard output of the console.

Putting All The Layers Together

Now we actually need to run the program:

#!/bin/env ruby

quiz_repository = QuizRepository.new "/data/quiz.txt"
controller = QuizController.new quiz_repository
controller.run

The whole application is started, run and exits in three lines of code.