Refractoring OOP vs. Functional Programming Approach I am writing a python script that will call mandrills export activity api for every blog on a worpress mu install except for the main site. The script is designed to be called from a cron job on a regular schedule. The code I have is here.

__author__ = 'Taylor'

import datetime
import mandrill
import phpserialize
import mysql.connector

class BlogReporter(object):
    blog_id = None
    table_name = 'wp_%s_options'
    notify_email = None
    sender_email = None
    date_to = None
    date_from = None

    def __init__(self, blog_id_, database_config_, mandrill_client_):
        self.blog_id = blog_id_
        self.mandrill_client = mandrill_client_
        self.table_name = 'wp_' + str(blog_id_) + '_options'
        try:
            cnx_ = mysql.connector.connect(**database_config_)
        except mysql.connector.Error as err:
            print(err)
        else:
            query_ = "SELECT option_value FROM " + self.table_name + " WHERE option_name=%s"
            try:
                cursor_ = cnx_.cursor()
                cursor_.execute(query_, ('admin_email',))
                result = cursor_.fetchone()
            except Exception as err:
                print(err)
            else:
                # set notify email from cursor
                self.notify_email = result
                cursor_.close()
            try:
                cursor_ = cnx_.cursor()
                cursor_.execute(query_, ('wysija',))
                encoded_result = cursor_.fetchone()[0]
            except Exception as err:
                print(err)
            else:
                decoded_result = phpserialize.unserialize(encoded_result.decode('base64', 'strict'))
                self.sender_email = decoded_result['from_email']
                cursor_.close()
            cnx_.close()
        self.date_to = datetime.datetime.now()
        # if it monday lets get activity since saturday
        if self.date_to.weekday() is 0:
            self.date_from = self.date_to - datetime.timedelta(days=2)
        # else just get the activity from the past day
        else:
            self.date_from = self.date_to - datetime.timedelta(days=1)

    def export_activity(self):
        self.mandrill_client.exports.activity(self.notify_email, self.date_from, self.date_to, None,
 self.sender_email,'Sent')

# database name and credentials
# these should eventually be parsed from command line arguments
database_config = {
    'user': 'root',
    'password': 'password',
    'database': 'wpmudb'
}
# Mandrill API Key
# this should also be parsed from command line
mandrill_api_key = 'xxxxx_xxxxxxxxxxxxxxxx'
mandrill_client = mandrill.Mandrill(mandrill_api_key)

try:
    cnx = mysql.connector.connect(**database_config)
except mysql.connector.Error as err:
    print(err)
else:
    try:
        cursor = cnx.cursor()
        # Get all blogs but blog_id 1 since the main site needs no report
        query = "SELECT blog_id FROM wp_blogs WHERE blog_id != '1'"
        cursor.execute(query)
    except Exception as err:
        print(err)
    else:
        for blog_id in cursor:
            blog = BlogReporter(blog_id[0], database_config, mandrill_client)
            blog.export_activity()
            del blog
        cursor.close()
    cnx.close()

I wrote it like this because it is the first program I have written in python and I was unsure of how to structure it and wanted to learn more about classes in python. However, recognizing that in the programs current state Class BlogReporter's constructer and method export_activity could be refactored into one function that accepts the arguments of class BlogReporter's constructor and is simply the class's constructor plus the method export_activity; I was curious if doing so would be beneficial to the program in any way.

I would also like to welcome comments as to whether the mandrill client should be reinstantiated for each instance of BlogReporter or whether it is better to pass the same client to each instance.

Here are some points:

  and wanted to learn more about classes in python

If you write some piece of code to learn something, delete it and rewrite it.

  construct[o]r and method export_activity could be refactored into one function

If you have a class with a single method converting to a function is what you should do by default. By by default I mean unless you have a really solid reason to do otherwise.

  I was curious if doing so would be beneficial to the program in any way.

Simplicity is second only to (actually needed) functionality. (As XP people say Do the simplest thing that could possibly work.) You do not have to justify making your program simpler by appealing to a higher value.

Some more specific points:

The sections of code where you read an option from the database is the same almost word for word. If you find yourself writing the same code twice or more tellingly *Copy&Paste*ing some snippet you should put it in a function and call it instead. This way you will save yourself a lot of effort. As a general rule you should write everything once and only once.

Your constructor does too much. Anything other that assigning fields should not be done in a constructor. But your constructor is too long even for a method. Extract every snippet of code that is doing something in a function so that when you (or someone else) read it again instead of trying to guess what you are trying to do by reading how you do it you would just read what you are trying to do. This way you (or others) may skip reading the extracted function altogether if they are not interested.

Clean up of resources (files, connections, cursors etx) are usually done by initializing them in with blocks. If the resource does not support with blocks out-of-the box but cleaning-up consists of calling .close() on it, you can wrap the resource in contextlib .closing The clean-up should otherwise be done in finally blocks to ensure they are executed even if an exception is thrown; at least in long running programs, server or desktop applications.

When you decide to catch an exception, catch the most specific exception you expect. Look what else are you catching when you say catch Exception. Here root exception of all mysql.connector errors (apparently mysql.connector.Error) is more suitable.

If you are not going to do anything useful (recover, wrap&rethrow etc) do not catch exceptions. You can do print(err) somewhere else.

In a script put your code in a function, for example main, and call it if __name__ == "__main__": main(). That way you can debug your code in repl, or import it in other scripts/programs as a module. This also allows further refactorings, which I won't elaborate but you can see in the code below.

If a method has many parameters and you are calling it many times with same parameters you can use functools.partial to reduce the noise. (see below)

Also cursor.fetchone() apparently returns tuple and I'm assuming you do not want to pass a tuple as notify_email parameter so you are probably missing a [0] after the fetchone() when reading the admin_email.

Caveat: I am not a python programmer. And haven't tried the program below.
Also the refactored code still needs some Inversion of control refactoring, but ignore this if you do not already know what that means from other languages.

def get_option(cnx, blog_id, option_name):
    table_name = 'wp_' + str(blog_id) + '_options'
    query_ = "SELECT option_value FROM " + table_name + " WHERE option_name=%s"
    with closing( cnx.cursor() ) as cursor_:
        cursor_.execute(query_, (option_name,))
        return cursor_.fetchone()[0]

def get_timespan():
    date_to = datetime.datetime.now()
    # if it monday lets get activity since saturday
    if date_to.weekday() is 0:
        date_from = date_to - datetime.timedelta(days=2)
    # else just get the activity from the past day
    else:
        date_from = date_to - datetime.timedelta(days=1)
    return (date_from, date_to)

def export_blog_activity(blog_id, cnx, mandrill_client):
    notify_email = get_option(cnx, blog_id, 'admin_email')
    encoded_result = get_option(cnx, blog_id, 'wysija')
    decoded_result = phpserialize.unserialize(encoded_result.decode('base64', 'strict'))
    sender_email = decoded_result['from_email']
    date_from, date_to = get_timespan()
    mandrill_client.exports.activity(notify_email, date_from, date_to, None, sender_email,'Sent')

def main():
    # database name and credentials
    # these should eventually be parsed from command line arguments
    database_config = {
        'user': 'root',
        'password': 'password',
        'database': 'wpmudb'
    }
    # Mandrill API Key
    # this should also be parsed from command line
    mandrill_api_key = 'xxxxx_xxxxxxxxxxxxxxxx'
    mandrill_client = mandrill.Mandrill(mandrill_api_key)
    with closing(mysql.connector.connect(**database_config)) as cnx:
        export_activity = functools.partial(export_blog_activity, mandrill_client = mandrill_client, cnx=cnx)
        with closing( cnx.cursor() ) as cursor_:
            # Get all blogs but blog_id 1 since the main site needs no report
            query = "SELECT blog_id FROM wp_blogs WHERE blog_id != '1'"
            cursor_.execute(query)
            for row in cursor_:
                export_activity(row[0])

if __name__ == "__main__":
    try:
        main()
    except mysql.connector.Error as e:
        print "Sql error:", e


Wrapper for dynamically-allocated memory block I have built a single header-only memory class (just a C++ wrapper around a dynamically-allocated pointer and a size) that supports copy and move semantics (I hope I implemented them correctly) for a game I'm building. Its use case is to wrap the raw contents of assets imported (from any source) before processing them. So there is:

#include <cstdint>
#include <algorithm>

class Memory
{
    uint8_t *ptr;
    size_t size;

public:
    inline Memory() : ptr(nullptr), size(0) {}
    inline Memory(size_t size) : ptr(new uint8_t[size]), size(size) {}
    inline Memory(const Memory &other) : Memory(other.size)
    {
        std::copy(other.ptr, other.ptr+size, ptr);
    }
    inline Memory(Memory &&other) : Memory() { swap(*this, other); }

    inline Memory& operator=(Memory other)
    {
        swap(*this, other);
        return *this;
    }

    inline ~Memory() { delete[] ptr; }

    inline const void* get_ptr() const { return ptr; }
    inline void* get_ptr() { return ptr; }
    inline size_t get_size() const { return size; }

    friend inline void swap(Memory &m1, Memory &m2)
    {
        using std::swap;
        swap(m1.ptr, m2.ptr);
        swap(m1.size, m2.size);
    }
};

