Reversing a String in Python I have a simple function that takes a string and reverses it:

def reverse(string):
    begin = 0
    end = len(string) - 1
    strlist = [i for i in string]
    while(begin < end):
        temp = strlist[begin]
        strlist[begin] = strlist[end]
        strlist[end] = temp
        begin += 1
        end -= 1
    return ''.join(strlist)

print reverse('flash')

I know it's only a simple function but I'm keen to see where my thought process could be improved (even in such a simple context) for future projects.

Well, this is technically a way to invalidate your function, but either the built in slicing operator or the reversed function would do this for you in one line:

"flash"[::-1]
>>> "hsalf"

The slice operator takes three parameters, where it starts and ends, but also what step to take as it increments. You could pass in 2 to get every second letter, but you can also pass in a negative value to start at the end and work backwards, hence reversing your string.

The reversed function can take any iterator and return it in reverse as a generator object. This means that it wont strictly print properly, ie. this is what you would get when printing it:

reversed("flash")
>>> <reversed object at 0x0000000002F50898>

But calling join with it will give the result you need:

''.join(reversed("flash"))
>>> "hsalf"

Note that as erip points out this can have trouble with unicode characters in Python 2. In 3 all strings are unicode, but not before then. So reversing non unicode strings actually messes up the byte order, causing erratic behaviour like this:

>>> print("mañana"[::-1])
ana±am

To safely avoid this in 2, make sure to declare it as unicode with a u before the string u"mañana". This will make either method work fine. Alternatively from __future__ import unicode_literals will make all strings unicode like they are in Python 3.

But for your approach I'll still give suggestions. You use a slightly complicated approach. Swapping the values at opposite ends is not entirely obvious so I'd recommend adding comments to elaborate a bit on what the intent is as code is not always readable on its own.

Also you could use Python's multiple assignment to clean up some code. A more simple example is your begin and end could be set at the same time:

    begin, end = 0, len(string) - 1

This doesn't add a lot but can look a little neater. It essentially lets you assign two variables at once. It might be unnecessary, but the same principle can be used to eliminate the need for your temp.

    strlist[end], strlist[begin] = strlist[begin], strlist[end]

This allows you to safely assign both at the same time. The right hand side of the expression is fully evaluated before any assignment, so you wont lose either value.


Thread Pool in C for web server Please review the following code for thread pool in C (I wrote it to implement a threaded web server for my personal project). I have been programming in C++ and Java but never did any serious programming in C.

threadpool.h

#ifndef THREAD_POOL_H
#define THREAD_POOL_H

#include <pthread.h>

#define MAX_THREAD 10

typedef struct {
    void *(*function) (void *);
} runnable_task;

typedef struct safe_queue_node {
    runnable_task *task;
    struct safe_queue_node *next;
} safe_queue_node;

typedef struct safe_queue{
    struct safe_queue_node *head;
    struct safe_queue_node *tail;
    pthread_mutex_t mutex;
} safe_queue;

typedef struct {
    pthread_t threads[MAX_THREAD];
    safe_queue *queue;
    int count;
    volatile int should_close;
} thread_pool;

runnable_task *
new_runnable_task(
        void *(*function) (void *));

thread_pool *
new_thread_pool();

void
free_thread_pool(
        thread_pool *pool);

void
add_task_to_pool(
        thread_pool *pool,
        runnable_task *task);

void
shutdown_thread_pool(
        thread_pool *pool);

#endif

threadpool.c

#include "threadpool.h"
#include <stdlib.h>

runnable_task *
new_runnable_task(
        void *(*function) (void *)) {
    runnable_task *task = malloc(sizeof(runnable_task));
    task->function = function;
    return task;
}

safe_queue_node *
_new_safe_queue_node(
        runnable_task *task) {
    safe_queue_node *node = malloc(sizeof(safe_queue_node));
    node->task = task;
    node->next = NULL;
    return node;
}

safe_queue *
_new_safe_queue() {
    safe_queue *queue = malloc(sizeof(safe_queue));
    pthread_mutex_init(&queue->mutex, NULL);
    queue->head = NULL;
    queue->tail = NULL;
    return queue;
}

void
_free_safe_queue(
        safe_queue *queue) {
    free(queue);
}

runnable_task *
_get_from_queue(
        safe_queue *queue) {
    runnable_task *task = NULL;

    pthread_mutex_lock(&queue->mutex);
    if (queue->head != NULL) {
        task = queue->head->task;
        queue->head = queue->head->next;
        if (queue->head == NULL)
            queue->tail = NULL;
    }
    pthread_mutex_unlock(&queue->mutex);

    return task;
}

void *
_run_task(
        void *arg) {
    thread_pool *pool = (thread_pool *) arg;
    while (!pool->should_close) {
        runnable_task *task = _get_from_queue(pool->queue);
        if (task == NULL)
            continue;
        task->function(NULL);
    }

    return NULL;
}

thread_pool *
new_thread_pool(
        int count) {
    thread_pool *pool = malloc(sizeof(thread_pool));
    pool->queue = _new_safe_queue();
    pool->count = count <= MAX_THREAD ? count : MAX_THREAD;
    pool->should_close = 0;
    for (int i = 0; i < pool->count; i++) {
        pthread_create(pool->threads + i, NULL, _run_task, pool);
    }

    return pool;
}

void
free_thread_pool(
        thread_pool *pool) {
    _free_safe_queue(pool->queue);
    free(pool);
}

void
_add_to_queue(
        safe_queue *queue,
        runnable_task *task) {
    pthread_mutex_lock(&queue->mutex);
    if (queue->head == NULL) {
        queue->head = queue->tail = _new_safe_queue_node(task);
    } else {
        queue->tail->next = _new_safe_queue_node(task);
        queue->tail = queue->tail->next;
    }
    pthread_mutex_unlock(&queue->mutex);
}

void
add_task_to_pool(
        thread_pool *pool,
        runnable_task *task) {
    _add_to_queue(pool->queue, task);
}

void
shutdown_thread_pool(
        thread_pool *pool) {
    while (pool->queue->head != NULL) ;

    pool->should_close = 1;

    for (int i = 0; i < pool->count; i++)
        pthread_join(pool->threads[i], NULL);
}

Busy waiting on an empty queue is doubleplusungood. Add a condition variable to the queue, let the threads to pthread_cond_wait on it, and pthread_cond_signal it when adding the task to the queue.
The functions you don't want to export (those with the leading underscores) shall be static.
sizeof(type) may lead to a double maintenance problem. Prefer sizeof(var), as in

thread_pool *pool = malloc(sizeof(*pool));


Longest consecutive sequence of ascending, descending, or equal integers Given an array of integers, find the longest consecutive sequence, where a sequence is defined as being either (strictly) ascending, (strictly) descending, or all-equal.

836926 then has the longest sequence 369, which happens to be ascending. 241455556 has the longest sequence 5555, which happens to be all equal.

Please comment on the algorithm (correct? complexity?) and how it can be improved.

#include <iostream>
#include <vector>

int sign(int someInt)
{
    if (someInt > 0) { return 1; };
    if (someInt < 0) { return -1; };
    return 0;
}

bool isSequence(int a, int b, int c)
{
    if (sign(a - b) == sign(b - c))
    {
        return true;
    }

    return false;
}

void findSeq(const std::vector<int> &vect)
{
    int maxLength = 0;
    int startingIndex = 0;

    int currentLength = 2;
    int findIndex = 0;

    for (int i = 0; i < vect.size() - 2; i++)
    {

        if (isSequence(vect[i], vect[i + 1], vect[i + 2]))
        {
            currentLength++;
            if (currentLength > maxLength)
            {
                startingIndex = i - findIndex;
                maxLength = currentLength;
            }
            findIndex++;
        }

        else
        {
            findIndex = 0;
            currentLength = 2;
        }
    }

    for (int j = startingIndex; j < startingIndex + maxLength; j++)
    {
        std::cout << vect[j];
    }

}

The code is generally good and clear.  I like the names you've used for your variables - it's very obvious what each does, with the possible exception of findIndex.

There's an issue with the problem statement (which might not be your fault): it doesn't say what to do if there's more than one "longest" sequence.  In this code, it appears that we use the first match if there's another of the same length; it's worth writing a comment to be clear that this is what we want (and including such a case in the tests, so that we know if that changes).

isSequence is a bit more long-winded than it needs to be.  This pattern is redundant:

if (condition)
    return true;
else
    return false;

It can always be replaced with

return condition;

In findSeq itself, we're doing pretty well.  I get a compiler warning about comparing (signed) i against (unsigned) vect.size(); that's easily fixed by changing i to be a std::size_t instead of an int.  Most of the other int variables would be better represented as std::size_t, too.

One thing we might want to do is to use iterators rather than indexes, to give us an opportunity to work with different collections in future.  And instead of printing to std::cout, we might want to return the start and end iterators of the longest matching sequence.  (If we do print the values, it's a good idea to separate them from each other, otherwise we can't tell 33,3 from 3,3,3, for example).

If we're using iterators, we'll want to remember the previous and ante-previous values, because we can't necessarily go back to them.  Equivalently, we can remember the previous value and the direction of difference.

I don't know if this is beyond your current knowledge, but this is what I came up with when I followed my own suggestions, and took it a little further to work generically as a template:

#include <iterator>
#include <utility>

// return the sign of a-b (or the sign of a, if b is defaulted)
// result: -1 if a<b, 0 if a==b, +1 if a>b
template<typename T>
int compare(T a, T b = {})
{
    // This is a "clever" way of determining the sign.  Some
    // compilers recognise this idiom and reduce it to a single
    // instruction.
    return (a > b) - (a < b);
}

template<typename ForwardIterator>
std::pair<ForwardIterator,ForwardIterator>
findSeq(const ForwardIterator first, const ForwardIterator last)
{
    if (first == last) {
        // empty range -> empty result
        return { first, first };
    }

    auto best_start = first;
    auto best_end = first;
    std::size_t best_length = 0;

    auto current_start = first;
    auto previous = first;
    int current_direction = 0;
    std::size_t current_length = 0;

    auto const update_best = [&](ForwardIterator end){
        best_start = current_start;
        best_end = end;
        best_length = current_length;
    };

    for (auto it = std::next(first);  it != last;  ++it) {
        const auto new_direction = compare(*previous, *it);

        if (new_direction == current_direction) {
            ++current_length;
        } else {
            if (current_length > best_length) {
                update_best(it);
            }
            current_direction = new_direction;
            current_start = previous;
            current_length = 1;
        }

        previous = it;
    }

    if (current_length > best_length) {
        update_best(last);
    }

    return { best_start, best_end };
}

// provide an interface compatible with the original
template<typename Collection>
auto findSeq(const Collection& c)
{
    using std::begin;
    using std::end;

    return findSeq(begin(c), end(c));
}

#include <array>
#include <iostream>
#include <forward_list>
#include <vector>

