Light Python pubsub lib, but is it too light? In attempting to decouple the object from the messaging I came up with the following short code for adding publish-subscribe functionality to a Python script.

from collections import defaultdict

Topics = defaultdict(set)

def subscribe(obj, message):
    """Adds key-value pairs to the Topics dict.
    Keys are messages, values are placed in a set."""
    Topics[message].add(obj)

def unsub(obj, message):
    """Removes a subscriber from a message."""
    Topics[message].remove(obj)

def unsub_all(obj):
    for message in Topics:
        if obj in Topics[message]:
            Topics[message].remove(obj)

def publish(message, *args, **kwargs):
    """Sends message to subscribers."""
    for obj in Topics[message]:
        getattr(obj, message)(*args, **kwargs)

def publish_with_results(message, *args, **kwargs):
    """Same as publish, but returns list of results."""
    return [getattr(obj, message)(*args, **kwargs) for obj in Topics[message]]

There's no Message or Signal object; objects in question need only (a) be subscribed, and (b) have a matching method name. So for example:

from stickpubsub import subscribe, publish

class A(object):
    def __init__(self, x):
        self._x = x

    @property
    def x(self):
        return self._x

    @x.setter
    def x(self, value):
        self._x = value
        publish('x_was_set', self._x)

class B(object):
    def __init__(self):
        subscribe(self, 'x_was_set')

    def x_was_set(self, value):
        if getattr(self, 'last_x', False):
            print "The last x I saw was {}.".format(self.last_x)
        else:
            print "I guess I wasn't around for the last x."
        self.last_x = value

>>> Dave = A(15)
>>> Jupiter = B()
>>> Dave.x = 17
I guess I wasn't around for the last x.
>>> Dave.x *= 2
The last x I saw was 17.

Objects can of course be subscribed to events outside their __init__ call.

This is actually an early version; I've a few different implementations - one that uses proxy objects in the stead of just adding the object to a set, another that separates the message from the methods, and so on. This just seemed like a nice and short example to put out there for the purpose of chewing the fat.

Primarily I'm curious as to what I might be "missing out on" by not expressly having a Message() or Signal() or Event() object, as I see many other 'pubsub example' code snippets employ. There's no attempt to obfuscate the kinds of errors being thrown or to replace one error with a frivolous custom exception, and the whole thing takes like 30 lines and seems to be fairly fast. The only thing I can see creating another object doing is adding another point of failure, but perhaps I'm just naïve!

So what is the upshot of an Event-like object in such a module?  Also, is my code alright?

1. Code review

It's hard to evaluate this kind of thing in the abstract without knowing what the requirements are. However, here are a few thoughts.

There's no top-level documentation. How am I supposed to use this code? You could provide a module-level docstring.
The Python style guide (PEP8) recommends that global variables should be "lowercase, with words separated by underscores". So topics would be preferred to Topics (which looks like the name of a class). You're not obliged to follow PEP8, but it makes it easier for you to collaborate with other Python programmers.
The use of a global variable to store persistent data makes code hard to debug and test. When you have persistent data (like your Topics dictionary) together with a group of functions that operate on the data, it's usually best to organize this into a class.
You have a global variable Topics but the things it contains are called "messages". It would be clearer if you stuck to one word to describe these things.
Your implementation has the constraint that the only thing that can subscribe to a topic is an object with a method that has the same name as the topic. This is likely to be an annoying constraint. For suppose that I have a function f and I want to subscribe it to the topic named "t". Here's one way to do it:

obj = type('_dummy', (object,), {})()
obj.t = f
subscribe(obj, 't')

but this seems like the kind of boilerplate that a well-designed publish/subscribe library would not need.
The implementation of publish_with_results seems dodgy to me. A topic is a set of subscribers (that is, the subscribers have no particular order), but publish_with_results returns a list of results (that is, in a particular order). So there is no way to match up the results with the subscribers.

It's hard to say whether this is really a problem without knowing more about what you would use this for.
Your implementation requires all topics to share the same namespace: they are all string keys in the Topics dictionary. But this means that if different modules want to use your publish/subscribe library, they need to coordinate their choices of names, to avoid clashes.

It would be better to use Python's own namespace mechanism to handle name clashes, by making each topic into its own object, perhaps like this:

class Topic(set):
    """A publish/subscribe topic, implemented as a set of functions.

    Subscribe by adding a function to the topic using the set
    methods. Send a message to the subscribers by calling the publish
    method.

    >>> t = Topic()
    >>> t.add(print)
    >>> t.publish('hello, world!')
    hello, world!

    """

    def publish(self, *args, **kwargs):
        """Publish message by calling all subscribers."""
        for s in self:
            s(*args, **kwargs)

The example code in the docstring can be run using the doctest module.

2. Response to comments

"You say it's harder to debug or test without a class - why?"

When you're testing code you want to start each test case from a known state (not from whatever happens to be left over from the previous test cases). If you're testing code that's encapsulated in a class, then that's easy: you can just create a new instance of the class for each test case and throw it away when you're done. But if you're testing code that uses global variables, you have to explicitly reset all the global variables before starting the next test case.
"Wouldn't each message ideally be unique anyway?"

What I have in mind here is that a program may be made up of several modules or libraries that use the publish/subscribe library. For example, there may be a user interface toolkit that publishes a message when a user presses a button, and there may be a networking module that publishes a message when a networking event takes place.

This would go wrong if these two modules happen to choose the same name for two different events. For example, the user interface toolkit publishes the "login" message when the user presses the "Login" button, and the networking module publishes the "login" message when the user successfully signs in to a remote service. These message names clash and so their subscribers incorrectly see the messages for the other event.

So in this situation there needs to be some coordination in the choice of message names. But these two modules might be written by different programmers who have no idea that they would be combined in the same program.

It would be better not to put all the messages in the same namespace.


Bad code to handle image upload I'm using this form to submit articles into the database:

<form enctype="multipart/form-data" method="post" action="add.php">

  <input name="title" type="text">
  <input name="image" type="file">
  <textarea name="text"></textarea>
  <input type="submit" name="go" value="Spremi">

</form>

include ("database.php");
$upload_path = "files/images/";
$prefix= date("Hi-mdY")."-";

$file_name = $HTTP_POST_FILES['image']['name'];
$file_name = str_replace(' ', '-', $file_name);
$file_name = str_replace('_', '-', $file_name);
$file_name = strtolower($file_name);

$upload_path = $upload_path . basename($prefix.$file_name);
move_uploaded_file($_FILES['image']['tmp_name'], $upload_path);

$final_file_name = ("/files/images/".$prefix.$file_name);
$date=date("Y.m.d");

$sql="INSERT INTO articles (title, image_link,  text, date) VALUES ('$_POST[title]', '$final_file_name', '$_POST[text]', '$date')"
if (mysql_query($sql)){
    echo "done";
} else {
    echo "error
" . mysql_error();
}

And here is the problem:

One of my friends told me that the PHP code in add.php is incorrect.

This is working for me, but can someone correct the code please.

EDIT:

Thanks guys, I've corrected the code :

<form enctype="multipart/form-data" method="post" action="">

  <input name="title" type="text">
  <input name="image" type="file">
  <textarea name="text"></textarea>
  <input type="submit" name="go" value="Submit">

</form>

<?php
  if (isset($_POST['go'])){
    include ("database.php");
    $upload_path = "files/images/";
    $prefix= date("Hi-mdY")."-";

    $file_name = $_FILES['image']['name'];
    $file_name = str_replace(' ', '-', $file_name);
    $file_name = str_replace('_', '-', $file_name);
    $file_name = strtolower($file_name);

    $upload_path = $upload_path . basename($prefix.$file_name);
    move_uploaded_file($_FILES['image']['tmp_name'], $upload_path);

    $final_file_name = ("/files/images/".$prefix.$file_name);

    $title=$_POST['title'];
    $text=$_POST['text'];
    $date=date("Y.m.d");

    $title = mysql_real_escape_string($title);
    $final_file_name = mysql_real_escape_string($final_file_name);
    $text = mysql_real_escape_string($text);

    $sql="INSERT INTO articles (title, image_link,  text, date) VALUES ('$title', '$final_file_name', '$text', '$date')";
    if (mysql_query($sql)){
      echo "done";
    } else {
      echo "error
" . mysql_error();
    }
  }
?>

Now it's good?

There are quite few things to note in your code.

First of all, you are using deprecated $HTTP_POST_FILES where as you should use $_FILES

You are not using mysql_real_escape_string function in your query variables and therefore are vulnerable to sql injection which is caused through poor sql queries.

You are not checking whether or not the submit button was clicked, you should wrap your entire code in between:

if (isset($_POST['go'])){
  // your code
}


Sorting and Filtering Maps Using Generics I had a test for a Java-position where they asked me to manipulate JSON-data from an API.

Once my overall parsing of the data into Maps were done.

The test required me among other things to implement:

Parameter for sorting the data (ASC, DESC) in Maps.
Parameter for filtering the selection of names in Maps.
Parameter for only showing data with odd or even occurrences in Maps.

I passed the test, but did not get the job with the feedback: "How you implemented the sorting and filtering of the maps seemed sloppy, and it seems like you rushed through that part of the test and there's no real code-maintainability in mind for these."

So my question is: How could I have done this implementation in a better way? What flaws do you see?

These maps contains the parsed JSON-data, you will see these being used below.

public static Map<String, Integer> firstNames; /** K= names, V= occurrences of that name **/
public static Map<String, Integer> surNames;

SortMap.java

public class SortMap {

    /**
     * ascending = 1, descending != 1.
     */
    public static <K, V extends Comparable<V>> Map<K, V> sortByValues (final Map<K, V> unsortedMap, int ascending) {
        Map<K,V> sortedMap = new LinkedHashMap<>();
        unsortedMap.entrySet()
                .stream()
                .sorted(Map.Entry.comparingByValue((Comparator<? super V>) (ascending == 1 ? Comparator.naturalOrder() : Comparator.reverseOrder())))
                .forEachOrdered(x -> sortedMap.put(x.getKey(), x.getValue()));
        return sortedMap;
    }

    public static <K extends Comparable<? super K>, V> Map<K, V> sortByKey(Map<K, V> unsortedMap, int ascending) {
        Map<K,V> sortedMap = new LinkedHashMap<>();
        unsortedMap.entrySet()
                .stream()
                .sorted(Map.Entry.comparingByKey((Comparator<? super K>) (ascending == 1 ? Comparator.naturalOrder() : Comparator.reverseOrder())))
                .forEachOrdered(x -> sortedMap.put(x.getKey(), x.getValue()));
        return sortedMap;
    }
}

Usages I provided to solve the tasks:

Map<String, Integer> firstNamesSorted = SortMap.sortByKey(firstNames,-1);
Map<String, Integer> surNamesSorted = SortMap.sortByKey(surNames,1);

FilterMap.java

public class FilterMap {

    public static <K, V> Map<K, V> filterByKey(Map<K, V> map, Predicate<K> predicate) {
        return map.entrySet()
                .stream()
                .filter(entry -> predicate.test(entry.getKey()))
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
    }