Is there something I forgot to add? Are there any errors? Can it be improved in any way?

Code Review
Namespace
It would be nice to put your code inside its ownnamespace to avoid potentially collisions. Memory would seem like a good general name that would be reused by other people at some point.
Style
This is minor.
But most C++ developers put the * beside the type not the name. This is the reverse of the standard C style which is to put the * beside the name.
    uint8_t *ptr;

C++ is all about the type of the object. Putting the star with the type helps to correctly associate the type as a pointer.
Don't use inline unless you are required.
Anything inside the class declaration also automatically marked inline. Also the keyword inline has little to no affect on the compiler (ie it is ignored by every modern compiler) and its decision to actually inline the code.
    inline Memory() : ptr(nullptr), size(0) {}

The only time it is needed is when a method is defined in a header file included in multiple compilation units. Then it is used by the linker to note that the one definition rule is not being broken.
Constructors
All the constructors are good. Though I did have to double check the copy constructor.
    inline Memory() : ptr(nullptr), size(0) {}
    inline Memory(size_t size) : ptr(new uint8_t[size]), size(size) {}
    inline Memory(const Memory &other) : Memory(other.size)
    {
        std::copy(other.ptr, other.ptr+size, ptr);
    }
    inline Memory(Memory &&other) : Memory() { swap(*this, other); }

BUT: The move constructor should also be marked noexcept (except that your move constructor will not throw). This does not do anything for the class itself. But it allows for optimizations in the standard library if you put your class in a container.
Example: When resizing a std::vector<Memory> it will use a noexcept move constructor if it exists otherwise it has to fall back to the copy constructor to provide that strong exception guarantee.
Assignment
This is wrong. I missed the copy caused by pass by value in the parameter

    inline Memory& operator=(Memory other)
    {
        swap(*this, other);
        return *this;
    }

