iTunes playlist copier When you own an Android device and want your iTunes playlist to be copied over onto it, what do you do?

What if your playlist hits something in the region of 20GB+ ? Well, I am sure I could've paid for a tool, to do this for me and keep things in sync, but I wanted to challenge myself into writing something in Python to resolve it.

I tried dragging and dropping the files on the playlist onto the phone via Android Device Manager and often found it would just hang. This is what I ended up with:

from xml.etree.ElementTree import ElementTree
import os
from shutil import copyfile
import urllib

srcfile = "/path/to/playlist.xml"
dst = '/path/to/destination';
overwrite = 0

tree = ElementTree()
tree.parse(srcfile)
songs = tree.findall('/dict/dict/dict/string')

# Run through all the found elements
for song in songs:
    # Is this element a file?
    if song.text[0:7] == "file://":

        # Grab everything after file://
        myfile = urllib.unquote(song.text[7:])

        # Grab the filename without the path
        basefile = os.path.basename(myfile)

        # Grab the last dirname
        dirname = myfile.split('/')[-2]

        # If the dir does not exist on the destination, create it
        if not os.path.exists(dst+dirname):
            os.mkdir(dst+dirname)

        # Create the final filename
        dstFile = dst + dirname + '/' + basefile

        # If the file exists and we have chosen to not overwrite it, dont copy the file
        if os.path.exists(dstFile) and not overwrite:
            print "Skipping " + myfile
        else:
            copyfile(myfile, dst + dirname + '/' + basefile)
            print "Copying " + myfile

print "Finished...."

I have found a bug in it when copying over bands or songs with UTF-8 names in it e.g. Fantômas. Because the UTF-8 elements are encoded in the Playlist XML the names do not match the directory so I need to fix that. Have kept a copy on Gist, so if you want a copy go grab/comment/whatever. I'm new to Python so any feedback is welcome on better ways to do things.

overwrite would be better as True or False.

Since each file is treated independently, you should define a function to make the independence obvious.

To split and join path components, use os.path.split() and os.path.join() instead of doing your own string analysis.  The interface is slightly nicer, and it also ensures that there won't be missing or extra / separators.

If you reorder the conditionals, you could reduce the number of os.path.exists() tests in the don't-overwrite case, where the destination file already exists.

Print 'Copying filename' before copying the file, so that if the copying fails, you'll see which file it failed on.  Print "Finished." instead of "Finished....", because ellipses imply that the job is incomplete.

import os
from shutil import copyfile
import urllib
from xml.etree.ElementTree import ElementTree

def copy_song(url, dest, overwrite=False):
    if not url.startswith('file://'):
        return

    orig_path = urllib.unquote(url[len('file://'):])
    orig_dirname, basename = os.path.split(orig_path)
    dest_dirname = os.path.join(dest, orig_dirname)
    dest_path = os.path.join(dest_dirname, basename)

    if not overwrite and os.path.exists(dest_path):
        print 'Skipping ' + orig_path
    else:
        if not os.path.exists(dest_dirname):
            os.mkdir(dest_dirname)
        print 'Copying ' + orig_path
        copyfile(orig_path, dest_path)

playlist = "/path/to/playlist.xml"
dest = '/path/to/destination';

playlist_xml = ElementTree.parse(playlist)
for song in playlist_xml.findall('/dict/dict/dict/string'):
    copy_song(song.text, dest)
print 'Finished.'


Project Euler #2 in Java Project Euler #2:

Each new term in the Fibonacci sequence is generated by adding the previous two terms. By starting with 1 and 2, the first 10 terms will be:
1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...
By considering the terms in the Fibonacci sequence whose values do not exceed four million, find the sum of the even-valued terms.

Here is my solution:
public class EvenFibonacciFinder {

    private static final double PHI = 1.618033989;
    private static final double _PHI = -(PHI - 1);
    private static final double SQRT_5 = Math.sqrt(5);
    private static final int MAX_NUM = 4_000_000;

    public static void main(String[] args) {
        long time = System.nanoTime();
        double max = MAX_NUM * SQRT_5;
        long sum = 0;
        for (int i = 3, result = (int) Math.round(2 * SQRT_5); result < max; result = (int) Math.round(Math.pow(PHI, i += 3) - Math.pow(_PHI, i))) {
            sum += result;
        }
        sum = Math.round(sum / SQRT_5);
        System.out.println("Result: " + sum + "\nTime used for calculation in nanoseconds: " + (System.nanoTime() - time));
    }

}

Output:

 Result: 4613732
 Time used for calculation in nanoseconds: 60243

Questions:

Is this the most efficient way of doing it?
Is there any "bad code" in the short solution?

Is this the most efficient way of doing it?

Sometimes the simplest solution is also the most efficient solution.  I'm not sure why you decided to use Math.pow and PHI to compute something that can be easily (and more quickly) be computed with simple addition:

public class EvenFibonacciFinder {
    private static final int MAX_NUM = 4_000_000;

    public static void main(String[] args)
    {
        long time = System.nanoTime();
        int  f1   = 1;
        int  f2   = 2;
        long sum  = 0;

        while (f2 <= MAX_NUM) {
            int f3;
            sum += f2;
            // This skips three ahead in the sequence.
            f3 = f1 + f2;
            f1 = f2 + f3;
            f2 = f1 + f3;
        }
        long end = System.nanoTime();
        System.out.println("Result: " + sum +
                "\nTime used for calculation in nanoseconds: " +
                (end - time));
    }
}

Output:

  Original code using pow:
  Result: 4613732
  Time used for calculation in nanoseconds: 9952

  Using addition instead of pow:
  Result: 4613732
  Time used for calculation in nanoseconds: 934

(Note: both programs were modified to measure the end time before the println(), otherwise some of the string concatentation time was being counted).


Report time on whole hours Report time at whole hours

#!/usr/bin/python3
import time
from datetime import datetime
from pydub import AudioSegment
from pydub.playback import play

bell_sound = AudioSegment.from_wav("/home/me/Music/audio/bell.wav")

while True:
    now = datetime.now()
    # repeats = 1 if now.hour % 3 == 0 else now.hour % 3
    repeats = now.hour % 3 + 1

    if now.hour < 6:
        break
    # report on whole hour
    if now.minute == 0:
        for _ in range(repeats):  # bell the hours
            play(bell_sound)
        time.sleep(60 * 58)  # save the computation
    if now.minute != 0:
        time.sleep(10)
    # quit 23 later
    if now.hour == 23:
        break

Any ideas to improve it?

while True:
    now = datetime.now()
    # repeats = 1 if now.hour % 3 == 0 else now.hour % 3
    repeats = now.hour % 3 + 1

    if now.hour < 6:
        break

The repeats is not used in the condition of the break. Therefore you should reorder your code like this, to avoid computing things you don't need later:

    now = datetime.now()
    if now.hour < 6:
        break

    repeats = now.hour % 3 + 1

I was confused by the last line of code. I thought it would mean to ring the bell 3 times at 2 o'clock. What confused me was that now.hour does not correspond to my intuitive understanding of now anymore, since by the time the bell will ring, some time has passed.

To avoid this confusion, you should define a variable next_hour = now.hour + 1. When you then define repeats = (next_hour - 1) % 3 + 1, that may look even more confusing because of the - 1 at the beginning and the + 1 at the end. That's probably why you considered repeats = 3 if next_hour % 3 == 0 else next_hour % 3, and it's good that you did that.

Continuing with your code:

    # report on whole hour
    if now.minute == 0:
        for _ in range(repeats):  # bell the hours
            play(bell_sound)
        time.sleep(60 * 58)  # save the computation

I hope that each bell takes only a few seconds. As soon as they become longer, you might miss the next hour completely. The "save the computation" part feels dangerous to me. This is because on practically all operating systems you can put a process to sleep and later resume it. I don't know how exactly this affects the clock and the measurement of datetime.now(), but that's definitely something you should experiment with.

It's better to define a high-level overview first:

def ring_until_midnight():
    current_hour = datetime.now().hour
    if current_hour < 6:
        break

    while current_hour < 23:
        wait_until_hour(current_hour + 1)
        ring_the_bell(current_hour + 1)
        current_hour += 1

This raises the question what to do if the hour steps forward fast, such as when the daylight saving time changes. Since you probably don't want the clock to ring 5 times in a row, it's better to replace the last line above with:

        current_hour = datetime.now().hour  # instead of current_hour + 1

Next, it's time to fill the missing details, which are wait_until_hour and ring_the_bell. For wait_until I propose this simple solution:

def wait_until_hour(hour):
    while datetime.now().hour < hour:
        time.sleep(1)

Note that I intentionally wrote < hour instead of != hour. Just in case the process is stopped for 3 hours. If I had written != hour, the code would continue to wait until the next day. Again, it depends on your exact requirements whether < or != is appropriate in this situation. In any case, you have to make this choice consciously.

Having a possible clock offset of 1 second (from the above sleep(1)) sounds acceptable to me. If you need more precision, you can say time.sleep(0.1) instead as well.

The second detail, ring_the_bell, is similarly simple:

def ring_the_bell(hour):
    repeats = (hour - 1) % 3 + 1

    for _ in range(repeats):
        play(bell_sound)

Now that I think of it, in the definition of ring_until_midnight above, I made an unfortunate choice:

        ring_the_bell(current_hour + 1)

This should rather be:

        ring_the_bell(datetime.now().hour)

The point is that during all this waiting, there is much that can happen, and there is no guarantee that waiting for "the next hour" will finally arrive at exactly that hour. Therefore better ask for the then-current time again.

When you build your programs out of composable pieces like these, you will find that you can reuse some parts by just copying them to your next project, which is good because it saves you the work of writing the same code over and over again, plus you will build up your library of small, hopefully tested code that you know you can trust.

As you saw in the process I outlined above, even writing a simple ring-the-bell program can be made quite complicated. It needs lots of decisions that you might not think about when you first decide to write such a program. The complexity arises mainly because this program is dealing with time and measurement and calendars, which always gets complicated. That's the way it is.

You also saw that it is of utmost importance to get the exact wording right in every line of the code. Is saying "the current hour" really the best way to express your requirement? What does "current" mean, when was it measured, for how long is that measurement valid, what happens in between? Lots of questions, and you sometimes have to revise your initial program. I'm quite experienced, and I didn't get it right either on the first attempt.


Creating an array that loops out on the frontend - Don't Repeat Yourself? Entire Code Here