    public static <K, V> Map<K, V> filterByValue(Map<K, V> map, Predicate<V> predicate) {
        return map.entrySet()
                .stream()
                .filter(entry -> predicate.test(entry.getValue()))
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
    }

}

Usages I provided to solve the tasks:

Map<String, Integer> filterUnevenNumbers = FilterMap.filterByValue(firstNames, value -> value % 2 == 0);
Map<String, Integer> filterNames = FilterMap.filterByKey(firstNames, value -> value.matches("(?i).*as.*"));

My own reflections of my code is:

Sorting:

-Could have used an enum instead of an int, but using any modern codeeditor you will see my javaDocs comment and also the ascending variable that you provide input for (removed javaDocs comments to reduce clutter in this instance).
+Uses generics.

Filtering:

+Uses generics.
+Dynamic implementation that can filter basically anything as long as you can create the expression using lambdas.
+Can be used for alot more purposes than what they asked for.

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

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


Rearrange page numbers - follow-up This is a follow-up of: Rearrange page numbers for printing a book

Input:

Total pages of a book: int
Pages per booklet: int (divisible by 4 evenly)

Processing:

Rearrange the pages in each booklet to follow a specific format, [n, f, f+1, n-1], where n is the last page of the booklet, and f is the first.

Output:

Print a list/tuple of the rearranged integers

Bugs:

If pages is not a multiple of 4 the last booklet (list that get processed by arrange_booklet()) always has errors at the end.

Misc:

int pages will generally be a large number (50~500). I wrote the program because my printer software doesn't allow me to just print the pages in this order. I have to manually type every integer, comma-separated. Which would obviously get tedious very quickly.

from math import ceil

# f is the first page (smallest input integer), n is the last (largest)
def arrange_booklet(f, n):
    '''Creates a list by using the page # order formula.'''
    _ = list()
    while n > f:
        _.extend([n, f, f+1, n-1])
        f += 2
        n -= 2
    return _

def make_book():
    '''Combines all the lists into one and returns the result.'''
    pages = list(range(1, int(input("Enter a number of pages: "))+1))
    while True:
        len_booklet = int(input(
            "Enter booklet length, must be a multiple of four: "))
        if len_booklet % 4 == 0:
            break
    book = list()  # permanent list, to be returned
    _ = list()  # temporary list, to populate permanent list
    for page in pages:
        _.append(page)
        if page % len_booklet == 0:
            _ = arrange_booklet(_[0], _[-1])
            book.extend(_)
            _.clear()
    # the last booklet, if pages % 4 != 0, is still in the temp list and broke
    # out of the loop it has to be handled separately below, or it will never
    # reach the book
    if _:
        _ = arrange_booklet(_[0], _[-1])
        book.extend(_)
    print(book)

if __name__ == '__main__':
    make_book()

Functions

you should split a lot more of the small pieces of code to separate functions, For example getting and parsing the input has no place in the make_book

Generators

Instead of using temporary lists, using generators is a lot cleaner.

so instead of

def my_func():
    _ = []
    for i in range(10):
        _.append(i**2)
    return _

with a generator this becomes:

def my_gen():
    for i in range(10):
        yield i**2

This is a lot cleaner, and easier to maintain.

Assembling the booklet

instead of iterating over the indices f and n, you can use a collections.dequeand use pop and popleft

from collections import deque
def assemble_booklet(pages):
    """assembles the booklet according to the order [-1, 0, 1, -2] and going in.

    Assumes an iterable with the length a multiple of 4"""
    pages = deque(pages)
    assert not len(pages) % 4, 'booklet length, must be a multiple of 4'
    while pages:
        selection = pages.pop(), pages.popleft(), pages.popleft(), pages.pop()
        yield from selection

again, the generator style allows us to express this a lot clearer than using the intermediary lists

grouping in pages

grouping the pages can be easily done with the grouper itertools recipe

from itertools import zip_longest

def grouper(iterable, n, fillvalue=None):
    """Collect data into fixed-length chunks or blocks

    grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx"
    https://docs.python.org/3/library/itertools.html#itertools-recipes
    """
    args = [iter(iterable)] * n
    return zip_longest(*args, fillvalue=fillvalue)

Then:

for page in pages:
    _.append(page)
    if page % len_booklet == 0:
        _ = arrange_booklet(_[0], _[-1])
        book.extend(_)
        _.clear()

becomes:

def make_book(pages, booklet_length):
    '''Combines all the lists into one and returns the result.'''
    assert not booklet_length % 4, 'booklet length, must be a multiple of 4'
    for group in grouper(pages, booklet_length, fillvalue='blank'):
        yield tuple(assemble_booklet(group))
        # yield from assemble_booklet(group) # dependent on the output you want

If you want the pages in 1 long list instead of separated per booklet, you can change the last line of make_book to yield from assemble_booklet(group)

And in effect, this is the whole code for this method, that gets an iterable of pages, and form booklets out of them

if __name__ == '__main__':
    pages = range(1, 16)
    booklet_length = 8
    result = list(make_book(pages, booklet_length))
    print(result)

[(8, 1, 2, 7, 6, 3, 4, 5), ('blank', 9, 10, 15, 14, 11, 12, 13)]


Parse and Redirect to uri in Javascript Example that could be turned into a button to view user's public ftp where initial page is "http://user.school.edu/":