The copy assignment operator has the semantics of doing move assignment. I don't think this is a good idea. You might get some surprised people that don't see that coming when there Memory object changes under the noses when they do an assignment.
I would convert this to a copy operator and then create a move assignment operator (don't forget to mark the move assignment as noexcept).

I would still add a move assignment operator (since you have move constructor).


Compute the number of ways a given amount (cents) can be changed Given an infinite number of different coin types (such as pennies,
  nickels, dimes, quarters) find out how many ways n cents can be
  represented.

My code appears to work (although I am curious to know if it has any correctness issue). But I feel like the memoization I am doing is a bit inelegant. Can we do without dictionaries/maps, perhaps a dynamic programming based approach using 2d arrays? Or is that even worse in terms of time and space complexity?

Also is my code to update the memoized_sol good in terms of coding technique?

'''
Parameters:
    cents: amount to get change for.
    coin_vals: list of coin denominations in no particular order.

Returns:
    number of ways <cents> can be changes using any number of coins from the given list
'''
def get_coin_change_count (cents, coin_vals):
    memoized_sol = {}
    return compute_coin_change_count(cents, coin_vals, 0, memoized_sol )

def compute_coin_change_count (rem_cents, coin_vals, coin_index, memoized_sol ):

    if coin_index in memoized_sol:
        if rem_cents in memoized_sol[coin_index]:
            return memoized_sol[coin_index][rem_cents]
    else:
        memoized_sol[coin_index] = {}

    if rem_cents == 0:
        return 1

    if coin_index >= len(coin_vals):
        return 0

    coin_val = coin_vals[coin_index]

    i = 0
    count = 0
    while i*coin_val <= rem_cents:
        count = count + compute_coin_change_count\
            ( rem_cents - i*coin_val, coin_vals, coin_index+1, memoized_sol )
        i = i + 1

    memoized_sol[coin_index][rem_cents] = count
    return count

w = get_coin_change_count ( 37, [10, 1, 5, 25])
print (w)

I don't see any correctness issues, but it could be more idiomatic.

    if coin_index in memoized_sol:
        if rem_cents in memoized_sol[coin_index]:
            return memoized_sol[coin_index][rem_cents]
    else:
        memoized_sol[coin_index] = {}

Check out first defaultdict, and then functools.lru_cache for ways to simplify the memoisation - although note that lru_cache has a subtlety in that it doesn't like list as an argument type.

    if rem_cents == 0:
        return 1

    if coin_index >= len(coin_vals):
        return 0

IMO these should have gone before the memoisation code, because they are non-memoised special cases. But I freely admit that this is mainly a matter of opinion. The only real difference effected would be to avoid initialising memoized_sol[len(coin_vals)].

    i = 0
    count = 0
    while i*coin_val <= rem_cents:
        count = count + compute_coin_change_count\
            ( rem_cents - i*coin_val, coin_vals, coin_index+1, memoized_sol )
        i = i + 1

Using sum and range this could be simplified to

count = sum(compute_coin_change_count(
        surplus, coin_vals, coin_index + 1, memoized_sol)
    for surplus in range(rem_cents, -1, -coin_vals[coin_index]))

(I'm not quite sure what the most Pythonesque indentation would be - I wouldn't normally follow PEP8 on maximum line lengths).

I would also be tempted to use sublists and eliminate the need for coin_index, but that depends on how you're handling memoisation.

Putting it all together, I get

from functools import lru_cache

def get_coin_change_count(cents, coin_vals):
    @lru_cache(None)
    def inner(remaining, idx):
        if remaining == 0:
            return 1

        if idx == len(coin_vals):
            return 0

        return sum(inner(surplus, idx + 1)
                   for surplus in range(remaining, -1, -coin_vals[idx]))
    return inner(cents, 0)

print(get_coin_change_count(37, [10, 1, 5, 25]))

and here the indentation does pass PEP8, in part because of shorter names. Note that using an inner function allows memoisation with lru_cache and allows the short name inner because its scope is restricted enough that this is sufficiently expressive.


Abstract classes and interfaces in a game inventory system After reading a lot about interfaces, which I'm new to, I've tried implementing it in my inventory system for a game I'm working on. I have managed to create something proof of concept which seems to work as intended, but I wanted to check with more experienced C# programmers if this is the correct/best approach?

The player has an inventory, an inventory has several slots, and each slots can have several items. The player can use any of the different items (or, more precisely, item types), but how they are used differs; food items can be eaten, weapon items can be equipped etc.

So I have started with this:

public interface IUser<Item> {
    void Use ( Item usableItem );
}

public class Player : IUser<Item> {
    public float Energy { get; set; }

    public Player () {
        Energy = 0.0f;
    }

    public void Use ( Item usableItem ) {
        usableItem.Use ( this );
    }
}

public interface IUsable<Player> {
    void Use ( Player player );
}

public abstract class Item : IUsable<Player> {
    public string Name { get; set; }

    public Item () {
        Name = "ITEM WITH NO NAME";
    }

    public virtual void Use ( Player player ) {
        // Nothing here?
    }
}

public abstract class Food : Item {
    public float Energy { get; set; }

    public Food () {
        Name = "FOOD WITH NO NAME";
        Energy = 1.0f;
    }

    public override void Use ( Player player ) {
        player.Energy += Energy;
    }
}

public class Meat : Food {
    public Meat () {
        Name = "Meat";
        Energy = 10.0f;
    }
}

Does this at all make sense?

I think your design introduces a few more dependencies than it should.

Let's have a look at your IUsable interface

public interface IUsable<Player> {
    void Use ( Player player );
}

Do you really need to have a dependency on Player? Why don't you just require it to depend on IUser?

I'd also completely get rid of the Item class. What do you need it for? If you want an abstract class to represent a NamedItem you can create it but you should do it in the following way:

public abstract class NamedItem implements IUsable<IUser>
{
    public string Name {get; private set;} // Better to keep it immutable

    protected NamedItem(string name)
    {
        Name = name;
    }

    public abstract void Use(IUser user);
    /*
     * This way we don't need to provide a
     * default implementation and your subclasses
     * will be forced to provide it.
     */
}

I think you should probably also revise who should be in charge of implementing the Use method. From the names you're using it is quite clear that it is more natural to think at a player that uses an item. That's fine but what your code suggests is that different objects should be used in different ways, hence you need to implement it on the specific Item instance.

Does spreading the logic on how a player uses an item across a lot of different items makes sense in your project? If you don't want to do it you might want to have a look at dobule dispatch as a possible way to retain all the behaviours on player instead than on objects. If you do double dispatch you can have interfaces such as IFood and then have a method in Player with this signature void User(IFood food). IFood should expose a way to determine how many energy is added by the food to the player and the mutation of the state of the player should happen in the specific method you just created.

If you show us how you intend to use those classes we can help you find the best way to design them.


Applying EF core in Universal Windows Apps using a large database I am building my UWP app and really stuck in the architecture. For the past few days, I have been reading MVVM, Entity Framework, SQLite, Code First, etc and I am confused how I will implement everything.

In this tutorial, it shows the usage of a dbcontext derived class inside a function called Page_Loaded in MainPage.xaml.cs. Is this a bad design?

I made my own implementation below because I don't want to expose SQL queries on my view XAML files. I am completely unsure if this is better or if I can still improve it or replace it with a better one?

MyDbContext.cs

public class MyDbContext : DbContext
{
    internal DbSet<Person> People { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlite("Filename=mydbcontext.db");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Person>().Property(p => p.Id).IsRequired();
        modelBuilder.Entity<Person>().Property(p => p.Name).IsRequired();
    }
}

Repository.cs

public class Repository
{
    public List<Person> People()
    {
        using (var db = new MyDbContext())
        {
            return db.People.ToList();
        }
    }

    public Person GetPersonByName(string name)
    {
        using (var db = new MyDbContext())
        {
            Person person = (from p in db.People
                             where p.Name == name
                             select p).FirstOrDefault();

            return person;
        }
    }
}

MainPage.xaml.cs

private void Page_Loaded(object sender, RoutedEventArgs e)
{
    entriesListView.ItemsSource = _repo.People();
}

Another thing: if I add many tables, my Repository.cs will grow so large that it might reach thousands of lines, how do you break this one? I might add 30 more tables inside so I could imagine how it will look cluttered inside Repository.cs file.

In this tutorial, it shows the usage of a dbcontext derived class inside a function called Page_Loaded in MainPage.xaml.cs. Is this a bad design?

Yes, I think it is a bad design and implementing a repository the way you did it is the right approach.

  Another thing: if I add many tables, my Repository.cs will grow so large that it might reach thousands of lines, how do you break this one? I might add 30 more tables inside so I could imagine how it will look cluttered inside Repository.cs file.

Let's talk about it later when you have that many tables but for now in order to avoid a file with thousands of lines (which is highly unlikely) you should already start with different repositories. Now you have only the Respository which is not that bad by itself but it can be better. In your case it should be a PersonRepository. So now you have one for people. When you add more tables later I'm pretty sure you can come up with another repository for them. It doesn't have to always be one repo for a table. You have have repositories that are all about people and work internally with mutliple person-related tables. Then there might be repositories that work with other data like files etc.

Just don't put all your code in a single file. Group the talbles together by what makes more sense.


Converting a state from hash into a JavaScript object This code gets a state from a query string hash part, converts the hash part to a JavaScript object and then tries to get a view, dashboard or a mode value. If those keys are not present in object, it returns $.pulse.qs.views.home.

Does this JavaScript code with ternary operator follow common best practices? It looks like ternary operator abuse and it is not readable. Or is it just me?

var state = $.query.getState();
var screen = state[$.pulse.qs.view]
    ? state[$.pulse.qs.view]
    : state[$.pulse.qs.dashboard] || state[$.pulse.qs.mode]
        ? $.pulse.qs.views.dashboard
        : $.pulse.qs.views.home;

I think a good practice is to name what you mean, cause that makes the code more readable. So I would create a var for each state and grap a good name: stateOfViewAvailable ...


HTML+CSS Template: Search Results List I'm trying to develop some templates for common HTML + CSS tasks I've been dealing with. One of which is a general search results list either from client- or server-side.

So far I'm using the following reusable code which seems to be working fine without any noticeable bug:

[Dummy Title] Lorem ipsum dolor sit amet, consectetur adipiscing elit. Suspendisse quis accumsan felis. Maecenas vel libero mi. Suspendisse potenti. Sed tempus ex eu diam imperdiet mattis ac ut orci. Sed non lectus libero. Suspendisse vitae nunc a quam mattis congue id eget tortor. Vivamus id arcu eros. Duis eu risus semper, dapibus quam in, tristique magna. Duis semper tempus dolor eget elementum. Duis arcu ipsum, suscipit a egestas quis, lobortis sit amet purus.

                        dummy notes 1
                        &bull; dummy notes 2
                        &bull; dummy notes 3

                        dummy notes 4
                        &bull; dummy notes 5
                        &bull; dummy notes 6

Some main characteristics of this code are:

the text content has a maximum of 5 lines (title + notes) independently of the amount of text content
independently of the number of title lines, the notes are always immediately bellow (no variable gap)
the maximum number of title lines automatically adjust according to the number of notes lines included or removed

Interested in:

Is it possible to simplified it? There is a nest of
,  and flexbox that seems to me overly complicated for such a simple task ...
Any standard nomenclature or other best practices?
Any ideas to improve its design and appearance (without JS)?

If the results are ranked (typically by relevance), you should use an ol instead of a ul.

It’s best practice to use sectioning content elements (article, aside, nav, section) if you use heading elements (h1-h6). In the generel case, go with section. If the results are articles (e.g., blog posts) or pages, go with article.

It’s a usability question, but typically only the title is linked, not everything. This could make it easier for your users to mark the text in the description, and to scroll on touch devices without accidentally opening the links.

The img must have an alt attribute. The value depends on what kind of images you display here. If it’s just an icon representing that it’s a search result, the value should be empty.

Title

notes 1

notes 2

Related: my answer about HTML for search engine result lists


Allowing users to view and manage articles This is a controller from an Express app that lets users view and manage articles. There are at least a couple of issues I'd like to address...

The program displays a list of articles in a sidebar that appears on each page. Right now, the app queries the database for article names, caches them, and includes them in the payload. It does this for each route that loads a view.

Is there a more efficient way to do this?

A good bit of code is repeated in the first two methods (retrieve and settings). Both methods get a list of articles for the sidebar and show an article or the settings for an article.

Is there a way to DRY up this code?

const { Article } = require('../models');
import CacheService from '../services/cache';

const ttl = 60 * 60 * 1;
const cache = new CacheService(ttl);

module.exports = {
  // display one article
  retrieve (req, res, next) {
    // list of articles displayed in the sidebar
    const allArticles = module.exports.getUserArticles(req.user.id);

    // the article to display
    const thisArticle = module.exports.getArticleById(req.params.publicId);

    Promise
      .all([allArticles, thisArticle])
      .then((result) => {
        const articles = result[0];
        const article = result[1];

        if (!article) {
          const error = new Error('Article not found');
          error.status = 404;
          return next(error);
        }

        return res.render('article', { articles, article, title: article.name });
      })
      .catch(next);
  },
  // show settings page for an article
  settings (req, res, next) {
    // list of articles displayed in the sidebar
    const allArticles = module.exports.getUserArticles(req.user.id);

    // the article to display
    const thisArticle = module.exports.getArticleById(req.params.publicId);

    Promise
      .all([allArticles, thisArticle])
      .then((result) => {
        const articles = result[0];
        const article = result[1];

        if (!article) {
          const error = new Error('Article not found');
          error.status = 404;
          return next(error);
        }

        const { page } = req.params;
        const pageTitle = module.exports.getSettingsPageTitle(page);
        const pageTemplate = `settings-${page}`;

        return res.render(pageTemplate, { articles, article, title: `${article.name}: ${pageTitle}` });
      })
      .catch(next);
  },
  getUserArticles (userId) {
    return cache.get(`getUserArticles_${userId}`, () => Article.findAll({ where: { userId }, order: [['name']] }));
  },
  getArticleById (publicId) {
    return cache.get(`getArticleById_${publicId}`, () => Article.findOne({ where: { publicId } }));
  },
  getSettingsPageTitle (page) {
    return {
      edit: 'Edit name, URL',
      response: 'Edit email response',
      notifications: 'Configure notifications'
    }[page];
  }
};

Try to envision how you would want your code to look, then build from there.

For example, I would like for the articles in the sidebar to just be there, so I pretend I have a function I can call that just fixes this for me.

res.render('article', { articles, article, title: article.name })
vs
render('article', {article, title: article.name })

Where does this new render function come from? I don't care, just take it as a dependency.

The same thing could be said about the cache and the article lookup. Just pretend it is there already.

Other than that, you could:

Refactor to async/await
Handle errors using a wrapper around your controller functions instead of .catch(err)
Remove a few redundant comments
Use a class with dependencies injected

I ended up with the following after a quick refactor:

class ArticleController{
    constructor(articleRepo){
        this._articleRepo = articleRepo
    }

    async retrieve(req, res, render){
        const article = await this._getArticleOrFail(req.params.publicId)
        return render('article', {article, title: article.name})
    }

    async settings(req, res, render){
        const article = await this._getArticleOrFail(req.params.publicId)

        const {page} = req.params
        const pageTitle = this._getSettingsPageTitle(page)
        const pageTemplate = `settings-${page}`

        return render(pageTemplate, {article, title: `${article.name}: ${pageTitle}`})
    }

    async _getArticleOrFail(id){
        const article = await this._articleRepo.getArticleById(id)

        if(!article){
            const error = new Error('Article not found')
            error.status = 404
            throw error
        }

        return article
    }

    _getSettingsPageTitle(page){
        return {
            edit: 'Edit name, URL',
            response: 'Edit email response',
            notifications: 'Configure notifications'
        }[page]
    }
}

module.exports = ArticleController

To get this working you need a couple of higher-order functions around your controller functions. One to create the render function that automatically adds articles, and one that handles errors and passes them into next(). Lastly, you need to create an ArticleRepo that you can pass as a dependency.

Some ideas:

class SidebarLayout{
    constructor(articleRepo){
        this._articleRepo = articleRepo
    }
    withSidebarData(controllerFunction){
        return async (req, res) => {
            const articles = await this._articleRepo.getUserArticles(req.user.id)
            const render = (template, data) => res.render(template, {...data, articles})

            return controllerFunction(req, res, render)
        }
    }
}

const createRoute = routeFunction => (req, res, next) => routeFunction(req, res, next).catch(next)

const articleRoute = createRoute(
    sidebarLayout.withSidebarData(articleController.retrieve.bind(articleController))
)

app.get('article', articleRoute)

There are multiple other ways to do it as well. You could have a more general templating class to inject alongside articlerepo instead of the middleware solution, or you could even decide on the actual template to use outside of the controller and simply pass in a template. But the point is to offload the necessary bits of logic outside of the controller.


PHP review of object oriented GeoIP class I wrote a little PHP wrapper class for GeoIP which looks like:

<?php
class GeoIP {
    private $ip;
    private $geo_ip_details;

    function __construct($ip) {
        $this->ip = $ip;
        $this->fetch();
    }

    private function fetch() {
        if(filter_var($this->ip, FILTER_VALIDATE_IP)) {
            $curl = new Curl();
            $json = $curl->get_request("http://freegeoip.net/json/" . $this->ip, true);
            $result = json_decode($json);

            if($result !== null) {
                $this->geo_ip_details = $result;
            }
        }
    }

    public function __get($property) {
        if(property_exists($this, $property)) {
            return $this->$property;
        }
    }

    public function __toString() {
        if(isset($this->geo_ip_details) && !empty($this->geo_ip_details)) {
            if(isset($this->geo_ip_details->city) && !empty($this->geo_ip_details->city) && isset($this->geo_ip_details->region_name) && !empty($this->geo_ip_details->region_name) && isset($this->geo_ip_details->country_name) && !empty($this->geo_ip_details->country_name)) {
                return $this->geo_ip_details->city . ", " . $this->geo_ip_details->region_name . " " . $this->geo_ip_details->country_name;
            } else if(isset($this->geo_ip_details->region_name) && !empty($this->geo_ip_details->region_name) && isset($this->geo_ip_details->country_name) && !empty($this->geo_ip_details->country_name)) {
                return $this->geo_ip_details->region_name . " " . $this->geo_ip_details->country_name;
            } else if(isset($this->geo_ip_details->country_name) && !empty($this->geo_ip_details->country_name)) {
                return $this->geo_ip_details->country_name;
            }
        }
    }
}
?>

Usage is like:

//Constructor, and autocalls fetch()
$geoip = new GeoIP('64.87.28.98');

//Getter of ip property
print_r($geoip->ip);

//Getter of geo_ip_details property
print_r($geoip->geo_ip_details);

//toString of the class
echo $geoip;

Basically, I am going back and forth on implementation details. Should I expose fetch() publicly, and not automatically call it in the constructor? I.E.

$geoip = new GeoIP('64.87.28.98');
$result = $geoip->fetch();

Is using a getter a good idea? Should I have a class method called stringify() instead of overriding the __toString?

Also, right now, once you instantiate GeoIP with an ip address, there is no way to pass a new ip address into the existing object. You must instantiate a new GeoIP object. Does making a method:

public function set($ip) {
    $this->ip = $ip;
    $this->fetch();
}

Make sense, or simply just do something like: $geoip = new GeoIP('new-ip-address');

What is the best practice for object oriented design for this pattern?

I'm not a fan of magic getters. Without really good documentation, they're just a pain for consumers of your code. For example, when I first looked at your code, I had no idea at all what properties the class exposes. I basically had to follow the cURL request to find out. Proper documentation could fix this, but why not just have proper properties instead (or a getter)?
As a general rule of thumb, a class should never successfully construct if it will be left in a useless state. In other words, your constructor shouldn't finish unless the IP request actually went through and the data was retrieved.

If the IP is invalid, throw an exception.
If the cURL request fails, throw an exception

cURL isn't really necessary here (at least not with the way you're using it). Just use file_get_contents("http://.../...");
Your getter should return null if the property doesn't exist. null will be returned implicitly, but I think the code would be a bit clearer (alternatively, you could throw an exception depending on how you want the class to be used)
isset($var) && !empty($var) is redundant. !empty($var) is equivalent to isset($var) && $var meaning you can just do !$empty.
Your __toString doesn't have a real use at the moment. The return is too unpredictable to reliably use it to print the data. I would either normalize it to always print the same fields in the same format or get rid of it.