template<typename ForwardIterator>
void printSeq(std::pair<ForwardIterator,ForwardIterator> range)
{
    auto [first,last] = range;
    for (auto it = first;  it != last;  ++it)
        std::cout << *it << ' ';
    std::cout << std::endl;
}

int main()
{
    printSeq(findSeq(std::vector{8,3,6,9,2,6,12}));
    printSeq(findSeq(std::array{-0.1, -0.2, -0.3, -0.4, 0.1, 0.2, 0.3, -1.0}));
    printSeq(findSeq(std::forward_list<std::string>{
                "foo", "bar", "bar", "bar", "baz", "quux"}));
}


Text-based dice rolling game in C++ I have made this small text-based dice rolling game in C++. How could I improve it? Be as picky as you'd like.

#include <iostream>
#include <cstdlib>
#include <ctime>

using namespace std;

void mainGame(int &numDice);

int main()
{
    srand (time(NULL));

    int gameRunning = true;
    int numDice;

    while (gameRunning == true)
    {
        system("cls");
        cout << "\n How many dice would you like to use? (1-4)" << endl;
        cout << "\n Enter '0' to exit." << endl;
        cout << "\n> ";
        cin >> numDice;

        if (numDice == 1 || numDice == 2 || numDice == 3 || numDice == 4)
        {
            mainGame(numDice);
        }

        else if (numDice == 0)
        {
            return 0;
        }

        else {
            system("cls");
            cout << "\n That was invalid input!" << endl;
            cout << "\n ";
            system("pause");
        }
    }
}

void mainGame(int &numDice)
{
    bool goAgain = true;
    int dice[4];
    int input;

    while (goAgain == true)
    {
        for (int x = 0; x != numDice; x++)
        {
            dice[x] = (rand() % 6) + 1;

            cout << "\n Dice " << x + 1 << " shows a " << dice[x] << "!" << endl;
            cout << "\n ";
            system("pause");
        }

        system("cls");
        cout << "\n Would you like to go again with " << numDice << " dice?" << endl;
        cout << "\n Enter '1' for yes or '2' for no." << endl;
        cout << "\n> ";
        cin >> input;
        switch (input)
        {
            case 1:
                break;

            case 2:
                goAgain = false;
        }
    }
}

using namespace std; is considered a bad practice.

Don't do this:

int gameRunning = true;

If you want to use a boolean variable, use a bool instead of int.

You can use boolean values directly in conditions,
no need to compare them to true / false.
So instead of this:

while (gameRunning == true)

You can write simply:

while (gameRunning)

Still about that while loop earlier,
since gameRunning is initialized to true,
the loop condition will be inevitable true for the first time.
That's kind of a waste to re-evaluate.
In situations like this,
considering rewriting it as a do { ... } while (...) loop.


AutoResetEventAsync, am I missing something? So I wrote an asynchronous version of AutoResetEvent:

public sealed class AutoResetEventAsync {
    private readonly Task<bool> falseResult = Task.FromResult(false);
    private readonly ConcurrentQueue<Handler> handlers = new ConcurrentQueue<Handler>();
    private readonly Task<bool> trueResult = Task.FromResult(true);
    private int isSet;

    public AutoResetEventAsync(bool initialState) {
        this.isSet = initialState ? 1 : 0;
    }

    public void Set() {
        this.isSet = 1;

        Handler handler;

        // Notify first alive handler
        while (this.handlers.TryDequeue(out handler))
            if (handler.TryUnsubscribe()) {
                handler.Invoke();
                break;
            }
    }

    public Task<bool> WaitAsync() {
        return this.WaitAsync(CancellationToken.None);
    }

    public Task<bool> WaitAsync(TimeSpan timeout) {
        if (timeout == Timeout.InfiniteTimeSpan)
            return this.WaitAsync();

        var tokenSource = new CancellationTokenSource(timeout);
        return this.WaitAsync(tokenSource.Token);
    }

    public Task<bool> WaitAsync(CancellationToken cancellationToken) {
        // Short path
        if (this.TryReset())
            return this.trueResult;

        if (cancellationToken.IsCancellationRequested)
            return this.falseResult;

        // Wait for a signal
        var completionSource = new TaskCompletionSource<bool>(false);

        var handler = new Handler(() => {
            if (this.TryReset())
                completionSource.TrySetResult(true);
        });

        // Subscribe
        this.handlers.Enqueue(handler);

        if (this.TryReset()) {
            // Unsubscribe
            handler.TryUnsubscribe();
            return this.trueResult;
        }

        cancellationToken.Register(() => {
            // Try to unsubscribe, if success then the handler wasn't invoked so we could set false result,
            // else the handler was invoked and the result is already set.
            if (handler.TryUnsubscribe())
                completionSource.TrySetResult(false);
        });

        return completionSource.Task;
    }

    private bool TryReset() {
        return Interlocked.CompareExchange(ref this.isSet, 0, 1) == 1;
    }

    private sealed class Handler {
        private readonly Action handler;
        private int isAlive = 1;

        public Handler(Action handler) {
            this.handler = handler;
        }

        public void Invoke() {
            this.handler();
        }

        public bool TryUnsubscribe() {
            return Interlocked.CompareExchange(ref this.isAlive, 0, 1) == 1;
        }
    }
}

In my application it works well.

The question is: am I missing some concurrency issue?

I tested concurrent invocation of WaitAsync method, so, maybe you could suggest a better test scenario?

Your code does have race conditions, so it won't work correctly. I think your main problem is that you make some “destructive” operation (e.g. dequeue a handler from the queue) without knowing that the followup operation will actually succeed and not handling the eventual failure in any way.

For example, the following sequence of events could happen:

The class is created in unset state.
WaitAsync() is called, creating a Handler, adding it to the queue and returning a Task.
Set() is called, setting isSet to 1.
The handler from step 2 is dequeued.
WaitAsync() is called from another thread. Since isSet is 1, it sets isSet back to 0 and immediately returns a completed Task.
Back on the previous thread, TryUnsubscribe() is called in the handler from step 4 and succeeds.
The handler is Invoke()d, but TryReset() fails.
Set() now returns.

After this, the Task from step 2 is now orphaned, it will never be completed, because its handler is not in the queue anymore.

I suggest you don't write code like this by yourself, but instead use code written by others with more experience in concurrent programming. Specifically, you could use Stephen Toub's AsyncAutoResetEvent or AsyncAutoResetEvent from Stephen Cleary's AsyncEx library.


Unit testing Video Service class I wrote my first unit testing code on a service class using Mockito. The code looks like this:

// Test Class
public class TestVideoService {
    private VideoService videoService;
    @Mock private VideoRepository videoRepository;
    @Rule public MockitoRule mRule = MockitoJUnit.rule();

    @Before
    public void setUp(){
        this.videoService = new VideoService(videoRepository);
    }

    @Test
    public void testGetAllVideos() throws Exception{
        when(videoRepository.findAll()).thenReturn(new ArrayList<Video>());
        assertEquals(new ArrayList<Video>(), videoService.getAllVideos(), "tested");
    }

}

I also have several questions. I feel like I'm just comparing the same mock values (ArrayList of Video) – what's the point of doing this, or am I just doing it the wrong way?

Some other classes:

@Service
public class VideoService {
    @Autowired
    private VideoRepository videoRepository;

    public VideoService(VideoRepository videoRepository){
        this.videoRepository = videoRepository;
    }

    public List<Video> getAllVideos(){
        return videoRepository.findAll();
    }
}

@Repository
public interface VideoRepository extends JpaRepository<Video, Long> {
}

Welcome to code review and thanks for sharing your code.

When writing UnitTests keep in mind that they are documentation.
Test code has less restrictions for identifier names.
This means that method names are usually longer and you should not "shorten" your unit test methods.

The test name should describe the expected behavior in detail

@Test
public void getAllVideos_returnsCompleteListFromRepository() throws Exception{
    // ...

A unit test always has three parts: arrange, act and assert.
You should always have this three parts visible in your test methods.

In your example you do the act part implicitly in the assert line.
This prevents you from introducing variable that could give useful information about the result:

@Test
public void getAllVideos_returnsCompleteListFromRepository()
    // arrange
    List<Video> allVideosInRepository = new ArrayList<>();
    when(videoRepository.findAll()).thenReturn(new ArrayList<Video>());
    // act
    List<> allVideosFromRepository = videoService.getAllVideos();
    // assert
    assertEquals(allVideosInRepository, allVideosFromRepository, "tested");
}

For the same reason the String parameter of the assert* method should always be a useful hint in case the test fails because it is part of the error message:

    assertEquals(allVideosInRepository,
                 allVideosFromRepository,
                 "is same object");

  I feel like i just compare the same mock values ArrayList of Video and what's the point of doing this or i just do it wrong way.

This is not a problem of your unit tests but of your current design.

For the time being your VideoService class is nothing more than a facade to a Collection, there is no business behavior (yet) to be tested.


Optimize Sieve of Eratosthenes for big numbers in Python I'm solving Euler exercise number 7 and trying to go a bit further on bigger primes.
It is too slow with big numbers. I've been reading ways to optimize the sieve, but I would like to ask people with more experience with this.
import time
from math import log, ceil

start = time.time()
primes = [2]
op_count = 0
limitN = 0
pr_count = 0

def primes_goto(prime_index):
    global primes
    global op_count
    global limitN
    global pr_count

    if prime_index < 6:
        limitN = 100
    else:
        limitN = ceil(prime_index * (log(prime_index) + log(log(prime_index)))) #bound

    not_prime = set()

    while pr_count < prime_index:
        for i in range(3, limitN, 2):
            if i in not_prime:
                continue

            for j in range(i*3, limitN, i*2):
                not_prime.add(j)

            primes.append(i)
            pr_count += 1

    return primes

ind = int(10001)
primes_goto(ind)
ind_prime = primes[ind-1]

end = time.time()

print("Prime number at posizion: {} = {}".format(ind, ind_prime))
print("Runtime: {}".format(end-start))

You can make this part a bit faster:
            for j in range(i*3, limitN, i*2):
                not_prime.add(j)

Better start at i*i, and better don't do your own loop. So it becomes:
            not_prime.update(range(i*i, limitN, i*2))


Is it proper TPT Inheritance I have following model and database created using Entity Framework. Is it proper TPT Inheritance?

Is it possible to make the base class as abstract?

Model

Database

CODE

namespace LijosEF
{
public partial class Book
{
    public override void Sell()
    {
        this.AvailabilityStatus = "BookSOLD";
    }

}

public partial class DigitalDisc
{
    public override void Sell()
    {
        this.AvailabilityStatus = "DiscSOLD";
    }

}

public partial class SellingItem
{
    public virtual void Sell()
    {
        //Do nothing
    }
}
}

Client

namespace LijosEF
{
class Program
{

    static string connectionStringVal;