javascript:(function( {
   var h,i,t;
   h=window.location.hostname;i=h.IndexOf('.school.edu');t=h.substring(0,i);
   window.location.assign("ftp://ftp.school.edu/public/"+t
});

Help me refine this idea, please.

It can also be written in "one line" using regular expression:

window.location = window.location.hostname.replace(
    /^(.*)\.school\.edu$/,
    "ftp://ftp.school.edu/public/$1"
);

I believe in this case regular expression expresses the intent more clearly than using indexOf and string slicing, though tastes may vary.


Implementation of a unique collection I am pretty new to JavaScript and learning the intricacies of this language. I wanted a unique collection which will overwrite while adding if an item already exists else add. Rest of the behavior should be just like any other collection. I had written the type and its as below. Instead of using an array internally, I have used JavaScript's notion of storing an object as key-value pair to store the items.

My question is, is this implementation valid? Are there any disadvantage with this implementation over using an array internally to hold the collection items instead of an object?

Why I chose objects to store items over array is:

To avoid iterating through the array while adding an item, so that I don't have to check  if objects exists(instead of adding a duplicate).
To avoid iterating while retrieving or deleting an item.

function isValid(obj) {
    return (obj != undefined && obj != null && typeof (obj) != 'undefined');
}

function uniqueCollection() {
    var collection = {};
    var length = 0;
    return {
        removeItem: function (item) {
            if (isValid(collection[item])) {
                delete collection[item];
                --length;
            }
        },
        addItem: function (item) {
            if (!isValid(collection[item])) {
                collection[item] = item;
                ++length;
            }
        },
        getLength: function () {
            return length;
        },
        clear: function () {
            collection = {};
            length = 0;
        },
        exists: function (item) {
            if (isValid(collection[item])) {
                return true;
            }
            return false;
        },
        getItems: function () {
            var items = [];
            var i = 0;
            for (var o in collection) {
                if (collection.hasOwnProperty(o)) {
                    items[i] = o.toString();
                    i++;
                }
            }

            if (i !== length) {
                alert("Error occurred while returning items");
            }

            return items;
        }

    };//end of object literal

};//end of function

This mostly seems like a layer of sugar coating that just adds overhead and doesn't add any improved functionality over what a javascript object already has.  The only new functionality I see is keeping track of a length, but this is a lot of extra overhead just for that.  The length could be calculated at any time on a plain javascript object.

Here are the analogs to your methods:

var collection = {};

addItem:
    collection[key] = value;

removeItem:
    delete collection[key]

clear:
    collection = {};

exists:
    if (key in collection)

getItems:
    collection.keys()

getLength:
    collection.keys().length

So, all you're getting out of your implementation is a slightly more efficient length and every other operation is less efficient than just using the native code way of doing it.  Is this collection really useful?

There are some older browsers that don't offer the .keys() method, but there's a pretty simple shim that implements it if not available.

In addition, your implementation loses functionality that a plain javascript object has.  For example, you can't pass your collection to any function that expects a javascript object with the keys and values on it because those are hidden inside, they aren't actually properties of the collection object itself.  Then further, you can't do custom iteration of the keys and values without first creating an array of all the keys because you've hidden the natural ability to iterate the keys of a javascript object.


Read a directory of CSV files in R I'm writing a lot of R in my spare time and am trying to get into good habits by using TidyVerse as much as possible. A large part of this has required reading entire directories of dated log files into R for processing. The volumes of data are well into the GBs so I won't post one here and it shouldn't matter so long as you have filenames matching "./accounts_20180217.csv" or "./symbols_20180217.csv" in the working or data directory.

Example file contents is:

./accounts_20180217.csv

 16 109=18A
175 109=1FN
 10 109=ABP
  6 109=ABW
787 109=ACI
226 109=ACL
163 109=AD
644 109=AGM
 79 109=AGN
 40 109=AIG
  2 109=AJT
991 109=ALG
  4 109=ALGR
 14 109=AM1
 17 109=AN1
 34 109=AP1
136 109=APA
267 109=APJ
160 109=ARE
 64 109=ARX
329 109=AS1

I've been using the following to read in and categorise the file data:

library(tidyverse)
library(stringr)

all_data <- tibble(
              filename = list.files(
                   "./", pattern = "^(accounts|symbols)_201[7-9][0-9]{2}[0-9]{2}\\.csv$"
               , full.names = TRUE
              )
            ) %>%
            mutate(
              date = str_extract(filename, "201[7-9][0-9]{4}")
              , type = str_extract(filename, "(accounts|symbols)")
              , data = map2(
                      filename
                      , date
                      , ~read.csv(
                           .x
                           , header = FALSE
                           , sep = "="
                           , as.is = TRUE
                           , col.names = c("count", "names")
                           , colClasses = c("integer", "character") ))
  )

My question is whether this is a good and Tidy way to achieve this or is there a "better" or "Tidier" way?

A consistent tidyverse solution would use readr::read_csv I guess.

You don't need map2 if you don't use .y, map is enough.

You can use read.csv as your .f argument, and leave the rest to the ... in your map call, might be a matter of taste but I prefer it that way as with the formula notation you have to read through to make sure read.csv is the only function called.

as.is doesn't seem necessary if you give column classes already.

Your mutate call is not necessary, tibble will evaluate lazily the definition of filename given at the previous step.

It seems you really want a "one liner" but in my opinion it would be much clearer in a few steps, I propose the following:

files <- list.files(
  "./",
  full.names = TRUE,
  pattern = "^(accounts|symbols)_201[7-9][0-9]{4}\\.csv$")

data_lst <- map(
  files,
  read_csv,
  header = FALSE,
  sep = "=",
  coltypes = "ic",
  col_names = c("count", "names"))

tibble(
  file_name = files,
  date = str_extract(filename, "201[7-9][0-9]{4}"),
  type = str_extract(filename, "(accounts|symbols)"),
  data = data_lst)

In fact as you're working with a lot of data files I'd tend to write a for loop instead of a map call, so I could more easily debug and proceed without restarting all if an issue arises.


Checking referential integrity of a database schema I am using PHP PDOs to parse the results I am receiving from MySQL queries against a database. I am now running into an issue with running out of allocated memory. Are there any suggestions on improving the efficiency of my code or another more efficient way to handle query results other than parsing PDOs?

This will return whether or not a DB has referential integrity:

function generateDSN($host, $dbname)
{
    return 'mysql:host='. $host . ';dbname=' . $dbname;
}

$dbName = $argv[2];
//Used for query construction

$dsn = generateDSN($argv[1], $argv[2]);
//create DSN

$link = new PDO($dsn, "username", "password");
//Connect to the database
//PDO($dsn, username, password)

if ($link->connect_error)
{
    die("Connection failed: " . $link->connect_error);
}
else
{
    echo "Connected successfully\n";
}
//validate database connection

$query = "SELECT TABLE_CONSTRAINTS.TABLE_NAME, KEY_COLUMN_USAGE.COLUMN_NAME, KEY_COLUMN_USAGE.REFERENCED_TABLE_NAME, KEY_COLUMN_USAGE.REFERENCED_COLUMN_NAME FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS RIGHT OUTER JOIN INFORMATION_SCHEMA.KEY_COLUMN_USAGE ON INFORMATION_SCHEMA.TABLE_CONSTRAINTS.CONSTRAINT_NAME = INFORMATION_SCHEMA.KEY_COLUMN_USAGE.CONSTRAINT_NAME WHERE INFORMATION_SCHEMA.KEY_COLUMN_USAGE.CONSTRAINT_NAME <> 'PRIMARY' AND INFORMATION_SCHEMA.TABLE_CONSTRAINTS.CONSTRAINT_TYPE = 'FOREIGN KEY' AND INFORMATION_SCHEMA.TABLE_CONSTRAINTS.TABLE_SCHEMA = '".$dbName."';";
//Create query to return list of tables with foreign keys
/* Query return format:
+--------------+------------------------+-----------------------+------------------------+
| TABLE_NAME   | COLUMN_NAME            | REFERENCED_TABLE_NAME | REFERENCED_COLUMN_NAME |
+--------------+------------------------+-----------------------+------------------------+
*/

$result = $link->query($query);
//Get query results

$x = 0;
$tableA = [];
$tableB = [];
//Create arrays to hold data from FK queries

while($tables = $result->fetch(PDO::FETCH_ASSOC))
{
    $test[] = $tables;
    $assoc = $test[$x];
    //assign assoc[] the value of $query (will iterate through query table row by row)

    $tableName = $assoc['TABLE_NAME'];
    $columnName = $assoc['COLUMN_NAME'];
    $refTableName = $assoc['REFERENCED_TABLE_NAME'];
    $refColumnName = $assoc['REFERENCED_COLUMN_NAME'];
    //get data values for each column

    $fkQueryA = "SELECT DISTINCT " . $columnName . " FROM " . $dbName . "." . $tableName . " ORDER BY ".$columnName." ;";
    $fkQueryB = "SELECT DISTINCT " . $refColumnName . " FROM " . $dbName . "." . $refTableName . " ORDER BY ".$refColumnName." ;";
    //A -- Table with column that is the foreign key
    //B -- Table with column that the foreign key references

    $resultA = $link->query($fkQueryA);
    $resultB = $link->query($fkQueryB);
    //Get query results

    while($var = $resultA->fetchColumn())
    {
        $tableA[] = $var;
    }
    //Push query results to table

    while($vari = $resultB->fetchColumn())
    {
        $tableB[] = $vari;
    }
    //Push query results to table

    $x++;
    //increment counter to move through $tables
}

$resultCompAB = array_diff($tableA, $tableB);
//return array with all values of A that are not in B

if(empty($resultCompAB))
{
    echo "Database ".$dbName." has referential integrity.";
}
else
{
    echo "Orphan Values in database ".$dbName.": \n";
    array_diff($tableB, $resultCompAB);
}
//print the results

This code does not work properly. Basically you build two enormous arrays containing all values of foreign keys in your database. That's probably a lot.

1: The $x and $test variables serve no purpose and should be removed.

2: The comparison of tablet A and B should be moved inside the while() loop.

I also suggest some unset's and closures of resources that are no longer needed.


"Factory" which produces a "connection" object I am a newbie in C# and in OOP in general. I decided to create a class that should give out a "connection" object. I would like your advice and critique on improving it.

public class Connection_Factory
{

  private string Last_Connection_Trying   = "";
  private string Last_Connection_Success = "";
  public OracleConnection Build(string connection_string)
  {
    Last_Connection_Trying = connection_string;
    return new OracleConnection(connection_string);
  }

  public OracleConnection Build()
  {
    return Build(Last_Connection_Success);
  }

  public void Mark_Connection_as_Successful(string connection_string)
  {
    Last_Connection_Success = connection_string;
  }

  public void Mark_Connection_as_Successful()
  {
    Last_Connection_Success = Last_Connection_Trying;
  }

}

Firstly your code lacks any comments saying what it does and why.  When I first looked at your post I assumed it was a TCP socket class and was looking for the port number.  Also the layout could be improved, regular blank lines, etc.

I would do this:

public void Mark_Connection_as_Successful()
{
    Mark_Connection_as_Successful(Last_Connection_Trying);
}

just like you have with Build.  At the moment they are trivial functions, but that might not always be the case.

Would it be appropriate to verify the format of the connection string, or is this done in the OracleConnection constructor?


Simple Java password rule enforcing rule 1: length must be between 8 and 50 chars.
rule 2: at least 3 of the 4 following rules:
--------2a: at least one upper case
--------2b: at least one lower case
--------2c: at least one numeral
--------2d: at least one non-alphanumeric.

I have a function which will take in two Strings (the password and the confirmation) and then return a boolean depending on if it verified. It also sets a label on my JavaFX screen to the error or confirmation message. It works fine, but the code seems cumbersome. any thoughts?

public boolean validatePassword(String newPass, String confirmPass)
{
    if (newPass == null ||
        confirmPass == null ||
        newPass.length() == 0 ||
        confirmPass.length() == 0 )
    {
        responseLabel.setText("");
        return false;
    }

    boolean equalsRule = newPass.equals(confirmPass);
    boolean lengthRule = newPass.length() >= 8 && newPass.length() <= 50;

    boolean upperRule = !newPass.equals(newPass.toLowerCase());
    boolean lowerRule = !newPass.equals(newPass.toUpperCase());
    boolean numeralRule = newPass.matches("(.*)[0-9](.*)");
    boolean nonAlphaRule = newPass.matches("(.*)[^A-Za-z0-9](.*)");

    int ruleCount = (upperRule ? 1 : 0) + ( lowerRule ? 1 : 0) + ( numeralRule ? 1 : 0) + (nonAlphaRule ? 1 : 0);

    if (ruleCount >= 3 && equalsRule && lengthRule)
    {
        responseLabel.setText("Password set successfully.");
        responseLabel.setTextFill(Color.GREEN);
        return true;
    }
    else
    {
        if (!equalsRule)
        {
            responseLabel.setText("Passwords must match.");
        }
        else if (ruleCount < 3 || !lengthRule)
        {
            responseLabel.setText("Password does not follow instructions above.");
        }
        responseLabel.setTextFill(Color.RED);
        return false;
    }
}

I see at least one big problem in your method: there is no separation of concerns. Everything is mixed into this method: common rule for password double-checking, business rules for password validation and UI calls for user response.

Extracting the business logic

The business logic in your case if the password validation specific rules, as you named them rule1, rule2a, rule2b, rule2c and rule2d.
Everytime you will need a password validation, you will need these rules to validate a password, therefore I would extract these rules into a BO.

import java.util.function.Predicate;
import java.util.stream.Stream;

public final class Password {
    private final String value;
    private final Predicate<String> rule;

    public Password(String value) {
        this.value = value;
        Predicate<String> rule1 = s -> s.length() >= 8 && s.length() <= 50;
        Predicate<String> rule2a = s -> !s.equals(s.toLowerCase());
        Predicate<String> rule2b = s -> !s.equals(s.toUpperCase());
        Predicate<String> rule2c = s -> s.codePoints().anyMatch(Character::isDigit);
        Predicate<String> rule2d = s -> s.codePoints().anyMatch(i -> !Character.isAlphabetic(i));
        Predicate<String> rule2 = s -> Stream.of(rule2a, rule2b, rule2c, rule2d)
                                             .filter(p -> p.test(s))
                                             .count() >= 3;
        this.rule = rule1.and(rule2);
    }

    public boolean isValid() {
        return rule.test(value);
    }
}

If the rules are likely to change, it would also be possible to refactor this code and take the "final predicate" as a constructor argument.

Extracting "standard" logic

In your case, it is only the rule that assert the password and the confirmation password are equals. This is not a business-specific rule but rather a commonly accepted rule everytime a user creates a password. Moreover it seems you have a "validation flow" defined (first check the equality between the two password, then check if the password in itself is valid) that can be extracted into a separate class

public final class PasswordInteractor {
    public ValidationResult validatePassword(String pwd, String confirmPwd) {
        if (!pwd.equals(confirmPwd)) {
            return new ValidationResult("Passwords must match.", false);
        }

        if (!new Password(pwd).isValid()) {
            return new ValidationResult("Password does not follow instructions above.", false);
        }

        return new ValidationResult("Password set successfully", true);
    }
}

I created a DTO ValidationResult to transmit to the upper layer a composite result since it can be more than true/false.

public final class ValidationResult {
    private final String message;
    private final boolean success;

    public ValidationResult(String message, boolean success) {
        this.message = message;
        this.success = success;
    }

    public String getMessage() {
        return message;
    }

    public boolean isSuccess() {
        return success;
    }
}

Extracting UI calls

Finally the UI calls must stay in the UI-part, namely into the JavaFX controller. Typically it could looks like this (simplified):

import javafx.fxml.FXML;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.paint.Color;

public final class PasswordController {
    @FXML
    private Label responseLabel;

    @FXML
    private TextField pwd;

    @FXML
    private TextField confirmPwd;

    public void validateForm() {
        // validate other parts of the form

        ValidationResult result = new PasswordInteractor().validatePassword(pwd.getText(), confirmPwd.getText());

        if (result.isSuccess()) {
            responseLabel.setTextFill(Color.GREEN);
        } else {
            responseLabel.setTextFill(Color.RED);
        }
        responseLabel.setText(result.getMessage());
    }
}


Calculating absolute position in % I'm making a website for a friend (just for fun) where we have to move a lot of svg images in absolute positions. To make it easier I wrote some code to calculate the % position of each element so we can do it faster.

I'm wondering what you think about it and what can I do to improve it. And also any improvements to my style of coding are welcome.

About Constructor

It is considered a bad practice for a constructor to have side effects for multiple reasons. You can browse numerous resources online on this subject. Here are a few whys:

The reader/maintainer of the code does not expect a constructor to have any side effects;
Such constructor is hard or impossible to test;
It violates a whole bunch of principles, including Principle of least astonishment, SRP.

So, yeah instead of

constructor(htmlObject,parent, position){
  // ...
  this.init();
}

you should rather create an instance and invoke the init() on it; like this

const draggableElement = new Mover(
  document.querySelector('.dragable'),
  document.querySelector('body'),
  'absolute',
);
draggableElement.init();

If you are not planning to add the function that disables "moving" of an element, you may even not need a local variable holding the reference to a Mover:

new Mover(
  document.querySelector('.dragable'),
  document.querySelector('body'),
  'absolute',
)
.init();

Naming Things

Naming is important. Method init() is not informative. You may want to rename it to something like enableElementDragging(). A symmetric counter part is now too easy too name – disableElementDragging().

Nano variable names are evil. What is e? An error? An exception? An event? To figure that out I need to look at a place where the function is being invoked. That's mental burden.

Same applies to pos, w, and h... If you are in VS Code or any other smart editor, it's a matter of hitting F2 and renaming it.

Names mx and mh are particularly unfortunate since they are meant to be symmetric to/consistent with w and h. Thus, mx should be mw! Also, I could not decipher what the m prefix means in both of these variables.

Just spell things out: position, width, height, parentWidth, parentHeight, calculatePosition().


Byte Array to Hexadecimal String Conversion Another journey down the difficult path! I'd like to know if there's anything I haven't accounted for, if this can be significantly improved, etc.

(Just as a side-note, I know that some of the formatting is a bit off; some places are spaced out a bit to make it more visually appealing.)

using System;

namespace Data
{
    public class BinaryHelper
    {
        public static string ByteToHexString (byte[] bytes)
        {
            char[] c = new char[bytes.Length * 2];
            byte b;

            for (int i = 0; i < bytes.Length; i++) {
                b = ((byte)(bytes [i] >> 4));
                c [i * 2] = (char)(b > 9 ? b + 0x37 : b + 0x30);
                b = ((byte)(bytes [i] & 0xF));
                c [i * 2 + 1] = (char)(b > 9 ? b + 0x37 : b + 0x30);
            }

            return new string (c);
        }

        public static byte[] HexStringToByte (string hex)
        {
            int len = hex.Length;
            byte[] bytes = new byte[len / 2];

            for (int i = 0; len > i; i += 2) {
                bytes [i / 2] = Convert.ToByte (hex.Substring (i, 2), 16);
            }

            return bytes;
        }
    }
}

Ask yourself "how can I make the meaning and intention of the code more clear?"

Consider making small helper methods rather than duplicating code:

static byte HighNibble(byte b) => (byte)(b >> 4);
static byte LowNibble(byte b) => (byte)(b & 0xF);
static char NibbleToHex => ... etc.

And now instead of

    b = ((byte)(bytes [i] >> 4));
    c [i * 2] = (char)(b > 9 ? b + 0x37 : b + 0x30)
    b = ((byte)(bytes [i] & 0xF));
    c [i * 2 + 1] = (char)(b > 9 ? b + 0x37 : b + 0x30);

Which is overwhelming in the sheer amount of punctuation, we have:

 c [i * 2] = NibbleToHex(HighNibble(bytes[i]));
 c [i * 2 + 1] = NibbleToHex(LowNibble(bytes[i]));

So much easier to read and understand.


Locker Puzzle in Java The author of my book does not release odd-numbered solutions to students, so I can't check my solutions. I was wondering if there was a better way to solve this problem without simply printing out the square roots.

Here is the question:

  (Game: locker puzzle) A school has 100 lockers and 100 students. All
  lockers are closed on the first day of school. As the students enter,
  the first student, denoted S1, opens every locker. Then the second
  student, S2, begins with the second locker, denoted L2, and closes
  every other locker. Student S3 begins with the third locker and
  changes every third locker (closes it if it was open, and opens it if
  it was closed). Student S4 begins with locker L4 and changes every
  fourth locker. Student S5 starts with L5 and changes every fifth
  locker, and so on, until student S100 changes L100.

  After all the students have  passed through the building and changed
  the lockers, which lockers are open? Write a program to find your
  answer.

  (Hint: Use an array of 100 Boolean elements, each of which indicates
  whether a locker is open (true) or closed (false).  Initially, all
  lockers are closed.)

Here is my solution:

public class Main {
    public static void main(String[] args) {
        boolean[] lockers = new boolean[101];
        //Open all multiples of 1 before moving on to 2
        for (int i = 1; i < lockers.length; i++) {
            lockers[i] = true;
        }

        //open every locker for every multiple of i
        for (int i = 2; i <= 100; i++) {
            for (int j = 1; i * j <= 100; j++) {
                lockers[i * j] = (lockers[i * j] == true) ? false : true;
            }
        }

        //Display the indices of the open lockers
        for (int i = 0; i < lockers.length; i++) {
            if (lockers[i] == true)
                System.out.println("locker " + i + " is open.");
        }
    }
}

You are using i = 1 as a special case, when in fact that can be handled by your other code already:

for (int i = 1; i <= 100; i++) {
    for (int j = 1; i * j <= 100; j++) {
        lockers[i * j] = !lockers[i * j];
    }
}

However, to be more readable, I suggest changing the inner loop:

for (int i = 1; i <= 100; i++) {
    for (int j = i; j <= 100; j += i) {
        lockers[j] = !lockers[j];
    }
}

Now it reads as "Start j at i, repeat as long as j is less than or equal to 100, increase j by the value of i"


Contra - 30 More Lives Just for the fun of it, I wanted to have a hidden keypress function on my website that performs an event when the user types in the Contra code for thirty lives. I'm curious how the implementation for this simple piece of code holds up and where it could be improved.

Note: keypress doesn't seem to detect the arrow keys so I used the arrows from the numpad

var codeCount = 0;

//[up, up, down, down, left, right, left, right, b, a, start]
var contraCode = [38, 38, 40, 40, 37, 39, 37, 39, 66, 65, 13];

$(window).keydown(function(input) {
  var key = input.which;
  if (contraCode[codeCount] === key) {
     codeCount++;

     //if 11 keys have sequentially matched the cheat code
     if (codeCount === 11) {
        alert('You got 30 lives!');
     }
  }
  else {
     codeCount = 0;
  }
});

One thing you could improve is to remove the jQuery dependency and just use addEventListener directly. This should be fine as all modern browsers support this already. Although, the tradeoff would be to normalize your keycodes yourself.

Also, I would suggest separating the code that updates codeCount from the code that checks if you achieved all keys in sequence. This way, there's clear separation in logic. The one problem I often come across with code not clearly separating logic is that in the long run, code becomes hard to untangle. Kill this habit while still early.

Taking it a bit further, you can assign the key values to an object. This way, you can represent the key in code using a enum-like structure and avoid hardcoding keycodes all over your code. This also allows you to change the keycode value without having to dive in to the code to find and change keycodes everywhere.

var codeCount = 0;

var Keys = {
  UP: 38,
  DOWN: 40,
  LEFT: 37,
  RIGHT: 39,
  B:66,
  A: 65,
  START: 13,
];

//[up, up, down, down, left, right, left, right, b, a, start]
var contraCode = [
  Keys.UP,
  Keys.UP,
  Keys.DOWN,
  Keys.DOWN,
  Keys.LEFT,
  Keys.RIGHT,
  Keys.LEFT,
  Keys.RIGHT,
  Keys.B,
  Keys.A,
  Keys.START,
];

window.addEventListener('keydown', function(event){

  var key = event.keyCode? event.keyCode : event.charCode;

  codeCount = contraCode[codeCount] === key ? codeCount + 1 : 0;

  if(codeCount === 11) alert('You got 30 lives!');

});


Project Euler #9 - Pythagorean triplets This code works, and I'm trying to figure out if there's a better way to optimize it. As you can see, I've cut down drastically on time just by using numbers that work for a < b < c. Is there any outstanding thing that can reduce the time? It currently takes ~7 seconds.

Problem:

  A Pythagorean triplet is a set of three natural numbers, \$a < b < c\$, for which

  $$a^2 + b^2 = c^2$$

  For example, \$3^2 + 4^2 = 9 + 16 = 25 = 5^2\$.

  There exists exactly one Pythagorean triplet for which \$a + b + c = 1000\$.  Find the product \$abc\$.

My code:

import time

start = time.time()

def pythag(a, b, c):
    if a ** 2 + b ** 2 == c ** 2:
        return True
    return False

a = [x for x in range(1, 1000)]

for num in a:
    for dig in range(num, 1000 - num):
        for i in range(dig, 1000 - dig):
            if num + dig + i == 1000:
                if pythag(num, dig, i):
                    print(num, dig, i)
                    print("Product: {}".format(num * dig * i))
                    elapsed = time.time() - start
                    print("Time: {:.5f} seconds".format(elapsed))
                    exit(1)

# 200 375 425
# Product: 31875000
# Time: 7.63648 seconds

Without changing your time too much I got these results:

Original run:

>>>
200 375 425
Product: 31875000
Time: 8.19322 seconds
>>>

New code:

>>>
200 375 425
Product: 31875000
Time: 0.28517 seconds
>>>

What I changed:

I moved the timing to completely surround the code, instead of when it hit the triplet.
I inlined the check for the triplet, as functions are slow in Python
Instead of generating a list for num, I used a range object straight up to generate them as needed
I eliminated the i loop and condition by using the fact that i will need to be 1000 - num - dig.

Resulting code:

import time

start = time.time()

for num in range(1, 1000):
    for dig in range(num, 1000 - num):
        i = 1000 - num - dig
        if num * num + dig * dig == i * i
            print(num, dig, i)
            print("Product: {}".format(num * dig * i))

elapsed = time.time() - start
print("Time: {:.5f} seconds".format(elapsed))

Fun fact: the check for a triplet in this case can be reduced to:

num * dig + 1000 * i == 500000

Where did I get these magic numbers you ask? Math. Check it out:

$$(\text{num}+ \text{dig}+ i)^2 = 1000^2$$
Expand:
$$\text{num}^2 + \text{dig}^2 + i^2 + 2 \; \text{num}  \; \text{dig}+ 2  \; \text{num}  \; i + 2  \; \text{dig}  \; i = 1000000$$
Use triplet equality:
$$2 i^2 + 2  \; \text{num}  \;  \text{dig}+ 2  \; \text{num}  \; i + 2  \; \text{dig}  \; i = 1000000$$

Factor:

$$i(\text{num}+ \text{dig}+ i) + \text{num} \; \text{dig}= 500000$$

Use fact that \$\text{num}+ \text{dig}+ i = 1000\$ in our case:

$$\text{num} \; \text{dig}+ 1000 i = 500000$$


Non-recursive implementation of MergeSort I've been trying to internalize some of the basic sorting algorithms recently, and to do so I've been looking at their wikipedia pages to refresh myself with how they work, and then coding my own "version" of the algorithm from there.

I did this with MergeSort recently, and I got an algorithm that for what I can tell works but I'm not sure it runs in \$O(n \log{n})\$ time like MergeSort does.

def mergeSortAlgorithm(x):

    listOfLists = []

    for i in range(len(x)):
        temp = [x[i]]
        listOfLists.append(temp)                          #I first put every item in the array into
                                                      #its own list, within a larger list -> divide step

                                                      #so long as there is more than one sorted list, then we need to keep merging
    while(len(listOfLists) != 1):
        j = 0

                                                      #for every pair of lists in the list of lists, we need to merge them
        while(j < len(listOfLists)-1):
            tempList = merge(listOfLists[j], listOfLists[j+1])
            listOfLists[j] = tempList
            del listOfLists[j+1]                          #I shift the new list to index j, and delete the second list from the now-merged pair at index j+1
            j = j+1                                       #increment the value of j, basically a counter
        print(listOfLists)

    print(listOfLists[0], "is sorted!")

def merge(a, b):                                          #function to merge two lists in order
    newList = []
    count1, count2 = 0, 0

                                                      #basically, walk through each list, adding whichever element is smallest
    while((count1 < len(a)) and (count2 < len(b))):

        if(a[count1] > b[count2]):
            newList.append(b[count2])
            count2 = count2 + 1                           #update the counter along b

        elif(a[count1] < b[count2]):
            newList.append(a[count1])
            count1 = count1 + 1                           #update the counter along a

        elif(a[count1] == b[count2]):                     #if they're equal, add the value from a first
            newList.append(a[count1])
            newList.append(b[count2])
            count1, count2 = count1 + 1, count2 + 1       #update the counter on each

    if(count1 < len(a)):                                  #if the while loop exited with left-over values in a, add them
        for f in range(count1, len(a)):
            newList.append(a[f])
    elif(count2 < len(b)):                                #do the same for b - the values are already in order so you're good
        for k in range(count2, len(b)):
            newList.append(b[k])

    return newList

The algorithm correctly sorts what I give it, but I'm thinking that the runtime is more like \$O(\log{n} * \log{n} * n)\$ because the outer while loop in mergeSortAlgorithm() will do \$log(n)\$ passes, as will the inner while loop, and the merge() algorithm will take in total \$n\$ operations.

Is that right?  If so, in what respect does my algorithm differentiate from MergeSort? (i.e. where am I going wrong?)

The approach may lose the stability. If list a contains a1, a2, and list b contains b1, b2 and all of them are compared equal, the resulting list will have a a1, b1, a2, b2, whereas the stable result should be a1, a2, b1, b2. The fix is to not special case a[count1] == b[count2]:

if b[count2] < a[count1]:
    merge from b
else:
    merge from a

Your big-O worries are unfounded. At the \$k\$'th iteration there remain \$O(\frac{n}{2^k})\$ sublists, but the length of each sublist is \$O(2^k)$\$, so the entire body of the inner loop executes in \$O(n)\$ time regardless of the iteration; and as you noticed there are \$O(\log{n})\$ of them.


Calculating relay operating times, according to IEC 60255 The script below is used to determine relay operating times, as defined in the standard IEC 60255. The equation and relevant information can be found here.

curve_equation takes three input values, and outputs a list of values representing the trip times for currents in the fixed range np.logspace(np.log10(Is), 5, 1e2).

The part in the bottom plots the trip times for the different curves. The function curve_equation will be used to much more than that, so I'm mostly interested in improvements to that one.

# -*- coding: utf-8 -*-
"""
Created on Sun Apr 29 13:28:26 2018

@author: Stewie
"""
import numpy as np
import matplotlib.pyplot as plt

def curve_equation(curve_name, T, Is):
    """
    This function calculates and returns the inverse curves for
    the selected curve type.
    Input:
        curve_name: one of the strings: "SIT", "VIT", "LTI", "EIT", "UIT"
        T: The time dial setting. Typically in the range 0.1 ... 1
        Is: The current setting. Typically in the range 100-10000

    The appropriate k, alpha and theta values are selected based on the
    curve type.
    """

    # Define constants to be used in the curve equation
    k = (0.14, 13.5, 120, 80, 315.2)
    alpha = (0.02, 1, 2, 2, 2.5)
    beta = (2.97, 1.5, 13.33, 0.808, 1)
    curve_types = ("SIT", "VIT", "LTI", "EIT", "UIT")

    # idx is the index of the values we want
    try:
        idx = curve_types.index(curve_name)
    except:
        print("Non-existing curve-type")
        return

    I = np.logspace(np.log10(Is), 5, 1e2)

    def td_equation():
        td = k[idx]/((I/Is)**alpha[idx]-1) * (T / beta[idx])
        return(td)

    return(td_equation())

# For testing / plotting purposes:
curve_types = ("SIT", "VIT", "LTI", "EIT", "UIT")
I = np.logspace(1, 5, 1e2)
for curve_type in curve_types:
    plt.loglog(I, curve_equation(curve_type, 1, 1e2), label=curve_type)

plt.xlabel("Current [A]")
plt.ylabel("Time [s]")
plt.title("IEC Time/current curves")
plt.grid()
plt.legend()

You need not declare # -*- coding: utf-8 -*- in Python 3: UTF-8 is the default.

Writing I = np.logspace(…) inside your curve_equation function is bad practice, because it hard-codes certain x-values for your plot.  In fact, your plot is wrong, because the calculations are for \$I_s\$ values ranging from 102 A to 105 A (due to the inner I = np.logspace(np.log10(Is), 5, 1e2)), but your plot's x axis is labelled as if the values ranged from 101 A to 105 A (due to the outer I = np.logspace(1, 5, 1e2)).  Specifically, four of the curves should intersect at 1 kA, 10 s.

I also find the way in which the curves are specified very cumbersome.  You have five magic strings ("SIT", "VIT", etc.) which you search for in curve_types.  Then, you retrieve the k, alpha, and beta parameters by using the corresponding index.  Printing an error message and returning None is an inappropriate error-handling mechanism; you should raise or propagate an exception for such a fatal error.

I'd rather specify the parameters the other way: each curve should be defined as a \$(k, \alpha, \beta)\$ tuple.  To do that, I would use namedtuples.  I'd go a bit further and make each namedtuple smart enough to know what its own \$td(I)\$ function looks like, when \$T\$ and \$I_s\$ are specified.

from collections import namedtuple
import numpy as np
import matplotlib.pyplot as plt

class IEC_Curve(namedtuple('IEC_Curve', 'name k alpha beta')):
    def td(self, T, Is):
        """
        The td(I) function for a given IEC curve, time dial setting, and
        current setting.
        """
        return lambda I: self.k * T / self.beta / ((I / Is)**self.alpha - 1)

SIT = IEC_Curve('SIT', k=0.14, alpha=0.02, beta=2.97)   # Standard inverse / A
VIT = IEC_Curve('VIT', k=13.5, alpha=1, beta=1.5)       # Very inverse / B
LTI = IEC_Curve('LTI', k=120, alpha=2, beta=13.33)      # Long time inverse / B
EIT = IEC_Curve('EIT', k=80, alpha=2, beta=0.808)       # Extremely inverse / C
UIT = IEC_Curve('UIT', k=315.2, alpha=2.5, beta=1)      # Ultra inverse

I = np.logspace(2, 5, 1e2)
for curve in (SIT, VIT, LTI, EIT, UIT):
    plt.loglog(I, curve.td(T=1, Is=1e2)(I), label=curve.name)
plt.xlabel("Current [A]")
plt.ylabel("Time [s]")
plt.title("IEC Time/current curves")
plt.grid()
plt.legend()
plt.show()

Note that td is a method that returns a function.  To calculate values of \$td\$ with fixed \$T\$, fixed \$I_s\$, and swept \$I\$:

UIT.td(T=1, Is=1e2)(I=np.logspace(2, 5, 1e2))

To calculate a value of \$td\$ for a given \$T\$, \$I_s\$, and \$I\$:

UIT.td(T=1, Is=1e2)(I=1e2)

(This happens to cause a ZeroDivisionError, which manifests itself as a RuntimeWarning: divide by zero encountered in divide at the beginning of the sweep.)

In case you feel intimidated by my use of inheritance of namedtuple, you could use a manually written constructor instead:

class IEC_Curve:
    def __init__(self, name, k, alpha, beta):
        self.name = name
        self.k = k
        self.alpha = alpha
        self.beta = beta

    def td(self, T, Is):
        return lambda I: self.k * T / self.beta / ((I / Is)**self.alpha - 1)

One difference is that namedtuples are immutable, whereas the members here are mutable.


Paper-scissors-stick-stone game in Python I would like to condense the code under def game(). I was told through a response to my question on Stack Overflow that I could use dictionary which goes something like this:

mychoice = {1:"paper", 2:"scissors", 3:"stick", r: "stone"}

import random
import time
from getpass import getuser

def my_input():

    #validinput = False
    #while not validinput:
    while True:
        try:
            mychoice = int(input("Your choice? (1. paper, 2. scissor, 3. stick or 4. stone) "))

        except ValueError:
            print('You did not enter a valid input. Enter again.')
            continue

        else:
            if mychoice == 1:
                return 'paper'
            elif mychoice == 2:
                return 'scissor'
            elif mychoice == 3:
                return 'stick'
            elif mychoice == 4:
                return 'stone'
            else:
                print('Invalid choice. Enter again.')

def randomized():
    choice = ['paper', 'scissor', 'stick', 'stone']
    return random.choice(choice)

def game():
    user_continue = True
    while user_continue:
        myinput = my_input()
        print('YOU: ' + str(myinput))
        randomval = randomized()
        time.sleep(1)
        print('COMPUTER: ' + randomval)
        won = 'Congratulations ' + getuser() + '. You won!'
        lost = 'Sorry ' + getuser() + '. You lost.'
        draw = "It's a draw."
        if myinput == 'paper' and randomval == 'scissor':
            time.sleep(1)
            print(lost)
        elif myinput == 'scissor' and randomval == 'paper':
            time.sleep(1)
            print(won)
        elif myinput == 'paper' and randomval == 'stick':
            time.sleep(1)
            print(lost)
        elif myinput == 'stick' and randomval == 'paper':
            time.sleep(1)
            print(won)
        elif myinput == 'paper' and randomval == 'stone':
            time.sleep(1)
            print(won)
        elif myinput == 'stone' and randomval == 'paper':
            time.sleep(1)
            print(lost)
        elif myinput == 'scissor' and randomval == 'stick':
            time.sleep(1)
            print(lost)
        elif myinput == 'stick' and randomval == 'scissor':
            time.sleep(1)
            print(won)
        elif myinput == 'scissor' and randomval == 'stone':
            time.sleep(1)
            print(lost)
        elif myinput == 'stick' and randomval == 'stone':
            time.sleep(1)
            print(lost)
        elif myinput == 'stone' and randomval == 'stick':
            time.sleep(1)
            print(won)
        else:
            print(draw)

        #continue looping until user says no
        user_yn = str.upper(input('Do you want to continue? Press Enter or type any value other than N '))
        if user_yn != 'N':
            print('continuing...')
            continue
        else:
            print('ending...')
            break
game()

So instead of this part of your code

    else:
        if mychoice == 1:
            return 'paper'
        elif mychoice == 2:
            return 'scissor'
        elif mychoice == 3:
            return 'stick'
        elif mychoice == 4:
            return 'stone'
        else:
            print('Invalid choice. Enter again.')

you may first create that recommended dictionary

allowed_choices = {1: "paper", 2: "scissors", 3: "stick", r: "stone"}

and then replace you code with

    else:
        if mychoice in allowed_choices:
            return allowed_choices[mychoice]
        else:
            print('Invalid choice. Enter again.')

The part mychoice in allowed_choices checks only in keys of the allowed_choices dictionary (so it checks if it is 1, 2, 3, or 4).

The part return allowed_choices[mychoice] returns the value of mychoice key (e. g. "stick" for the key 3).


The Morra game implementation in Haskell Me and my friends play a variant of Morra when drinking. The rules are as follows:

The players sit in a circle and hold one of their hands in front of
them.
One player guesses a number. All the players (including this one)
then hold out between 0 and 5 fingers on their hand.
If there are as many fingers held out as the player guessed, he is
"saved", leaves the circle and doesn't have to play anymore. Next turn another player tries to guess
(otherwise the game just goes on with the player staying in the circle).
The last player to remain in the circle has to drink.

I'm just learning about monad transformers, so I thought it would be nice to implement this simple game.

module Morra where

import Control.Monad.Trans.State
import Control.Monad.Trans
import Control.Monad
import Data.List
import Data.Char

main :: IO ()
main = do
  putStrLn "Welcome to Morra. The last to remain has to drink."
  putStr "Please input the names of players: "
  psraw <- getLine
  putStrLn ""
  ([p], n) <- execStateT (play 0) (words psraw, 0)
  putStrLn $ p ++ " has lost, after " ++ show n ++ " rounds. (S)he has to take a shot."

play :: Int -> StateT ([String], Int) IO ()
play p = do
  (ps, n) <- get
  let currp = ps !! p
  if length ps == 1
    then put (ps, n)
    else do
      liftIO $ putStr $ "It's now " ++ currp ++ "'s turn. How many fingers will be help up? "
      a <- liftIO safeGetLine
      putNewLine
      liftIO $ putStrLn "[INPUT PHASE] Each player now chooses, how many fingers does (s)he hold up."
      fs <- liftIO $ getFingers (length ps)
      liftIO $ putStr "[END OF INPUT PHASE]"
      putNewLine
      if sum fs == a
        then do
          liftIO $ putStrLn $ currp ++ " has guessed right, he's out."
          putNewLine
          let newps = delete currp ps
          put (newps, n + 1)
          play ((p + 1) `mod` length newps)
        else do
          liftIO $ putStrLn $ currp ++ " hasn't guessed right, the game goes on!"
          putNewLine
          put (ps, n + 1)
          play ((p + 1) `mod` length ps)
  where
    putNewLine = liftIO $ putStrLn ""

getFingers :: Int -> IO [Int]
getFingers n = replicateM n helper
 where
  helper = do
    putStr "Input the number of fingers you hold up: "
    fs <- safeGetLine
    putStrLn "Pass the computer to the next player now. [ENTER]"
    getLine
    return fs

safeGetLine :: IO Int
safeGetLine = do
  x <- getLine
  if all isNumber x
    then return $ read x
    else do
      putStrLn "Please input a number"
      safeGetLine

Firstly, you can see the strings I print are pretty long, longer than the 80 chars limit. What is the common approach to tackle these and keep all the lines below 80 chars?

And secondly, please point out anything that could be made more elegant, whatever that might be (even changing the underlaying StateT). I'm also open to new ideas and challenges.

You don't really use the StateT to your advantage. Your play function can be written as

play :: Int -> ([String], Int) -> IO ([String], Int)
play p (ps, n) = do
  let currp = ps !! p
  if length ps == 1
    then return (ps, n)
    else do
      putStr $ "It's now " ++ currp ++ "'s turn. How many fingers will be help up? "
      a <- safeGetLine
      putNewLine
      putStrLn "[INPUT PHASE] Each player now chooses, how many fingers does (s)he hold up."
      fs <- getFingers (length ps)
      putStr "[END OF INPUT PHASE]"
      putNewLine
      if sum fs == a
        then do
          putStrLn $ currp ++ " has guessed right, he's out."
          putNewLine
          let newps = delete currp ps
          play ((p + 1) `mod` length newps) (newps, n + 1)
        else do
          putStrLn $ currp ++ " hasn't guessed right, the game goes on!"
          putNewLine
          play ((p + 1) `mod` length ps) (ps, n + 1)
  where
    putNewLine = putStrLn ""

That's not really surprising, since State s a is more or less just an abstraction of s -> (s, a). Since a is () in your case, we can just get rid of it and end up with s -> IO s. We can use State(T) to our advantage if we have several Stateful functions. However, we only have play.

Speaking about play, there are several nitpicks:

You use length several times, although a single length call would suffice.
You should use guess instead of a as name.

If we apply both, we end up with

play :: Int -> StateT ([String], Int) IO ()
play p = do
  (ps, n) <- get
  let currp = ps !! p
  let plength = length ps
  unless (plength == 1) $ do
      liftIO $ putStr $ "It's now " ++ currp ++ "'s turn. How many fingers will be help up? "
      guess <- liftIO safeGetLine
      putNewLine
      liftIO $ putStrLn "[INPUT PHASE] Each player now chooses, how many fingers does (s)he hold up."
      fs <- liftIO $ getFingers plength
      liftIO $ putStr "[END OF INPUT PHASE]"
      putNewLine
      if sum fs == guess
        then do
          liftIO $ putStrLn $ currp ++ " has guessed right, he's out."
          putNewLine
          put (delete currp ps, n + 1)
          play ((p + 1) `mod` (plength - 1))
        else do
          liftIO $ putStrLn $ currp ++ " hasn't guessed right, the game goes on!"
          putNewLine
          put (ps, n + 1)
          play ((p + 1) `mod` plength)
  where
    putNewLine = liftIO $ putStrLn ""

We didn't  gain that much, did we? So let's split play:

type Player     = String
type Turn       = Int
type MorraT m a = StateT ([Player], Turn) m a
type Morra a    = MorraT IO a

play :: Morra ()
play = do
   pcount <- getPlayerCount
   unless (pcount == 1) $ do
     currp <- getNextPlayer
     guess <- guessFingers currp
     fingers <- getFingers
     if fingers == guess
       then do
         putStrLn' $ currp ++ " has guessed right, they're out!"
         removePlayer currp
       else do
         putStrLn' $ currp ++ " hasn't guessed right, the game goes on!"
         increaseTurnCount
         play
  where
    putStrLn' xs = liftIO $ putStrLn xs >> putStrLn ""

That's easy enough to undestand. We also added some types so that we can change our program in a single line, for example if you want to change the StateT to something else later.

Now we need our helper functions:

players :: Monad m => MorraT m [Player]
players = fst <$> get

That's a convenient small function and makes it possible to change our Morra later without changing too many functions.

For our getNextPlayer we want to get the next player as well as queue them to the back of the line. A list isn't the perfect data structure here, by the way, but we keep it for simplicity:

getNextPlayer :: Monad m => MorraT m Player
getNextPlayer = do
  (p:ps) <- players
  put (ps ++ [p])
  return p

guessFingers looks exactly how you'd imagine it. It doesn't need to be in Morra, as it does not inspect the state, but let's keep it there for simplicity, again:

guessFingers :: Player -> Morra Int
guessFingers p = do
  liftIO $ putStrLn $ p ++ ", what's your guess?"
  liftIO $ safeGetLine

Your variant of getFingers only tells a person to give the PC to the next player, but it does not tell them who the next player is going to be. That can be fixed by using forM instead of replicateM:

getFingers :: Morra Int
getFingers = do
  ps <- players
  fmap sum $ forM ps $ \p ->
    liftIO $ putStrLn $ "How many fingers do you hold up, " ++ p ++ "?"
    liftIO $ getSafeLine

removePlayer looks the same as yours:

removePlayer :: Monad m => Player -> MorraT m ()
removePlayer p = modify $ \(ps, n) -> (delete p ps, n)

Note that a setPlayers or withPlayers function would make both this and getNextPlayer simpler.

increaseTurnCount :: Monad m => MorraT m ()
increaseTurnCount = modify $ \(ps, n) -> (ps, n + 1)

That's it. We now have several functions that can be used independently but act on the same state. By the way, the single increaseTurnCount hints that the amount of Turns isn't really part of the State, but could be used as the return value of play instead.

For completeness, we can provide

runMorra :: Monad m => [Player] -> MorraM m a -> m (Player, Int)
runMorra ps = do
  ([p], n) <- execStateT play (ps, 0)
  return (p, n)

One last remark on safeGetLine. Instead of all isNumber use readMaybe from Text.Read:

safeGetLine :: IO Int
safeGetLine = do
  x <- getLine
  case readMaybe x of
    Just n  -> return n
    Nothing -> putStrLn "Please input a number" >> safeGetLine

Otherwise your safeGetLine isn't safe, since isNumber also returns True on Unicode Characters in the 'Number, Other' Category.


Fetching objects and adding a calculated field in Doctrine 2 I'm making a section that shows a list of blogs and how many comments it has without loading any comment data. My Blog entity is as follows:

/**
 * @ORM\Entity
 * @ORM\Table(name="blogs")
 */
class Blog
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    protected $id;

   // This field is aggregated. Not in DB table
    protected $totalComments = 0;

    /**
     * @ORM\Column(type="string")
     */
    protected $title;

     /**
     * @ORM\OneToMany(targetEntity="Comment", mappedBy="blog")
     */
    protected $comments;

    // More attributes

    public function __construct(){
        $this->comments = new ArrayCollection();
    }

    // Setters and Getters
}

and my Comment Entity:

class Comment
{
    /**
     * @ORM\Id
     * @ORM\Column(type="integer")
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    protected $id;

    /**
     * @ORM\ManyToOne(targetEntity="Blog", inversedBy="comments")
     * @ORM\JoinColumn(name="blog_id", referencedColumnName="id")
     */
    protected $blog;

   // more attributes

    public function __construct(){     }

    // Setter and Getters
}

They have a oneToMany relationship. 1 Blog has N comments.

My next step is convert this query to the Doctrine2 World:

'SELECT b.*, COUNT( b.id ) AS totalComments FROM  `blogs` b LEFT JOIN comments c ON b.id = c.blog_id GROUP BY b.id'

That was an easy task:

function getLatestBlogs(){
        $qb = $this->createQueryBuilder('b')
        ->select('b, c')
        ->addSelect('count(b.id) as totalComments')
        ->leftJoin('b.comments', 'c')
        ->groupBy('b.id');

        $result = $qb->getQuery()->getResult();

        foreach($qResult as $row){ //tweaking original result
            $row[0]->setTotalComments($row['totalComments']);
            $list[] = $row[0];
        }
        return $list;
    }

In the foreach loop is where I fix my fetched result because doctrine's getResult was returning an array of arrays like this:

// var_dump($result)
array (size=8)
  0 =>
    array(2) =>
      0 =>
          object Blog
          'id' => int 330
          'title' => string 'title blog'
          // ... more attributes

      "totalComments" => int 5

  1 => ...
  );