I've not a fan of __toString for most classes. Unless a class has a canonical string represenation (like a BigInt would, or a IPAddress, etc), I wouldn't bother having a __toString.
__toString can ease debugging though, so if you like to use it for that, it might be worth keeping it around

I think you need to decouple the data object (the city/state/area code/etc) from the retrieval of it. I would have two classes: GeoIP and GeoIPInformation (or whatever you want to call it).

GeoIP would then have a method public function getInformation($ipAddress); that returns a GeoIPInformation.

if you think there's a chance you'd ever want to have a different method for ipv4 or ipv6 or the ability to lookup host names, you might want to name it getInformationByIPv4

If you wanted to go all out, you could code the retriever to an interface. This would remove your ties to freegeoip.net (which might stop existing or working at any time)

A second retriever that immediately comes to mind is the maxmind geoip databases
Interfaces are good for decoupling, blah blah blah... ask me if you more details on this.

Update

Dave Jarvis had a very good point with the encapsulation issue. Concepts like a country should be stored as an object, not a simple scalar. This allows the consuming code to interact with the data in a much cleaner way. For example, imagine that one data source returns a 2 letter country code whereas another returns a country name. If the coutnry is a plain string, this presents a problem: how do we know what echo $loc->getCountry(); is going to be? Is it going to be a country code? Or a name? Or maybe an ID? Who knows.

But, if instead of leaving it a scalar we turn it into an object, we get the benefit of being able to do echo $loc->getCountry()->getName(); and knowing that it will be a proper name. There's an added benefit too of we now have much more data at hand. We are no longer constrained to one small snippet of data about the country. We now have the ability to have the name, code, etc all wrapped up in one convenient object.

Different strategies are possible to create these objects, but for your uses, I would just stick with a simple lookup table or something similar in this situation.

If you wanted to go all out, you could connect it to some kind of data source and then have your code interact with that to load a country. This would be a lot more flexible as the data is no longer part of the code (which tends to be bad), but it's also more complicated, and I'm lazy. If you did go this route, you could create a CountryFinder interface that knows how to create a Country based on a given criteria. You would then inject a CountryFinder implementation into your GeoIP implementations and it would use it.

I've updated my code below to reflect a basic sketch of these updates. I've only demonstrated by encapsulating the IP address and country, but hopefully the idea is clear. (Also, my error handling is pretty terrible in a few places such as the IP address constructor where an exception should be thrown if the argument is invalid).

Oh, and Location should either be named IpLocation to signify that it embodies a Location/IP pair, or it should have the IP address removed from it. (A Location object really has no business having an IP address as an attribute, but as it is actually GeoIP::Location, I might be tempted to leave it.)

All in all, I might consider something like this:

Note that this is a bit crude -- the error handling is bad, the exceptions should be subclasses, I put half a second of thought into names, etc

<?php

namespace GeoIP;

interface GeoIP
{
    /**
     * Retrieves geo information for an IP address.
     * @param string|IpAddress $ipAddress Either an IpAddress or an IP address formatted as a string in dotted decimal notation
     * @return \GeoIP\Location
     * @throws \InvalidArgumentException if the IP address is invalidly formatted
     * @throws \RuntimeException if some kind of network/disk/etc error occurs
    */
    public function getLocationByIpAddress($ipAddress);
}

class IpAddress
{

    private $_addr;

    public function __construct($addr)
    {
        //You can either guess what $addr is (string, 32 bit int, etc),
        //or you could provide static fromString, fromLong, etc methods.
        //For simplicity, I've assumed it's a decimal dotted string
        $this->_addr = $addr;
    }

    public function asDecimalDotString()
    {
        return $this->_addr;
    }

    public function asLong()
    {
        $octets = explode(".", $this->_addr);
        return ($octets[0] << 24) + ($octets[1] << 16) + ($octets[2] << 8) + $octets[3];
    }

}

class Location
{
    private $_ip;
    private $_country;

    public function __construct($ip, Country $country)
    {
        $this->_ip = $ip;
        $this->_country = $country;
    }

    /**
     * @return IpAddress The IP address that was used in retrieving this geo information
    */
    public function getIpAddress()
    {
        return $this->_ip;
    }

    /**
     * @return Country|null the country the IP belongs to or null is one could not be determined
    */
    public function getCountry()
    {
        return $this->_country;
    }

    // ...

}

interface CountryFinder
{
    public function findByCountryCode($code);
    public function findByCountryName($name);
}

class Country
{

    private $_name;
    private $_code;
    private $_population;

    public function __construct($name, $code, $population)
    {
        $this->_name = $name;
        $this->_code = $code;
        $this->_population = $population;
    }

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

    public function getCode()
    {
        return $this->_code;
    }

    public function getPopulation()
    {
        return $this->_population;
    }

}

class ArrayLookupCountryFinder implements CountryFinder
{

    private $_countryData = array(
        'US' => array('name' => 'United States', 'code' => 'US', 'population' => 300000000),
        'CA' => array('name' => 'Canada', 'code' => 'CA', 'population' => 34500000)
    );