    static void Main(string[] args)
    {

        SqlConnectionStringBuilder sqlBuilder = new SqlConnectionStringBuilder();
        sqlBuilder.DataSource = ".";
        sqlBuilder.InitialCatalog = "LibraryReservationSystem";
        sqlBuilder.IntegratedSecurity = true;

        // Initialize the EntityConnectionStringBuilder.
        EntityConnectionStringBuilder entityBuilder = new EntityConnectionStringBuilder();
        entityBuilder.Provider = "System.Data.SqlClient";
        entityBuilder.ProviderConnectionString = sqlBuilder.ToString();
        entityBuilder.Metadata = @"res://*/MyModelFirstTest.csdl|res://*/MyModelFirstTest.ssdl|res://*/MyModelFirstTest.msl";

        connectionStringVal = entityBuilder.ToString();

        //AddBook();
        //AddDisc();

        //MakePurchase();

        int billAmount = GetTotalPurchaseValue(1);

    }

    private static void AddBook()
    {
        using (var db = new MyModelFirstTestContainer(connectionStringVal))
        {
            Book book = new Book();
            book.AvailabilityStatus = "Available";
            book.Price = 150;
            book.Title = "Maths Easy";

            db.SellingItems.AddObject(book);
            db.SaveChanges();

        }

    }

    private static void MakePurchase()
    {

        using (var db = new MyModelFirstTestContainer(connectionStringVal))
        {
            DigitalDisc disc = db.SellingItems.OfType<DigitalDisc>().FirstOrDefault(p => p.Artist == "Turtle Violin");
            disc.Sell(); //Changes State

            Book book = db.SellingItems.OfType<Book>().FirstOrDefault(p => p.Title == "Maths Easy");
            book.Sell(); //Changes State

            Purchase purchase = new Purchase();
            purchase.Date = DateTime.Now;
            purchase.SellingItems.Add(book);
            purchase.SellingItems.Add(disc);

            db.Purchases.AddObject(purchase);
            db.SaveChanges();

        }

    }

    private static int GetTotalPurchaseValue(int purchaseID)
    {
        using (var db = new MyModelFirstTestContainer(connectionStringVal))
        {

            var purchase = db.Purchases.FirstOrDefault(p => p.PurchaseId == purchaseID);
            if( purchase == null)
            {
                return 0;
            }

            var total = purchase.SellingItems.Sum(x => Convert.ToInt32(x.Price));
            return total;

        }

    }

    private static void AddDisc()
    {

        using (var db = new MyModelFirstTestContainer(connectionStringVal))
        {
            DigitalDisc disc = new DigitalDisc();
            disc.AvailabilityStatus = "Available";
            disc.Price = 300;
            disc.Artist = "Turtle Violin";

            db.SellingItems.AddObject(disc);
            db.SaveChanges();

        }

    }

}
}

REFERENCE:

https://stackoverflow.com/questions/4254182/inheritance-vs-enum-properties-in-the-domain-model/4259033#comment15499738_4259033
https://stackoverflow.com/questions/11683536/entity-framework-get-subclass-objects-in-repository#comment15499860_11683536
https://softwareengineering.stackexchange.com/questions/158472/why-should-i-add-check-constraint

Yes, it is a proper TPT inheritance and yes, you can mark your base class as abstract.


Building nested dictionaries to dump it to .json later I am looking for some way to improve my code. It works but it doesn't look good and I am almost sure that lot of Best Practices rules are violated.

The save_session() function, creates dictionary with other dictionaries nested in. At the end the dictionary is dump to .json file.

I will appreciate any suggestions and advice for improvement of this code.

For better understanding the code of save_session():

workouts_instances is the list of current WorkoutPlan instances,

WorkoutPlan stores instances of Training and Exercise,

Training stores instances of Exercise,

class WorkoutPlan:
    def __init__(self, name, trainings=list(), exercises=list()):
        self.name = name
        self.trainings = trainings
        self.exercises = exercises
    ...

class Training:
    def __init__(self, name, exercises=list()):
        self.name = name
        self.exercises = exercises
    ...

class Exercise:
    def __init__(self, name, details=dict()):
        self.name = name
        self.details = details
    ...

Here is the code of save_session():

def save_session():
    """Exports current data to .json file."""
    data_to_save = {}

    # add workout plans
    for workout in workouts_instances:
        data_to_save[workout.name] = {}

        # add trainings
        if workout.trainings:
            data_to_save[workout.name]["trainings"] = {}

            for training in workout.trainings:
                data_to_save[workout.name]["trainings"][training.name] = {}

                # add training exercises
                if training.exercises:
                    data_to_save[workout.name]["trainings"][training.name]['exercises'] = {
                        exercise.name: {} for exercise in training.exercises
                    }

                    for exercise in training.exercises:

                        if exercise.details:
                            data_to_save[workout.name]["trainings"][training.name]['exercises'][exercise.name][
                                'details'] = {detail: value for detail, value in
                                              exercise.details.items()}

        # add exercises
        if workout.exercises:
            data_to_save[workout.name]["exercises"] = {}

            exercises_to_save = data_to_save[workout.name]["exercises"]

            for exercise in workout.exercises:
                exercises_to_save[exercise.name] = {}

                if exercise.details:
                    details = exercise.details
                    exercises_to_save[exercise.name]['details'] = {detail: value for detail, value in details.items()}

    with open(WORKOUTS_FILE, 'w') as json_file:
        json.dump(data_to_save, json_file, indent=4)

and here is the example of output, data stored in .json file:

{
    "FBV - Full Body Workout": {
        "trainings": {
            "Training A": {
                "exercises": {
                    "squats": {
                        "details": {
                            "description": "squats with barbell",
                            "series": 4,
                            "repeats": 4,
                            "load": 70
                        }
                    }
                }
            }
        },
        "exercises": {
            "some exercise name": {
                "details": {
                    "description": "some description",
                    "series": 5,
                    "repeats": 5,
                    "load": 60
                }
            },
            "bench press - wide": {
                "details": {
                    "description": "bench press with wide grip",
                    "series": 5,
                    "repeats": 5,
                    "load": 60
                }
            }
        }
    }
}

Some suggestions:

Do not use mutable default arguments, they will mess with your subsequent calls. There are some very rare cases where mutable default arguments make sense, but this does not look like one of those.
Split generation of data from saving that data to a file. That way either method can be reused. If you also pull out the file handle to a main method you can dependency inject the file handle and test the whole functionality.
details have the same pattern everywhere, so they should also be a class or should be part of Exercise.
Like most Python code this could benefit from type annotations using MyPy.


Dapper "async" and "await using" I have been building my data access layer and performance is key for the project I am working on.

I have chosen to use Dapper and dotnet core 3. I want to make use of the latest and greatest async features in dotnet core and Dapper to allow for smarter resource allocation under heavy load, however there is not much documentation out there.

All the dapper wrappers seem to be using non async and most examples I find for SqlConnection use non async methods.

I was wondering if the use of all async and await is worth the overhead.

using System;
using System.Collections.Generic;
using System.Data;
using System.Data.SqlClient;
using System.Threading.Tasks;
using Dapper;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Outible.API.Model;
using Outible.DataAccess.Interfaces;

namespace Outible.DataAccess.Models
{
    public class Db : IDb
    {
        private readonly IOptionsMonitor<ConnectionStrings> _connectionStrings;
        private readonly ILogger<Db> _logger;

        public Db(IOptionsMonitor<ConnectionStrings> connectionStrings, ILogger<Db> logger)
        {
            _connectionStrings = connectionStrings;
            _logger = logger;
        }

        private async Task<T> CommandAsync<T>(Func<IDbConnection, IDbTransaction, int, Task<T>> command)
        {
            var connection = new SqlConnection(_connectionStrings.CurrentValue.MainDatabase);
            await connection.OpenAsync().ConfigureAwait(false);
            await using var transaction = await connection.BeginTransactionAsync();
            try
            {
                var result = await command(connection, transaction, 300).ConfigureAwait(false);

                await transaction.CommitAsync().ConfigureAwait(false);

                return result;
            }
            catch (Exception ex)
            {
                await transaction.RollbackAsync().ConfigureAwait(false);
                _logger.LogError(ex, "Rolled back transaction");
                throw;
            }
        }

        public Task<int> ExecuteAsync(string sql, object parameters)
        {
            return CommandAsync((conn, trn, timeout) => conn.ExecuteAsync(sql, parameters, trn, timeout));
        }

        public Task<T> GetAsync<T>(string sql, object parameters)
        {
            return CommandAsync((conn, trn, timeout) => conn.QuerySingleOrDefaultAsync<T>(sql, parameters, trn, timeout));
        }

        public Task<IEnumerable<T>> SelectAsync<T>(string sql, object parameters)
        {
            return CommandAsync((conn, trn, timeout) => conn.QueryAsync<T>(sql, parameters, trn, timeout));
        }
    }
}

I see you have already applied using to the transaction.

But given that a new SqlConnection is created for each command, I would suggest wrapping it in a using statement as well.

private async Task<T> CommandAsync<T>(Func<IDbConnection, IDbTransaction, int, Task<T>> command)
{
    using(var connection = new SqlConnection(_connectionStrings.CurrentValue.MainDatabase))
    {
        await connection.OpenAsync().ConfigureAwait(false);
        await using var transaction = await connection.BeginTransactionAsync();
        try
        {
            T result = await command(connection, transaction, 300).ConfigureAwait(false);

            await transaction.CommitAsync().ConfigureAwait(false);

            return result;
        }
        catch (Exception ex)
        {
            await transaction.RollbackAsync().ConfigureAwait(false);
            _logger.LogError(ex, "Rolled back transaction");
            throw;
        }
    }
}


Print largest/smallest no (even, odd) from user defined list of given size After reviews of my previous codes 1st, 2nd learned many new things. So with those in mind, this is my solution for another question.

Question: Write a program that ask user for list size and values and find largest and smallest no, even no, odd no?
Expected output: For user defined list,

[13, 7, 19, 21, 3, 4, 27, 47, 50, 11] -> 50, 3, 50, 4, 47, 3

[2, 4, 6, 8, 10] -> 10, 2, 10, 2, None, None

[1, 3, 5, 7, 9] -> 9, 1, None, None, 9, 1

Format: largest_no, smallest_no, largest_even, smallest_even, largest_odd, smallest_odd

My solution:
def largest_and_smallest(given_list):
    """Extracts required numbers from given_list.

    >>> largest_and_smallest([13, 7, 19, 21, 3, 4, 27, 47, 50, 11])
    (50, 3, 50, 4, 47, 3)

    >>> largest_and_smallest([2, 4, 6, 8, 10])
    (10, 2, 10, 2, None, None)

    >>> largest_and_smallest([1, 3, 5, 7, 9])
    (9, 1, None, None, 9, 1)

    """
    sorted_list = sorted(given_list)
    largest_no = max(sorted_list)
    smallest_no = min(sorted_list)

    list_of_even = [even_no for even_no in sorted_list if ((even_no % 2) == 0)]
    if len(list_of_even) == 0:
        largest_even = None
        smallest_even = None
    else:
        largest_even = max(list_of_even)
        smallest_even = min(list_of_even)

    list_of_odd = [odd_no for odd_no in sorted_list if ((odd_no % 2) != 0)]
    if len(list_of_odd) == 0:
        largest_odd = None
        smallest_odd = None
    else:
        largest_odd = max(list_of_odd)
        smallest_odd = min(list_of_odd)

    return largest_no, smallest_no, largest_even, smallest_even, largest_odd, smallest_odd