Is there any fashion way that I'm missing ? I've really dig in to depth and couldn't find anything that fix this situation.

If you wanna just show them, just use

count($blog->getComments()) or blog.comments|length in template, or something similar.


FIFO Queue Implementation My Implementation of a FIFO queue. I am mainly curious about my initialization of empty generic arrays.

public class Queue<T> {

    T queue[];
    public Queue()
    {
        queue = (T[])new Object[0];
    }

    public T pop()
    {
        T queue2[] = (T[])new Object[queue.length-1];
        T obj = queue[0];
        for(int i = 0; i < queue.length-1; i++)
            queue2[i] = queue[i+1];
        queue = queue2;
        return obj;
    }

    public void push(T node)
    {
        T queue2[] = (T[])new Object[queue.length+1];
        for(int i = 0; i < queue.length; i++)
        {
            queue2[i] = queue[i];
        }
        queue2[queue.length] = node;
        queue = queue2;
    }

    public T peek()
    {
        return queue[0];
    }

    public int size()
    {
        return queue.length;
    }

}

Trying to use a generic array in Java, is a disaster waiting to happen. The T[] field is really an Object[] at runtime, and trying to do otherwise just hides this fact. It makes you think you're dealing with a T[], but it's a lie; you're really dealing with an Object[]. There are very good reasons not to use generic arrays. I'll add that if you peek into the current Oracle implementation of ArrayList or CopyOnWriteArrayList, you won't see a generic array being used, but Object[] explicitly. It makes it clear to the developers and to the readers that the array being handled does not have a generic type.