    public function __construct(array $countryData = null)
    {
        if ($countryData) {
            $this->_countryData = $countryData;
        }
    }

    public function findByCountryCode($code)
    {
        if (isset($this->_countryData[$code])) {
            $country = $this->_countryData[$code];
            return new Country($country['name'], $country['code'], $country['population']);
        } else {
            return null;
        }
    }

    public function findByCountryName($name)
    {
        // Yeah...
        return null;
    }

}

class FreeGeoipNet implements GeoIP
{

    private $_countryFinder;

    public function __construct(CountryFinder $countryFinder)
    {
        $this->_countryFinder = $countryFinder;
    }

    public function getLocationByIpAddress($ipAddress)
    {

        if (!($ipAddress instanceof IpAddress)) {
            $ipAddress = new IpAddress($ipAddress);
        }

        $response = file_get_contents("http://freegeoip.net/json/" . $ipAddress->asDecimalDotString());
        if ($response === false) {
            throw new \RuntimeException("Request to freegeoip.net failed");
        }
        $data = json_decode($response, true);
        if (!is_array($data)) {
            throw new \RuntimeException("Parsing response from freegeoip.net failed: {$response}");
        }
        /* In a real application, you would likely want to somehow handle if the country code wasn't empty but a
         * country couldn't be found in your data source. I'm lazy though, so I've not done that. */
        return new Location($ipAddress, $this->_countryFinder->findByCountryCode($data['country_code']));
    }
}

Usage:

$geoip = new GeoIP\FreeGeoipNet(new GeoIP\ArrayLookupCountryFinder());

$location = $geoip->getLocationByIpAddress("x.x.x.x");

$ip = $location->getIpAddress();
$country = $location->getCountry();

echo $ip->asDecimalDotString() . " (" . $ip->asLong(). ") " . " is in " . $location->getCountry()->getName()
     . " which has a population of " . $location->getCountry()->getPopulation();


First four Project Euler in Scala I was working on teaching myself some Scala and decided to tackle some Project Euler problems.  The first four of these turned out to be one liners.

/* Find the sum of all the multiples of 3 or 5 below 1000 */
object Problem1 extends App {
    println((1 to 999).filter((i: Int) => i % 3 == 0 || i % 5 == 0).sum)
}

/* By considering the terms in the Fibonacci sequence whose values do not
   exceed four million, find the sum of the even-valued terms. */
object Problem2 extends App {
  def fib: Stream[Long] = {
    def tail(h: Long, n: Long): Stream[Long] = h #:: tail(n, h+n)
    tail(0,1)
  }

  println(fib.takeWhile(_ < 4000000).filter(_ % 2 == 0).sum)
}

/* What is the largest prime factor of the number 600851475143? */
object Problem3 extends App {
  val v = BigInt("600851475143")
  val z = BigInt(0)

  println(PrimeSeq.ps.takeWhile{p => v.>(BigInt(p).pow(2))}.filter(v.%(_) == z).max)
}

/* Find the largest palindrome made from the product of two 3-digit numbers. */
object Problem4 extends App {
  def isPalindrome (a:Int) : Boolean = {
    a.toString.equals(a.toString.reverse)
  }

  println((100 to 999).map { rangeIdx => (rangeIdx to 999).map{ i => i * rangeIdx}.map{ x => if(isPalindrome(x)) x else 0}.max }.max)
}

PrimeSeq is in a separate object because I expect that I'll be using it again in the future and is included here for completeness. The code itself is from this StackOverflow question and isn't code I wrote.

object PrimeSeq {
  lazy val ps: Stream[Int] = 2 #:: Stream.from(3).filter(i =>
    ps.takeWhile{j => j * j <= i}.forall{ k => i % k > 0})
}

I am most interested in the idiomatic corrections of the Scala code, though other parts will be of interesting too.

You should avoid the natural tendency in Scala to make everything a one-liner.  It hinders the readability and maintainability of the code.  I don't think it's much of a problem in the first two, but the third and fourth solutions seem a bit cryptic.  In most cases the compiler will collapse your more verbose collection of defs down to the same result as your one-liner, but the multiple defs are easier to grok for humans.

I can simplify Problem4 with filter and flatten to reduce the lists without resorting to use of '0' as a placeholder for "not to be considered".

println( (100 to 999).map(x=>(x to 999).map(_*x).filter(isPalindrome)).flatten.max)

But it still reads better to me if not so composed.

For example:

  def products = (100 to 999).map(x=>(x to 999).map(_*x)).flatten
  def palProducts = products.filter(isPalindrome)
  println( palProducts.max )

Even that combines too much in products, I think.  For-comprehension can make it more readable even without separate defs:

  def palProducts =
    for { a <- (100 to 999)
          b <- (  a to 999)
          c = a*b
          if (isPalindrome(c)) } yield c

  println( palProducts.max )

Also, you should add @tailrec annotations to code which is reliant on being tail-recursive, only to prevent them becoming non-tailrec through some accidental change in the future.  This is a minor quibble in these small examples, but it will become more important in larger projects.


Generate lines from points in Python I have two lists of coordinates,

lat = [-23.23, -44.23, -12.23]
lon = [-21.23, -14.23, -62.23]

and I generate a tuple of lat long pairs with zip:

latlon = zip(lat, lon)

and that gives the expected result. Now there is a very common pattern of generating lines with the points. It is common to do something like

    line_coords = []
    for i in xrange(len(coords)-1):
        line_coords.append([tuple(coords[i]), tuple(coords[i+1])])

so the result is now a list of tuples, each tuple an list of beginning and end of a line:

line_coords
[[(-23.23, -21.23), (-44.23, -14.23)], [(-44.23, -14.23), (-12.23, -62.23)] ]

and it get worse if the line needs to come back to the beginning. This looks like an anti-pattern. Is there a name for this pattern and a more elegant way to do it?

When it comes to iteration in Python, there is often an itertools recipe or function that come close to the needs.

Here, I’m thinking about pairwise which is defined as:

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

If you’re interested in cycling back to the beginnig, you should consider itertools.cycle too:

def get_line_coordinates(iterable, close=False):
    """s -> (s0,s1), (s1,s2), (s2, s3), ...
    ends with (sN, s0) if close is True else (sN-1, sN)
    """
    a, b = itertools.tee(iterable)
    if close:
        b = itertools.cycle(b)
    next(b, None)
    return itertools.izip(a, b)

Lastly, itertools.izip_longest will let you fill that last value by consumming less memory than cycle:

def get_line_coordinates(iterable, close=False):
    """s -> (s0,s1), (s1,s2), (s2, s3), ...
    ends with (sN, s0) if close is True else (sN-1, sN)
    """
    a, b = itertools.tee(iterable)
    beginning = next(b, None)
    if close:
        return itertools.izip_longest(a, b, fillvalue=beginning)
    return itertools.izip(a, b)