def input_handler():
    """Checks validity of input. Any mistakes causes the program to stops."""
    size = input("Enter a size of list?\n>")
    if size.strip() == "":
        exit("Program Terminated. (No Input)")
    elif size.isdigit():
        list_size = int(size)
        list_of_no = [int(input("Enter value:")) for _ in range(list_size)]  # bad input causes error.
        return list_size, list_of_no
    else:
        exit("Program Terminated. (Bad Input)")

def main():
    print("All input should be integers.")
    list_size, list_of_no = input_handler()
    l_no, s_no, l_even, s_even, l_odd, s_odd = largest_and_smallest(list_of_no)
    print()
    print(f"From provided values of list of a size {list_size}:")
    print(f"Largest no.: {l_no}, Smallest no.: {s_no}, Largest even no.: {l_even}, "
          f"Smallest even no.: {s_even}, Largest odd no.: {l_odd}, Smallest odd no.: {s_odd}")
    return

if __name__ == '__main__':
    import doctest
    doctest.testmod(main())

So, from this review I would like to know:

I have used list comprehension as per suggestion in previous code, have I used that correctly? For example, I made separate list for even and odd numbers could I have done that in different way?

I made a separate function to handle input is that a bad practice? What is the best way to handle inputs, that checks different corner cases?

If there is any recommendations going forward from this will help me a lot.

Your variables are long, without needing to be.
You don't have to define largest_even, amongst others, it'd be simpler to just return the tuple.
If you're targeting 3.4+, and I don't see why you wouldn't, you can use the default keyword. "New in version 3.4: The default keyword-only argument."

Leaving you simply with:

def largest_and_smallest(numbers):
    even = [n for n in numbers if (n % 2) == 0]
    odd  = [n for n in numbers if (n % 2) != 0]
    return (
        max(numbers, default=None),
        min(numbers, default=None),
        max(even, default=None),
        min(even, default=None),
        max(odd, default=None),
        min(odd, default=None)
    )

If you don't like the verbosity of max, you could use functools.partial to set default twice.

from functools import partial

def largest_and_smallest(numbers):
    min_ = partial(min, default=None)
    max_ = partial(max, default=None)
    even = [n for n in numbers if (n % 2) == 0]
    odd  = [n for n in numbers if (n % 2) != 0]
    return (
        max_(numbers),
        min_(numbers),
        max_(even),
        min_(even),
        max_(odd),
        min_(odd)
    )


Missing level of technical depth (Recursive to Iterative) I recently have given a coding test in a reputable IT company. There were three coding questions.

They refused me by saying that

  as we felt they didn't demonstrate the level of technical depth we're
  seeking from candidates

Please help me understanding what type of the technical depth my solution is missing? Thanks

Question 2 : Missing level of technical depth (Flatten Tree)

Question 3 : Missing level of technical depth (Common Ancestor)

and this is the 1st question.

Question 1:

  Reimplement this code so that its results will always be the same, but
  that it does not cause a stack overflow on large inputs.  Your
  solution must still implement the Folder interface.

Answer 1:

package iteration;

import java.util.Queue;

public interface Folder<T, U>
{
    U fold(U u, Queue<T> list, Function2<T,U,U> function);
}

package iteration;

public interface Function2<T, U, R>
{
  R apply(T t, U u);
}

package iteration;

import java.util.Iterator;
import java.util.LinkedList;
import java.util.Queue;

public class MyFolder<T, U> implements Folder<T, U>
{

     public U fold(U u, Queue<T> ts, Function2<T, U, U> fun) {

        if(u == null || ts == null || fun == null)
            throw new IllegalArgumentException();

        if (ts.isEmpty()) {
            return u;
        }

      //get an iterator first instead of forEach loop, because poll() is modifying underlying collection/queue
        Iterator<T> iter = ts.iterator();
        while (iter.hasNext()) { //loop over the items

            u = fun.apply( ts.poll() , u);

        }

        return u;
    }

    public static void main(String[] args) {

        Folder<Integer,Integer> folder = new MyFolder<Integer, Integer>();

        Queue<Integer> q = new LinkedList<Integer>();

        for(int lop =0; lop < 1000000; lop++ ){ //add some values in the Queue
            q.add(lop);
        }
        Integer result = folder.fold(0, q, new Function2<Integer, Integer, Integer>() {
            public Integer apply(Integer val1, Integer val2) {
                return val1 + val2;
            }
        });

        System.out.println("Result: " + result);
    }
}

as we felt they didn't demonstrate the level of technical depth we're seeking from candidates

This is a stock response. The engineers who interviewed you probably said something like “nah, not good enough”, so someone in HR sent you their stock response. Don't worry about the exact words; it wasn't written about you.

Some problems they might have objected to:

fold shouldn't throw an IllegalArgumentException when U is null; null is a reasonable input.
It traverses the Queue with both Queue.poll (which removes the first element) and an iterator (which doesn't modify the queue). Pick one or the other. Doing both may not even have well-defined behavior — what happens to the iterator when you remove the current element?
The initial isEmpty check is redundant. If the queue is empty, the iteration loop will do nothing.
There are lots of blank lines that take space without contributing to readability. That you include them in such a small program suggests you're not used to large programs.

There are also some design flaws in the specification.

Folder should not be an interface. It makes no sense to create an object in order to call its fold method; fold should simply be a static method.
If the original version used Queue.poll instead of an iterator, then it destroys the contents of the queue. It's usually not a good idea to modify arguments unnecessarily, because it tends to create bugs.
fold should probably accept any Iterable, not just a Queue.

If you didn't point out these problems, they might take that as a sign you wouldn't notice design flaws in real work either. Part of a programmer's job is noticing problems and (when feasible) fixing them!


Python 3 - simple temperature program version 1.3 Thank you to everyone for the feedback provided to the initial version of this program posted here.

Please find below the newest version, revised based on the comments provided, for review and comment on how I can further improve my coding.

I have removed the portion of the program that checked if the provided value was a legitimate temperature, as this was useless code only showing that I had recently learned what the opposite of absolute zero was....who cares, and who cares what value the user wants to convert!

I look forward to your feedback.

#!/usr/bin/python
"""

Program: Temperature Conversion (C to F, or F to C)
Date:    05 May 2019
Version: 1.2
Author:  Jason P. Karle

Remark:  This program was inspired by a Python exercise that
asks you to create a program that will convert one Celsius value to Fahrenheit;
so a program that can be executed with three lines of code.

However, I wanted to make something that would allow the user to
convert to and from either C of F, and do so multiple times, until the user
decides to end the program. This was also an exercise for me to
advance not only my code skills, but how I structure a program.

version history:
    1.0 Initial draft
    1.1 Correction of runtime; posted to StackExchange for feedback
    1.2 Re-coded based on feedback; trying to improve flow control
"""

def read_selection():
    selection = input('''Welcome to the temperature conversion program!

Please make a selection:

    c to convert from Celcius to Fahrenheit;
    f to convert from Fahrenheit to Celsius; or
    q to quit the program.

Enter your selection: ''').lower()
    return selection

def value_input(selection):
    value = input('''\nPlease enter the temperature you
want to convert: ''')
    try:
        value = float(value)
    except ValueError:
        print('That is not a number!\n')
    else:
        if selection == 'c':
            convert_c2f(value)
        else:
            convert_f2c(value)
            # return value

def convert_c2f(value):
    print(f'The answer is: {(value * (9/5)) + 32}°F\n')
    return

def convert_f2c(value):
    print(f'The answer is: {(value-32) * (5/9)}°C\n')
    return

def main():
    while True:
        selection = read_selection()
        if selection == 'q':
            return
        elif selection == 'c' or selection == 'f':
            value_input(selection)
            '''convert_c2f()
        elif selection == 'f':
            convert_f2C()'''
        else:
            print('Invalid selction. Try again.\n')

if __name__ == '__main__':
    main()

You're not following PEP8 and you still have a spaghetti mind-set. Each function should have a single responsibility.

value_input however is in charge of:

Asking and validating user input.
Handling how to convert the input.
Convert and display the input.

This should instead only perform the first task I've said above. After this you should have the calling code perform 2 and 3.

convert_c2f also is responsible for two things converting and displaying the input.

It can be seen in main that you were originally doing something better than you have now, so it's unclear why you changed this.

def read_selection():
    return input('''Welcome to the temperature conversion program!

Please make a selection:

    c to convert from Celcius to Fahrenheit;
    f to convert from Fahrenheit to Celsius; or
    q to quit the program.

Enter your selection: ''')

def float_input():
    value = input('''\nPlease enter the temperature you
want to convert: ''')
    try:
        return float(value)
    except ValueError:
        print('That is not a number!\n')

def convert_c2f(value):
    return (value * (9 / 5)) + 32

def convert_f2c(value):
    return (value - 32) * (5 / 9)

def main():
    while True:
        selection = read_selection().lower()
        if selection == 'q':
            return
        elif selection == 'c' or selection == 'f':
            value = float_input(selection)
            if selection == 'c':
                converted = convert_c2f(value)
                print(f'The answer is: {converted}°F\n')
            else:
                converted = convert_f2c(value)
                print(f'The answer is: {converted}°C\n')
        else:
            print('Invalid selction. Try again.\n')


Is there a better way to condense the while loop? I have been trying to write a number guessing game and I am trying to condense the second while loop when guess =! random_number. I can't figure out if there is any better way to shorten the code but to still achieve the same result. I wonder if someone is able to help me out.  If there's anything else which is redundant or not efficient in my code or how i could code it better please criticize the hell out me my work!
import random

def inputNumber(message):
    while True:
        try:
            userInput = int(input(message))
        except ValueError:
            print("Not an integer! Try again.")
            continue
        else:
            return userInput
            break

#ranges must be integers
min_range = int(inputNumber("Please insert the min range: "))
temp_range = int(inputNumber("Please insert the max range: "))

#max range must be bigger than min
while min_range >= temp_range:
    temp_range = int(inputNumber("Please insert a max range which is smaller than min range!: ")) + 1

max_range = temp_range

random_number = random.randint(min_range, max_range)

guess = int(input("Please enter your guess: "))
counter = 1;  # you guessed once already
while guess != random_number:  # 1 guess incorrect
    print(f"{random_number} is the random number")# to see for myself which is the random numb
    if min_range < guess < max_range:  # WITHIN THE RANGE
        if random_number - random_number / 12 < guess-random_number < random_number + random_number / 12:  # 3 super hot
            print("Please try again, you are super hot!")
        elif random_number - random_number / 8 < guess-random_number < random_number + random_number / 8:  # 3 hot
            print("Please try again, you are hot!")
        elif guess < random_number - random_number / 2 or guess-random_number > random_number + random_number / 2:  # 3 cold
            print("Please try again, you are cold!")

    else:  # OUTSIDE RANGE
        print("You are outside the range, try again!")
    guess = int(input())
    counter += 1
if guess == random_number:  # guess correct
    print(f"Congratulations, you guessed correctly in {counter} tries!")

edit: thanks you all so much for taking the time to review my code, i will try to improve on the noted points, much appreciated (:

Miscellaneous notes:

you don't need a break after a return
you could (attempt to) return directly from the input; "except" will still catch and cycle errors
inputNumber() explicitly returns an integer; no need to cast again.
temp_range isn't really needed - you can just use max_range, no loss to readability.
no need to add one to max_range (in the validation loop) - randint() includes the top limit.
random_number is an unhelpful variable name - I've called this target
using inputNumber() for the guess also seems sensible.
guess should be checked against the inclusive range - min and max are possible values.
personally I think the temperature categories should reflect the size of the range, rather than the size of the chosen number... however
your calculation is wrong anyway; you are testing the difference between guess and target so you should test against the small fraction of target, not target reduced by a fraction (or should just check guess against the numbers you calculate).
the "hot" range (outside "super hot") is actually smaller than the "super hot" range - I adjusted this out to 1/4 of target
after correction, there's a range that gets no message, which can be covered in an else clause. I reordered so all tests are for "in this range" and the else covers the "cold" option.

To condense the message selection, you could set up a list of boundaries changing from one condition to another, and step through (or search) to find the applicable message. However I'm not sure that's a lot clearer in this case.
With updates reflecting most of these:
import random
CHEATING = True

def inputNumber(message):
    while True:
        try:
            return int(input(message))
        except ValueError:
            print("Not an integer! Try again.")

#ranges must be integers
min_range = inputNumber("Please insert the min range: ")
max_range = inputNumber("Please insert the max range: ")

#max range must be bigger than min
while min_range >= max_range:
    max_range = inputNumber("Please insert a max range which is bigger than min range!: ")

target = random.randint(min_range, max_range)

guess = inputNumber("Please enter your guess: ")
counter = 1;  # you guessed once already
while guess != target:  # 1 guess incorrect
    if CHEATING: print(f"{target} is the random number")# to see for myself which is the random numb
    if min_range <= guess <= max_range:  # WITHIN THE RANGE
        if  -target / 12 < guess-target < target / 12:  # 3 super hot
            print("Please try again, you are super hot!")
        elif  -target / 4 < guess-target < target / 4:  # 3 hot
            print("Please try again, you are hot!")
        elif -target / 2 < guess-target < target / 2:   # a bit meh
            print("Not great, not terrible - please try again")
        else:                                           # 3 cold
            print("Please try again, you are cold!")

    else:  # OUTSIDE RANGE
        print("You are outside the range, try again!")
    guess = inputNumber("Next guess? ")
    counter += 1
if guess == target:  # guess correct
    print(f"Congratulations, you guessed correctly in {counter} tries!")```

PS: "please criticize the hell out my work" is an excellent attitude, if you have the fortitude for it, and a great way to learn.


Calculating the squares dominated by a queen on a chessboard I have a JavaScript function that increments a counter for each square on the chessboard as it becomes dominated by a queen and decrements it if one of the dominating queens are removed (only queens, this isn't actually a chess game with all of the pieces, it's essentially the 8 queens puzzle). If a square is dominated by at least one queen, (e.g. square.dominatedBy > 0), a CSS class is later applied to change the color of the square.

The function seems far longer than it needs to be though, and there has to be a better way to DRY it up.

(chessBoardSquares is an array of an array of Squares on the board, which are objects with properties such as row, column, and dominatedBy)

function setDominatedSquares(queenPosition, remove = false) {
    // handle squares in the same row as the queen
    for (var column = 0; column < boardSizeX; column++) {
        if (remove) {
            chessBoardSquares[queenPosition.row][column].dominatedBy--;
        } else {
            chessBoardSquares[queenPosition.row][column].dominatedBy++;
        }
    }
    // handle squares in the same column as the queen
    for (var row = 0; row < boardSizeY; row++) {
        if (remove) {
            chessBoardSquares[row][queenPosition.column].dominatedBy--;
        } else {
            chessBoardSquares[row][queenPosition.column].dominatedBy++;
        }
    }
    // handle diagonals down and to right
    var row = queenPosition.row;
    var column = queenPosition.column;
    while (row < boardSizeY && column < boardSizeX) {
        if (remove) {
            chessBoardSquares[row][column].dominatedBy--;
        } else {
            chessBoardSquares[row][column].dominatedBy++;
        }
        row++;
        column++;
    }
    // handle diagonals up and to left
    row = queenPosition.row;
    column = queenPosition.column;
    while (row >= 0 && column >= 0) {
        if (remove) {
            chessBoardSquares[row][column].dominatedBy--;
        } else {
            chessBoardSquares[row][column].dominatedBy++;
        }
        row--;
        column--;
    }

    // handle diagonals down and to left
    row = queenPosition.row;
    column = queenPosition.column;
    while (row >= 0 && column < boardSizeX) {
        if (remove) {
            chessBoardSquares[row][column].dominatedBy--;
        } else {
            chessBoardSquares[row][column].dominatedBy++;
        }
        row--;
        column++;
    }

    // handle diagonals up and to right
    row = queenPosition.row;
    column = queenPosition.column;
    while (row < boardSizeY && column >= 0) {
        if (remove) {
            chessBoardSquares[row][column].dominatedBy--;
        } else {
            chessBoardSquares[row][column].dominatedBy++;
        }
        row++;
        column--;
    }
    // update the UI
    updateDominatedSquares();
}

Update: For anyone interested, this code is now running live at queensoptimization.azurewebsites.net

I would probably break this up into a function setDominatedForLine that would work for each direction. Instead of a true/false remove parameter I used a dominatedIncrement parameter with a value of +1 or -1.

function setDominatedSquares(queenPosition, dominatedIncrement = +1) {

  function setDominatedForLine(rowInc, columnInc) {
    let row    = queenPosition.row;
    let column = queenPosition.column;
    while (row >= 0 && row < boardSizeY && column >= 0 && column < boardSizeX) {
      chessBoardSquares[row][column].dominatedBy += dominatedIncrement;
      row    += rowInc;
      column += columnInc;
    }
  }

  // Column
  setDominatedForLine( 0,  1);
  setDominatedForLine( 0, -1);
  // Row
  setDominatedForLine( 1,  0);
  setDominatedForLine(-1,  0);
  // Diagonals
  setDominatedForLine( 1,  1);
  setDominatedForLine( 1, -1);
  setDominatedForLine(-1,  1);
  setDominatedForLine(-1, -1);

  // update the UI
  updateDominatedSquares();
}

You could also modify it slightly with something like:

let directions = [ {r:-1, c:-1}, {r:-1, c: 0}, {r:-1, c: 1},
                   {r: 0, c:-1},               {r: 0, c: 1},
                   {r: 1, c:-1}, {r: 1, c: 0}, {r: 1, c: 1} ]
for (let dir of directions)
  setDominatedForLine( dir.r, dir.c );

But I'm not sure if that is an improvement.


Implementation of a PID Controller I've implemented a PID controller, but seeing as I'm not an expert in control theory, have I missed any edge cases?

public class PIDController
{
    public enum PIDMode
    {
        Manual,
        Auto,
    }

    public enum PIDAction
    {
        Indirect,
        Direct,
    }

    public PIDMode Mode { get; set; }
    public PIDAction Action { get; set; }

    public double Proportional { get; set; }
    public double Integral { get; set; }
    public double Derivative { get; set; }

    public double Minimum { get; set; }
    public double Maximum { get; set; }

    public double DeltaMinimum { get; set; }
    public double DeltaMaximum { get; set; }

    private double _ProportionalTerm;
    private double _Integrator;
    private double _Derivator;

    public double Setpoint { get; set; }

    private double _Feedback;
    public double Feedback
    {
        get
        {
            return _Feedback;
        }
    }

    public void Calculate(double Input, long Time)
    {
        double output;

        // Compute the error value
        double Error = Setpoint - Input;

        if (Mode == PIDMode.Auto)
        {
            if (Action == PIDAction.Direct)
                Error = 0 - Error;

            // Compute the proportional component
            _ProportionalTerm = 1000.0f * (Error - _Derivator) / (double)Time;

            // Compute the integrator component, clamped to min/max delta movement
            _Integrator += (float)Error * (float)Time / 1000.0f;
            if (_Integrator < DeltaMinimum)
                _Integrator = DeltaMinimum;
            if (_Integrator > DeltaMaximum)
                _Integrator = DeltaMaximum;

            // Add the proportional component
            output = (Proportional * Error);

            // Add the integral component
            output += Integral * _Integrator;

            // Add the derivative component
            output += Derivative * _ProportionalTerm;

            // Clamp output to min/max
            if (output < Minimum)
                output = Minimum;
            if (output > Maximum)
                output = Maximum;
        }
        else
        {
            output = Input;
        }

        // Store values
        _Derivator = Error;

        // Returns the result
        _Feedback = output;
    }
}

It looks reasonable, but I don't see why you need to make the Proportional term a member of the class, it doesn't need to be saved, at best it might make sense to be able to query it with a get, but a set seems misleading.

I'd be reluctant to allow public setting of the Integral and Derivatives too, querying them might be useful, but letting users of the class set it seems odd.

Forcing the scaling by 1000.0 seems arbitrary and inflexible too, this would make sense to expose as a settable constant in my view.


Fixed-size block allocator I have written a fixed-size block allocator implementation and would like some feedback as to what I could improve in my code and coding practices. Your comments or notes are welcomed!

An auxiliary allocator (non-typed, instead unique for the given size of a chunk; without necessary interface for STL containers):

#include <iostream>
#include <list>
#include <vector>

template <size_t ChunkSize>
class TFixedAllocator {
    union TNode { // union
        char data[ChunkSize];
        TNode *next; // free chunks of memory form a stack; pointer to the next (or nullptr)
    };

    TNode *free; // the topmost free chunk of memory (or nullptr)
    std::vector<TNode*> pools; // all allocated pools of memory
    int size = 1; // size of the last allocated pool of memory
    const int MAX_SIZE = 1024;

    void new_pool() { // allocate new pool of memory
        if (size < MAX_SIZE) {
            size *= 2;
        }
        free = new TNode[size];

        // form a stack of chunks of this pool
        pools.push_back(free);
        for (int i = 0; i < size; ++i) {
            free[i].next = &free[i+1];
        }
        free[size-1].next = nullptr;
    }
    TFixedAllocator() { // private for singleton
        new_pool();
    }
public:
    TFixedAllocator(const TFixedAllocator&) = delete; // for singleton
    static TFixedAllocator& instance () { // singleton
        static TFixedAllocator instance;
        return instance;
    }
    void* allocate() {
        if (!free) {
            new_pool();
        }
        TNode* result = free; // allocate the topmost element (saved in free)
        free = free->next; // and pop it from the stack of free chunks
        return static_cast<void*>(result);
    }
    void deallocate(void* elem) {
        TNode* node = static_cast<TNode*>(elem);

        // add to the stack of chunks
        node->next = free;
        free = node;
    }
    ~TFixedAllocator() {
        for (auto ptr : pools) {
            delete ptr;
        }
    }
};

An allocator wrapper for STL containers:

template <class T>
class TFSBAllocator {
public:
    using value_type = T;
    using pointer = T*;
    using const_pointer = const T*;
    using reference = T&;
    using const_reference = const T&;
    template <class U>
    class rebind {
    public:
        using other = TFSBAllocator<U>;
    };

    pointer allocate(size_t n) {
        if (n == 1) {
            return static_cast<T*>(TFixedAllocator<sizeof(T)>::instance().allocate());
        } else {
            return std::allocator<T>().allocate(n);
        }
    }
    void deallocate(pointer p, size_t n) {
        if (n == 1) {
            TFixedAllocator<sizeof(T)>::instance().deallocate(static_cast<void*>(p));
        } else {
            return std::allocator<T>().deallocate(p, n);
        }
    }
    void construct(pointer p, const_reference t) {
        new (p) T(t);
    }
    void destroy(pointer p) {
        p->~T();
    }
};

A tester:

#include <chrono>
using namespace std::chrono;

template <class List>
void test(std::string comment, List l) {
    std::cout << comment;
    auto start_time = high_resolution_clock::now();
    for (int i = 0; i < 1e7; ++i) {
        l.push_back(i);
    }
    auto end_time = high_resolution_clock::now();
    std::cout << duration_cast<milliseconds>(end_time - start_time).count() << "ms" << std::endl;
}

int main() {
    // CodeBlocks 12.13, Release mode
    test("std::allocator: ", std::list<int>()); // std::allocator: 1816ms
    test(" TFSBAllocator: ", std::list<int, TFSBAllocator<int>>()); // TFSBAllocator: 204ms
}

By the way, what is about thread-safety?

About thread safety: it is not thread safe. There is nothing that prevents member-functions such as  TFixedAllocator::allocate,  TFixedAllocator::new_pool and others to be executed by multiple threads at the same time.  Inside these functions several variables are modified. Modifying them from multiple threads without synchronization is an undefined behavior according to the C++ standard.

How to fix it? The easiest way to do it is to use one std::mutex for the allocate and deallocate member-functions of the TFixedAllocator class(acquiring it in the very beginning of the function and releasing it in the very end). It is convenient to use an std::lock_guard for this purpose. Something like this:

template <size_t ChunkSize>
class TFixedAllocator {
    ...

    std::mutex allocator_mutex;

public:
    ...

    void* allocate() {
        std::lock_guard<std::mutex> allocator_lock_guard(allocator_mutex);
        if (!free) {
            new_pool();
        }
        TNode* result = free; // allocate the topmost element (saved in free)
        free = free->next; // and pop it from the stack of free chunks
        return static_cast<void*>(result);
    }

    void deallocate(void* elem) {
        std::lock_guard<std::mutex> allocator_lock_guard(allocator_mutex);
        TNode* node = static_cast<TNode*>(elem);

        // add to the stack of chunks
        node->next = free;
        free = node;
    }
};

Functions and variables naming: it is conventional to name functions with a verb(to describe an action). I would use get_instance instead of instance and create_new_pool instead of new_pool, for example.
You TFixedAllocator class violates the single responsibility principle. Despite managing memory allocation(which is its main responsibility) it also implicitly implements some data structure(a stack?(I mean things related to the union TNode, the free pointer and so on)). It makes the code rather hard to follow: those operations with free and next pointers are not easy to understand(it is not clear why they are performed in the first place). There are two ways to fix it:

Create a separate class that implements this data structure.
Use a standard container. If what you need is a stack, std::stack is a good option(again, I'm not completely sure if it is feasible because I do not fully understand how this data structure is used in your code).

Writing useless comments is definitely a bad practice. For instance, union TNode { // union doesn't make much sense: it is absolutely clear that it is a union without any comments. I'd recommend simply deleting comments like this one. Comments inside a function that tell what a block of code does, like here:

void deallocate(void* elem) {
    TNode* node = static_cast<TNode*>(elem);

    // add to the stack of chunks
    node->next = free;
    free = node;
}

usually indicate that this block of code should have been a separate function(in this case, as I have mentioned before, there should probably be another class that implements this data structure). In general, you should try to write self-documenting code.
Spacing and indentation: separating different functions with an empty line makes the code more readable.


WebApi Get with paging How can I improve this code?

public virtual HttpResponseMessage Get(int pageNo, int pageSize)
{
    pageNo = pageNo > 0 ? pageNo - 1 : 0;
    pageSize = pageSize > 0 ? pageSize : 0;

    int total = repository.Table.Count();
    int pageCount = total > 0 ? (int)Math.Ceiling(total / (double)pageSize) : 0;

    var entity = repository.Table.OrderBy(c => c.ID).Skip(pageNo * pageSize).Take(pageSize);

    if (entity.Count() == 0 || entity == null)
    {
        var message = string.Format("{0}: No content", GenericTypeName);
        return ErrorMsg(HttpStatusCode.NotFound, message);
    }

    var response = Request.CreateResponse(HttpStatusCode.OK, entity);
    response.Headers.Add("X-Paging-PageNo", (pageNo + 1).ToString());
    response.Headers.Add("X-Paging-PageSize", pageSize.ToString());
    response.Headers.Add("X-Paging-PageCount", pageCount.ToString());
    response.Headers.Add("X-Paging-TotalRecordCount", total.ToString());

    return response;
}

Let's start with the BUG.

if (entity.Count() == 0 || entity == null)

If entity is null, then you'll get a NullReferenceException when Count is called, so there's currently no possible way the second half of this statement will ever be called. I think you meant to do this.

 if (entity == null || entity.Count == 0)

Which works, but isn't great. I would prefer "not Any" here. I find it's good practice to get into the habit of using Any. It's just more readable IMO.

if ( entity == null || !entity.Any() )

To clarify. Any() returns as soon it finds an element, whereas Count has to iterate over the entire Enumerable before it returns.

Note that this isn't true for List. Any() still returns early when called on a List, but so will Count because a list keeps track of how many items it has as they're added/removed. Enumerables do not already "know" how big they are. They must be iterated to get a count.

Now let's back up a bit and stop your Linq from scrolling off the screen.

var entity = repository.Table
                 .OrderBy(c => c.ID)
                 .Skip(pageNo * pageSize)
                 .Take(pageSize);

Maybe it's because I "grew up" with a language where everything is passed ByRef by default (.Net passes ByVal by default), but I don't like assigning values to arguments.

public virtual HttpResponseMessage Get(int pageNo, int pageSize)
{
    pageNo = pageNo > 0 ? pageNo - 1 : 0;
    pageSize = pageSize > 0 ? pageSize : 0;

I'd introduce another variable, but I'm having a hard time thinking of good names at the moment. I'd also consider extracting the logic into private methods to clarify the intent. It doesn't matter that you're unlikely to ever call them from anywhere else, some well named methods can really clarify what code is doing. You can always look inside them if you're interested in the implementation, but at this level, I don't care how we're calculating these numbers, I just care that we are.

public virtual HttpResponseMessage Get(int pageNo, int pageSize)
{
    pageNo = NormalizePageNo(pageNo);
    pageSize = NormalizePageSize(PageSize);

Here's another great opportunity to give logic a name.

int pageCount = total > 0 ? (int)Math.Ceiling(total / (double)pageSize) : 0;

Mmmm hmmm... Now what is that doing exactly?

  If the total number of records is greater than zero,
    Then return the percentage of total divided by page size, rounded up
    Else return Zero.

Again, these are implementation details. At the current level of abstraction, we just want to know

int pageCount = CalculatePageCount(total, pageSize);


Find all instances of a given weekday in February for a given year A friend of mine was practicing his programming skills with a textbook meant to prepare students for computer science exams. He asked for help with a specific task.

The task is to capture user input (day of week and a year in the range 1500 to 2005 inclusive), and output all instances of the weekday in February that year.

The differences between the Julian and Gregorian calendars are to be accounted for.

Now, most likely the idea behind this task was to have the student create an algorithm to manually calculate the dates. However, as Java SE seems to be allowed in exams in my country, I came up with the idea of utilizing the GregorianCalendar class (which, despite its name, combines the Julian and Gregorian calendars).

package calendar;

import java.text.DateFormat;
import java.util.GregorianCalendar;
import java.util.HashMap;
import java.util.Locale;
import java.util.Scanner;

public class CalendarTask {
    public static void main(String[] args) {
        GregorianCalendar cal = new GregorianCalendar();

        HashMap<String, Integer> daysOfWeek = new HashMap<>();
        daysOfWeek.put("monday", cal.MONDAY);
        daysOfWeek.put("tuesday", cal.TUESDAY);
        daysOfWeek.put("wednesday", cal.WEDNESDAY);
        daysOfWeek.put("thursday", cal.THURSDAY);
        daysOfWeek.put("friday", cal.FRIDAY);
        daysOfWeek.put("saturday", cal.SATURDAY);
        daysOfWeek.put("sunday", cal.SUNDAY);

        System.out.print("Enter day of week: ");
        Scanner sc = new Scanner(System.in);
        int dayOfWeek, year;
        try {
            dayOfWeek = daysOfWeek.get(sc.next().toLowerCase());
            System.out.print("Enter year (1500-2005 inclusive): ");
            year = Integer.parseInt(sc.next());
            if (year < 1500 || year > 2005) throw new Exception();

            System.out.println("Output:");
            DateFormat df = DateFormat.getDateInstance(DateFormat.MEDIUM, Locale.GERMANY);
            cal.set(cal.YEAR, year);
            trySetDay(cal, dayOfWeek, 1);
            do {
                System.out.println(df.format(cal.getTime()));
                cal.add(cal.DAY_OF_MONTH, 7);
            } while (cal.get(cal.MONTH) == cal.FEBRUARY);
        } catch (Exception e) {
            System.out.println("Incorrect input!");
        } finally {
            sc.close();
        }
    }

    private static void trySetDay(GregorianCalendar cal, int dayOfWeek, int weekOffset) {
        cal.set(cal.MONTH, cal.FEBRUARY);
        cal.set(cal.WEEK_OF_MONTH, weekOffset);
        cal.set(cal.DAY_OF_WEEK, dayOfWeek);
        if (cal.get(cal.MONTH) != cal.FEBRUARY) trySetDay(cal, dayOfWeek, weekOffset + 1);
    }
}

Example input and output (dates are output in DD.MM.YYYY format):

Enter day of week: Monday
Enter year (1500-2005 inclusive): 2000
Output:
07.02.2000
14.02.2000
21.02.2000
28.02.2000

The trySetDay() method is meant for cases when the attempted day belongs to the previous month. In such a case, the week offset is increased by one to make sure we're dealing with February.

This code works fine and I'm satisfied with it. The catch is there to handle NullPointerException (when attempting to assign null, a possible result of HashMap.get(), and also when parsing the year) and a generic Exception set to limit the possible input to the range 1500 to 2005.

What can be improved about this code? Is there anything that caught your eye instantly and that could be done better? Any and all feedback is appreciated.
Also, is it a good idea to access static class members via an instance of the class? Such as cal.MONDAY (where cal is an instance of GregorianCalendar), instead of GregorianCalendar.MONDAY?

Java 8 Time APIs

Instead of the 'legacy' Calendar and DateFormat classes, you can rely on Java 8's new java.time.* APIs for more fluent chronology-related calculations.

For starters, your manually constructed Map can be replaced with a simple look-up on the DayOfWeek enum:

// scanner will be a wrapper over System.in
private static DayOfWeek getDayOfWeek(Scanner scanner) {
    String values = Arrays.toString(DayOfWeek.values());
    System.out.printf("Enter a day of week:%n%s%n", values);
    while (true) {
        try {
            return DayOfWeek.valueOf(scanner.nextLine().trim().toUpperCase());
        } catch (IllegalArgumentException e) {
            System.err.printf("Please try again with one of these:%n%s%n",
                    values);
        }
    }
}

The looping-validation ensures that only a valid DayOfWeek value is returned.

Next, you can use a couple of TemporalAdjusters to get the LocalDates you require:

firstInMonth(DayOfWeek): the first day-of-week of the month.
lastDayOfMonth(): the last day of the month.
next(DayOfWeek): the next day-of-week, i.e. 7 days from the LocalDate instance.

Then, with a helpful serving of DateTimeFormatter, the main processing logic can just be:

try (Scanner scanner = new Scanner(System.in)) {
    DayOfWeek dayOfWeek = getDayOfWeek(scanner);
    // getYear(Scanner) returns an int between the year range, MONTH = 2
    LocalDate first = LocalDate.of(getYear(scanner), MONTH, 1)
                                .with(TemporalAdjusters.firstInMonth(dayOfWeek));
    LocalDate last = first.with(TemporalAdjusters.lastDayOfMonth());
    DateTimeFormatter formatter = DateTimeFormatter.ofLocalizedDate(FormatStyle.MEDIUM)
                                                    .withLocale(Locale.GERMANY);
    for (LocalDate date = first; !date.isAfter(last);
            date = date.with(TemporalAdjusters.next(dayOfWeek))) {
        System.out.println(formatter.format(date));
    }
}

Other observations

It's not recommended to throw a generic Exception as it's... too vague. You can consider using IllegalArgumentException for invalid year inputs outside \$[1500, 2005]\$, as the exception type is then more defined.
More importantly, you shouldn't be throwing your own Exceptions and then catching it purely as a form of flow control.
HashMap<String, Integer> daysOfWeek = new HashMap<>() can be better written as Map<String, Integer> daysOfWeek = new HashMap<>(), to program to an interface rather than the implementation.
As illustrated above, you should also consider using try-with-resources on the Scanner instance for safe and efficient handling of the underlying I/O resource.


Plot daily time spent on codereview.stackexchange.com This draws an xyplot from csv data obtained from RescueTime API for activity time for "http://codereview.stackexchange.com".

# -------------------------------------------------------
# PLOT TIME SPENT ON CODEREVIEW SE
# USING RESCUE TIME API AND R
# -------------------------------------------------------

# -------------------------------------------------------
# LIBRARIES
if(!require("RCurl")){
  install.packages("RCurl")
}
if(!require("lattice")){
  install.packages("lattice")
}
require("RCurl")
require("lattice")
# -------------------------------------------------------

print("Initializing...")

# -------------------------------------------------------
# RESCUE TIME API ACCESS

# api key
# get from https://www.rescuetime.com/anapi/manage
api_key <- "..."

# query from date
from_date <- "2014-01-01"

# query to date
to_date <- format(Sys.Date(), "%Y-%m-%d")

# activity to get details of
activity <- "codereview.stackexchange.com"

# --api description--
# for more details see https://www.rescuetime.com/anapi/setup/documentation
# key - API key
# format - Output format
# rb - Restrict Start Date ("%Y-%m-%d")
# re - Restrict End Date ("%Y-%m-%d")
# rk - Restrict Kind (in this case it's activity)
# pv - Perspective (in this case it's interval)
# rs - Resolution Time (if it is day it's a daily report)
# rt - Restrict Thingy (Restrict Kind of Type)
url_format  <- "https://www.rescuetime.com/anapi/data?key=%s&format=csv&rb=%s&re=%s&rk=activity&pv=interval&rs=day&rt=%s"

# create url
api_url  <- sprintf(url_format,api_key,from_date,to_date,activity)
# -------------------------------------------------------

# -------------------------------------------------------
# PLOT

# get csv as a text
csv_text_data <- textConnection(getURL(api_url))

# parse csv
activity_data <- read.csv(csv_text_data,header = TRUE)

print(sprintf("Mean Time Spent on '%s':%f (seconds)",activity,mean(activity_data$Time)))

activity_time_spent <- activity_data$Time
activity_dates <- activity_data$Date

# create an index for dates
activity_date_index  <- 1:length(activity_dates)

# plot
print(xyplot(activity_time_spent ~ activity_date_index, type="b", xlab = "Date Index", ylab = "Time (seconds)" ))
# -------------------------------------------------------

# -------------------------------------------------------
# CLEAN UP
detach("package:RCurl", unload = TRUE)
detach("package:lattice", unload = TRUE)
rm(list = ls())
# -------------------------------------------------------

Does this follow R conventions?

Execute by loading it to R console using source("./codereview.R")

Download

CSV and R file

Output

Loading required package: RCurl
Loading required package: bitops
Loading required package: lattice
[1] "Initializing..."
[1] "Mean Time Spent on 'codereview.stackexchange.com':2764.559322 (seconds)"

Here is how I would rewrite your code. I will add my comments after:

plotActivity <- function(
   activity  = 'codereview.stackexchange.com',
   api_key   = getOption('rescuetime_api_key'),
   from_date = NULL,
   to_date   = NULL) {

## This function plots time spent on a given URL
## using the rescue time API
##
## Arguments:
##    - activity:  the url you want to check for usage
##    - api_key:   user API key, you can get yours at
##                 https://www.rescuetime.com/anapi/manage
##    - from_date: a Date or string in "YYYY-MM-DD" format,
##                 defaults to January 1st of the same year as to_date
##    - to_date:   a Date or string in "YYYY-MM-DD" format,
##                 defaults to today
##    - verbose:   adds verbose, defaults to FALSE

   library("RCurl")
   library("ggplot2")

   # arguments parsing and default settings
   if (is.null(api_key)) stop("please set your rescue time API key...")

   if (is.null(to_date)) to_date <- Sys.Date()
   to_date <- as.Date(to_date)
   if (is.na(to_date)) stop("error parsing to_date...")

   if (is.null(from_date)) from_date <- format(to_date, "%Y-01-01")
   from_date <- as.Date(from_date)
   if (is.na(from_date)) stop("error parsing from_date...")

   # build the API request url
   # for more details see https://www.rescuetime.com/anapi/setup/documentation

   base_url  <- "https://www.rescuetime.com/anapi/data"
   arguments <- c(key = api_key,   # API key
                  format = 'csv',  # Output format
                  rb = from_date,  # Restrict Start Date
                  re = end_date,   # Restrict End Date
                  rk = 'activity', # Restrict Kind
                  pv = 'interval', # Perspective
                  rs = 'day',      # Resolution Time
                  rt = activity)   # Restrict Thingy (Restrict Kind of Type)

   arg_str <- paste(names(arguments), arguments, sep = '=', collapse = '&')
   api_url <- sprintf("%s?%s", base_url, arg_str)

   # read data
   csv_text_data <- textConnection(getURL(api_url))
   activity_data <- read.csv(csv_text_data, header = TRUE)

   # print diagnostic and plot
   cat(sprintf("Total Time Spent between %s and %s on '%s':%f (seconds)",
               activity, from_date, end_date, sum(activity_data$Time)))

   activity_data$Date <- as.Date(activity_data$Date)
   ggplot(activity_data, aes(x=Date, y=Time.Spent..seconds.)) +
      geom_bar(stat="identity") +
      xlab("Date") +
      ylab("Time (seconds)")
}

So here are a few ideas:

I went from a script to a function. This way it is a lot easier to use and share. I have abstracted what I believe were all the right inputs and chosen sensible defaults. Of interest is the use of getOption for the api_key so the user can set it once for all by running options(rescuetime_api_key = "xyz123abc"). So now, all you have to do is source the file where this function will be stored, then call the function. A function, when executed, runs in its own environment so all the variables that are created at runtime are deleted as you exit the function. Your global environment is never polluted, which removes the need for your "clean up" section (which was otherwise pretty harmful as janos pointed out.)
Like rm(list=ls()), forcing the install of packages via install.packages() is a bit harmful. No, you just want to use library and it will die right there, leaving the user with the decision of installing the package. There is a nice blog about why some people -me included- prefer library over require: http://yihui.name/en/2014/07/library-vs-require
I made use of R's Date object wherever it made sense.
I rewrote the way the api url is built, making use of a named vector for the arguments. With inline comments. I hope you'll agree the code is a bit more readable and easier to maintain, something you should always aim for.
I replaced print with cat. This way you don't get the [1] prefix that comes with printing a vector.
This has less to do with code reviewing but I thought that it would be more useful to see the total time spent on the website. Average time spent per day can be a little confusing: the user might be wondering if it is including all days or only those when the website was visited (you chose the latter.) While total time removes that ambiguity. Also, I thought it would be more useful to plot the data as a barplot with a real timescale, i.e. including holes for periods when the website was not visited. I used ggplot2 for that, see the picture below. The x-axis might look weird but you did spend six seconds on the website on July 1st so that's why it starts on that day, although you won't really notice there is a data point there.

My rewrite got rid of activity_date_index <- 1:length(activity_dates) but had I needed it, I would have used activity_date_index <- seq_along(activity_dates). Functions like seq_len or seq_along are better practice than 1:n because they properly handle the special case when n is zero. They return an empty vector (c()) while 1:n will return the vector c(1, 0) and create havoc.

Disclaimer: I don't have an account to that rescuetime website so I was not able to fully test my code. If you find a mistake, please feel free to edit my post, I won't mind.

Thanks for sharing your code, I like R and particularly enjoy seeing people use it to connect and analyze online databases like you did here. I hope my comments will help you.


PHP script to connect to MySQL database using PDO Introduction

I am preparing to use a basic script to cover how you can connect to a MySQL database using PDO. This script is meant for educational purposes (introductory class on PHP/MySQL) and does of course not cover all security aspects of database connections.

Points of focus

Conformity to the PSR-1 and PSR-2 standards.
Structure for connecting to database connection.
Security aspects that has not been covered.

Code (settings.php):

<?php
  // Defines database connection information
  $settings = [
    'host'     => '127.0.0.1',
    'name'     => 'c9',
    'port'     => '3306',
    'charset'  => 'utf8',
    'username' => 'admin',
    'password' => 'root'
  ];
?>

Code (db.php):

<?php
  // Includes database connection information
  require_once('../settings.php');

  // Establishes connection to database server
  try {
    $dbh = new PDO(
            sprintf(
              'mysql:host=%s;dbname=%s;port=%s;charset=%s',
              $settings['host'],
              $settings['name'],
              $settings['port'],
              $settings['charset']
            ),
            $settings['username'],
            $settings['password']
    );
    // Prevents emulated prepare statements and sets error mode
    // to PDO::ERRMODE_EXCEPTION
    $dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
  }
  // Prints out errors raised by PDO
  catch (PDOException $e) {
    die('Error: ' . $e->getMessage());
  }
?>

The ../ for settings.php indicates that the file is outside document root for security purposes. Much appreciated in advance for any comments that could improve this code.

I don't use PHP often, but here are a few comments:

Use Objects to Manage Resources

Like any other language with object-oriented capabilities, PHP serves you best when you  manage resources with objects. I'd strongly recommend holding your PDO object within a custom class that manages the connection. This makes the code easier to maintain and doesn't actually have to change much about what you did. The code will also be more extensible. Also, introducing OOP is a very good lesson for students starting out.***

You could then also show them lazy-loading, making sure the PDO isn't opened until there is actually communicating between the server and the database.

Password length

Of course you'd want to point out to your students that you'd need a decent password and not the one you're using for illustrative purposes here. For example, a password that was not a real world and more than 4 letters in length. This is more of a teaching point to make sure everyone has some common sense!

Error handling

Perhaps show your students what you would really do with the error message. For example, how could they wait and call the connection again? It could be helpful to show students they can do something in response to an error message apart from printing it out (like trying again, or waiting and trying again, or redirecting to a different page, or sending a warning email to the sys admin, etc).

***I first learned to make a dynamic website with Robin Nixon's much villified book. When I first learned from it, I didn't understand why people poo-poo the book so much. The reason is that it gets folks off the on the wrong foot. I resisted object-oriented design for websites for a long time because I started with that book and wanted to keep things simple, but of course nothing stays simple for long. If I ever teach coding to beginners, I will start them with objects on day 1!


Remove element slice of struct pointers I have an application with a recent addition that requires the removal of a struct pointer from a slice that is itself an element of another struct. A summary of the usage.

I am wondering if this is idiomatic and okay usage of the go languages. Things appear to work and this appears to be inline with the golang recommendations for slice element deletion.

package main

import (
    "fmt"
    "strconv"
)

const ARBITRARY= 5

type myData struct {
    det detailSlicePtr
}

type detail struct {
    id   int
    name string
}

// Slice of pointers to details
type detailSlicePtr []*detail

// Returns true if item removed succesfully,
// false all other cases
func(s *myData ) remove(d *detail) bool {
    for i:= range s.det {
        if s.det[i].name == d.name && s.det[i].id == d.id {
            s.det[i] = new(detail)
            s.det = append(s.det[:i],s.det[i+1:]...)
            return true
        }
    }

    return false
}

// Find and remove a struct element from a slice of struct pointers within a     struct
func main() {

    var d myData
    details := make(detailSlicePtr , ARBITRARY)

    for j := 0; j < ARBITRARY; j++ {
        details [j] = &detail{
            id:   j,
            name: strconv.Itoa(j),
        }
    }

    d.det = details

    // Print out what we have so far
    for index := range d.det {
        fmt.Printf("Item %d is %s\n", index, d.det[index].name)
    }

    findMe := ARBITRARY / 2
    middle := &detail{id: findMe ,name:strconv.Itoa(findMe)}

    if ok := d.remove(middle); !ok {

        fmt.Printf("Unable to find element: %v\n", middle)

    } else {

        fmt.Printf("Found and removed element: %v\n", middle)

        // Print what is left
         for index := range d.det {
            fmt.Printf("Item %d is %s\n", index, d.det[index].name)
        }
    }
}

It is not idiomatic and it is not okay usage. It should be:

  SliceTricks

  Delete (pointers)

copy(a[i:], a[i+1:])
a[len(a)-1] = nil // or the zero value of T
a = a[:len(a)-1]

For example,

package main

import (
    "fmt"
    "strconv"
)

const ARBITRARY = 5

type myData struct {
    det detailSlicePtr
}

type detail struct {
    id   int
    name string
}

// Slice of pointers to details
type detailSlicePtr []*detail

// Returns true if item removed succesfully,
// false all other cases
func (s *myData) remove(d *detail) bool {
    for i := range s.det {
        if s.det[i].name == d.name && s.det[i].id == d.id {
            copy(s.det[i:], s.det[i+1:])
            s.det[len(s.det)-1] = nil
            s.det = s.det[:len(s.det)-1]
            return true
        }
    }
    return false
}

// Find and remove a struct element from a slice of struct pointers within a     struct
func main() {

    var d myData
    details := make(detailSlicePtr, ARBITRARY)

    for j := 0; j < ARBITRARY; j++ {
        details[j] = &detail{
            id:   j,
            name: strconv.Itoa(j),
        }
    }

    d.det = details

    // Print out what we have so far
    for index := range d.det {
        fmt.Printf("Item %d is %s\n", index, d.det[index].name)
    }
    // Print what is hidden
    for i := range d.det[len(d.det):cap(d.det)] {
        j := len(d.det) + i
        det := d.det[:cap(d.det)]
        fmt.Printf("Hidden item %d is %v\n", j, det[j])
    }

    findMe := ARBITRARY / 2
    middle := &detail{id: findMe, name: strconv.Itoa(findMe)}

    if ok := d.remove(middle); !ok {

        fmt.Printf("Unable to find element: %v\n", middle)

    } else {

        fmt.Printf("Found and removed element: %v\n", middle)

        // Print what is left
        for index := range d.det {
            fmt.Printf("Item %d is %s\n", index, d.det[index].name)
        }
        // Print what is hidden
        for i := range d.det[len(d.det):cap(d.det)] {
            j := len(d.det) + i
            det := d.det[:cap(d.det)]
            fmt.Printf("Hidden item %d is %v\n", j, det[j])
        }
    }
}

Output:

Item 0 is 0
Item 1 is 1
Item 2 is 2
Item 3 is 3
Item 4 is 4
Found and removed element: &{2 2}
Item 0 is 0
Item 1 is 1
Item 2 is 3
Item 3 is 4
Hidden item 4 is <nil>

NOTE: There is a bug in the Slice Tricks that you linked to. It will be corrected soon. Until then use the link in my answer.


The Great Julya Calendar Challenge

  An integer is provided as an input. In each step, we have to subtract
  a number from the input which already exists as a digit of the input.
  In this way, we have to make the input equal to zero(0). The problem
  is to output the minimum number of such steps.

Code

n=int(input())
count=0

while n:
    l=list(int(i) for i in str(n))
    n=n-max(l)
    count+=1
    if n == 0:
        break

print (count)

The code outputs the correct results; but on larger input it exceeds the time limits.
That is, for every input memory consumption increases and the time limit is reached up until the 30th test case.

You don't need to actually build a whole list of the digits. It's unnecessary memory consumption. You can pass a generator to max.
use python's -= syntax when subtracting from a variable.
The break is unnecessary as it's covered by the while condition

Final code:

n=int(input())
count=0

while n:
    n -= max(int(i) for i in str(n))
    count += 1

print (count)

This code would be fairly difficult to improve much further for efficiency. You could really penny pinch performance on the generator by building a generator that will stop iteration after it encounters a 9, as it knows it cannot ever get a number bigger than that. You could also use modulo and integer division logic to extract digits to avoid having to go from int -> str -> int.

Okay. This is now straying pretty far from the original question, but I liked the challenge and @Mast in the comments expired me by mentioning memoization.

To utilize memoization the problem needed to be formulated in a recursive way where I can store the result of intermediate calls to avoid re-computation of things already computed. The strategy was to define the recursive function "compute_count(previous_max, rest)", where previous_max is initially called with 0, and rest as the entire number.

The idea is that a max, num pair uniquely identifies a solution, so every time we end up with the same max, num pair we can use the precomputed result. Formulating the problem in this way led to writing a fairly complicated function, but on the example you provided in the comments it returns instantly. The memoization table ends up with 293 entries, so it's well within your 256MB limit.

Anyway, here's the final code:

memoization_table = {}

def compute_count(previous_max, rest):
    global memoization_table
    original_rest = rest

    if (previous_max, rest) in memoization_table:
        return memoization_table[(previous_max, rest)]

    num_digits = len(str(rest))

    if num_digits == 1:
        memoization_table[(previous_max, original_rest)] = 1, min(0, rest-previous_max)
        return 1, min(0, rest-previous_max)

    sum_count = 0
    while rest > 0:
        s = str(rest).zfill(num_digits)
        new_max = max(previous_max, int(s[0]))
        new_rest = int(s[1:])
        count, leftover = compute_count(new_max, new_rest)

        sum_count += count
        rest -= new_rest
        rest += leftover

    memoization_table[(previous_max, original_rest)] = sum_count, rest
    return sum_count, rest

print(compute_count(0, 1278564645)[0])
print(len(memoization_table))

I imported the length of the memoization table just to see the memory implications of the solution, and it turned out to be very reasonable. Obviously remove this from any submitted solutions.

Here's the output of the run:

$ python compute_count.py
154026551
293

Example:

For calculating 24, these are the function calls made:

compute_count(0,24)
compute_count(2,4)
compute_count(2,0)
compute_count(1,8)
compute_count(1,0)
compute_count(0,9)

To calculate compute_count(0, 24):

get new previous_max, which is the max of 0 and 2.
do a compute_count on 2,4. Since this is a single digit number, it's our base case, and we return that we can get to 0 with a single move (as is the case with all single digit numbers).
Since we did a compute_count(2,4) which told us it did a single computation, we need to subtract the number computed for (4) from our running number (24). This gives us a remainder of 20, and a running count of 1.
The new previous_max is still 2, as our remainder is 20.
do a compute_count on 2,0. This one is a special case. Getting from 0 to 0 should technically take no moves, but we have additional logic for these cases because the previous max is 2. Which means this isn't really a single digit number, and I need to return the "leftover" which is -2 so the caller can account for the underflow of the subtraction. So this function returns a count of 1, and a leftover of -2
Since we passed 0 to compute_count, we subtract 0 from our 20 which keeps us at 20. But because a leftover of -2 was returned, we also have to add that to our 20 which gives us a total of 18 for this iteration.
The above process continues in kind until our remaining is 0 or less and we've summed all the counts.