In your particular case, there are no unsafe operations done with it, so that's good. But be careful if you add a toArray() method in the future... Just take a look to see how easy it is to make a mistake: add the method

public T[] toArray() {
    return queue; // or queue.clone();
}

Looks fine. Even compiles without warnings. Now do:

Queue<String> queue = new Queue<>();
queue.push("foo");
String[] array = queue.toArray();

Looks also fine. Still no warnings. But this will fail at run-time:

  Exception in thread "main" java.lang.ClassCastException: [Ljava.lang.Object; cannot be cast to [Ljava.lang.String;

Runtime is the worst moment to have those exceptions. It is always better to detect bugs as early as possible, and fail directly at compile-time. And this exception, is because the array really isn't a String[], but an Object[]. There are ways around that of course (the simpler being to return Object[] from the method), but the point is that you're opening yourself to a lot of unforeseen troubles. All in all, if you really want a backing array, don't lie to yourself and:

Just use Object[]. You'll have to cast when poping or pushing elements, but you can ensure that it is safe to do by having the proper generic type for the methods, just like you have in the current code.
If you want strong typing, pass the class of the elements as constructor to the Queue.

Couple of side-notes:

It would be preferable to throw an NoSuchElementException if the queue is empty inside peek, instead of failing with an out of bounds exception.
To copy arrays, you can use System.arraycopy, like mentioned in this other answer.


Optimizing jQuery functions for mobile menu I'm using Bootstrap 3 as the front-end framework for this. It is quite simple but I feel I'm doing it wrong. I'm sure this code can be optimized not to duplicate the $('a[href=""]') functions but I just can't wrap my head around it.

For now, this code is only for mobile. I'll have to figure out how to control the menu behavior for larger screens (not requested at this time).

jsFiddle (you need to resize your browser to see the effect)

$(document).ready(function () {

/*
 * Modification du boutton MENU pour CLOSE
 */

$('button').on('click', function(e){
    var str = $(this).text();
    if(str =="CLOSE"){
        $(this).html("MENU");
    }
    else{
        $(this).html("CLOSE");
    }

    if(str == "MAIN MENU"){
        e.stopPropagation();
        $('#menuContacts').css('display', 'none');
            $('#menuWines').css('display', 'none');
            $('#menuMain').css('display', 'block');

            $('button').html('CLOSE');
    }
});

/*
 * Modification du menuMain pour le menuContacts
 */

$('a[href="#contacts"]').on('click', function(e){
    e.preventDefault();
    $('#menuMain').css('display', 'none');
    $('#menuContacts').css('display', 'block');

    $('button').html('MAIN MENU');

});

/*
 * Modification du menuMain pour le menuWine
 */

$('a[href="#wines"]').on('click', function(e){
    e.preventDefault();
    $('#menuMain').css('display', 'none');
    $('#menuWines').css('display', 'block');

    $('button').html('MAIN MENU');

});
});

you can add some data attribute to the HTML like this

Préface

Maison Ilan

History

Vineyard

Cuverie

Chai

Wines

Contacts

so instead of .on('click') for each link you can do something like this

//all the links in #menuMain
$('#menuMain a').on('click', function(e){
    e.preventDefault();
    $('#menuMain').css('display', 'none');
//$(this).data("menu") have the id of the element to display
    $($(this).data("menu")).css('display', 'block');
    $('button').html('MAIN MENU');
});

http://jsfiddle.net/zXe7C/4/
for the example i only add the data attribute to the last two links but if you dont have something on the attribute or some element with that id then it will only hide "menuMain" but you can check first if the element $($(this).data("menu")) exists


Simple star rating system I'm working on a simple star rating system, and before I move on to actually "tallying" the votes, which I figure I'll just do with $.ajax, I wanted to make sure what I've done so far has been done as well as it can be.

JavaScript

//max number of stars
var max_stars = 5;

//add images to the ratings div
function fill_ratings() {
    var $rating = $("#rating");
    if($rating.length) {
        for(i = 1; i <= max_stars; i++) {
            $rating.append('');
            }
        }
    }

//variables...
var hovered = 0;
var has_clicked = false;
var star_clicked = 0;

function handle_ratings() {

    if(has_clicked == true) {
        for(i = 1; i <= star_clicked; i++) {
            $('#rating img#' + i).attr('src', 'star.png');
        }
    }

    $('#rating img').hover(function() {
        hovered = Number($(this).attr('id'));
        for(i = 1; i <= max_stars; i++) {
            if(i <= hovered) {
                $('#rating img#' + i).attr('src', 'star.png');
                } else {
                    $('#rating img#' + i).attr('src', 'star_default.png');
                }
            }
        }).click(function() {
                //if they click a star, only set the stars to grey that are after it.
                star_clicked = Number($(this).attr('id'));
                has_clicked = true;
                    for(i = 1; i <= star_clicked; i++) {
                        $('#rating img#' + i).attr('src', 'star.png');
                        //handle the vote here. eventually.
                    }
                }).mouseout(function() {
                    //if they haven't clicked a star, set all stars to grey.
                    if(has_clicked !== true) {
                            $('#rating img').attr('src', 'star_default.png');
                        } else {
                            //show their rating
                            for(i = 1; i <= max_stars; i++) {
                                if(i <= star_clicked) {
                                    $('#rating img#' + i).attr('src', 'star.png');
                                } else {
                                    $('#rating img#' + i).attr('src', 'star_default.png');
                                }
                            }
                        }
                    });
                }

$(function() {
    fill_ratings();
    handle_ratings();
    });

HTML

The code works, but I was wondering if anyone notices any code that I can improve. Also, is there anything wrong with the way it...looks? Should I be commenting more, indenting differently, or any other "formatting" type things?

The obvious improvement to the code you have here is that you should package it up as a jQuery plugin, to improve code reusability and reduce the number of global variables created.

You'll also want to use a class instead of id, to improve reusability (if you chose not to change this to a plugin), and to store as much information on the element themselves, instead of using global variables to store information, to create less coupled functions.

Otherwise, going down the code line by line:

Inside the fill_ratings function:

$rating.append('<img id="' + i + '"src="star_default.png" />');

You are creating elements with id attribute starting with a number. This is illegal before HTML5. Also, you can create elements in a more 'jQuery' manner with this syntax instead:

$('<img>', {
    id: 'star-' + i,
    src: 'star_default.png'
}).appendTo($rating);

The main benefit of this is that now you have a reference to element you just created. You can then push it to an array, to reuse later, instead of creating new jQuery objects by attempting to select the same elements again later in the code.

Variable declaration - mostly a style suggestion, but you can avoid multiple vars by using the comma operator:

var hovered = 0,
    has_clicked = false,
    star_clicked = 0;

Don't use $(this) to get element attributes:

$(this).attr('id');

is about 97% slower than

this.id;

the latter is less verbose and much much more efficient.

Inside each of the event handlers attached to #rating img - instead of looping through all #rating img element, like you're doing:

    for (i = 1; i <= max_stars; i++) {
        if (i <= hovered) {
            $('#rating img#' + i).attr('src', 'star.png');
        } else {
            $('#rating img#' + i).attr('src', 'star_default.png');
        }
    }

We can always use the information already given (the current element triggering the event) to do the processing. The above code, for instance, can be written as:

$(this).prevAll().attr('src', 'star.png');

thereby reducing the all those lines into one, and removing a variable. Most of the other event handlers can be similarly rewritten.

Type coercion to Number can be done without using Number - just append a + in front of the variable. In your case, it is completely unnecessary, since you're coercing it back to String again after joining it to another string in the selector passed to the jQuery selector


User registration - Controller, service and repository layers in Spring framework I'm writting a registration feature currently and I I'm confused how to divide logic between layers. Can you review this code and tell if it's good way to do? In try-catch block I publish event to send verification email to new User.

Should I keep in my Controller class just AuthService field? I'm using userRepository to check if username and email is available.
In AuthService should I keep just authRepository and other services fields? Currently I'm using there RoleRepository.
Is return type in my controller a good practise? ApiRespose is two-field class.

Controller:

@PostMapping("/signup")
public ResponseEntity<?> registerUser(@Valid @RequestBody SignUpRequest signUpRequest, HttpServletRequest httpServletRequest) {
    if (userRepository.existsByUsername(signUpRequest.getUsername())) {
        return new ResponseEntity(new ApiResponse(false, "Username is already in use!"), HttpStatus.BAD_REQUEST);
    }

    if (userRepository.existsByEmail(signUpRequest.getEmail())) {
        return new ResponseEntity(new ApiResponse(false, "Email is already in use!"), HttpStatus.BAD_REQUEST);
    }

    User user = new User(signUpRequest.getName(), signUpRequest.getUsername(), signUpRequest.getEmail(), signUpRequest.getPassword());

    User resultUser = authService.createUser(user);

    try {
        String host = httpServletRequest.getHeader("apphost");
        eventPublisher.publishEvent(new OnRegistrationCompleteEvent(resultUser, host));
    } catch (Exception e) {
        e.printStackTrace();
    }

    URI location = ServletUriComponentsBuilder
            .fromCurrentContextPath().path("/users/{username}")
            .buildAndExpand(resultUser.getUsername()).toUri();

    return ResponseEntity.created(location).body(new ApiResponse(true, "User registered successfully"));
}

Service:

@Override
public User createUser(User user) {
    user.setPassword(passwordEncoder.encode(user.getPassword()));

    Role userRole = roleRepository.findByName(RoleName.ROLE_USER)
            .orElseThrow(() -> new AppException("User Role not set."));

    user.setRoles(Collections.singleton(userRole));

    return userService.saveRegisteredUser(user);
}

Should I keep in my Controller class just AuthService field? I'm using userRepository to check if username and email is available.

I think it is ok to use the UserRepository directly, but that is a matter of taste. You could create an additional UserService to wrap the calls to the UserRepository, or you could also create these wrapping methods in your AuthService.

Example:

public class UserService/AuthService {

   private UserRepository userRepository;

   public boolean userExistsByUsername(String username) {
       return userRepository.existsByUsername(username);
   }
}

Then you can use these UserService/AuthService methods in your controller.

For me, at this point, this looks like an unnecessary indirection, but this is the classic way of dividing layers.

  In AuthService should I keep just authRepository and other services
  fields? Currently I'm using there RoleRepository.

Same answer as for question 1. You could write a RoleService class which wraps the RoleRepository calls.

  Is return type in my controller a good practise? ApiRespose is two-field class.

I think that's totally ok, because you want to set different HTTP status codes.

Other things:

HTTP status code

new ApiResponse(false, "Username is already in use!"), HttpStatus.BAD_REQUEST);

Just a hint, the correct HTTP status code for existing entities is 409, so I would use HttpStatus.CONFLICT. But 400 is also ok.

Exception handling

This code

Role userRole = roleRepository.findByName(RoleName.ROLE_USER)
        .orElseThrow(() -> new AppException("User Role not set."));

is not inside a try-catch block so if this exception is not handled in another way by a @Provider or some @ExceptionHandler, you may improve this part. If you handling it globally, everything is fine.


Cancelling A Task Without Cancellation Token Some Task methods don't take cancellation tokens. This is a problem because some long-running tasks may never finish and there is no way to send them a cancellation request. This seems a bit dodgy to me but this is a workaround. Is there anything wrong with this? Is there a better way?

public static Task<T> SynchronizeWithCancellationToken<T>(this Task<T> task, int delayMilliseconds = 10, CancellationToken cancellationToken = default)
{
    if (task == null) throw new ArgumentNullException(nameof(task));

    while (!task.IsCompleted && !task.IsFaulted && !task.IsCanceled)
    {
        Thread.Sleep(delayMilliseconds);

        if (cancellationToken.IsCancellationRequested)
        {
            return Task.FromCanceled<T>(cancellationToken);
        }
    }

    return task;
}

Example

The methods where this is a problem tend to be obscure APIs that haven't had much love in a long time or APIs that are wrappers for other platforms. One example is RequestWaitAsync on UsbDeviceConnection. It's a wrapper for an Android USB API . The issue can be worked around like so:

var buffers = await GetReadResultAsync(bufferLength, byteBuffer).SynchronizeWithCancellationToken(cancellationToken);

Another way to workaround the issue might be with Task.Run

public async Task<ReadResult> ReadAsync(uint bufferLength, CancellationToken cancellationToken = default)
{
    return await Task.Run(async () =>
    {
        try
        {

            //...

            return await GetReadResultAsync(bufferLength, byteBuffer);

        }
        catch (Exception ex)
        {
            //...
        }
    }, cancellationToken);
}

Which of these two work arounds are the safest?

Unit Test

This is a pathetic attempt at a unit test. It passes:

[TestMethod]
public async Task TestSynchronizeWithCancellationToken()
{
    var stopWatch = new Stopwatch();
    stopWatch.Start();

    var completed = false;

    var task = Task.Run<bool>(() =>
    {
        //Iterate for one second
        for (var i = 0; i < 100; i++)
        {
            Thread.Sleep(10);
        }

        return true;
    });

    var cancellationTokenSource = new CancellationTokenSource();

    //Start a task that will cancel in 500 milliseconds
    var cancelTask = Task.Run<bool>(() =>
    {
        Thread.Sleep(500);
        cancellationTokenSource.Cancel();
        return true;
    });

    //Get a task that will finish when the cancellation token is cancelled
    var syncTask = task.SynchronizeWithCancellationToken(cancellationToken: cancellationTokenSource.Token);

    //Wait for the first task to finish
    var completedTask = (Task<bool>) await Task.WhenAny(new Task[]
    {
        syncTask,
        cancelTask
    });

    //Ensure the task didn't wait a long time
    Assert.IsTrue(stopWatch.ElapsedMilliseconds < 1000);

    //Ensure the task wasn't completed
    Assert.IsFalse(completed);
}

Starting with the first function, I would make a few changes:

Task.IsCompleted covers all completion states already (https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.iscompleted?view=netframework-4.8)
Tasks are all about not blocking the current thread. Never use Thread.Sleep and always use Task.Delay.
Cancellation can be more succinctly handled with Task.ThrowIfCancellationRequested

public static async Task<T> SynchronizeWithCancellationToken<T>(this Task<T> task, int delayMilliseconds = 10, CancellationToken cancellationToken = default)
{
    if (task == null)
        throw new ArgumentNullException(nameof(task));

    while (!task.IsCompleted)
    {
        await Task.Delay(delayMilliseconds);
        cancellationToken.ThrowIfCancellationRequested();
    }

    return await task;
}

This is about as good as it gets to "add cancellation" to a Task that doesn't do cancellation. The key to understanding Task cancellation in .NET is that it is cooperative. This means that simply passing a cancellation token around doesn't do anything on its own; instead the Task implementation itself has to handle cancellation. This makes a lot of sense since only the Task implementation itself knows when best to allow cancellation without causing invalid state or corruption.

I would definitely recommend taking the time to do a read-through of the MSDN article on this: https://docs.microsoft.com/en-us/dotnet/standard/parallel-programming/task-cancellation?view=netframework-4.8

Your second function attempts to use the Task.Run overload that takes a CancellationToken in order to cancel the operation. However, Task.Run only uses the token to avoid starting the Task (if it's already cancelled when it goes to run it), and it's left to the user to use the token inside the implementation passed to Task.Run to handle further cancellation. See here: https://stackoverflow.com/questions/22637642/using-cancellationtoken-for-timeout-in-task-run-does-not-work

The first method is definitely the one you want to use. Just remember that it isn't true cancellation which would require you to modify the implementation of the API function and add the cooperative cancellation bits. When you add cancellation like this, without the implementation bits, you're only cancelling the wait for the Task to finish, not the Task itself. If the Task has further operations to carry out, they will still be carried out. This can often times be a deal breaker with some APIs, and you might be forced to find an alternative implementation that handles cancellation, or even implement your own.

Edit:

It also slipped my mind that you can use CancellationToken.Register to asynchronously wait and get rid of the polling...

    public static async Task ToTask(this CancellationToken token)
    {
        var source = new TaskCompletionSource<object>();
        using (token.Register(() => source.SetResult(null)))
            await source.Task;
    }

    public static async Task<T> SynchronizeWithCancellationToken<T>(this Task<T> task,
        CancellationToken cancellationToken = default, string message = default)
    {
        if (task == null)
            throw new ArgumentNullException(nameof(task));

        if (await Task.WhenAny(task, cancellationToken.ToTask()) == task)
            return await task;
        else
            throw new OperationCanceledException(message ?? "The operation was canceled.");
    }


Model object for form validation In order to perform form validation, I have created a custom object:

define(["jquery", "validate", "activities/new/common/components/BaseFormdata"],
    function($, Validate, BaseFormData){

    var FormData = function(options){

        this.defaults = {
            name: "",
            privacy: "",
            price: "",
            short_description: "",
            long_description: ""
        };

        //Constraints to be used by the validation library
        this.constraints = {
            name: {
                presence: true,
                length: { maximum: 100, minimum: 1 }
            },
            privacy: {
                presence: true
            },
            short_description: {
                short_description: true,
                length: { maximum: 20 }
            },
            max_description: {
                length: { maximum: 1000 }
            }
        };

        this.formData = $.extend({}, this.defaults, options);

    };

    //Inherit from the BaseFormData component
    FormData.prototype = new BaseFormData();

    return FormData;
});

The base FormData object:

define(["underscore", "validate"], function(_, Validate){

    //Defines a Base FormData object that modules can use to inherit from
    function BaseFormData(){};

    /*@param:
     * object containing to be validated properties
     * @return:
     * Success returns false (no error), Error returns an error object
     */
    BaseFormData.prototype.validateProperties = function(objToValidate){

        var constraints = this.getConstraints(objToValidate);

        var status = validate(objToValidate, constraints, {format: "detailed"});
        if(status === undefined){
            //There is no error
            for(var key in objToValidate){
                if (this.formData.hasOwnProperty(key)) {
                    //Set the value on the this.formData object
                    this.setProperty(key, objToValidate[key]);
                };
            };
            //Return no error
            return false;

        } else {
            //An error object is returned
            return status;
        }
    };

    //Set property on object
    BaseFormData.prototype.setProperty = function(key, value){
        this.formData[key] = value;
    };

    //Get relevant constraints from constraints object
    BaseFormData.prototype.getConstraints = function(objToValidate){
        //Get keys in array
        var arrKeys = _.allKeys(this.constraints);
        var constraints = {};
        _.each(arrKeys, _.bind(function(key){
            if(objToValidate.hasOwnProperty(key)){
                constraints[key] = this.constraints[key];
            };
        }, this));

        return constraints;
    };

    return BaseFormData;
});

Is it bad practice to set the form properties immediately on the FormData object? Currently, I have per form one custom FormData object that inherits from the Base FormData object, for which I have set custom properties depending on the form components.

I wonder if it would be better to inject the form fields as a collection of objects in a single FormData object (instead of creating one for each form); this would allow me to define only the base FormData object, but it would require the calling function to also send the constraints along (which is in my case a presenter object that coordinates between the different views that manage the form).

Which is the best approach architectural-wise?

First of all, I think creating a constructor for each form type is overkill. All you need from that subclass are defaults and constraints. Why not remove all that wrapper code and just have the module return defaults and constraints?

// FormDataRules.js
define(function(require, exports, module){
  exports.defaults = { ... };
  exports.constraints = { ... };
});

Then create a validation module that contains all your validation APIs. Doing the above, setProperty falls off since you'll be operating on plain objects.

getConstraints also falls off leaving you with validateProperties which we can just rename to validate - which is a huge operation which just delegates checking to subfunctions inside the module.

// Validator.js
define(function(require, exports, module){

  exports._validateNumbers = function(data, constraint){ ... }
  exports._validatePresence = function(data, constraints){ ... }

  exports.validate = function(data, constraints){
    /* Huge operation to validate each item with each constraint it comes */
  };
});

Here's sample code using above.

define(function(require){
  var Validator = require('Validator');
  var FormDataRules = require('FormDataRules');
  var _ = require('lodash');

  var dataWithDefaults = _.defaults({
    // instance stuff
  }, FormDataRules.defaults);

  var isDataPassed = Validator.validate(dataWithDefaults, FormDataRules.constraints);

});

This makes everything easy to test. FormDataRules are just objects you can simply check values. Validator is a bunch of functions you can simply test by throwing values at it. All the data that mutates lives in a plain object that we have hold to (dataWithDefaults), and not something internal to the validator nor the rules.