Simple username validation This checks if a given username is valid, before doing any database lookups.
(I know it's possibly better to use regular expressions, but I wanted to be able to give specific errors.)
I'm curious if there is a cleaner way to represent the set of valid characters.
bool check_username_valid(const string& str, string &out_error) {
    static const struct LUT {
        bool a[256]{};
        LUT() {
            for (int x = 'a'; x <= 'Z'; x++) a[x] = true;
            for (int x = 'A'; x <= 'Z'; x++) a[x] = true;
            for (int x = '0'; x <= '9'; x++) a[x] = true;
            a['_'] = true;
        }
        inline bool operator [] (char x) const { return a[int(((unsigned char)(x)))]; }
    } allow;

    if (str.length() < MinUsernameLen) { out_error = "Username Too Short"; return false; }
    if (str.length() > MaxUsernameLen) { out_error = "Username Too Long"; return false; }
    for (char c : str)
        if (!allow[c]) {
            out_error = "Invalid Character in Username";
            return false;
        }
    return true;
}

Instead of the loop, we can use std::all_of() (or std::any_of() with the opposite condition).  That can make the code clearer, because it signals intent at the beginning, and we don't need to unpick control flow as we do with for/return.  With C++20, we are able to pass a range object such as std::string as argument instead of an iterator pair (as we do in C++17).
And we have std::isalnum(), which is likely implemented as a lookup table like this (with the caveat that we need to be careful about signed char).  It's more portable (the code we have will be wrong if there are non-alphabetic characters between A and Z, as there are on EBCDIC systems, for example) and easier to get right (witness the typo - Z instead of z - that means on ASCII-like systems we allow no lower-case letters).
We could change the interface to just return the error string, with a null pointer indicating success, to avoid the "out" parameter.
#include <algorithm>
#include <cctype>
#include <string>

// return a null pointer if valid, else a pointer to the error message
const char *check_username_valid(const std::string& str)
{
    if (str.length() < MinUsernameLen) { return "Username Too Short"; }
    if (str.length() > MaxUsernameLen) { return "Username Too Long"; }
    auto const char_permitted
        = [](unsigned char c){ return c == '_' || std::isalnum(c); };

    if (std::all_of(str.begin(), str.end(), char_permitted)) {
        return nullptr;
    }

    return "Invalid Character in Username";
}

If we'll be adding more permitted username characters, we might want to use the lookup-table approach - but we don't need a new type, and this static const can be built at compilation time:
#include <array>
#include <climits>

bool legal_username_char(unsigned char c)
{
    static auto const table
        = []{
              std::array<bool,UCHAR_MAX+1> a;
              for (unsigned char i = 0;  i++ < UCHAR_MAX; ) {
                  a[i] = std::isalnum(i);
              }
              // additional non-alnum chars allowed
              a['_'] = true;
              return a;
          }();
    return table[c];
}

That idiom is an immediately-invoked lambda, and it's very handy for creating complex constants like this.  Note also the use of UCHAR_MAX rather than making assumptions about the size of the type.
Minor: no need for cast to int when indexing (a[int(((unsigned char)(x)))]).  An unsigned char is perfectly fine as array index.


Converting from Celsius to Fahrenheit and vice versa My code asks the user for the type of conversion, the degrees and spits the result back. It will prompt the user to ask for the type of conversion again if it's invalid. It will also prompt the user to see if he wants to quit or rerun the program again.

I was wondering if there is a more efficient way of doing this. I feel like I have a lot of redundant code on here.

public class Problem4 {

public static double celsiusToFahrenheit(double degree_C)
{
    double fahrenheitResult =  (9*(degree_C) / 5) + 32;
    return fahrenheitResult;
}

public static double fahrenheitToCelsius(double degree_F)
{
    double celsiusResult = 5*(degree_F - 32)/9;
    return celsiusResult;
}

public static void init(){
    Scanner scan = new Scanner(System.in);
    double result;
    char cont= 'y';
    char type;

    while (cont!='q' && cont!='Q')
    {
     System.out.println("Please enter the degree:");
     double degree = scan.nextDouble();

     System.out.println("Now enter the type of conversion (For Celsius, type 'C'. For Fahrenheit, type 'F')");
     type = scan.next().charAt(0);

     if(type!='c' && type!='C' && type!='f' && type!='F')
        {
             System.out.println("Error! You've entered an invalid conversion type!");
             System.out.println("Please enter a valid selection (Type C for celsius and F for fahrenheit");
             type = scan.next().charAt(0);
        }
     if (type == 'F' || type =='f')
        {
             result = fahrenheitToCelsius(degree);
             System.out.println("The conversion of "+ degree+"°F "+ "to Celsius is: " + result + "°C");

        }
     if (type == 'C' || type =='c')
        {
             result = celsiusToFahrenheit(degree);
             System.out.println("The conversion of "+ degree+"°C "+ "to Fahrenheit is: " + result + "°F");
        }

        System.out.println("Would you like to quit? Type 'q' to quit or type any other letter to continue:");
        cont = scan.next().charAt(0);
    }
    scan.close();
}

public static void main(String[] args)
{
    init();
}
}

I'm not excited about the repetition in this sequence:

 if(type!='c' && type!='C' && type!='f' && type!='F')
    {
         System.out.println("Error! You've entered an invalid conversion type!");
         System.out.println("Please enter a valid selection (Type C for celsius and F for fahrenheit");
         type = scan.next().charAt(0);
    }
 if (type == 'F' || type =='f')
    {
         result = fahrenheitToCelsius(degree);
         System.out.println("The conversion of "+ degree+"°F "+ "to Celsius is: " + result + "°C");

    }
 if (type == 'C' || type =='c')
    {
         result = celsiusToFahrenheit(degree);
         System.out.println("The conversion of "+ degree+"°C "+ "to Fahrenheit is: " + result + "°F");
    }

I think I'd prefer:

if (type == 'c' || type == 'C')
     // ...
else if (type == 'f' || type == 'F')
     // ...
else
    // Error

Although it conflicts somewhat with the preceding, I'm also less than excited about your error handling. You check for a valid input ([cCfF]), and get the next character of input if the first is invalid, but then you don't check whether that's valid or not. I'd rather do something like:

system.out.println("Please enter the conversion (C or F)");
do {
    type = scan.next().charAt(0);
} while (type != 'c' && type != 'C' && type != 'f' && type != 'F');

I'd also consider using a case statement instead of if statements:

switch (type) {
    case 'F':
    case 'f':
        result = fahrenheitToCelsius(degree);
        break;
    case 'c':
    case 'C':
         result = celsiusToFahrenheit(degree);
         break;
    default:
         // handle error
}

I'm also a little less than excited about the wording you used. It's not immediately apparent (at least to me) that: For Celsius, type 'C'. For Fahrenheit, type 'F' means that "C" of "F" designates the type of the input rather than the type of the intended result.

If at all possible, I'd prefer to support command line input. Something like "convert 10c" seems a lot easier to use (and script, etc.) than having to read and respond to a couple of prompts to tell it what I want.


Programming challenge "Friend Request in Social Network" I am trying to solve a programming problem on a coding platform. When I execute it on my PC, it works perfectly, but when I submit the code on the coding platform, it throws a "Time Limit Exceeded" error. Can someone check my solution and help optimize it?

  In a social network, a person can invite friends of his/her friend.
  John wants to invite and add new friends. Complete the program below
  so that it prints the names of the persons to whom John can send a
  friend request.

  Input format:

  The first line contains the value of the N which represent the number
  of friends. N lines contain the name of the friend F followed by the
  number of friends of F  finally their names separated by space.

  Input:

3
Mani 3 Ram Raj Guna
Ram 2 Kumar Kishore
Mughil 3 Praveen Naveen Ramesh

  Output:

Raj Guna Kumar Kishore Praveen Naveen Ramesh

  Explanation:

  Ram is not present in the output as Ram is already John's friend.

My Approach

Extract the first word of each line and store them in HashSet and remove them from the string. Names stored in HashSet are already friends of the person (John).
Now extract the names from the String using StringTokenizer and check whether the name is contained in the HashSet. If it is not present then print it.

And can we solve this problem using graph/trees. The problem statement and my code can be found here.

import java.util.HashSet;
import java.util.Scanner;
import java.util.StringTokenizer;

class Ideone {
    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        int N = sc.nextInt();   // No of friends
        sc.nextLine();
        HashSet<String> hs = new HashSet<>();   // to store name of the John's friends
        String invitation = "";
        for(int i =0; i < N; i++)
        {
            String str = sc.nextLine();
            String friend = "";
            int j =0;
            while(str.charAt(j)!= ' ')
            {
                friend = friend + str.charAt(j++);
                hs.add(friend);     // add the name before the number to HashSet
            }
            j= j+2;
            invitation=invitation+str.substring(j)+" "; // remove name of John's friend from the string and store back remaining string
        }
        int j =0;
        StringTokenizer st = new StringTokenizer(invitation); // divide string into tokens
        while(st.hasMoreTokens())
        {
            String str = st.nextToken();
            if(!hs.contains(str))   {
                /* check if token(string) is already in hashset ie if the person already friend with John or not
                if he is not then print str
                */
                System.out.print(str+" ");
            }
        }
    }
}

The big performance issue with the current solution is that you're relying on building a String to keep the list of persons that were sent a friend request. A String is immutable in Java so every single operation on it will result in the creation and allocation of a new String. When doing those sorts of repeated allocation in a loop, the performance degrades rapidly.

We need another approach to the problem that doesn't rely on a simple String, and with that, a better data structure.

From the problem description, we need to maintain a collection of the friends of John. To pick a suitable data structure, let's examine what we'll need to do with it: we want to add elements to it (each friend name read), and we want to check whether that collection contains a specific name (to not add it to the list of invited persons). Additionally, we don't care about a particular order for this collection. The perfect data structure  for that is a HashSet, just like the one you used: it has a constant-time add and contains methods.
Then, we also need to maintain a collection of the invited persons, those that were sent the friend request. This is where you used a String and we can do better. Like the above, we want a data structure capable of adding elements (the name of the invited persons), but we also want it to be capable of removing elements (the friend names we might read further down the input). Also, we want to keep an order here, specifically, the order in which the elements were added. For that, LinkedHashSet is perfect: add and remove are both constant-time operations, and it keeps the insertion order.

With that in mind, we can refactor the code to use those data structures:

Collection<String> friends = new HashSet<>(); // to store name of the John's friends
Collection<String> invited = new LinkedHashSet<>();
for (int i = 0; i < N; i++) {
    Scanner scanLine = new Scanner(sc.nextLine());
    String friend = scanLine.next();
    friends.add(friend);
    invited.remove(friend); // potentially remove this invited as it is actually a friend
    scanLine.next(); // ignore the number of friends, we can deduce it anyway
    while (scanLine.hasNext()) {
        String name = scanLine.next();
        if (!friends.contains(name)) {
            invited.add(name);
        }
    }
}

Notice that I changed other things in there:

When you have the line of input, that is space-separated, you don't need to read manually characters by characters. You can either split(" "), which would return an array of all the tokens space separated, or you can use another Scanner, which by default tokenizes over white spaces, meaning every call to Scanner.next() will return the next word.
The variables have a meaningful name: friends represent the collection of John's friend and invited represents the collection of persons to whom John sent a friend request.

At the end of the process, we have our wanted collection inside invited, so we can finally print it. For that, we can use String.join starting from Java 8, which is built-in.

System.out.println(String.join(" ", invited));