Don't repeat yourself

I know of the "Don't repeat yourself". Still it's getting messy in some places. The code below contains three blocks of code.

They are similar in some ways and different in others. It doesn't follow the same pattern in all ways.

What it does

It creates an array that I loop out on the frontend.

Question

Is there any better way to do this kind of stuff? Less messy, better structure?

$message_start = 'h2 tags - ';
$message_end = '' . $this->count_h2 . '';
if( $this->count_h2 == 0 ) {
    $message = 'No tags found. Add some!';
    $array['content_editor']['count_h2']['status'] = 2;
} elseif( $this->count_h2 == 1 ) {
    $message = 'Some found. Too few!';
    $array['content_editor']['count_h2']['status'] = 1;
} else {
    $message = 'Many found. Great!';
    $array['content_editor']['count_h2']['status'] = 0;
}
$array['content_editor']['count_h2']['message'] = $message_start . $message . $message_end;
$array['content_editor']['count_h2']['count'] = $this->count_h2;

$message_start = 'h3-h6 tags - ';
$h2_h6 = $this->count_h3 + $this->count_h4 + $this->count_h5 + $this->count_h6;
$counter = ( $h2_h6 == 0 ) ? '' : $h2_h6;
$message_end = '' . $counter . '';
if( $h2_h6 == 0 ) {
    $message = 'No found. Add some!';
    $array['content_editor']['count_h3_h6']['status'] = 1;
} else {
    $message = 'Found, great!';
    $array['content_editor']['count_h3_h6']['status'] = 0;
}
$array['content_editor']['count_h3_h6']['message'] = $message_start . $message . $message_end;
$array['content_editor']['count_h3_h6']['count'] = $this->h2_h6;

$message_start = 'Title keywords - ';
$counter = ( $this->found_keywords1_post_title == 0 ) ? '' : $this->found_keywords1_post_title;
$message_end = '' . $counter . '';
if( count( $this->keywords1 ) == 0 ) {
    $message = 'No primary added.';
    $array['content_editor']['missing_keywords1_post_title']['status'] = 2;
} elseif( $this->found_keywords1_post_title == 0 ) {
    $message = 'No primary found.';
    $array['content_editor']['missing_keywords1_post_title']['status'] = 2;
} else {
    $s = ( $this->found_keywords1_post_title != 1 ) ? 's' : '';
    $message = 'Primary found.';
    $array['content_editor']['missing_keywords1_post_title']['status'] = 0;
}
$array['content_editor']['missing_keywords1_post_title']['message'] = $message_start . $message . $message_end;
$array['content_editor']['missing_keywords1_post_title']['count'] = $this->found_keywords1_post_title;

Architecturally, I'm adverse to hard-coding HTML tags within PHP code. If, for example, you wanted to migrate the code base from PHP to another language (Java, Python, Ruby, Go, Haxe,  whatever) then you have to rewrite both the logic (PHP) and the presentation (PHP) code.

Consider decoupling the logic from the presentation so that you can change how the data appears without having to change how the data is retrieved. This simplifies switching implementation languages and opens the possibility to create different views of the same data for different devices.

To do this, you have to create an interface or contract between the logic and the system that renders the logic for the user. Fortunately, this is where XML excels.

Instead of hard-coding <strong>, you would create an XML document that represents the information you want to display. The XML document becomes the contract between the logic and presentation, which allows both to vary independently. Once the XML document is in place you can pass it to an XSLT Processor to generate the desired HTML content.

This offers the following advantages:

Decoupled. The presentation logic moves to XSLT, completely decoupling it from the PHP code.
Migration. Migrating from PHP to another language means the presentation logic need not change.
Devices. It becomes trivial to change the presentation for different devices (use a new template).
DRY. With display logic in a central location, duplicated code is easier to eliminate.
Databases. Many databases now allow constructing XML documents within stored procedures, eliminating complex code readily expressed in SQL.
SaaS. It simplifies creating a SaaS API -- transforming XML to JSON is fairly easy a solved problem.

This has the following disadvantages:

Effort. Migrating to an XML/XSLT solution for presentation takes a lot of work.
Knowledge. Introduces a second programming language (XSLT) into the software.
Developers. Far fewer developers know XSLT than PHP.
XSLT 1.0. Using PHP, the processor is limited to XSLT v1.0 (many great features are in XSLT 2.0).
Bugs. The XSLT Processor has some pesky known bugs, which could be avoided by moving to XSLT 2.0 with the SAXON processor.


Avoiding repetition when using the "snprintf idiom" to write text I'm writing a function to stringify a nested struct. I'm using snprintf to pre-calculate how much space I'll need to allocate.

The problem is, it's extremely repetitious, and will only get worse if I ever need to add more fields to any of the structs. The duplication also creates more of an opportunity for accidental asymmetrical changes.

Is there an idiomatic way of reducing the duplication?

The repetitious code:

char* stringify_state(State* state) {
    Pet* p = state->pet;
    Settings* s = state->settings;

    char* format_string = "%lld %f %f %f %f %f %f %f %f %f %f";

    size_t len = snprintf(NULL, 0, format_string,
                          state->last_update_time,
                          p->health, p->max_health, p->satiation, p->max_satiation,
                          s->milliseconds_per_tick, s->hunger_pain_per_tick, s->hunger_per_tick, s->satiated_heal_per_tick,
                          s->pain_per_wrong_answer, s->satiation_per_right_answer);

    char* buff = malloc(len + 1);

    if (buff) {
        snprintf(buff, len + 1, format_string,
                 state->last_update_time,
                 p->health, p->max_health, p->satiation, p->max_satiation,
                 s->milliseconds_per_tick, s->hunger_pain_per_tick, s->hunger_per_tick, s->satiated_heal_per_tick,
                 s->pain_per_wrong_answer, s->satiation_per_right_answer);

        return buff;

    } else {
        return NULL;
    }
}