If you are on a lower Java version, you would need to write the for loop explicitely and use a StringBuilder to construct the result, like shown here. Don't use the old StringTokenizer, it isn't officially deprecated but it is only kept for compatibility reasons and is considered a legacy class.


Monte Carlo Pi with R + Fortran + MPI I created a R package to calculate Pi with a Monte Carlo simulation.

My goal is to optimize the parallel function SnowPi as much as possible. This is the SnowPi.R code:

FSnowPi <- function(DARTS, ROUNDS, proc_num, numprocs) {
  retvals <- .Fortran("snowpi", avepi = as.numeric(1),
                      DARTS =  as.integer(DARTS),
                      ROUNDS =  as.integer(ROUNDS),
                      proc_num = as.integer(proc_num),
                      numprocs = as.integer(numprocs))
  return(retvals$avepi)
}
#'@export
SnowPi  <- function(DARTS, ROUNDS, numprocs){
  cl <- snow::makeCluster(numprocs, type = "MPI")
  r<<-ROUNDS
  d<<-DARTS
  snow::clusterExport(cl, c("r", "d"))
  x<-snow::clusterEvalQ(cl, MyPi::FSnowPi(DARTS = d, ROUNDS = r, proc_num = mpi.comm.rank(), numprocs = mpi.comm.size()-1))
  snow::stopCluster(cl)
  return(mean(unlist(x)))
}

One thing that I think may not be optimal is the use of <<-. Alas, I don't know how to get the code to work without that (I think the problem is how to pass the correct environment to clusterExport)

Finally, these are the 2 relevant Fortran subroutines:

subroutine snowpi(avepi, DARTS, ROUNDS, proc_num, numprocs) bind(C, name="snowpi_")
use, intrinsic                         :: iso_c_binding, only : c_double, c_int
real(c_double), intent(out)            :: avepi
integer(c_int), intent(in)             :: DARTS, ROUNDS, proc_num, numprocs
integer                                :: i, n, mynpts
integer, allocatable                   :: seed(:)
double precision                       :: pi_est

  if (proc_num .eq. 1) then
      mynpts = ROUNDS - (numprocs-1)*(ROUNDS/numprocs)
    else
      mynpts = ROUNDS/numprocs
  endif

  ! initialize the random number generator
  ! we make sure the seed is different for each task
  call random_seed()
  call random_seed(size = n)
  allocate(seed(n))
  seed = 12 + proc_num*11
  call random_seed(put=seed(1:n))
  deallocate(seed)

  avepi=0.0d0
    do i = 0, mynpts-1
      call dboard(darts, pi_est)
      ! calculate the average value of pi over all iterations
      avepi = ((avepi*i) + pi_est)/(i + 1)
  end do

end subroutine snowpi

and

subroutine dboard(darts, dartsscore)
  integer, intent(in)                    :: darts
  double precision, intent(out)          :: dartsscore
  double precision                       :: x_coord, y_coord
  integer                                :: score, n

score = 0
do n = 1, darts
  call random_number(x_coord)
  call random_number(y_coord)

  if ((x_coord**2 + y_coord**2) <= 1.0d0) then
  score = score + 1
  end if
end do

dartsscore = 4.0d0*score/darts

end subroutine dboard

I'm not sure if there is something that I could improve there.

Here are a few pointers. I should first say that I am more familiar with R;  I know next to nothing about Fortran but I could read your code and guess about a few things that will likely improve your code: both in simplicity and speed.

1) Why the need for two inputs DARTS and ROUNDS? You know that in the end you will have a total of DARTS * ROUNDS simulations: could there just be a single input num_simulations that will be split evenly among your numprocs? This should get you rid of the loop inside snowpi so it could only call dboard once.

2) You should make your slave (snowpi) as simple as possible. IMO, it should not know about things like proc_num or numprocs if you just tell it how many simulations it needs to do. Let R do the math upstream and pass it down to the slave. I don't know much about random generators but maybe the same can apply with the random seeds: if you could let R compute a seed for each slave and pass it down as an (optional) argument to the slave, that would be best. (However, if knowing the proc_num and numprocs is crucial for seed selection and can only happen on the Fortran side, then disregard my comment...)

3) This one I feel is important: do not compute averages too early. They are costly and can make you lose precision. This line in your code (avepi = ((avepi*i) + pi_est)/(i + 1)) is particularly problematic: you are doing so many unnecessary multiplications and divisions... Instead, you should just be counting the number of darts, i.e. incrementing a counter. Your slave should just be returning an integer. Let R add all these integers together and do a single division by num_simulations at the end.

To summarize 1, 2, and 3, it would be great (if possible) if you could just tell your slave: here is your seed number n; now throw x darts and tell me how many ended up in the circle.

4) Now looking at your R code. Yes, the use of <<- in your code is not very elegant. That's because you are trying to use clusterEvalQ which may not be the best option from the snow package. Instead, clusterApply or clusterMap are better suited for functional programming. Your code could look like this, I let you figure out the rest:

FSnowPi <- function(num_simulations, seed_num = 123456L) {
  retvals <- .Fortran("snowpi",
                      num_inside      = 1L,  # an integer
                      num_simulations = as.integer(num_simulations),
                      seed_num = as.integer(seed_num))
  return(retvals$num_inside)
}

SnowPi  <- function(num_simulations, num_procs) {
  # input checks
  stopifnot(is.integer(num_simulations),
            is.integer(num_procs),
            num_simulations > 0L,
            num_procs > 0L)
  # initialize cluster
  cl <- snow::makeCluster(num_procs, type = "MPI")
  # balance the number of simulations per node
  node_num_simu <- balance_evenly(num_simulations, num_procs)
  # compute random seeds
  node_seeds <- sample.int(1e6, num_procs)   # please review...
  # run in parallel
  num_inside <- snow::clusterMap(cl, MyPi::FSnowPi, node_num_simu, node_seeds)
  # close the cluster
  snow::stopCluster(cl)

  pi_estimate <- 4 * Reduce(`+`, num_inside) / num_simulations
  return(pi_estimate)
}

balance_evenly <- function(total, among) {
   min_load <- floor(total / among)
   max_load <- min_load + 1L
   rest <- total - min_load * among
   load <- c(rep(max_load, rest),
             rep(min_load, among - rest))
   return(load)
}

I hope some of it is helpful. Please let me know. And thanks for sharing your Fortran code; I know at least one other R package that uses it:  fields whose rdist() function for computing euclidean distances is extremely fast. I want to give a try one day.


Loading array of themes I am currently writing a simple PHP script that uses themes and I found myself writing these methods as part of a class that handles theme config files:

public function getArrayOfThemeNames()
{
    $this->loadUnfilteredDirectoryListing();
    $this->loadThemeFiles();
    return $this->theme_files;
}

private function loadUnfilteredDirectoryListing()
{
    $this->unfiltered_dir_listing = scandir($this->theme_config_directory);
}

private function loadThemeFiles()
{
    foreach($this->unfiltered_dir_listing as $file)
    {
        if($file != ".." and $file != ".")
            $this->theme_files[] = $file;
    }
}

Does the getArrayOfThemeNames() method violate SRP, as it could be argued it does two (or potentially even three)?  It loads the theme files array and then returns it.

Am I over-thinking this or should this be split into two methods -- one that fills the themes array, and another that returns it?

Well, the function kind-of sort-of violates SRP...but according to Uncle Bob, I think your class violates the SRP moreso than the function.

Your class above, has three functions within it: getArrayOfThemeNames(), loadUnfilteredDirectoryListing(), and loadThemeFiles(). One of them scans the directory, one of theme creates an array of theme names, and another loads the theme files. One of those methods should not be in there: loadUnfilteredDirectoryListing should be in its own class called DirectoryLister{} or something. Which means, by extension, that getArrayOfThemeNames() is in violation of SRP. To quote Atwood - SRP is about "choosing a single, clearly defined goal for any particular bit of code: Do One Thing ... But in choosing one thing, you are ruling out an infinite universe of other possible things you could have done. It also means consciously choosing what your code won't do.". So in the end, its up to you whether this violates SRP and whether you would choose to follow SRP so deeply that you violate OOP or DRY.

Perhaps someone with more experience can chime in or prove me wrong (heck, always good to learn something from someone else)