Context (Not what I'm asking about, but feel free to mention anything):

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

typedef struct {
    double health;
    double satiation;

    double max_health;
    double max_satiation;

} Pet;

void init_pet(Pet* pet, double max_health, double max_satiation) {
    pet->max_health = max_health;
    pet->max_satiation = max_satiation;

    pet->health = max_health;
    pet->satiation = max_satiation;

typedef struct {
    double milliseconds_per_tick;

    double hunger_pain_per_tick;
    double hunger_per_tick;

    double satiated_heal_per_tick;

    double pain_per_wrong_answer;
    double satiation_per_right_answer;

} Settings;

void init_settings(Settings* s,
                   double ms_per_tick,
                   double hung_pain_per_tick, double hung_per_tick,
                   double sat_heal_per_tick,
                   double pain_per_wrong, double sat_per_right) {

    s->milliseconds_per_tick = ms_per_tick;

    s->hunger_pain_per_tick = hung_pain_per_tick;
    s->hunger_per_tick = hung_per_tick;

    s->satiated_heal_per_tick = sat_heal_per_tick;

    s->pain_per_wrong_answer = pain_per_wrong;
    s->satiation_per_right_answer = sat_per_right;
}

typedef struct {
    time_t last_update_time;
    Pet* pet;
    Settings* settings;

} State;

void init_state(State* state, Pet* pet, Settings* settings, time_t t) {
    state->pet = pet;
    state->settings = settings;

    state->last_update_time = t;
}

A test main:

int main() {
    Pet pet;
    init_pet(&pet, 100, 100);

    Settings settings;
    init_settings(&settings, 91, 82, 73, 64, 55, 46);

    State state;
    init_state(&state, &pet, &settings, 0);

    char* str = stringify_state(&state);

    if (str) {
        printf("%s", str);

        free(str);
    }
}

Prints

0 100.000000 100.000000 100.000000 100.000000 91.000000 82.000000 73.000000 64.000000 55.000000 46.000000

@vnp's code is solid and helpful, but his stringify_state_helper is a single-purpose function, and still leaves a degree of repetition and memory management in stringify_state. I'd rather have general-purpose to_string that takes printf-style arguments, allocates sufficient space for the converted result, and prints into that space, and returns the result:

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

char *to_string(char const *fmt, ...) {
    va_list args;
    va_start(args, fmt);

    va_list args_dupe;
    va_copy(args_dupe, args);

    int size = vsnprintf(NULL, 0, fmt, args);

    char *ret = malloc(size+1);

    if (ret != NULL)
        vsnprintf(ret, size+1, fmt, args_dupe);

    va_end(args);
    va_end(args_dupe);

    return ret;
}

In fairness, this does require a little more code, and the v*printf functions (and argument-list macros) are a bit less known, so some may find it a bit more difficult to understand.

On the other hand, in exchange for that bit of extra investment, we get something that's more general, and works much more as I think most people would expect--for example, something like this:

char *s = to_string("%d, %d", 1, 2);

Using this, stringify_state should turn out something like this:

char* stringify_state(State* state) {
    Pet* p = state->pet;
    Settings* s = state->settings;

    return to_string("%lld %f %f %f %f %f %f %f %f %f %f",
                     state->last_update_time,
                     p->health,
                     p->max_health,
                     p->satiation,
                     p->max_satiation,
                     s->milliseconds_per_tick,
                     s->hunger_pain_per_tick,
                     s->hunger_per_tick,
                     s->satiated_heal_per_tick,
                     s->pain_per_wrong_answer,
                     s->satiation_per_right_answer);
}

Levels of abstraction

It may just be a pet peeve of mine, but I think this divides into cleaner layers of abstraction. With your original code, we have a single function that mixes a fairly high level of abstraction (format a State into a string) with lower levels of abstraction (e.g., memory management).

@vnp's code sort of inverts the layers of abstraction, so we have the higher level of abstraction at the bottom, and the lower level of abstraction above it.

This one gets the layers of abstraction closer to how I'd like to see them: the memory management and general purpose stringifying is at the bottom of the stack, and the more specific higher level concern of Stringifying a State is at an upper layer. So, something like a call graph accurately reflects the levels of abstraction being dealt with in the code.

Other Points

Given the number (and length) of arguments you're passing, I'd prefer to see the arguments passed one per line as I've formatted them above, rather than a variable number per line to justify out to some particular right margin.
Since stringify_state isn't intended to modify the State whose address is being passed, it's probably better to define the parameter as a State const *. Likewise with the local variables, so the first lines would probably be better written something like this:

char* stringify_state(State const* state) {
    Pet const* p = state->pet;
    Settings const* s = state->settings;


Very unorthodox placement of if-else clauses in C for handling special cases I've encountered a situation with an if-else clause. Specifically, the positive case has only 1 expression, whereas the negative case is a sequence of statements wrapped in braces. Like this:
if( i >= list->count ) *result = 0; else
{
    *result = i * 32 + 31;
    while( mask >> (*result % 32) ) (*result)--;
}

I personally don't see this as bad formatting, as it's common to see something like this in POSIX shell scripts:
if [ "not-null-string" ] ; then
    # the "then" clause didn't start on a new line
    commands >redirections | pipeline
fi

And sometimes, the condition can be even more complex in shell scripts (at least, I may write that way if I see fit and logically intuitive)
But I know, the better style of writing that if-else clause should be like this:
if( i < list->count )
{
    *result = i * 32 + 31;
    while( mask >> (*result % 32) ) (*result)--;
}
else *result = 0;

So is there some kind of strong arguments against if-else clause formatting such as that in my first fragment of C code?

I would say that the strong argument against the first example is that the else block starts right below the if and can be confused with a then block at first sight.
It's not about the order of the clauses, but only about formatting.
This is has the same condition and clauses, but it's not ambiguous:
if( i >= list->count )
    *result = 0;
else
{
    *result = i * 32 + 31;
    while( mask >> (*result % 32) ) (*result)--;
}

That said, I would personally prefer this formatting (though this second rewrite is less important and more subjective than the previous one):
if( i >= list->count )
    *result = 0;
else {
    *result = i * 32 + 31;
    while( mask >> (*result % 32) )
        (*result)--;
}


Validator extended to handle domain objects / entities I started improving my validator library even more, and I think I'm at the last final element (domain object / entity validations) before I can gladly say that my Validator library is completely done and it supports all of it.

Here you can see that my validator can already deal with regular variable inputs or non-entity inputs (certain forms for example).

I started figuring out how I could extend my validator to create domain object validators and here is my attempt.

Could anybody review it and see if you find any room for improvements and give constructive feedback?

User

class User
{
    public $name;
    public $email;
    public $age;
}

UserValidator

class UserValidator extends Validator
{
    public function __construct()
    {
        $rules['name']  = [new MaxChars(20), new Alpha(true)];
        $rules['email'] = [new Email()];
        $rules['age']   = [new Numeric(), new Between(13, 17)];

        $this->setRule('name',  $rules['name']);
        $this->setRule('email', $rules['email']);
        $this->setRule('age',   $rules['age']);
    }

    public function validate(User $user)
    {
        $this->errors = [];

        parent::validate('name',  $user->name);
        parent::validate('email', $user->email);
        parent::validate('age',   $user->age);

        return empty($this->errors);
    }
}

Validator

class Validator
{
    protected $rules  = [];
    protected $errors = [];

    public function getErrors($rulename = null)
    {
        if ($rulename) {
            return isset($this->errors[$rulename]) ? $this->errors[$rulename] : [];
        }

        return $this->errors;
    }

    public function setRule($name, Array $rules, $required = true)
    {
        $this->rules[$name]['rules'] = $rules;

        switch (true) {
            case ($required === true):
                $this->rules[$name]['required'] = 'Must not be null or empty.';
                break;
            case ($required === false):
                $this->rules[$name]['required'] = false;
                break;
            default:
                $this->rules[$name]['required'] = $required;
                break;
        }
    }

    public function validate($rulename, $input)
    {
        if ($this->rules[$rulename]['required']) {
            if (empty($input)) {
                $this->errors[$rulename][] = $this->rules[$rulename]['required'];
            }

            foreach ($this->rules[$rulename]['rules'] as $rule) {
                if (!$rule->check($input)) {
                    $this->errors[$rulename][] = $rule->getError();
                }
            }
        } elseif (!empty($input)) {
            foreach ($this->rules[$rulename]['rules'] as $rule) {
                if (!$rule->check($input)) {
                    $this->errors[$rulename][] = $rule->getError();
                }
            }
        }

        return isset($this->errors[$rulename]);
    }
}

Example

$user        = new User();
$user->name  = 'John Doe';
$user->email = 'john@doe.com';
$user->age   = 13;

$uv = new UserValidator();

$uv->validate($user)); //true
$uv->getErrors());     //empty array

I disagree with @Madara Uchiha on a several points.

If you couple business rule validations to the domain object, you cannot apply different business rules to the same domain object.

Never assume you will only have one set of business rules. By creating validators outside the domain object, you give yourself the flexibility of creating whole different swaths of business rules without touching the domain layer.

Basically, by the time you go to save a domain object to a database or other repository it should be completely valid from a business rule AND storage perspective. This allows you to communicate problems to the user in a manor that allows them to correct their mistake. If you let a SQL error propagate up the call stack and result in a 500 Internal Server Error, you as a programmer have not done your due diligence when validating data.

Now I could see a case for separating the storage validations from the business rules. Maybe you start out persisting your domain objects in a database, but later you move to a RESTfull web service. The web service may have different basic data validations, which you can then swap out for a different validator while keeping your existing business rule validations.
I disagree that this solution doesn't scale. Quite the opposite actually. It scales very well. Mashing your validations into the domain object is what doesn't scale. Changing the storage mechanism or needing to apply different categories of business rules gets complicated when your domain objects validate themselves. Domain objects should be mere boxes containing data. The decision of whether that data is correct is wholly out of scope for the domain object, in my opinion.
The storage manager should have absolutely no validations. It should accept a domain object, attempt to persist it and throw an exception if anything goes wrong.

Really, all of my objections are related to this tenet of software engineering, and especially object oriented programming: Do one thing, and do it well.

Putting validations into your domain objects means they are serving a dual purpose: Hold data from a storage medium AND ensuring it is valid. Do one thing and do it well. The domain object should hold data. The validation layer should ensure things are valid.

Putting validations into your storage manager means it is serving a dual purpose in the same manor. You should be able to swap out one layer for another with minimal refactoring. If you have to copy and paste of rewrite lots of code, then your application needs additional layers with looser coupling.


Finding area and perimeter of rectangles and circles that are instance of interface region The missing code in task 1 were the methodsarea(), perimeter() and toString().
In task 2 I know that using built in collection such as Arraylist would be efficient and simpler,So any new different idea and implementation are appreciated.

Make the classes Circle and Rectangle complete: write the missing code.

A static method selectRectangle accepts an array of areas of type Region, and returns an array that only contains the areas that are of type Rectangle. Create that method.

Create an array that contains both circles (objects of type Circle) and rectangles (objects of type Rectangle). Write code that determines and shows the perimeter and area of these regions. Call the method selectRectangles with the created array as argument.

public interface Region {
    double area();
    double perimeter();
}

class Circle implements Region {
    private double radius;

    public Circle(double radius) {
        this.radius = radius;
    }

    @Override
    public double area() {
        return Math.PI * radius;
    }

    @Override
    public double perimeter() {

        return Math.PI * 2 * radius;
    }
}

class Rectangle implements Region {
    private double with;
    private double length;

    public Rectangle(double with, double length) {
        this.length = length;
        this.with = with;
    }

    @Override
    public double area() {
        return length * with;
    }

    @Override
    public double perimeter() {
        return 2 * (length + with);
    }

    public String toString() {
       return "<" + this.with + this.with + this.area() + this.perimeter() + ">";
    }

    public static Rectangle[] selectRectangle(Region[] region) {
        int countRect = 0;

        for (int i = 0; i < region.length; i++) {
            if (region[i] instanceof Rectangle) {
                countRect++;
            }
        }
        Rectangle[] rect = new Rectangle[countRect];
        int companionVar = 0;
        for (int i = 0; i < region.length; i++) {
            if (region[i] instanceof Rectangle) {
                rect[companionVar++] = (Rectangle) region[i];
            }
        }
        return rect;
    }

    public static void main(String[] args) {
        Region[] region = {new Circle(5),
                new Rectangle(2, 4),
                new Circle(2),
                new Rectangle(7, 9),
                new Circle(6)

        };

        for (int i = 0; i < region.length; i++) {
            System.out.println(region[i].area() + region[i].perimeter());
        }

        Rectangle[] rectangle = selectRectangle(region);

    }

}

In my point of view you cannot implement
area() and perimeter() any clearer or simpler.

In the toString() method I would somehow separate the values and also not print  with two times and length not at all.
Here is my suggestion:

 public String toString() {
       return "<" + with + " x "+ length + ", area:" + this.area() + " perimeter: "+ perimeter() + ">";
    }

In Rectangle[] selectRectangle(Region[] region) I would use an ArrayList to avoid the loop to count the number of Rectangles.

public static Rectangle[] selectRectangle(Region[] region) {
    ArrayList<Rectangle> result = new ArrayList<>();
    final int lastIndex = region.length;
    for (int i = 0; i < lastIndex; i++) {
        if (region[i] instanceof Rectangle) {
            result.add((Rectangle) region[i]);
        }
    }
    return result.toArray(new Rectangle[result.size()]);
}


Bash script that checks if font is installed and installs it if necessary This checks if a font is installed by using fc-list and grep. If the particular font is not installed, it offers to install it for the user. This snippet is part of a larger script that installs a particular Conky configuration along with all of its dependencies with minimal effort from the user.

Concerns

I have to create the FONT_INSTALLED variable to avoid a "too many arguments" error by placing the code into the if statement, is there a better practice for handling this?
To check if the font is installed, I check if the FONT_INSTALLED variable is a null String or not. Is this considered good practice? I know in languages such as Java a String is normally checked for length/content since the default value is "".
I've seen that placing > /dev/null after a command that would normally have output hides the output. This seems slightly 'hacky' as I know commands such as wget have the -q or --quiet option to hide output. For commands that do not have such an option, what is the preferred method of hiding output?

I realize that I should not place semi-colons after each line as per the advice on my first question first question. I forgot to make the appropriate changes to this code snippet.

FONT_INSTALLED=$(fc-list | grep -i "roboto");
if [ -z "$FONT_INSTALLED" ]; then
    echo "The Roboto font is not currently installed, would you like to install it now? (Y/N)";
    read response
    if [[ "$response" == [yY] ]]; then
        echo "Installing the Roboto font to the ~/.fonts directory...";
        mkdir .fonts/Roboto/ -p && wget --quiet -O ~/.fonts/Roboto/tmp.zip http://www.fontsquirrel.com/fonts/download/roboto &&
        unzip ~/.fonts/Roboto/tmp.zip -d ~/.fonts/Roboto/ > /dev/null && rm ~/.fonts/Roboto/tmp.zip
        fc-cache -fv > /dev/null
        FONT_INSTALLED=$(fc-list | grep -i "roboto");
        if [ -n "$FONT_INSTALLED" ]; then
            echo "The Roboto font was sucessfully installed!";
        else
            echo "Something went wrong while trying to install the Roboto font.";
        fi
    else
        echo "Skipping the installation of the Roboto font...";
        echo "Please note that this Conky configuration will not work without the Roboto font.";
    fi
else
    echo "The Roboto font has already been installed.";
fi

If you are using bash then rather than mixing [ and [[, you should stick with one or the other, preferrably [[. See What is the difference between test, [ and [[ ?

You might also want to consider preferring printf over echo for a more robust approach. See this amazingly detailed analysis of why printf is better than echo.


Enabling stack overflow protection with Y/N prompt This asks the user if they would like to enable stack overflow protection. If they enter yes (Y or y), then a Boolean SOProtection is set to true. If they enter no (N or n), then SOProtection is set to false.

If neither Y, y, N nor n is entered, then it clears the input stream (to protect against a case where multiple characters are entered, which results in the input being run through multiple times).

How can this code be optimized (in both efficiency and performance)?

bool SOProtection;
char checkSOProtection;

while(true){
    std::cout << "Stack overflow protection? Y/N: ";
    std::cin >> checkSOProtection;

    if(checkSOProtection == 'Y' || checkSOProtection == 'y'){
        SOProtection = true;
        break;
    }

    if(checkSOProtection == 'N' || checkSOProtection == 'n'){
        std::cout << "Warning: If large values are entered, segmentation fault will occur!" << std::endl;
        SOProtection = false;
        break;
    }
    std::cin.clear();
    std::cin.ignore(std::numeric_limits<char>::digits10);
}

Your ignore can produce some erratic results, depending upon the number of characters entered by the user.  Rather than:

std::cin.ignore(std::numeric_limits<char>::digits10);

You might want to go with:

std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

Which will ignore all of the entered characters up to the end of the input line.  That way you don't start going round the loop multiple times, or with left over characters if the user enters:

  asdfasdfasdfasdfasdf

I'd also consider moving the warning to before the question.  Telling me there may be a seg fault after I've already selected that route seems like a waste of time.  I'd prefer to have that information prompted before I select Y/N (or given the question have it assume that I already know that information).


Is there any better way of defining respective data structures to read the sensor data into from driver/application file? So I have been wondering about the standard way of structuring your files/code when it comes to reading data from the driver file.
So what I now have is a buffer that's supposed to store the data that we, as user, requested from the driver file. Since we can't pass arrays to the function (decays to a pointer), what I do is have a struct member variable pRxBuff point to rxBuffer array which is located in main instead of defining inside a function because once the function returns, the array is no longer valid, and inside the Read() function, I populate rxBuffer by dereferencing the data at specific index.

typedef struct {
    I2C_TypeDef *pI2Cx;
    I2C_Config_t I2C_Config;
    I2C_State I2C_State;
    uint8_t *txBuffer;
    uint8_t *pRxBuffer;
    uint8_t rxStartIndex;
    uint8_t rxBufferSize;
    uint8_t txBufferLength;
    uint8_t rxBufferLength;
} I2C_Handle_t;

void ProcessData (uint8_t *rxBuffer) {
    uint8_t startIndex = 0;
    uint16_t temp;

    // process data
    uint8_t upperByte = rxBuffer[startIndex] & 0x1F; // mask out the 3 bits
    uint8_t signBit = upperByte & 0x10;

    if (signBit)
    {
        upperByte = upperByte & 0xF;    // clear out the sign bit
        temp = 256 - (upperByte << 4 | rxBuffer[startIndex+1] >> 4);
    }
    else
    {
        temp = upperByte << 4 | rxBuffer[startIndex+1] >> 4;
    }
}

// sensor.c
void ReadData(I2C_Handle_t *I2C_handle)
{
    // start I2C transaction
    while (HAL_I2C_StartInterrupt(I2C_TX_BUSY) != I2C_READY);

    I2C_handle->I2C_State = I2C_INIT;

    // read the data from the sensor
    for (int i = 0; i < I2C_handle->rxBufferSize/2; i++)
    {
        I2C_handle->I2C_State = I2C_INIT;
        while (HAL_I2C_StartInterrupt(I2C_RX_BUSY) != I2C_READY);
    }

    // at this point, I have `rxBuffer` populated with raw data
    // now I need to convert this raw data into human-readable

    for (int i = 0; i < I2C_handle->rxBufferSize; i+=2)
    {
        ProcessData(I2C_handle->pRxBuffer, i); // currently not storing processed data anywhere
    }
}

// main.c
const int bytesToRead = 6;
static uint8_t rxBuffer[bytesToRead];
I2C_Handle_t i2c;

void I2C_Initilization()
{
    i2c.pI2Cx = I2C1;
    i2c.I2C_Config.I2C_AckControl = I2C_ACK_ENABLE;
    i2c.I2C_Config.I2C_SCLSpeed = I2C_SCL_SPEED_SM;
    i2c.I2C_Config.I2C_DeviceAddress = MCP9808_ADDR;
    i2c.I2C_Config.I2C_FMDutyCycle = I2C_FM_DUTY_2;
    I2C_Init(&i2c);
}

uint16_t read_temp(uint8_t interrupt)
{
    uint16_t temperature;

    i2c.txBuffer = txBuffer;
    i2c.txBufferLength = txSize;
    i2c.pRxBuffer = rxBuffer;
    i2c.rxStartIndex = 0;
    i2c.rxBufferLength = BYTES_PER_TRANSACTION;
    i2c.rxBufferSize = bytesToRead;

    if (interrupt == SET)
    {
        temperature = read_temp_interrupt(&i2c);
    }
    else
    {
        read_temp_polling(&i2c, bytesToRead);
    }
    return temperature;
}

int main(void) {
    I2C_Initilization();
    read_temp(SET);
}

Issues with this:

though I have been able to populate the rxBuffer which I can access
in the main, is this still the right way to do it?
what if rxBuffer is of different size than what the processed data
would need. For e.g: 2 raw bytes represent one processed decimal
value. How do I avoid creating two different buffers each for storing
raw and processed data?
With this approach, I had to create a separate member variable
rxStartIndex to keep track of the index where the data is to be
written.

I will give you some tips about defining structs that later are going to be part of a file, of course you can do in other way but this is the way I follow in general. I understand that the following struct is the header of the file.

typedef struct {
    I2C_TypeDef *pI2Cx;
    I2C_Config_t I2C_Config;
    I2C_State I2C_State;
    uint8_t *txBuffer;
    uint8_t *pRxBuffer;
    uint8_t rxStartIndex;
    uint8_t rxBufferSize;
    uint8_t txBufferLength;
    uint8_t rxBufferLength;
} I2C_Handle_t;

In general header files contains some bytes for identification(check libmagic library), the second tip is to have all the types group to avoid misalignment of data. So your struct will be

typedef struct {
    uint32_t magic; // The magic value
    I2C_Config_t I2C_Config;
    I2C_State I2C_State;
    I2C_TypeDef *pI2Cx;
    uint8_t *txBuffer;
    uint8_t *pRxBuffer;
    uint8_t rxStartIndex;
    uint8_t rxBufferSize;
    uint8_t txBufferLength;
    uint8_t rxBufferLength;
} I2C_Handle_t;

Also you can use the attribute packed of the compiler if needed alignment of the data struct.


Prevent repetitive random numbers Is this a good way to prevent two sequential numbers from repeating, or is there a better, more efficient way of doing this? By efficient, I mean a less CPU and memory consuming process.

while (random === lastrandom) {
    random = Math.floor(Math.random() * 3) + 1;
}
lastrandom = random;

In practice, your code will be fine. In theory, it is not guaranteed to terminate, but the probability of that happening is literally negligible.

However, what you are doing is picking a random integer between 1 and 3 (both inclusive), with the number not repeating. With only three possibilities, we could draw up a state machine:

states = {
  1: [2, 3],
  2: [1, 3],
  3: [1, 2] };
if (lastRandom === undefined) {
    random = Math.floor(Math.random() * 3) + 1;
}
else {
    random = states[lastRandom][Math.floor(Math.random() * 2)];
}
lastRandom = random;

Unfortunately, that involves generating all the states first, which is impractical for larger ranges. There, we could use this more clever approach:

var min = 1;
var max = 3;
if (lastRandom === undefined) {
    random = Math.floor(Math.random() * (max - min + 1)) + min;
}
else {
    random = Math.floor(Math.random() * (max - min    )) + min;
    if (random >= lastRandom) random += 1;
}
lastRandom = random;

So if we are choosing the first number, everything works as usual. But for any subsequent number, we choose an integer from a set one smaller (to reflect that one number, the lastRandom, can't be chosen). If the random number obtained like this is less than the lastRandom, it can be used as is. Otherwise it has to be incremented by one, so that the lastRandom has been skipped.

Test to compare code:


Design pattern review: freeze/thaw (aka, "reversible builder" pattern) OK, this question may seem a little strange at first; however I'd like to have your comments on it.

Background: I do Java. A lot. Java is a statically typed language, it has means to restrict visibility of instance variables, etc. And as such the builder pattern (see here) is quite adapted to it.

I took this pattern a step further. The builder pattern is a one-step process: once you .build(), you obtain an instance, preferrably immutable; but once you have the instance, there is no going back. The freeze/thaw pattern is a "reversible builder".

I implemented it in Java and this gives the following interfaces, which I use quite a lot in my own projects (one-sided discussion about this pattern here):

// Frozen
@Immutable
public interface Frozen<T extends Thawed<? extends Frozen<T>>>
{
    /**
     * Return a thawed representation of this frozen object.
     *
     * @return a thawed object
     */
    T thaw();
}

// Thawed
@NotThreadSafe
public interface Thawed<F extends Frozen<? extends Thawed<F>>>
{
    /**
     * Obtain a frozen representation of this thawed object
     *
     * @return a frozen, immutable object
     */
    F freeze();
}

This pattern means that if you have a frozen instance f, you can obtained a thawed instance of that instance by calling .thaw(), modify it (since .thaw() returns a builder) and freeze it again:

// In java, again
final F newFrozen = frozen.thaw().setX().setY().etc().freeze();

I view this pattern as beneficial for several reasons:

thawing an instance gives you a builder with that instance's full state (and you can thaw as many times as you want, in a thread safe manner since frozen instances are immutable by contract);
you get all the power of builders on the thawed side.

I use this pattern a lot; I find it useful. Do you? What would be your gripes against it?

Sample implementation of a simple pair:

public final class MyClass
    implements Frozen<Builder>
{
    final int value;

    private MyClass(final Builder builder)
    {
        value = builder.value;
    }

    public static Builder newBuilder()
    {
        return new Builder();
    }

    public int getValue()
    {
        return value;
    }

    @Override
    public Builder thaw()
    {
        return new Builder(this);
    }

    public static final class Builder
        implements Thawed<MyClass>
    {
        private int value;

        private Builder()
        {
        }

        private Builder(final MyClass myClass)
        {
            value = myClass.value;
        }

        public Builder withValue(final int value)
        {
            this.value = value;
        }

        @Override
        public MyClass freeze()
        {
            return new MyClass(this);
        }
    }
}

For more complex cases, I generally create both classes in the same package and make instance variables for both package visible (this leaves of course the responsibility on me that what I inject into the frozen part is actually immutable, but I deal with it ;)

EDIT Right now the methods are called .thaw() and .freeze(), to reflect the pattern's intents; do you think of better names?

EDIT 2 Objections from @bowmore: the SRP (Single Responsibility Principle) is violated; this is true: the frozen instance has the added responsibility that it must generate a pre-filled thawed instance. The suggestion here is to create an additional constructor/static factory method/method (pick your poison) on the builder class so that it be able to "swallow" the contents of the frozen instance.

Pattern pros and cons :

Pros :

easy to use
readable (provided you are familiar with the pattern)

Cons :

forces a cyclic dependency between builder and the class being built (we're probably not crossing package boundaries, though)
moves part of the responsibility of the builder to the class being built, in addition to the responsibility that class already had (violating the SRP)

General remarks :

I see no potential clients for the interfaces, i.e. they are little more than marker interfaces.

Conclusion :

In my opinion the pattern adds too little value for what it trades off.


Representation of an artificial neuron I have started to interest myself in artificial intelligence a while ago, especially within the game industry. I have started with state machines moved on to fuzzy logic and now I want to learn and implement a neural network which will also work for games.

However to not overwhelm my tiny little head I wanted to start small. I have found on Wikipedia this article: Künstliches Neuron(Artificial Neuron) and if you hit strg+f you can search for "Darstellung boolescher Funktionen" which is basically what I have implemented.

So my questions regarding the implementation are:
 Is this class usefull for a later neural network (keeping in mind that this class only represents a neuron) so that I than can use this class inside a neuronal network?

 Are there any bad issues which I haven't seen?

One thing I have to mention is that in this situation I didn't want to implement a really good unit testing class or doctests (I hate doctests) thats why my test looks weird.

import numpy as np
import math

# From Stackoverflow
def find_nearest(array,value):
    idx = (np.abs(array-value)).argmin()
    return array[idx]

def hlim(x):
    return 0.5 if x == 0 else 0 if x < 0 else 1

class Synapse(object):
    def __init__(self, origin, destination, weight = 0):
        self.weight = weight
        self.origin = origin
        self.destination = destination

        self.origin.synapse = self
        self.destination.synapse = self

class Neuron(object):
    def __init__(self, bias, synapse = None):
        self.bias = bias # Schwellwert
        self.weight = 0
        self.synapse = synapse

    def addValue(self, value):
        self.weight += value
        self._output()

    def _output(self):
        print(1 if hlim(self.weight-self.bias) else "not fired")

class Input(object):
    def __init__(self, synapse = None):
         self.synapse = synapse

    def setInput(self, value):
         val = self.synapse.weight * value
         self.synapse.destination.addValue(val)

if __name__ == "__main__":
    # AND
    print("-AND-------")
    print("1&1")
    n = Neuron(1.5)
    x1 = Input()
    x2 = Input()
    s1 = Synapse(x1, n, 1)
    s2 = Synapse(x2, n, 1)
    x1.setInput(1)
    x2.setInput(1)

    # OR
    print("-OR--------")
    print("0|0")
    n = Neuron(0.5)
    s1 = Synapse(x1, n, 1)
    s2 = Synapse(x2, n, 1)
    x1.setInput(0)
    x2.setInput(0)

    # NOT
    print("-NOT----------")
    print("~0")
    n = Neuron(-0.5)
    x = Input()
    s = Synapse(x, n, -1.0)
    x.setInput(0)

I have started to interest myself in artificial intelligence a while ago, especially within the game industry. I have started with state machines moved on to fuzzy logic and now I want to learn and implement a neural network which will also work for games.

It's great that you're exploring different approaches! While applying state machines or say behavior trees to games is straightforward, applying "neural networks" to games does not make too much sense: imagine you were saying "I'm going to apply Python to games": the language is important, but much less important than what you're going to do with it. However, learning as you're doing here is a good way to move forward, so keep it up! You've gone further than many people here.

  However to not overwhelm my tiny little head I wanted to start small.

Great!

  Is this class useful for a later neural network (keeping in mind that this class only represents a neuron) so that I than can use this class inside a neuronal network?

Unfortunately, no. Neural networks are not implemented like this in practice. Neurons are not considered individually: the libraries model all the weights of the connections between one layer and the next, because it's much faster to train, and speed is crucial.

  Are there any bad issues which I haven't seen?

No, the only issue is that it won't be easy to do more things with the current structure.

import numpy as np
import math

# From Stackoverflow
def find_nearest(array,value):
    idx = (np.abs(array-value)).argmin()
    return array[idx]

Unused code.

def hlim(x):
    return 0.5 if x == 0 else 0 if x < 0 else 1

The threshold activation function is not very good, consider using at least a sigmoid or a ReLU (very easy to implement, works very well in practice). But it will only matter when you will start thinking about backpropagation.

class Synapse(object):
    def __init__(self, origin, destination, weight = 0):
        self.weight = weight
        self.origin = origin
        self.destination = destination

        self.origin.synapse = self
        self.destination.synapse = self

Notice that you never use .origin because you always go from the Input to the Synapse, not the other way around.

So this is nice object-oriented code, but I think this is overly complex: most of the code ties the objects together: you could get something much shorter:

def neuron_score(bias, weight_list, input_list):
    score = 0
    for weight, input in zip(weight_list, input_list):
        score += weight * input
    score -= bias

    return "fired" if hlim(score) > 0 else "not fired"

if __name__ == "__main__":
    print(neuron_score(1.5, [1, 1], [1, 1]))
    print(neuron_score(0.5, [1, 1], [0, 0]))
    print(neuron_score(-0.5, [0], [-1]))

See how this gets down to the essence of it? Of course this can be made much faster by using numpy arrays (and ndarray for more than one more neuron).

So, now, how do you implement XOR? :) Use the Tensorflow playground to try and get intuition about this.


Use Python to determine the repeating pattern in a string I am writing an algorithm to count the number of times a substring repeats itself. The string is between 1-200 characters ranging from letters a-z. There should be no left overs at the end of the pattern either and it should be split into the smallest possible combination.

answer("abcabcabcabc"):output = 4
answer("abccbaabccba"): output = 2
answer("abcabcd"): output = 1

My code:

import re
def answer(s):
    length = len(s)
    x=[]
    reg = ""
    for i in range(1, length+1):
        if length % i == 0:
            x.append(i)
    repeat = len(x)*10
    for _ in range(0, repeat):
        a = length/max(x)
        print(length)
        print(max(x))
        print(a)
        for _ in range(0, a):
            reg = reg + "."
        exp = re.findall(reg, s)
        print exp
        if all(check==exp[0] for check in exp):
            print len(exp)
            return len(exp)
        elif all(check!=exp[0] for check in exp):
            x.remove(max(x))

This is Python 2.7 code and I don't have to use regex. It just seemed like the easiest way.

Is there a better/faster/more optimal way to do this?

NOTE: it breaks if the string size is too big.

EDIT: Fixed indentation

I see that there already is an accepted answer, before I got around to writing this answer. That is a little discouraging for me as a reviewer, but I've written a code alternative and have some thoughts regarding your code.

Code review

Document your code – To guess what your code is doing by simply reading it is not very easy. It could very well have a few comments on what it tries to achieve.
Better variable names – In general try to use variable names describing the content. Names like x, a, exp (which I would read as expression?), and so on, are not very informative.
Regex are generally expensive – In general regex, whilst being a fantastic too, are somewhat overused. Do note that it takes time to build the patterns and match the patterns, especially compared to simple string matches.

Some new(?) constructs

I'm not sure if you know these already, but there are a few new constructs I would like to show you:

String repeat – Strings can be multiplied (aka duplicated) using the multiplication operator.

text = "abc"
print text * 3     # Will output "abcabcabc"

divmod with multiple outputs – divmod(a, b) will divide a by b and return the divisor and the rest. This can be stored directly into a tuple like in the following:

div, rest = divmod(13, 5)   # Giving div == 2, rest = 3

Slice and dice strings – As of Python 2.3 one can slice strings. The most common variants indicates how many characters you want from start like text[:3] or if you want the rest of the text like text[3:]. This can be very useful...
A slightly fancier print varant – Using .format in combination with print can produce nicer output rather easily:

print 'i: {}, sub: {}, div: {}, mod: {}'.format(i, source[:i], div, rest)

This would output on the same line, something like:

i: 4, sub: abcd, div: 2, mod: 3

else-block after for?! – This will make sense later on, but if a for loop completes normally, it'll not enter the optional else:-block. In other words, if you break out of a for loop Python won't enter the else block. This can be used to verify that the for loop actually found/did something, and provide an alternative if it didn't.

for i in range(5):
    print i
   if i == 3:
      break
else:
   print "Nothing larger than 3..."

Code alternative

I haven't commented too much on your chosen algorithm, as I find it a little confusing, so I thought what is he trying to achieve and what is an alternative approach. I then came up with these demands for the code:

You're looking for the maximum repeated substring completely filling out the original string. This means:

The candidate substring length, must then divide the original string length without any leftover (or rest characters)
The candidate substring can't be more than half the length of the original string, as it then can't be duplicated
The first (and shortest) candidate substring will always give the most repeats, if it matches the other criteria

So one way to write this out is like this:

def repeatedSubstring(source):
  """Look for the shortest substring which when repeated equals
     the source string, without any left over characters.
  """
  length = len(source)
  print '\nOriginal text: {}. Length: {}'.format(source, length)

  # Check candidate strings
  for i in range(1, length/2+1):
    repeat_count, leftovers = divmod(length, i)
    # print 'i: {}, sub: {}, div: {}, mod: {}'.format(i, source[:i], repeat_count, leftovers)
    # print 'repeated: {}'.format(source[:i]*repeat_count)

    # Check for no leftovers characters, and equality when repeated
    if (leftovers == 0) and (source == source[:i]*repeat_count):
      print 'Found repeated substring: {}, count: {}'.format(source[:i], repeat_count)
      break
  else:
    print "Couldn't find any repeated substring"

repeatedSubstring("abcdeabcde")
repeatedSubstring("abcdeabcdef")
repeatedSubstring("aaaaa")

I've commented out some debug print statements, and left it a little more verbose than the original code.

If you want, you can easily replace the remaining print statements with return div and return 1. A stripped down version would then look like:

def repeatedSubstringCount(source):
  """Look for the shortest substring which when repeated equals
     the source string, without any left over characters.
     Return the maximum repeat count, 1 if none found.
  """
  length = len(source)

  # Check candidate strings
  for i in range(1, length/2+1):
    repeat_count, leftovers = divmod(length, i)

    # Check for no leftovers characters, and equality when repeated
    if (leftovers == 0) and (source == source[:i]*repeat_count):
      return repeat_count

  return 1

I still left a few comments in there, so that it possible to have some idea on what is happening. I've also changed the name to "repeatedSubstringCount" to indicate both what the function does, but also what it returns.


Parameter list too long in initialize method I have an initialize method for a class that looks like this:

def initialize(
  site:,
  name:,
  date_online: '-',
  date_offline: '-',
  date_modified: '-',
  date_tagged: '-',
  tags: []
)
  @site          = site
  @name          = name
  @date_online   = date_online
  @date_offline  = date_offline
  @date_modified = date_modified
  @date_tagged   = date_tagged
  @tags          = tags
end

However, RuboCop is telling me that I should:

Metrics/ParameterLists: Avoid parameter lists longer than 5 parameters. [7/5]

So, what is the best way to rectify this? It's an initializer, so all those parameters are necessary. Is the desired solution to use a hash?

def initialize(args)
  @site          = args[:site]
  @name          = args[:name]
  @date_online   = args[:date_online]   || '-'
  @date_offline  = args[:date_offline]  || '-'
  @date_modified = args[:date_modified] || '-'
  @date_tagged   = args[:date_tagged]   || '-'
  @tags          = args[:tags]          || []
end

Then how should I manually handle the fact that site and name are required? I could add some code after the assignments that will throw an error if the required fields aren't present:

raise ArgumentError unless @site && @name

That makes sense to me. But I assume the makers of RuboCop know what they're talking about, which leads me to suspect that there's a standard for this sort of thing.

An in-between solution for you might be:

def initialize(site:, name:, **options)
  @site          = site
  @name          = name
  @date_online   = options[:date_online]   || '-'
  @date_offline  = options[:date_offline]  || '-'
  @date_modified = options[:date_modified] || '-'
  @date_tagged   = options[:date_tagged]   || '-'
  @tags          = options[:tags]          || []
end

You would probably want to raise an error if an unexpected key was passed in as part of options.


Watch for file changes in Linux I have installed Geekworm x735 on my Raspberry Pi, it uses the script below with an infinite loop to determine if the power button has been pressed. I know there are some Linux utils that watch for file changes (inotifywait, watch, fswatch to name a few), I wonder if they are more efficient than this solution or if under the hood they apply the same logic.

while [ 1 ]; do
  shutdownSignal=$(cat /sys/class/gpio/gpio$SHUTDOWN/value)
  if [ $shutdownSignal = 0 ]; then
    /bin/sleep 0.2
  else
    pulseStart=$(date +%s%N | cut -b1-13)
    while [ $shutdownSignal = 1 ]; do
      /bin/sleep 0.02
      if [ $(($(date +%s%N | cut -b1-13)-$pulseStart)) -gt $REBOOTPULSEMAXIMUM ]; then
        echo "X735 Shutting down", SHUTDOWN, ", halting Rpi ..."
        sudo poweroff
        exit
      fi
      shutdownSignal=$(cat /sys/class/gpio/gpio$SHUTDOWN/value)
    done
    if [ $(($(date +%s%N | cut -b1-13)-$pulseStart)) -gt $REBOOTPULSEMINIMUM ]; then
      echo "X735 Rebooting", SHUTDOWN, ", recycling Rpi ..."
      sudo reboot
      exit
    fi
  fi

Thank you.

Polling like this is a very inefficient mechanism, but for pseudo-files in /sys/, you may find that the usual kernel interfaces as used by inotifywait and the like don't get notified of changes in the same way that they do for real storage-backed files.  So it may be that you need to poll, unfortunately.  But do check, for the files you care about.
There doesn't seem to be any value creating shutdownSignal to immediately use - simpler to just insert the value directly:
while [ $(</sys/class/gpio/gpio$SHUTDOWN/value) = 1 ]

sudo is intended for interactive use, and doesn't work well in scripts.  I think it's better to assume (or check) that the script is invoked with sufficient capabilities to do the work it needs to do.
Don't forget to add a shebang line (#!/bin/bash).


Animated progress bar in C++ | How to simplify or alternatives using strings i'm a beginner in C++
i decided to learn the Scary C++ Programing Language
i am a slow learner and after 2 Months learning C++ i saw the need to have a Nice and Tidy progress bar for my programs.

Here what i wrote:

#include <iostream>
#include <string>
#include <iomanip>
#include <unistd.h>

// That function controls the inner Bar

void printProgressBar(char  fill = '.',
                      char  highlight = '*',
                      int   barSize = 10,
                      float startingProgress = 0.0f
                     )
{
        for (int pos = 0; pos < (barSize-(startingProgress*10)); ++pos)
        {
        std::cout << "\r[" << std::string(pos, fill) << highlight << std::flush;
        usleep(50000);
        }
}

// That function controls the outer Bar

void progressBarBox(char highlight = '*', float endPoint = 1.0f){
        std::cout << std::string(endPoint*10, highlight);
        std::cout << ']' << std::flush;
}

int main(int argc, char* argv[]) {
        const int  barSize{3};
        const char barFill{'.'};
        const char barHighlight{'*'};

        progressBarBox(barFill, barSize+0.1f);

        float progress {0.0f};
        while (progress < barSize)
        {
                  printProgressBar(barFill, barHighlight, barSize*10, progress);

        progressBarBox(barHighlight, progress);
        progress += 0.1;
        }

        std::cout << "\nDone" << std::endl;

        return 0;
}

The problem is: with python i never cared about best practices, clean of code or code readbility

I Have many questions about my solution

 My Questions:

How would you rate my code, what to improve ?
how to improve his readbility?
this code would be fine to work with or a headache to mantain, how to improve?
is there Better ways ( that are not so complex too ) to achieve the same ? with the star moving and everything.
Is there a best practice i'm not following or missing ?
i hardcoded some values is that bad ?
a friend of mine said that this code will have a bad performance because of my use of string, is that true ?

I know thats a lot of questions and i am grateful for any contribution or help.

Here are some things that may help you improve your program.

Use consistent formatting

The code as posted has inconsistent indentation which makes it a bit harder to read and understand. Pick a style and apply it consistently.

Think carefully about requirements

You wrote:

  a friend of mine said that this code will have a bad performance because of my use of string, is that true ?

Maybe, but it's important to think about the requirements first before chasing phantoms of imagined performance gains.  The first question is, "does it matter how fast it is?"  I'd say that any code that contains usleeep(50000); within a loop probably isn't performance-critical and so it doesn't much matter.

So then that leads to the question of what does matter which directly leads to the topic of requirements.  What does the code need to do?  Is there a performance requirement?  A memory footprint requirement?  An efficiency requirement?  My usual advice is to create the simplest thing that works.  This allows you (and others) to more easily understand the code and to avoid wasting your time on "optimizing" things that don't need to be optimal.

Use object-oriented programming

In this case, there's a thing called a "progress bar" that is expected to indicate the progress of some user-controlled process.  This suggests that it might be better to create a ProgressBar object.  That way, the current state of the bar, including the particular characters used, and its representation could be separate aspects of the same object. This would be much cleaner and also have the advantage of making it easier to create alternative output formats.

Consider the user(s)

Consider the user of this progress bar (presumably another programmer, or perhaps yourself).  You might want to have some actual lengthy work done by the computer, and the progress bar is just an indicator.  However, with the loop and usleep in the printProgressBar function, it's entirely possible that the progress bar itself would take up more time than the lengthy process that led the user to want a progress bar in the first place!  There are a couple of ways to address this.  The simplest would be to abandon the current look of the progress bar with its animation-like bar and adopt a simpler proportional bar.  Another option would be to keep the existing visual effect, but to allow the percentage complete to be indicated by both how many stars are to the right, but also where the advancing star is within the empty space.

That leads to the next consideration, which is the other user of this code, which is not the programmer, but a person using the program that includes this progress bar.  For that person, how clear is the intent of this progress bar?

Eliminate unused variables

This code declares main with arguments argc and argv in the usual way, but neither argc nor argv are actually used.  This could be addressed by either declaring int main() or by modifying the code to actually use the variables.


JavaScript ES6 import / export I am experimenting with JavaScript ES6 import and export and wanted to achieve the following:

Use the import / export style of including JavaScript files.
Import a class file and create an instance
Import a class file that extends another class
Expose functions to the index.html scope - call from an inline onclick event
Is document. available within the module scope

Is this the correct, or recommended way of doing this, or is there a better way?

index.html

<!DOCTYPE html>

        <meta charset="UTF-8">
        <title>Javascript - Import and Export class examples</title>

Javascript Import and Export class examples

        <button onclick="window.updateScreen()">Do it</button>

main.js

import { Animal } from './animal.js';
import { Human } from './human.js';
import { Dog, Cat } from './pets.js';

const petOwner = new Human('Bob', 'Male', '21/03/19');
const pets = [ new Dog('Riley', petOwner), new Cat('Sherry', petOwner) ];

petOwner.speak();
pets[1].speak();
pets[0].speak();

function updateScreen() {
    let elem = document.getElementById('myDiv');
    elem.textContent = 'HELLO';
}

export { updateScreen };

animal.js

class Animal {
    constructor(alive, hr) {
        this.isAlive = alive;
        this.heartRate = hr;
    }
}
export { Animal };

human.js

import {Animal} from './animal.js';
class Human extends Animal {
    constructor(name, sex, dob) {
        super(true, 60);
        this.name = name;
        this.dob = dob;
        this.sex = sex;
    }
    speak() {
        console.log('Hello');
    }
    get age() {
        return now-dob;
    }
}
export { Human };

pets.js

import {Animal} from './animal.js';

class Cat extends Animal {
    constructor(name, owner) {
        super(true, 200);
        this.name = name;
        this.owner = owner;
    }
    speak() {
        console.log('Meow');
    }
}

class Dog extends Animal {
    constructor(name, owner) {
        super(true, 120);
        this.name = name;
        this.owner = owner;
    }
    speak() {
        document.getElementById('myDiv').textContent = 'Woof';
    }
}

export { Cat, Dog };

Your main.js does not have a direct dependency on Animal class, so much there is no reason to import it there.  In fact, from standpoint of minimizing browser calls to include files, it might make better sense to keep all classes in a related hierarchy together in a single file. So, I would probably have Animal, Human, Cat and Dog in the same file unless the was a compelling reason to split them - like Human was used on every page while Cat an Dog were only used sparingly in the site.

Generally, it is a good idea to be consistent about what sort of “side effects” a piece of code (class) makes.  I know this is just an example, but it seems odd for these classes to log out the results of the speak() methods, when it really is the caller that should determine what happens with the result of the method call (logging it, updating Page with it, etc.)

Main.js seems a little odd with regards to why you are exporting the updateScreen function. This function doesn’t really do anything different after the first time it is executed so not sure why it is exported as a function. Also, this module creates the side effect off all of the instantiated objects speaking upon inclusion of the module.  This would happen even if the updateScreen function was never called. Again this seems like odd behavior to be happening upon import.

In Human class, is date of birth a property specific only to humans, or should this exist on Animal class.  You should consider passing and setting a proper date object for this property.  Should age be a property on the object?  This is able to be determined in the constructor, right? It seems odd to me to have getter method for this “property” and not the others. In the age getter, there is reference to “now” variable that is undefined and you are further trying to perform subtraction using dob string value that has not been converted to proper Date object.  Use Dates here for this math.

In Dog class to have especially questionable side effect in the speak method in that you have hard coded a reference to a div to be updated.  This class should not need to understand Dom at all. The way it is now limits reuse of this class because it is tightly coupled to the Dom via this logic which has nothing to do with a dog at all.


Reg expression for finding expression in sentence I have a regular expression that match the string that contains the word "duration", followed by a < or > operator and then a number. Here is the expression:

duration\s*(<|>)\s*([0-9]+)

Is there any way I can make this expression better?

Since you only have two characters to compete with, namely, < and >, use a character set. Other than that, you might consider \d matching your digits, instead of 0-9 as \d covers the numerical characters across the entire UTF8 range.

The final expression would be:

duration\s*([<>])\s*(\d+)

I've left the matching groups in place, considering that you might be referencing them in your later code.


Dispatcher for a JSON API without routes I am using this code for a fast JSON API without routes.

Does this code contain any useless parts?  Is the class loader efficient enough? Are there any better options? Is ReflectionMethod slow if used once?

<?php

/*
 * Init stuff
 */

session_start();

define('ROOT', realpath( dirname(__FILE__) ) . '/');
set_include_path(ROOT . 'src/');

spl_autoload_extensions('.php');
spl_autoload_register();

ini_set('error_reporting', E_ALL & ~E_DEPRECATED);
ini_set('display_errors',  1); //To change in prod to 0 <<<<<<<<<<<<<<<<<<<<<<<<<
ini_set('log_errors',      1);
ini_set('error_log',       ROOT . 'tmp/logs/application.log');

date_default_timezone_set('Europe/Paris');

/*
 * Process stuff
 */

$params      = explode( '/', trim( key($_GET), '/' ) );
$paramsCount = count($params);

if ($paramsCount >= 2) {
  try {
    $controllerMethod = new ReflectionMethod( '\app\controller\\' .
                                                  $params[1], $params[2]
                                                );

    if ($paramsCount - 2 >= $controllerMethod->getNumberOfRequiredParameters()) {

      \ens\controller\Request::$lang =& $params[0];
      \ens\controller\Request::$data =& $_POST;

      $response = $controllerMethod->invokeArgs( null, array_splice($params, 3) );

    } else {
      $response = array('status' => 400);
    }

  } catch (ReflectionException $e) {
    $response = array('status' => 300);
  }

} else {
  $response = array('status' => 301);
}

if (!headers_sent()) {
  header('Content-Type: application/json');
}

echo json_encode($response);

IMHO, all the ini_set doesn't belong here, but in the php.ini, unless you can't edit it for some reasons…

If so, it belong in a bootstrap file, as for the constant definitions and the include

  A bootstrap file usually starts by including those libraries that are necessary for the file itself to function correctly. This involves setting general directory and file paths, loading configuration files, etc.

This bootstrap file can also be very useful for many reasons described on the link i provided like:

  […] set up different application statuses such as Development, Production or Testing […]

An other thing:

IMHO, the test if (!headers_sent()) is pretty useless as is.

As you are returning JSON, you need to send this header('Content-Type: application/json'), so, if you can't for any reason, you should raise an error or an exception, not just doing nothing and echoing your JSON as if nothing happened…


Implementing classes in Javascript I'm building a simple chatApp. The UI has just 3 interaction areas: input, submit and previous messages button.
But I'm not interested in reviewing the app architecture as much as the javascript ideas and general understanding/errors.
I've commented the code, and would like to see if you can point out any bad practice. I'm sure most of the code is bad, but still, the most important errors.
I've also included the UI as a runnable snippet.
Any edits to make it better for reviewers will be done.

<!doctype html>

    <title>Socket.IO chat</title>

    <button id="wantMore">More</button>

    <form action="">
      <input id="m" autocomplete="off" /><button>Send</button>
    </form>

const prevMsgs = document.querySelector("button#wantMore")
const uri = "http://localhost:3000"

class Msg {
  constructor (msg, createdAt){
    this.msg = msg
    this.createdAt = createdAt
  }
static toHTML ({msg, createdAt}) {
// pass object, builds list item.
  try{
  //in case the date conversion fails.
    createdAt = (new Date(createdAt))
      .toTimeString()
      .substr(0,5)
  } catch(e){ console.log(e) }
  finally{ createdAt || "" }
return `<li><span>${msg}</span> &nbsp; <span>${createdAt}</span></li>`
}

static put (data) {
  $("#messages").prepend(this.toHTML(data))
}

static getMsgs (uri, skip, limit) {
  //returns a promise, errors are catch
  return fetch(`${uri}/messages/skip/${skip}/limit/${limit}`)
    .then(data => data.json())
    .then(items => items.forEach(item => this.put(item)))
}

static getLastMsgs (uri) { return this.getMsgs(uri, 0,10) }

}

$(function () { // $(document).ready(

  Msg.getLastMsgs(uri).catch(e => console.log(e))

  prevMsgs.addEventListener("click", () => {
      const msgLen = parseInt($("#messages").children().length)
      Msg.getMsgs(uri, msgLen, msgLen+10)
    })

  const socket = io(); //connects to / origin

  $('form').submit(function(e) {
    e.preventDefault(); // prevents page reloading

    const msg = new Msg($('#m').val(), new Date())

    //for the sender, append right away.
    $("#messages").append(Msg.toHTML(msg))
    // and emit, and broadcast
    socket.emit('chat message', msg);
    $('#m').val('');
    return false;
  });

  socket.on('chat message', data => {
    $("#messages").append(Msg.toHTML(data))
  })
})

```

Be careful of IDs, especially those with common names, since they (unfortunately) automatically create properties on the global object when the ID is a valid identifier, which is not an uncommon source of bugs 2. Here, you have #messages, which is probably a pretty common word related to your script. It's not likely to be a problem in most cases, but I'd feel more comfortable using a class instead.
CSS tweak Due to positioning, the send button appears to have an unusually thick border on the right. Consider using width: 9.5%; instead of width: 9%;. You could also consider using flex for the <form> instead, and only set an explicit width of a few % for the button, letting the <input> horizontally expand to take up the rest of the space automatically, without hard-coding.
jQuery? You have a few uses of $ that look like uses of jQuery, but it doesn't appear to exist as a


List enrolled users & show checkmark if all activities are complete I'm a beginner with rails working on a rails 4 project to use with my students. Users enroll in lessons; on enrollment, four activities are created for each user (expositions, scrambled, dictations, and spellings). Each of these activities has a number of words which are set to false in the db upon creation, and change to true as the user rewrites them correctly. At the bottom of each lesson's view I'm displaying a list of enrolled users. If the user completes all four activities, I'm displaying a font-awesome checkmark icon.

I'm noticing as more students enroll in each lesson that loading times for the lesson show view are increasing--most probably because my setup to find whether each user completed the lessons is inefficient.

In my lessonss controller I wrote helper methods to list the lesson's enrolled users, and to generate arrays with users who completed each of the lessons activities. In the view page, I'm testing whether each of the listed users completed all four activities in order to add the "completed" checkmark to his or her name.

As things are right now the code does what it's supposed to, but I'd love to learn and improve the student-user's experience. Any feedback regarding how I can improve this process will be well received.

In lessons controller:

  helper_method :enrolled_users
  def enrolled_users
    lesson = Lesson.find(params[:id])
    enrollments = lesson.enrollments
    enrolled_users = enrollments.map { |enrollment| enrollment.user }
  end

  helper_method :completed_scram
  def completed_scram
    lesson = Lesson.find(params[:id])
    completed_scram = enrolled_users.map do |user|
      current_enrollment = lesson.enrollment_for(user)
      all_unscramble = current_enrollment.scrambled_words
      user if all_unscramble.all? { |scrambled| scrambled.completed == true }
    end
  end

  helper_method :completed_expos
  def completed_expos
    lesson = Lesson.find(params[:id])
    completed_expos = enrolled_users.map do |user|
      current_enrollment = lesson.enrollment_for(user)
      all_expos = current_enrollment.word_expositions
      user if all_expos.all? { |expos| expos.completed == true }
    end
  end

  helper_method :completed_dicts
  def completed_dicts
    lesson = Lesson.find(params[:id])
    completed_dic = enrolled_users.map do |user|
      current_enrollment = lesson.enrollment_for(user)
      all_dicts = current_enrollment.word_dictations
      user if all_dicts.all? { |dict| dict.completed == true }
    end
  end

  helper_method :completed_spel
  def completed_spel
    lesson = Lesson.find(params[:id])
    completed_sp = enrolled_users.map do |user|
      current_enrollment = lesson.enrollment_for(user)
      all_sp = current_enrollment.image_spellings
      user if all_sp.all? { |spell| spell.completed == true }
    end
  end

In the view:

<% unless enrolled_users.count == 0 %>

Enrolled users:

    <% enrolled_users.uniq.each do |user| %>

          <%= user.username %>

          <% if completed_scram.include?(user) && completed_expos.include?(user) && completed_dicts.include?(user) && completed_spel.include?(user) %>

          <% end %>

    <% end %>

<% end %>

Before looking at the code, there's a simple and immediate option for speeding things up: When a user completes an activity (or a part of one), check the user's other activities, and set an overall completed flag on the user's enrollment accordingly. That way, you only need to check completion for one user at a time and only in connection with an activity update (whereas right now, you check completion for everyone, all the time). Whenever you need to display the list, you'll already have completed boolean values to use.

So that's one way to go.

Now, looking at the code, the first thing I see is a lot of duplication.

Second thing I see is that you unnecessarily abbreviate stuff. Ironic that "spelling" is spelt "spel"... Really, just write what you mean, and mean what you write. There's no reason whatsoever to abbreviate.

Anyway, duplication: The only thing that differs from method to method is the activity name.

So you could get rid of all the completed_* method, and just do this:

def users_with_completed_activity(name)
  lesson = Lesson.find(params[:id])
  enrolled_users.map do |user|
    current_enrollment = lesson.enrollment_for(user)
    user if current_enrollment.send(name).all?(&:completed?)
  end
end

Note that this:

all? { |x| x.completed == true }

is just checking "is true true?", so it's the same as:

all? { |x| x.completed }

which, with Ruby's shortcuts, is the same as:

all?(&:completed)

However there's another weird thing going on. You load the Lesson record. Then you call enrolled_users (which also loads the Lesson, by the way), which finds users from their enrollments, and then, for each user, you go back and get their Enrollment.

So:

Fetching the Lesson should probably be extracted into a before_action (Rails' scaffolded controllers include examples of exactly this)
You're getting the enrollments to get the users to get their enrollments... Notice the runaround there?

Sidestepping that, you get something like (assuming Lesson's been set in a before_action):

def users_with_completed_activity?(name)
  @lesson.enrollments.map do |enrollment|
    enrollment.user if enrollment.send(name).all?(&:completed?)
  end
end

Of course, I'm not exactly sure why you want an array with a mix of User records and nils. All it means is that you make it vastly more complicated in the view because you have to use include?. And that's slow, since it has to look through the array every time. So you have a system that'll get exponentially slower the more users you add.

It'd make more sense to send back an array of user/completed pairs:

def users_with_completed_activity?(name)
  @lesson.enrollments.map do |enrollment|
    [enrollment.user, enrollment.send(name).all?(&:completed?)]
  end
end

which you can then step through like:

users_with_completed_activity(:spelling).each do |user, completed|
  # do something
end

Getting a combined completed-all-activities boolean I'll leave to the reader (since this is all just stop-gap improvements anyway - read on).

I'd also ask why the Enrollment is also responsible for tracking activity completion. It's not necessarily wrong, but it doesn't seem the best fit either. Whether or not a user completes an activity doesn't really affect their enrollment. Whether or not you do well, you're presumably still enrolled. But with this structure, that's what happens. So that's a little odd.

Lastly, you have a call to uniq because - for some reason - it seems it's possible for users to be enrolled multiple times? I can't see any other reason. But why would someone be enrolled multiple times? It doesn't make sense in the real world, nor as a software model. How do you know which enrollment to update when an activity is completed? Which enrollment to check?

Now, none of this is likely to make things go much faster. The trouble is still that you're simply doing too much each request. As mentioned in the beginning, the simplest solution would probably be to update a flag on the enrollment, when a user completes an activity. Then that's all you need to check. It's not something that changes very often, so why go through all the motions of figuring it out every time? It's like a bank account balance: If every time you wanted to check your balance, the bank had to sum up all deposits and withdrawals, things would grind to a halt. So it stores the current balance and keeps it up to date.

There are many other ways to go about this (doing more processing in the database layer, doing different data modelling, in-memory caching, etc. etc.) but the simplest one is simply to store the things you need to access often.


K&R - The C programming language Exercise1-12 I am working through the K&R and just finished exercise 1-12 and below is my solution.
Exercise1-12: Write a program that prints input one word per line.

#include <stdio.h>

main()
{
    int c;

    while((c = getchar()) != EOF)
    {
        if(c == ' ' ||  c == '\t' || c == '\n')
        {
            putchar('\n');
            while(c == ' '|| c == '\t' || c == '\n')
             c = getchar();
        }
        putchar(c);
    }
}

My question is both author and solutions here defined and used the state variable.
when we compare my solution is pretty simple but it works. am I missing something? maybe there is a case that my solution won't work well?

Looking for your comments to improve, Thanks

To get a historically correct impression of programming in C, K&R is probably the best book available. In addition to that book, you should also get a more modern book about C since K&R doesn't teach you anything about function prototypes and buffer overflows (one of the main reasons that software written in C is often unreliable).

For example, the page you linked has a solution containing char buffer[1024];. That solution will fail with unpredictable behavior as soon as you pass it a file containing very long words, as the buffer will overflow and the C program will not reliably crash but is free to do anything it wants, including crashing or making daemons fly out of your nose. This is called undefined behavior.

There's not much to improve about your code. To make it modern, you simply have to replace one line:

main()              // before
int main(void)      // after

After that, you should tell your editor to format the source code. This will indent the innermost line c = getchar() a bit more, so that it is clearly inside the while loop.

// before:
while(c == ' '|| c == '\t' || c == '\n')
 c = getchar();

// after:
while (c == ' '|| c == '\t' || c == '\n')
    c = getchar();

Some other reviewers will say that you must always use braces in the if and while statements, but I disagree. It's enough to let your editor or IDE format the source code automatically, this will make it obvious when your code is indented wrongly.


Parsing a fixed-size message using variable length array in C My C program has to parse a fixed-length message like this:
uint8_t message[8] = {80, 75, 73, 71, 1, 1, 1, 1};

another message could be:
uint8_t message[8] = {80, 75, 73, 71, 41, 42, 1, 1};

The message contains an ASCII string and 1 shows the end of string. So the actual length of the first message is 4 and the second message is 6.
I use this method to extract the message:
1. Counting the message length
2. Initializing a variable length array with this value.
3. Using memcpy to copy the bytes.

decoder.h
#ifndef DECODER_H_INCLUDED
#define DECODER_H_INCLUDED

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

#define LENGTH 100
void parse(uint8_t * message);
void get_address(char * addr, int size);
uint8_t get_size(void);

#endif // DECODER_H_INCLUDED

decoder.c
#include "decoder.h"

// Global variables in library
uint8_t parsed_message[LENGTH];
uint8_t len = 0;

void parse(uint8_t * message)
{
len = 0;
for (int i = 0; i < LENGTH; i++)
    {
        if (message[i] != 1)
        {
            parsed_message[i] = message[i];
            len++;
        }
        else
        {
            parsed_message[i] = '\0';
            len++;
            break;
        }

    }
}

void get_address(char * addr, int size)
{
    memcpy(addr, parsed_message, size);
}

uint8_t get_size(void)
{
    return len;
}

In the main function:
#include <stdio.h>
#include <stdlib.h>
#include "decoder.h"

int main(void) {

    uint8_t input[8] = {80, 75, 73, 71, 1, 1, 1, 1};
    parse(input);

    char addr[get_size()];
    get_address(addr, get_size());
    printf("==== %s ==== ADDRESS\n", addr);

    return 0;
}

I use GCC compiler and C99 standard in ARM-Based 32-bit micro-controller.
My question is that is this method safe? Or I should use another approach.

Overall Observations
The int main(void) function is correctly declared since no arguments are expected.
The code definitely follows the Single Responsibility Principle and this is good!
The use of portable include guards is good.
The program would be more flexible if it accepted the message as input from either the console or a file (would require an alternate declaration for main()).
Use System Defined Constants
The main() function could be made more readable if the system defined constants EXIT_SUCCESS and EXIT_FAILURE were used (only EXIT_SUCCESS in this case). These constants are included in the stdlib.h header file which is already included in the code.
#include <stdio.h>
#include <stdlib.h>
#include "decoder.h"

int main(void) {

    uint8_t input[8] = { 80, 75, 73, 71, 1, 1, 1, 1 };
    parse(input);

    char addr[get_size()];
    get_address(addr, get_size());
    printf("==== %s ==== ADDRESS\n", addr);

    return EXIT_SUCCESS;
}

Avoid Global Variables
It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.
In C it is possible to have a variable that is global to a file, but does not impact the entire program by using the static keyword.
static uint8_t parsed_message[LENGTH];
static uint8_t len = 0;

In the current implementation this compiles properly.
If you are really going to use global variables throughout the program they should be declared as extern variables in the header file.
#ifndef DECODER_H_INCLUDED
#define DECODER_H_INCLUDED

#include <stdint.h>

#define LENGTH 100
extern uint8_t parsed_message[LENGTH];
extern uint8_t len;

void parse(uint8_t* message);
void get_address(char* addr, int size);
uint8_t get_size(void);

#endif // DECODER_H_INCLUDED

Header Files
Only include header files that are absolutely necessary to make the code compile in header files, it is better to include the necessary header files in the C source file. This makes the code clearer in the source file. In decoder.h the only necessary header file is stdint.h. This could affect compile time and cause link errors if include guards are missing. In most implementations of C a temporary source file is generated by the C pre-processor and all header files are copied into the temporary file.
Variable Types
It isn't clear from reviewing the program why the code is using uint8_t rather than unsigned char. The overall result would be the same and less header files would be necessary.