EDIT: See (http://www.oodesign.com/single-responsibility-principle.html) article as well for clarification.

REVISIT 01/26/2015: Revisiting this answer, I would rewrite the code as follows:

class Theme {
    public $theme_directory;
    public $theme_files;

    public function __construct($theme_directory){
        $this->theme_directory = $theme_directory;
    }

    public function get_theme_names() /* Array */ {
        $this->load_theme_files();
        return $this->theme_files;
    }

    private function load_theme_files(Directory $directory) /* Null */ {
        $directory->directory = $this->theme_directory;
        foreach($directory->load_unfiltered() as $file) {
            if($file != '..' && $file != '.') {
                $this->theme_files[] = $file;
            }
        }
    }
}

class Directory {
    public $directory;

    public function __construct() {
        $this->directory = $directory;
    }

    private function load_unfiltered() /* array */ {
        return  scandir($this->directory);
    }
}


Converting a string to a corresponding type in JavaScript I need to convert a string value to its primitive type in JavaScript. Other values that are not of type String should just be returned.

Example:

String '34' should be converted to 34
String '34px' should be kept as '34px'
true should return true
'true' should return true

Here is a working example (please look at console).

I would like to know:

If there is any better way to write the same function, in term of performance.
If I missed some type or any other issues.

function convert(value) {
    var result = value;
    if (typeof value === 'string') {
        // check if it is a empty character
        value.trim();
        if (value.length > 0) {
            if (/\S/.test(value)) {
                if (value === 'true') {
                    result = true;
                } else if (value === 'false') {
                    result = false
                } else if (!isNaN(value)) {
                    result = new Number(value);
                }
            }
        }
    }
    return result;
}

On thing to keep in mind is that certain inputs that you might not consider to be numbers will be considered numbers by the isNaN and Number functions.

Potentially Unexpected Numbers:

Infinity ("Infinity")
hexidecimal notation ("0x1a3")
scientific notation ("0e9" and "0e+9" and "0e-9")

function convert(value) {
    var result = value;
    if (typeof value === 'string') {
        // check if it is a empty character
        value.trim();
        if (value.length > 0) {
            if (/\S/.test(value)) {
                if (value === 'true') {
                    result = true;
                } else if (value === 'false') {
                    result = false
                } else if (!isNaN(value)) {
                    result = new Number(value);
                }
            }
        }
    }
    return result;
}

var rows = document.querySelectorAll(".row");
for(var i = 0; i < rows.length; i++){
  var input = rows[i].querySelector(".in");
  var output = rows[i].querySelector(".out");
  var type = rows[i].querySelector(".type")

  var value = input.getAttribute("value");
  if(value){input.innerHTML = "\""+value+"\""}

  var result = convert(input.getAttribute("value"));
  output.innerHTML = result

  var typeValue = typeof result === "object" ? isNaN(result) ? typeof result : result === null ? "null" : "number" : typeof result;
  type.innerHTML = typeValue;
  if(typeValue === "number"){if(type.classList){type.classList.add("highlight");}else{type.className = type.className + " " + "highlight";}}
}
.table{display:table;border-left:1px solid black;table-layout:fixed; min-width:300px;}
.headerRow{display:table-row;background-color:#dfdfdf;font-weight:bold;}
.headerCell{display:table-cell; border-right:1px solid black; border-bottom:1px solid black; border-top:1px solid black;}
.row{display:table-row;}
.cell{display:table-cell; border-right:1px solid black;border-bottom:1px solid #dfdfdf;}
.highlight{background-color:lightgreen;}

Input String

Output Value

Output Type

""

Boolean values are also considered numbers, but your function has that covered.


TicTacToe where the computer plays random moves against itself I wrote a quick program in Python to play against itself in a game of (random) tic-tac-toe.

It seems to work, but can you guys please look at it and let me know if I'm doing things the 'correct' (i.e. idiomatic) and efficient way.

import random

#  Tic Tac Toe

#  Board is laid out as:
#  0, 1, 2,
#  3, 4, 5,
#  6, 7, 8

winning_rows = [
    [0, 1, 2], [3, 4, 5], [6, 7, 8],    # Horizontal
    [0, 3, 6], [1, 4, 7], [2, 5, 8],    # Vertical
    [0, 4, 8], [2, 4, 6]                # Diagonal
    ]

def check_winner(board):
    for player in [1, 2]:
        for seq in winning_rows:
            for pos in seq:
                if (board[pos]!=player):
                    break
            else:
                return player

    #nobody won, check for draw (return 0 for draw, None for incomplete game)
    for pos in range(9):
        if (board[pos] == 0):
            return None

    #game is a draw
    return 0

def find_move(board, player):
    legal_moves = []

    for pos in range(9):
        if (board[pos] == 0):
            legal_moves.append(pos)

    return random.choice(legal_moves)

def make_move(board, move, player):
    board[move] = player

def write_board(board):
    #letter = {0: '-', 1: 'X', 2: 'O'}
    letter = "-XO"
    for row in range(3):
        for col in range(3):
            print letter[board[row*3+col]],
        print
    print

def play():
    board = []
    for pos in range(9):
        board.append(0)

    write_board(board)

    player = 1
    while True:
        move = find_move(board, player)
        make_move(board, move, player)
        write_board(board)
        winner = check_winner(board)
        if winner != None:
            if winner == 1:
                print "Player 1 (X) wins!"
            if winner == 2:
                print "Player 2 (O) wins!"
            if winner == 0:
                print "Game is a draw!"
            break
        player = 3 - player

play()

Some of your functions can use list comprehensions.

As you seem to know maths, you may have come across 'set-builder notation',
and that's what list comprehension are based off:

\$S = \{2 \times x | x \in \{0, 1, ..., 9\}, x^2 > 3\}\$

Which in Python is:

[2 * x for x in range(10) if x ** 2 > 3]

I would use it in:

find_move for an improvement in clarity.

def find_move(board, player):
    return random.choice([pos for pos in range(9) if board[pos] == 0])

write_board to use less prints. It's harder to read however.

print '\n'.join([
    ''.join([letter[row + col] for col in range(3)])
    for row in range(0, 3, 9)
])

check_winner you can make your for pos in seq one.
Using all.

for player in [1, 2]:
    for seq in winning_rows:
        if all([board[pos] == player for pos in seq]):
            return player

I'm surprised you didn't make a new_board function. Which could be:

def new_board():
    return [0 for _ in range(9)]

Alternatively, as noted by @i-live-in-a-storm-drain, if you are ok with shallow copy's you could change it to:

def new_board():
    return [0] * 9

The down-side to this is if you mutate any of the items, then you mutate all of them. For example you wouldn't want to make a 2D list with it.

>>> a = [[0] * 3] * 3
>>> a[0][0] = 1
>>> a
[[1, 0, 0], [1, 0, 0], [1, 0, 0]]

Your function make_move seems, kinda pointless...
I would use board[move] = player over the function.
You should use is when comparing to singletons, None.

# Bad
if winner != None:

# Good
if winner is not None:

You could make the win conditions a list, and print from that.

win_statments = ["Player 1 (X) wins!", "Player 2 (O) wins!", "Game is a draw!"]
print win_statments[winner]

For check_winner you can use the fact 0 == False (Not 0 is False).
With Python's builtin all to check if all of them are not 0.

if not all(board):
    return None

As this is Python2 it's recommended to use xrange rather than range for almost everything.
You'll get a, minimal, speed increase in most situations.

However as noted by @Janos, you could opt to keep them as range, for an easier transition to Python3. This is as there is no xrange in Python3.
You should only need to change your prints to functions for this to work in Python3.

# Change to these:
print("Player 1 (X) wins!")

# in `make_move`
for col in range(3):
    print(letter[board[row*3+col]], end='')
print()


My form error javascript code Simple really my javascript function for checking my form.

var errors = {};
errors.email = true;
errors.cemail = true;
errors.password = true;
errors.cpassword = true;
errors.username = true;
function joinAjax (id) {

var val = $('#' + id).val();
if (id == 'email') {

    $('#emailMsg').hide();
    var reg = /[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?/;
    if (val == '') {

        $('#' + id).after('
Enter your email
');
    }
    else if (reg.test(val) == false) {

        $('#' + id).after('
Email invalid
');
    }
    else {

        errors.email = false;
        $('#' + id).after('
Email OK
');
    }
    joinAjax('cemail');
}
if (id == 'cemail') {

    $('#cemailMsg').hide();
var email = $('#email').val();
    if (errors.email == false) {

        if (val != email) {

            $('#' + id).after('
Emails do not match
');
        }
        else {

            errors.cemail = false;
            $('#' + id).after('
Success
');
        }
    }
    else {

        $('#cemail').val('');
    }
}
if (id == 'password') {

    $('#passwordMsg').hide();
    if (val == '') {

        $('#' + id).after('
Enter a password
');
    }
    else if (val.length < 6) {

        $('#' + id).after('
Minimum length of 6
');
    }
    else {

        errors.password = false;
        $('#' + id).after('
Password OK
');
    }
    joinAjax('cpassword');
}
if (id == 'cpassword') {

    $('#cpasswordMsg').hide();
    var password = $('#password').val();
    if (errors.password == false) {

        if (val != password) {

            $('#' + id).after('
Passwords do not match
');
        }
        else {

            errors.cpassword = false;
            $('#' + id).after('
Success
');
        }
    }
    else {

        $('#cpassword').val('');
    }
}
if (id == 'username') {

    $('#usernameMsg').hide();
    if (val == '') {

        $('#' + id).after('
Enter a username
');
    }
    else if (val.length < 3) {

        $('#' + id).after('
Minimum length is 3
');
    }
    else {

        errors.username = false;
        $('#' + id).after('
Available
');
    }
}
$('#joinForm').submit(function(event){

    if ((errors.email == true) || (errors.cemail == true) || (errors.password == true) || (errors.cpassword == true) || (errors.username == true)) {

        event.preventDefault();
    }
    return true;
});
}

Can this be improved? I was looking for a way to create a function for the submit event, but I can't figure it how as I don't know how many parameters I will need each time.

Some tips:

you'd probably better have a look at some jquery validation plugin that would do what you are looking for out of the box
to solve your "unknown parameters", you could probably use the jquery .serialize() function and loop through the array