Async and error-handling I need to delete a file when an error occurs during loading it. It is not allowed to perform async operations in a catch block.

This has lead me to writing the following code:

    public async Task LoadFromStorageFile(string fileName)
    {
        StorageFile file = null;
        Exception ex1 = null;

        try
        {
            var folder = await ApplicationHelper.GetApplicationSaveFolder();
            file = await folder.GetFileAsync(fileName);
            // ...
        }
        catch (Exception ex)
        {
            ex1 = ex;
        }

        try
        {
            if (ex1 != null && file != null)
            {
                await file.DeleteAsync(StorageDeleteOption.PermanentDelete);
            }
        }
        catch (Exception)
        {
             //...
        }
        finally
        {
            if(ex1 != null)
                throw ex1;
        }
    }

Is there a nicer way of doing this? The rethrown exception especially bugs me.

I need to delete a file when an error occurs during loading it.

In other words:

Happy path:

Load a file
Move on

Unhappy path:

Load a file, exception is thrown
Delete the file
Move on

Exceptional path:

Load a file, exception is thrown
Delete the file, exception is thrown
Handle exception, move on

From these points, it appears we'd need a task that loads a file. If that task fails, we need to try to handle the unhappy path gracefully.

I haven't written async code with async/await, but I'd probably get away with this using a Task that loads the file, and then decide whether I'd .ContinueWith another Task that deletes the file, and then decide how to handle the exceptional path where that other task would have failed.

Signature

public async Task LoadFromStorageFile(string fileName)

This StackOverflow answer refers to an authoritative source for async method naming conventions:

  The Task-based Asynchronous Pattern (TAP) dictates that methods should
  always return a Task<T> (or Task) and be named with an Async suffix;
  this is separate from the use of async. Certainly, Task<bool> Connect()
  (or async Task<bool> Connect()) will compile and run just
  fine, but you won't be following the TAP naming convention.

The signature for Task LoadFromStorageFile(string) would follow the naming convention for the pattern, by appending "Async" to the method's name:

public async Task LoadFromStorageFileAsync(string fileName)

Async/Await Exception Handling

As I've found in this StackOverflow answer, an exception thrown in an async method will bubble up to the caller, so whoever called await LoadFromStorageFileAsync("somefile.txt"); can know whether to move on, or to handle the unhappy path.

  It is not allowed to perform async operations in a catch block.

Indeed. There's a question on Stack Overflow, with a nice answer from one of our top reviewers here on CR, that shows how that limitation can be circumvented. In a nutshell:

bool loadSucceeded;
try
{
    var folder = await ApplicationHelper.GetApplicationSaveFolder();
    file = await folder.GetFileAsync(fileName);
    // ...

    loadSucceeded = true;
}
catch
{
    loadSucceeded = false;
}

LoadFromStorageFileAsync can then look like this:

public async Task LoadFromStorageFileAsync(string fileName)
{
    bool loadSucceeded;
    StorageFile file;

    try
    {
        // happy path
        var folder = await ApplicationHelper.GetApplicationSaveFolder();
        file = await folder.GetFileAsync(fileName);
        // ...

        loadSucceeded = true;
    }
    catch(Exception exception)
    {
        // log exception

        loadSucceeded = false;
    }

    if (!loadSucceeded && file != null)
    {
        try
        {
            // unhappy path
            await file.DeleteAsync(StorageDeleteOption.PermanentDelete);
            deleteSucceeded = true;
        }
        catch(Exception exception)
        {
            // exceptional path
            // log exception
        }
    }
}

This code doesn't rethrow whatever exception caused loadSucceeded to be false - rethrowing that exception in the unhappy path makes no sense, as was said before, that finally block is not needed. Instead, you could catch(Exception exception) (or anything more specific) in the happy path's catch block, and write a trace, a debug, or a warning log entry with the exception and its stack trace. At that point, it's taken care of, there's no need to re-throw it.


Python REST API clients I am working on a project right now, where we need to search different APIs, then clean & persists data to database and finally pushing to Elasticsearch.

The high level code looks like this:

class BaseParser(object):

    def __init__(self, source):
        self.url = source.url
        self.api_key = source.api_key

    def _prepare_auth_header(self):
        headers = {
        "Authorization": "Basic {}".format(self.api_key)
        }
        return headers

    def get_response(self):
        raise NotImplementedError()

class UnitedexchangeParser(BaseParser):

    def get_response(self):
        headers = self._prepare_auth_header()
        response = requests.get(self.url, headers=headers)
        return response.json()

class DubaiParser(BaseParser):

    def get_response(self):
        headers = self._prepare_auth_header()
        response = requests.get(self.url, headers=headers)
        return response.json()["data"]

class ParserFactory(object):

    def parse(self, source):
        if source.name == "Unitedexchange":
            parser = Unitedexchange(source)

        elif source.name == "Dubai":
            parser = DubaiParser(source)

        return parser.get_response()

The application is working fine, but we are adding more and more new sources which leads to change ParserFactory with same block of code.

How can I improve the layout and design such a way so we do minimal change for adding new parser? Performance is also an issue, and right now we are loading lots of individual classes which basically doing the same thing. It would be great if you can give some hint on that, too.

A few small comments:

When not supplying any argument to an exception, the () can be skipped: raise NotImplementedError.
I'm assuming ParseFactory is actually longer, otherwise there would be no need for it to be a class. A parse function on its own would be perfectly fine.
It would be better to make a dictionary of parsers, especially if the number of parsers grows:

class ParserFactory(object):
    parsers = {"Unitedexchange": UnitedexchangeParser,
               "Dubai": DubaiParser}

    def parse(self, source):
        parser = ParserFactory.parsers[source.name]
        return parser.get_response()

This does not solve the problem of having to implement every parser.

If they are sufficiently similar, namely almost all have the same get_response, you could just put a standard implementation in BaseParser and use

class DubaiParser(BaseParser):
    pass

Or, you could use type to create those classes that do not need any modifications dynamically and put them in a dictionary:

parsers = {"DubaiParser": None, "OtherParser": None, ...}
for name in parsers:
    parser = type(name, (BaseParser,), {})
    parser.__module__ = __name__
    parsers[name] = parser

There is even the possibility to override some methods, but this only makes sense if there are two different implementations of get_response

def get_response_json(self):
    headers = self._prepare_auth_header()
    response = requests.get(self.url, headers=headers)
    return response.json()["data"]

other_methods = {"get_response": get_response_json}

parsers = {"DubaiParser": None, "OtherParser": None, ...}
different_get_response = {"OtherParser", ...}
for name in parsers:
    if name in different_get_response:
        parser = type(name, (BaseParser,), {}, lambda ns: ns.update(other_methods)
    else:
        parser = type(name, (BaseParser,), {})
    parser.__module__ = __name__
    parsers[name] = parser


Thread safe algorithm for arrays information manipulation I have a program which requires a usage of threads and it must be thread safe. I do not have much experience with threads and critical sections, but as much as I know you must lock code sections where resources are being read and written to. Program is working, but I do not know if I did it correctly.

DataB code was added to answer users Incomputable question. It is just a class which stores information required for program to work.

class DataB {
public:
    DataB() { }
    DataB(double sF, int sC, string n);
    double getField(void);
    void increaseFieldCount(void);
    void decreaseFieldCount(void);
    int getFieldCount(void);
    void setField(double f);
    void setCountField(int fC);
    string getName(void);
private:
    string name;
    double field;
    int fieldCount;
};

    DataB::DataB(double sF, int sC, string n) {
    field = sF;
    fieldCount = sC;
    name = n;
}

double DataB::getField(void) {
    return field;
}

void DataB::increaseFieldCount() {
    fieldCount++;
}

int DataB::getFieldCount(void) {
    return fieldCount;
}

void DataB::decreaseFieldCount(void) {
    fieldCount--;
}

void DataB::setCountField(int cF) {
    fieldCount = cF;
}

void DataB::setField(double f) {
    field = f;
}

string DataB::getName(void) {
    return name;
}

mutex threadLock;

void threadRemove(int dataStart, int dataEnd, int dataCount, DataB B[],  DataB V[], int &sortedCount) {
    bool changed = true;

    while (changed || sortedCount != dataCount ) {
        changed = false;

        for (int i = dataStart; i < dataEnd; i++) {

            threadLock.lock();

            int count = V[i].getFieldCount();

            threadLock.unlock();

            if (count > 0) {

                threadLock.lock();

                double delItem = V[i].getField();

                threadLock.unlock();

                for (int x = 0; x < dataCount; x++) {

                    threadLock.lock();

                    double compare = B[x].getField();

                    threadLock.unlock();

                    if (compare == delItem) {

                        threadLock.lock();

                        B[x].decreaseFieldCount();
                        V[i].decreaseFieldCount();

                        if (B[x].getFieldCount() == 0) {
                            B[x].setCountField(-1);
                            B[x].setField(-1);
                        }

                        changed = true;

                        threadLock.unlock();
                        break;
                    }
                }
            }
        }
    }
}

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

/* */

    thread G6(threadRemove, 0, 5, dataCount, B, V, ref(sortedCount));
    thread G7(threadRemove, 5, 10, dataCount, B, V, ref(sortedCount));
    thread G8(threadRemove, 10, 15, dataCount, B, V, ref(sortedCount));
    thread G9(threadRemove, 15, 20, dataCount, B, V, ref(sortedCount));
    thread G10(threadRemove, 20, 25, dataCount, B, V, ref(sortedCount));

    G6.join();
    G7.join();
    G8.join();
    G9.join();
    G10.join();
}

Contention

There is only one std::mutex, and every thread is permanently in contention for that same lock for every small partial operation. Thus, all the threads are permanently in contention over that lock, and at most one thread at a time is actually doing any work. A single threaded implementation should be faster - it has to do the same work, but doesn't have to fight over lock control.

So, how can this be fixed?

1) Separate independent data per thread

In the usage example, elements of V are split between threads so that no two threads are accessing the same elements. So, accesses to elements of V don't need a lock - if this convention is followed strictly.

If this is possible, locks aren't needed for those parts!

2) Finer granularity locks

Right now, taking the lock stops every thread from doing any work at all. Even with the improvement of option 1, only one thread at a time can access elements from B. This can be improved by introducing locks at a finer granularity:

simple: one lock per array element (so one for each DataB object)
more advanced: one lock for a subset of each array (e.g. one lock for every 10 array elements)

Doing so allows other threads to perform work on all unrelated elements.

3) Read/Write exclusivity

What's the difference between reading and writing B{0]? Reading can be done concurrently, writing can't. There is a lock that helps for this special case: std::shared_mutex allow multiple threads to read the related object(s), but only allows one thread to write to it (while no one else can access it). The catch? It's only available since C++17. Before that, there might be other libraries providing that functionality, though (e.g. boost), or you make do with a normal std::mutex.

Small problem

In the current version, every changes to an element of B is done in one transaction - no thread can see any partial state. If this property is required, this can be implemented with some special considerations. How do you change a BData object in one transaction?

Create a copy, change the copy, take the lock, overwrite the original with the copy, release lock (aka RCU = Read Copy Update, requires an external lock)
Lock the object, perform changes, unlock the object (this requires giving access to an internal lock)

Implementation

Headers are missing, At least <string>, <mutex> and <thread> need to be included.
using namespace std; is considered bad practice and should be avoided.
Some member functions of DataB and some variables could be marked const (e.g. DataB::getFieldCount(), DataB::getField(), compare, count, every function parameter). Doing so might help reasoning about code and enables the compiler to verify said reasoning and might enable it to generate better machine code.
Inconsistent naming: DataB::getFieldCount and DataB::setCountField.
DataB doesn't encapsulate any behavior, it just provides some getters and setters. Maybe make it a POD struct instead?
Prefer list initialization in the constructor(s).
The void in function_name(void) isn't needed in C++.

Fixed contention code

  For this, I made one internal mutex per DataB object and chose the second option for transactions. Also, I changed DataB to a POD struct to easily provide access to all internals (having getters and setters acquire locks on their own doesn't fit well with having a lock for a transaction).

#include <string>
#include <thread>
#include <shared_mutex>

struct DataB {
    mutable std::shared_mutex mut{}; // mutable so sonst DataB object can still be locked
    std::string name;
    double field;
    int fieldCount;

    DataB() { }
    DataB(const double sF, const int sC, const std::string& n) : name{n}, field{sF}, fieldCount{sC} {}
};

void threadRemove(const int dataStart, const int dataEnd, const int dataCount, const DataB B[], const DataB V[], const int &sortedCount) {
    bool changed = true;

    while (changed || sortedCount != dataCount) {
        changed = false;

        for (auto i = dataStart; i < dataEnd; i++) {
            if (V[i].fieldCount > 0) {
                for (auto x = 0; x < dataCount; x++) {
                    double compare;
                    {
                        std::shared_lock<std::shared_mutex> read_lock{ B[x].mut }; // shared_lock = read only access
                        compare = B[x].field;
                    } // read_lock goes out of scope and gets released
                    if (compare == V[i].field) {
                        std::unique_lock<std::shared_mutex> write_lock{ B[x].mut }; // unique_lock = write access
                        --B[x].fieldCount;
                        --V[i].fieldCount;

                        if (B[x].fieldCount == 0) {
                            B[x].fieldCount = -1;
                            B[x].field = -1;
                        }

                        changed = true;
                    }
                }
            }
        }
    }
}

Note

This could maybe be enhanced by strategic use of atomics. However, that would require more knowledge about the members and usages of DataB than I could gather/guess from the code given.


Speed up position check for embedded length on stream I received help with adding the length of a stream in the first 4 bytes, meaning it's there as an int.

I am adding it at the start and sending it, while the receiving part checks for its position.

However, the position checking is a bit slow, so I wonder if it's possible to improve it.  Maybe using unsafe pointers or something, but I'm not sure.

currentPosition = 0;
while (currentPosition < sizeof(int) && tt1.Connected)
{
    currentPosition += tt1.GetStream().Read(lenArray, currentPosition, sizeof(int) - currentPosition);
}

There is the example on what it does at the end of the receiving part.  It goes through it, and it's a bit slow.

I can't see much of what can be done here, but hopefully someone else has a better idea.

Here is the whole code, which may help show how it functions:

tempBytes = new byte[length];
ms = new MemoryStream(tempBytes);

int currentPosition = 0;

while (currentPosition < length && tt1.Connected)
{
    currentPosition += tt1.GetStream().Read(tempBytes, currentPosition, length - currentPosition);
}

newImage = Image.FromStream(ms);

gmp.DrawImage(newImage, 0, 0);

currentPosition = 0;
while (currentPosition < sizeof(int) && tt1.Connected)
{
    currentPosition += tt1.GetStream().Read(lenArray, currentPosition, sizeof(int) - currentPosition);
}
length = BitConverter.ToInt32(lenArray, 0);

I agree with the comments that it's not likely to be your position checking code that's slowing you down.  Maybe you could squeeze some performance out with unsafe code, but that's maybe saving a copy from one buffer to another.  It would be nice to put that reading loop into a separate function, something like:

byte[] ReadBytes(SomeType tt1, int bytesToRead);

I can't guess what type tt1 is, but is it possible that retrieving the value for Connected is slow?  What about GetStream?


BMI calculator in Python This is my first attempt at writing a programme: a BMI calculator written in Python. Please give me feedback on the programme overall, as well as how easy the code is to interpret and adapt.

from sys import exit

bmi = ["what is bmi?", "what is bmi", "bmi", "bmi?"]

male=["male","m"]

female=["female", "f"]

def bmi_q():
    print "Very good! Let's start with your age."
    age= float(raw_input("How old are you %s?   >   " %name))

    height= float(raw_input("How tall are you, in m?    >   "))

    weight=float(raw_input("What do you weigh in kg?    >   "))
    Gender=raw_input("Finally, are you male or female? (No offense.)    >   ")

    result = bmi_c(weight,height)

    analysis(result)

def analysis(result):
    if result < 18.5:
        print "Your BMI is %d." % round(result,2), "You are far too skinny, please eat more."
        print "A healthy BMI is between 18.5 and 25."
        exit(0)

    elif result > 25:
        print "Your BMI is %d." % round(result,2), "You are far too fat, please eat less."
        print "A healthy BMI is between 18.5 and 25."
        exit(0)

    else:
        print "Your BMI is %d." % round(result,2), "Congratulations! You're well fit."
        print "A healthy BMI is between 18.5 and 25."
        exit(0)

def bmi_c(weight,height):
    return weight / (height**2)

def consent_q():
    unsure = True

    while True:
        print "Would you like to calculate your BMI?"
        consent = raw_input("> Answer: \"Yes\", \"No\", or \"What is BMI?\" >   ")

        if consent.lower() == "no":
            print ("What did you open a script called \"BMI\" for then??? This is all I can do!")

        elif consent.lower() in bmi:
            print """Wise man at CDC say:
                \"Body Mass Index (BMI) is a person's weight in kilograms
                divided by the square of height in meters.

                A high BMI can be an indicator of high body fatness.

                BMI can be used to screen for weight categories that may
                lead to health problems but it is not diagnostic of the
                body fatness or health of an individual.\" """

        elif consent.lower() == "yes":
            unsure = False
            bmi_q()

        else:
            print "Sorry, me no hablo ingles so good. Try again."

print "Thank you for running the BMI script."
print "Before we begin, please tell me your name:"
name=raw_input(">   ")
print "Hello %s, nice to meet you." % (name)
consent_q()

The main issue I see is inconsistent style. Sometimes you put spaces around certain operators, sometimes you don't. You want your code to be consistently formatted.

Generally, it's best to always put a space before and after an operator, especially the assignment operator. It's even more strange to put a space before an operator, but not after (or vice versa).

This line, for example:

age= float(raw_input("How old are you %s?   >   " %name))

would look much better like this:

age = float(raw_input("How old are you %s?   >   " % name))

You also switch between putting a space after commas in lists, and not doing so. I would change these lines:

male=["male","m"]

female=["female", "f"]

to look like this (notice the space after the first comma):

male = ["male", "m"]

female = ["female", "f"]

Variables should always begin with a lowercase letter. Classes and constants can and should be capitalized, but not normal variables. So the Gender variable should be renamed to gender.

You should consider not asking for the user's gender and age unless you actually do something with that data, or at least don't assign it to variables because it'll go unused.

I would also rename bmi_q, bmi_c, and consent_q to something more clear, so other people reading your code can immediately know what those function are supposed to do. bmi_c could definitely be renamed to something like calculate_bmi, or even calc_bmi.

And lastly, I'd always make sure your indention doesn't get messed up when pasting your code into a site like this. There's a few problems with invalid indention, but I'd imaging that happened when you put it in this question. (the indention in the question has been fixed now though)

But overall, good job on writing your first program! Congrats, keep it up, and remember this quote from Phil Karlton:

  There are only two hard things in Computer Science: cache invalidation and naming things.


K&R Exercise 2-6 : Inserting part of a bitset into another "Inserting" part of a bitset into another

This is Exercise 2-6 on the K&R book, 2nd edition. The reader is asked to:

  Write a function setbits(x,p,n,y) that returns x with the n bits that begin at position p set to the rightmost n bits of y, leaving the other bits unchanged.

Parameters Description

Here's what each parameter does informally:

x -> serves as the "destination" bitset
y -> serves as the "source" bitset
p -> serves as the insertion point for the bits from y to be inserted on x
n -> serves as the length of the segment to "cut" from y and replace in x

Special Notes

I've tested with a number of different scenarios, and this code has passed all of them.
At the bottom of the question there's an outline of a test-case and my logic behind solving the problem

Code

#include <stdio.h>

//setbits: returns x with n bits that begin at p set to the rightmost n bits of y
//informally: "chop" n bits right->left from y, insert on x at position p
unsigned setbits(unsigned x, int p, int n, unsigned y)
{
    return ((~(~0 << n) & y) << p) | ((~(~(~0 << n) << p)) & x);
}

int main(void) {
    // Example test cases:
    // First scenario:
    // x = 100 ->           0110 0100
    // y = 5 ->             0000 0101
    // p = 4
    // n = 3
    // Expected outcome:    0101 0100  (84)
    // Second scenario:
    // x = 94 ->            0101 1110
    // y = 195 ->           1100 0011
    // p = 0
    // n = 3
    // Expected outcome:    0101 1011  (91)
    // Third scenario:
    // x = 94 ->            0101 1110
    // y = 195 ->           1100 0011
    // p = 6
    // n = 4
    // Expected outcome:    1101 1110  (222)

    printf("%d\n", setbits(100, 4, 3, 5));
    printf("%d\n", setbits(94, 0, 3, 195));
    printf("%d\n", setbits(94, 6, 4, 195));

    return 0;
}

Outline

 Click for full-size image

Avoid UB

Shifting a 1 into the sign position is undefined behavior (UB). (C11 §6.5.7 4)  Instead, use unsigned types.  0 is an int constant.  0u is an unsigned constant.

// return ((~(~0 << n) & y) << p) | ((~(~(~0 << n) << p)) & x);
return ((~(~0u << n) & y) << p) | ((~(~(~0u << n) << p)) & x);

Clarity

Code for clarity and let the compiler form optimal code.

// return ((~(~0u << n) & y) << p) | ((~(~(~0u << n) << p)) & x);
unsigned new_mask = ~(~0u << n);
unsigned old_mask = ~(mask << p);
return ((new_mask & y) << p) | (old_mask & x);

Specifier matching

"%d" with unsigned should have raised a compiler warning/note.  Enable all compiler warnings to avoid such easy to identify problems.

// printf("%d\n", setbits(100, 4, 3, 5));
printf("%u\n", setbits(100, 4, 3, 5));

For this task, using "%4x" would be more informative than "%u".

Wider testing

"I've tested with a number of different scenarios, and this code has passed all of them.". --> Testing with UINT_MAX, UINT_MAX-1, 1, 0 would help check out the "corners" of the function.

Document restrictions

Code limited to 0 <= p < bit_width and 0 <= n < bit_width.  Since code cannot handle all possible input values, stating  restrictions lessens incorrect usage.

Defining behavior for all possible inputs has the advantage of not needing to list limitations, yet may not be functional needed (and code over-kill).

Still good to post limitations.

On this last point, perhaps a slight code re-write is in order as one could easily expect n == bit_width to be valid.

// This assumes no padding in `unsigned`
#define UINT_BIT_WIDTH (CHAR_BIT * sizeof(unsigned))

unsigned setbits(unsigned x, int p, int n, unsigned y) {
  unsigned new_mask = n >= UINT_BIT_WIDTH ? UINT_MAX : ~(~0u << n);
  ...
}


Length of a C string, capped to some maximum If I have a oneliner in C such as

size_t maxSizeOf(const char *s)
{
     return strlen(s) > M_MAX_SIZE ? M_MAX_SIZE : strlen(s);
}

would it be better to have a macro defined such that

#define MAX_SIZE_OF(s) (strlen(s) > M_MAX_SIZE ? M_MAX_SIZE : strlen(s))

or would it be better to have

size_t maxSizeOf(const char *s)
{
     size_t len = strlen(s);
     return len > M_MAX_SIZE ? M_MAX_SIZE : len;
}

Also, is there a way to turn the second function into a macro for maximum efficiency?

All three versions can be improved.

The first function has the problem that strlen(s) is likely to be called twice.  strlen() is an expensive operation, as it walks along the entire string until it finds the NUL terminator.

The second version (the macro) has the same issue, with the additional caveat that it breaks if s is an expression with a side-effect, such as an assignment.  For example, MAX_SIZE_OF(str++) would expand to

(strlen(str++) > M_MAX_SIZE ? M_MAX_SIZE : strlen(str++))

and possibly lead to str being incremented twice.  (Even if you decide that the ALL_CAPS_NAME alerts programmers to the fact it is a macro and thus susceptible to this kind of thing, it's still a trap that is better avoided altogether.)

The last version is better, in that it avoids both the second call to strlen() and the double-argument-expansion problem.  However, I still have two objections.

First, I think that you would be better off decomposing the problem into two parts, like MIN(M_MAX_SIZE, strlen(s)).  The MIN() macro, although not in the C standard, is frequently reimplemented, and widely understood.  On the other hand, it is less obvious what your maxSizeOf() is supposed to do.  I'm not sure that it's even worth the brain power to keep track of the existence of this function.

Second, if the string that is passed to the function is much longer than M_MAX_SIZE, it still needs to walk all the way to the end of the string — even if M_MAX_SIZE is 5 and the string is a gigabyte long.  To avoid that problem, I think that you may be better off walking down s yourself.  And if you do hunt for the NUL terminator yourself, then it is suddenly worthwhile to have this function again.

More importantly, worrying about the efficiency of the function compared to the macro is a false optimization.  The operation that could potentially take a long time and scale poorly is strlen().  That's the performance problem that would bite you — not the function-vs.-macro stuff that you are worrying about.


Split a String by an indexes array Let's say I have a plain text string that I want to split into a string array, using an integer array containing the indexes of this string where I want it to be cut.

Example:

Input string: "001ABCD2T"

Indexes array: {3, 7, 8}

Expected resulting string array: {"001", "ABCD", "2", "T"}

Here's how I do it for now:

string line = "001ABCD2T";
int[] markers = new int[] {3, 7, 8};

for (int i = -1; i < markers.Length; i++)
{
    string value = String.Empty;
    if (i == -1)
        value = line.Substring(0, markers[0]);
    else if (i == markers.Length - 1)
        value = line.Substring(markers[i], line.Length - markers[i]);
    else
        value = line.Substring(markers[i], markers[i + 1] - markers[i]);

    Console.WriteLine(value);
}

The result is as expected, yet it is pretty eye-soring. Is there a cleaner way to write it, or even better, an already implemented method I didn't find?

Disclaimer: I'm not actually very familiar with C#. I'll also refrain from the style review and let someone more familiar with common practices take a crack.

You can prepend 0 and append line.Length to your array, simplifying the logic. There's probably a better way, but I got it working with a List:

List<int> list = new List<int>();
list.Add(0);
list.AddRange(markers);
list.Add(line.Length);

for (int i = 0; i < list.Count - 1; i++)
{
    string value = line.Substring(list[i], list[i + 1] - list[i]);
    Console.WriteLine(value);
}


Finding missing void keywords in C source code I'm writing a bash script to search C source files and find the lines where I missed the void keyword (unlike in C++, in C, empty parameter lists have to be indicated with the void keyword).

I read that I shouldn't use for but while: https://mywiki.wooledge.org/DontReadLinesWithFor

I find that for has an easier to follow, top-down structure (first I write the command that produces the items to be iterated, then I write what to do with each).

I understand that if a filename contained a newline it would cause trouble in case of for; however, with IFS=$'\n'; set -f I would think all other problems are solved.

My code using for:

#!/bin/bash

returnTypeOpt_BRE='\([a-zA-Z_][a-zA-Z0-9_]\{0,\}[*  ]\{1,\}\)\{0,1\}'
spaceOpt_BRE=$'[ \t\r]\\{0,\\}'
functionName_BRE='\([a-zA-Z_][a-zA-Z0-9_]\{0,\}\)'
declarationMissingVoid_BRE="^$returnTypeOpt_BRE$spaceOpt_BRE$functionName_BRE$spaceOpt_BRE($spaceOpt_BRE)$spaceOpt_BRE;$spaceOpt_BRE\$"

targetFolder_Path="$1"
if [ $# -eq 0 ]; then
    targetFolder_Path="."
fi

IFS=$'\n'; set -f; for file in $( find "$targetFolder_Path" -name '*.c' -or -name '*.h' ); do
    printf "%s\n" "$file"
    for line in $( grep --basic-regex --regexp=$declarationMissingVoid_BRE $file ); do
        printf "\t>%s\n" "$line"
    done
done; unset IFS; set +f

My code using while:

#!/bin/bash

returnTypeOpt_BRE='\([a-zA-Z_][a-zA-Z0-9_]\{0,\}[*  ]\{1,\}\)\{0,1\}'
spaceOpt_BRE=$'[ \t\r]\\{0,\\}'
functionName_BRE='\([a-zA-Z_][a-zA-Z0-9_]\{0,\}\)'
declarationMissingVoid_BRE="^$returnTypeOpt_BRE$spaceOpt_BRE$functionName_BRE$spaceOpt_BRE($spaceOpt_BRE)$spaceOpt_BRE;$spaceOpt_BRE\$"

targetFolder_Path="$1"
if [ $# -eq 0 ]; then
    targetFolder_Path="."
fi

while IFS=; read -u 3 -r -d $'\0' file; do
    printf "%s\n" "$file"
    while IFS=; read -u 4 -r line; do
        printf "\t>%s\n" "$line"
    done 4< <( grep --basic-regex --regexp=$declarationMissingVoid_BRE $file )
done 3< <( find "$targetFolder_Path" -name '*.c' -print0 -or -name '*.h' -print0 )

So, for me both scripts work correctly, and produce the same result.

Beyond the "filenames with newlines" issue is there anything else that would make while better? Can you show me a test case?

If I had to solve similar but different (possibly more complex) tasks would I have problems with for?

I'm writing a bash script to search C source files and find the lines where I missed the void keyword

This alone suggests that your approach is very problematic. To do this properly you'd effectively need to write most of a C parser, and that isn't a good use of your time. There are other approaches that would get you more mileage and accuracy:

Double-check that your compiler (gcc?) for sure doesn't have warnings that cover your use-case; if it does, then simply compile and look at the output.
Failing that, reuse a parser. I recommend the Clang AST.

Bash is just the hideously wrong tool for this. Save yourself the pain of writing regexes that are both complex and likely missing many edge cases.


I have an encrypted file, how to remove a specific string from that file I got a file with lots of ASCII characters. No extension, and has the Type Of file attribute 190000 File.

I have many characters like L¿ö  in the file however I required to remove lines from a word PYGR to another word MCG. It has several lines in between.

I tried the following code. And got everything working. Indexes are 3 and 19001 respectively,

VB:

Dim Load_File = System.IO.File.ReadAllText("C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000")
        Dim EditedString = ""
        Dim IndexofPYGR = Load_File.IndexOf("PYGR")
        Dim indexOfMCG = Load_File.LastIndexOf("MCG")
        Dim LengthMCG = "MCG"

        'System.Console.ReadKey()
        If ((IndexofPYGR > -1) And (indexOfMCG > -1)) Then
            EditedString = Load_File.Remove(IndexofPYGR, (indexOfMCG - (LengthMCG.Length + 1)))
            System.IO.File.WriteAllText("C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000V1", EditedString)
        End If

c#

//Getting the file as a string.
            string Load_File = System.IO.File.ReadAllText(@"C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000");

            string EditedString;
            int IndexofPYGR = Load_File.IndexOf("PYGR");
            int indexOfMCG  = Load_File.IndexOf("MCG");
            string LengthMCG = "MCG";
            //Console.WriteLine("Index of PYGR is : {0}", IndexofPYGR);
            //Console.WriteLine("Index of MCG is : {0}", indexOfMCG);
            //System.Console.ReadKey();
            if ((IndexofPYGR != -1) && (indexOfMCG != -1))
            {
                EditedString = Load_File.Remove(IndexofPYGR, (indexOfMCG - (LengthMCG.Length + 1)));

                System.IO.File.WriteAllText(@"C:\Documents and Settings\Fousu.s\Desktop\Files\SYSI091512.190000V1", EditedString);

Would like to request a review of the above code, or is there a better way to achieve this? and also would like to know how to get the Type Of File attribute to 190000.

Some clarifications

I got a file with lots of ASCII characters. [...] I have many characters like L¿ö in the file [...]

As others have pointed out, these are not ASCII characters.

No extension, and has the Type Of file attribute 190000 File.

Configure Windows Explorer to show file extensions.

I required to remove lines from a word PYGR to another word MCG. It has several lines in between.

Your code tells us otherwise. You are not removing the lines from PYGR to MCG; your code removes the word PYGR and every character that follows up to one character before MCG. I assume that is not quite what you were trying to achieve?
If you update your question to explain if you are trying to remove everything between (excluding) PYGR and MCG, including one of them or including both of them, I'll gladly adapt my answer. For now, I will assume the latter.
Edit: All right, in accordance with your comment I have changed my answer to replace all text from PYGR (inclusive) to MCG (exclusive).
Reviewing your code
Readability

Please use the well-established C# naming conventions: local variables are camelCase
Format your code with the correct indentation to make it more readable
Start your file with using System.IO to avoid repeating yourself
Get rid of unnecessary parenthesis
Choose proper names
Join declaration and usage of variables where possible

For example:
string EditedString;
// ...
EditedString = Load_File.Remove(IndexofPYGR, (indexOfMCG - (LengthMCG.Length + 1)));

should be more like
string editedContent = fileContent.Remove(startIndex, endIndex - end.Length);

Maintainability
Hard-coding the paths is something that should be avoided even during development. You can use Environment.GetFolderPath with Environment.SpecialFolder.Desktop to retrieve the path to the current user's desktop and add the file name with Path.Combine.
Regex
I really don't like the heave use of indexOf: your code seems to be all about how the replacement is done, instead of what is going on (i.e. it is imperative, not declarative).
Using a Regex, we can arrive at a more declarative style. You may need to adjust this to suit your needs (your question is ambiguous).

Refactored
string desktop = Environment.GetFolderPath(Environment.SpecialFolder.Desktop);
string inFilePath = Path.Combine(desktop, "SYSI091512.190000");
string outFilePath = Path.Combine(desktop, "SYSI091512.190000V1");

// you would probably pass the paths into your method as parameters

var fileContent = File.ReadAllText(inFilePath);
var result = Regex.Replace(fileContent, "PYGR.*MCG", "MCG", RegexOptions.Singleline);
File.WriteAllText(outFilePath, result);


Checking two text field for match and checking for null Is this the most efficient way to do this?  Any suggestions on some thing I might have missed or can do better?

    func checkPasscodeMatch(){
    println(self.passCodetext1.text)
    println(self.passCodetext2.text)
    // check if pass codes match
    if self.passCodetext1.text == self.passCodetext2.text {
    //check if pass code 1 is blank
        if self.passCodetext1.text == "" {
            let alertView = UIAlertController(title: "ALERT!!", message: "Pass code can not be blank", preferredStyle: UIAlertControllerStyle.Alert)
            alertView.addAction(UIAlertAction(title: "OK", style: UIAlertActionStyle.Default, handler: nil))

            self.presentViewController(alertView, animated: true, completion: nil)
        } else {
       defaults.setObject(passCodetext1.text, forKey: "passCode")//store pass code
        dispatch_async(dispatch_get_main_queue()) {
            self.performSegueWithIdentifier("setupTouchID", sender: self)
            }
        }
    } else {
        let alertView = UIAlertController(title: "ALERT!!", message: "Pass Codes do not match", preferredStyle: UIAlertControllerStyle.Alert)
        alertView.addAction(UIAlertAction(title: "OK", style: UIAlertActionStyle.Default, handler: nil))

        self.presentViewController(alertView, animated: true, completion: nil)
        passCodetext1.text = ""
        passCodetext2.text = ""
        passCodetext2.resignFirstResponder()
        println("Pass codes do not match.")
    }
}

Your indentation is a bit wonky.  Why is the first line indented?  Why isn't the stuff in the inner-else indented correctly?

There's no reason to println() our passcodes.  At the very least, we should be certain to wrap these in #if DEBUG so that they're taken out of the release build of our project.

I don't understand why we're dispatch_async-ing to the main thread.  Is this code not already being executed on the main thread?  If this isn't on the main thread, then okay... but now I have to ask how come the presentViewController calls aren't also being dispatched to the main thread?  Seems like dispatching to the main thread is unnecessary.

As for efficiency, I'm not sure there's a way to make your code execute any faster, however, there are some improvements we can make.

First of all, rather than checking if our string is equal to "", we should check its length.  If a text field has never had its text property set or become first responder, it's text property can actually be nil.  "" and nil are different.

But perhaps more importantly, our conditions are structure in such a way that it makes it difficult for the programmer to sort through everything that's going on.

Let's start by handling all of our failure conditions.

var errorDescription: String?

if !(count(self.passcodeText1.text) > 0) {
    errorDescription = "Pass code can not be blank."
} else if self.passcodeText1.text != self.passcodeText2.text {
    errorDescription = "Pass codes do not match."
}

if let message = errorDescription {
    // handle error condition
} else {
    // proceed
}

Now we've simplified things a bit, eh?

Finally, let's wait until the user dismisses the alert controller away before we change anything on the current controller.  This will make it more clear to the user what area is wrong.  And instead of just resigning first responder, let's make the first text field the first responder so it's ready for the user to start fixing the error.  Ready?

So, filling in the comments from the above snippet:

if let message = errorDescription {
    let action = UIAlertAction(title:"OK", stlye:.Default) {
        _ in
        passCodetext1.text = ""
        passCodetext2.text = ""
        passCodetext1.becomeFirstResponder()
    }

    let alertView = UIAlertController(title:"ALERT!!", message:message, preferredStyle:.Alert)
    alertView.addAction(UIAlertAction(title:"OK", style:.Default, handler: action))

    self.presentViewController(alertView, animated:true, completion:nil)
} else {
    self.performSegueWithIdentifier("setupTouchID", sender: self)
}

And as a final note, I'd recommend declaring all of your strings as constants in some file somewhere in your app where you store all of these.  Or perhaps better yet, make a .plist file in your project to store all these in.


Checking if command line input is an integer The code below accepts 1 command line argument from the user. The expected input is an integer. Hence, the code checks if the input is purely an integer. Any feedback is appreciated.

atoi() does not provide error-checking, thus the idea is to verify the input before passing the input to atoi().

I considered using strtol(), but it looks way too complex for a simple task.

stoi() considers 1a as valid, which is not valid in my use case.

//abc   ->  Invalid
//1a    ->  Invalid
//a1    ->  Invalid
//1     ->  Valid

#include <iostream>
#include <cstdlib>
#include <string>
#include <cctype>

using namespace std;

int correctInput;
string userInputStr;

int main(int argc, char* argv[])
{
    userInputStr = argv[1];
    for ( int x = 0; x < userInputStr.length(); x++)
    {
        if ( isalpha(userInputStr[x]) )
        {
            cout << "invalid input." << endl;
            return 0;
        }

    }

    correctInput = atoi(userInputStr.c_str());

    cout << correctInput << endl;

}

strtol will work just fine for this job:

char *end;
int correctInput = strtol(argv[1], &end, 10);

if (*end != '\0') {
    std::cout << "invalid input.\n";
    return 1;
}
std::cout << correctInput << "\n";

That said, let's do a quick review of your existing code.

Avoid using namespace std;

This is common in quite a few tutorials and such, largely of the sake of brevity. In any case, it's generally best avoided in real code.

Avoid global variables

Right now, you've defined correctInput and userInputString as global variables, even though they're only used inside of main. As a general rule of thumb, you want to restrict variables to the smallest scope necessary to do their job.

Prefer initialization to assignment

In this case, moving userInputString inside of main would have one more beneficial effect: it's better to initialize than to create an uninitialized variable (a string, in this case) and assigning a value to it later. I'd prefer to use:

std::string userInputString(argv[1]);

...or:

std::string userInputString{argv[1]};

This means that userInputString contains the correct value as soon as it exists.

Check argc before using arguments

You always want to check that argv[1] actually exists before trying to use it (and, by strong preference, print out a meaningful error message if it doesn't):

if (argc < 2) {
    std::cerr << "Usage: check <int>\n';
    return 1;
}

This tends to be more helpful than the default error message (e.g., Segmentation fault (core dumped)).

Use meaningful return values

You return 0 from main (indicating success) regardless of the input. You't typically want to indicate failure when the input was bad:

        cout << "invalid input." << endl;
        return EXIT_FAILURE;

Avoid std::endl

std::endl flushes the output stream, which is rarely needed, and tends to be undesirable. In this particular case it doesn't make much difference, but I'd get in the habit of using '\n' when you want to print a new-line, so you won't accidentally use std::endl when you're producing enough output for it to make a real difference.

Use standard algorithms where applicable

If you do decide to continue checking that the input is all digits, it's probably better to use std::all_of (or perhaps, std::any_of) instead of writing a loop to do the job:

std::string input(argv[1]);

if (std::any_of(input.begin(), input.end(), [](char ch) { return !isdigit(ch); }) {
    cout << "invalid input." << endl;
    return EXIT_FAILURE;
}


Python program to find the most frequent letter in a text Given a text like "Hello World!" the program outputs the most frequent letter, in this case the "l".

If there are two letters with the same frequency, it outputs the first in alphabetical order, for example in the text "One" it outputs "e".

In addition upper and lower letters are counted as the same letter.

How could be this code improved?

def most_frequent_letter(text: str) -> str:

    letters_dict = {}
    text = text.lower()
    text = re.sub("[^a-z]","",text)

    for letter in text:
        if letter in letters_dict:
            letters_dict[letter] += 1
        else:
            letters_dict[letter] = 1

    value_to_key = collections.defaultdict(list)
    for key_letter, frecuency_value in letters_dict.items():
        value_to_key[frecuency_value].append(key_letter)

    letters = list(value_to_key.values())
    frecuencies = list(value_to_key.keys())
    return (sorted(letters[frecuencies.index(max(frecuencies))])[0])

If you are going to use the collections module at all (as you did for collections.defaultdict), then why not use collections.Counter, which offers a most_common() method?


LeetCode 924: Minimize Malware Spread I'm posting my code for a LeetCode problem copied here. If you would like to review, please do so. Thank you for your time!
Problem

In a network of nodes, each node i is directly connected to another
node j if and only if graph[i][j] = 1.
Some nodes initial are initially infected by malware.  Whenever two
nodes are directly connected and at least one of those two nodes is
infected by malware, both nodes will be infected by malware.  This
spread of malware will continue until no more nodes can be infected in
this manner.
Suppose M(initial) is the final number of nodes infected with
malware in the entire network, after the spread of malware stops.
We will remove one node from the initial list.  Return the node that
if removed, would minimize M(initial).  If multiple nodes could be
removed to minimize M(initial), return such a node with the smallest
index.
Note that if a node was removed from the initial list of infected
nodes, it may still be infected later as a result of the malware
spread.

Inputs
[[1,1,0],[1,1,0],[0,0,1]]
[0,1]

[[1,0,0,0],[0,1,0,0],[0,0,1,1],[0,0,1,1]]
[3,1]

[[1,0,0],[0,1,0],[0,0,1]]
[0,2]

[[1,1,1],[1,1,1],[1,1,1]]
[1,2]

Outputs
0

3

0

1

Code
#include <vector>
#include <algorithm>

struct Solution {
    std::vector<int> parents;
    inline int minMalwareSpread(const std::vector<std::vector<int>> &graph, const std::vector<int> &initial) {
        size_t length = graph.size();

        for (size_t index = 0; index < length; index++) {
            parents.push_back(index);
        }

        for (size_t row = 0; row < length; row++) {
            for (size_t col = row + 1; col < length; col++) {
                if (graph[row][col]) {
                    union_uf(row, col);
                }
            }
        }

        std::vector<int> areas(length, 0);
        std::vector<int> malwares(length, 0);

        for (size_t area = 0; area < length; area++) {
            areas[find(area)]++;
        }

        for (size_t init : initial) {
            malwares[find(init)]++;
        }

        std::vector<int> min_spread = {1, 0};

        for (int init : initial) {
            min_spread = std::min(min_spread, {(malwares[find(init)] == 1) * (-areas[find(init)]), init});
        }

        return min_spread[1];
    }

private:
    const inline size_t find(const size_t x) {
        if (x != parents[x]) {
            parents[x] = find(parents[x]);
        }

        return parents[x];
    }

    const inline void union_uf(const size_t x, const size_t y) {
        parents[find(x)] = find(y);
    }
};

Algorithm
You can read about various algorithms here in this link:

We'd first map the graph using Disjoint Set Union (DSU).
We'd count all areas as well as all infected sets.
If there'd be only one malware set, we would return the largest. Otherwise, we'd return the set with lowest index using the last loop.

References

Problem

Solution

Discuss

Union Find

Function prefix qualifiers
I seem to remember this being covered in your reviews before, but:
const inline size_t find(const size_t x)

This benefits from neither of the const declarations nor the inline. Both the return value and the argument are passed-by-value scalars and do not benefit from const. inline, for all intents and purposes, has no effect, particularly where a compiler is told to optimize your code - and even if it did have an effect, it's a bad idea to tell the optimizer what it should and shouldn't inline. It will understand what's good for your code better than the programmer does 99% of the time.
If these concepts are confusing or difficult to apply to code on a practical basis, that is understandable - feel free to ask follow-up questions.


Streamlining series of try-except + if-statements for faster loop processing in Python I'm processing strings using regexes in a bunch of files in a directory. To each line in a file, I apply a series of try-statements to match a pattern and if they do, then I transform the input. After I have analyzed each line, I write it to a new file. I have a lot of these try-else followed by if-statements (I only included two here as an illustration). My issue here is that after processing a few files, the script slows down so much that it almost stalls the process completely. I don't know what in my code is causing the slowing down but I have a feeling it is the combination of try-else + if-statements. How can I streamline the transformations so that the data is processed at a reasonable speed?

Or is it that I need a more efficient iterator that does not tax memory to the same extent?

Any feedback would be much appreciated!

import re
import glob

fileCounter = 0

for infile in glob.iglob(r'\input-files\*.txt'):

    fileCounter += 1
    outfile = r'\output-files\output_%s.txt' % fileCounter

    with open(infile, "rb") as inList, open(outfile, "wb") as outlist:

        for inline in inlist:

            inword = inline.strip('\r\n')

            #apply some text transformations
            #Transformation #1
            try: result = re.match('^[AEIOUYaeiouy]([bcćdfghjklłmnńprsśtwzżź]|rz|sz|cz|dz|dż|dź|ch)[aąeęioóuy](.*\[=\].*)*', inword).group()
            except: result = None

            if result == inword:
                inword = re.sub('(?<=^[AEIOUYaeiouy])(?=([bcćdfghjklłmnńprsśtwzżź]|rz|sz|cz|dz|dż|dź|ch)[aąeęioóuy])', '[=]', wbWord)

            #Transformation #2 etc.
            try: result = re.match('(.*\[=\].*)*(\w?\w?)[AEIOUYaąeęioóuy]\[=\][ćsśz][ptkbdg][aąeęioóuyrfw](.*\[=\].*)*', inword).group()
            except: result = None

            if result == inword:
                inword =  re.sub('(?<=[AEIOUYaąeęioóuy])\[=\](?=[ćsśz][ptkbdg][aąeęioóuyrfw])', '', inword)
                inword =  re.sub('(?<=[AEIOUYaąeęioóuy][ćsśz])(?=[ptkbdg][aąeęioóuyrfw])', '[=]', inword)

            outline = inword + "\n"
            outlist.write(outline)

    print "Processed file number %s" % fileCounter
print "*** Processing completed ***"

It's not 100 % clear what exceptions you expect, but I presume you are trying to handle the case when the regex does not match. I suggest handling it this way:

    #Transformation #1
    match = re.match(pattern, inword)
    result = match.group() if match else None

"after processing a few files, the script slows down" Have you considered the possibility that a particular file, or even a particular line is slow to process? A possible explanation to that would be that regexes can suffer from catastrophic backtracking.


LZ-77 text compression algorithm This is my version of a LZ-77 lossless compression algorithm for text. It maintains a sliding window of 4095 characters and can pick up patterns up to 15 characters long. Basically, the compressed file is made of tuples (length, pos), with length on 4 bits and pos on 12 bits which makes 2 bytes each time. Besides, if I have to write a character that is not in the window, I set length=0, then I have 4 empty bits and the 8 bits left are used to encode the character.

I've spent a few hours on it correcting little bugs and it seems functional, although it appears not to work entirely for big files (size > window) but I'll correct this later. Anyway I'm not here for algorithm but for code design so tell me what's good or bad about this code, don't hesitate.

Also, I haven't implemented any file-manager system but the functions' names are quite explicit on how to use them.

#include <iostream>
#include <string>
#include <tuple>
#include <bitset>
#include <sstream>
#include <fstream>

using namespace std;

/**
Given two strings s1 and s2, find the longest prefix of s2 that is in s1
and returns the position of the prefix' first character in s1 and its length in a pair

If s1 = "lossless compression in opposition to " and s2 = "lossy compression", the longest prefix is "loss"
and the function returns (0, 4).
*/
pair<int, int> longest_prefix(string s1, string s2)
{
    int pos {0};
    int length {0};

    for(int i = 0; i < s1.size(); i++)
    {
        int l {0};
        while(i+l < s1.size() && l < s2.size() && s1[i+l] == s2[l])
            l++;

        if(l > length)
        {
            pos = i;
            length = l;
        }

        if(i == s1.size() - length || length == s2.size())
            return pair<int,int>(pos, length);
    }

    return pair<int,int>(pos, length);
}

/**
we have a sliding window of fixed size and instead of rewriting text that already is in the window,
we just refer to it with its position in the window and its size
*/
string lz77_compress(string s)
{
    string compressed { "" };
    int window_size { 4095 };
    int pattern_size { 15 };
    int i { 0 };

    while(i < s.size())
    {
        string s1 { i >= window_size ? s.substr(i - window_size, window_size) : s.substr(0, i) };
        string s2 { s.substr(i, min(pattern_size, (int)s.size() - i)) };
        pair<int,int> match = longest_prefix(s1, s2); // fst = pos; snd = length

        if(match.second == 0) // length = 0: no match, we just add a char (0, char)
        {
            compressed += (char)0;
            compressed += s[i];

            i++;
        }
        else // match, then we add (pos, length)
        {
            bitset<4> length(match.second);
            bitset<12> pos(match.first);
            bitset<16> concat(length.to_string() + pos.to_string()); // change a 4-bit and a 12-bit number into two 8-bits numbers
            bitset<8> bit1(concat.to_string().substr(0,8));
            bitset<8> bit2(concat.to_string().substr(8,8));
            compressed += (char)bit1.to_ulong();
            compressed += (char)bit2.to_ulong();

            i += match.second;
        }
   }

    return compressed;
}

string lz77_decompress(string compressed)
{
    string s { "" };
    int window_size { 4095 };

    for(int i = 0; i < compressed.size(); i += 2)
    {
        bitset<8> bit1(compressed[i]);
        bitset<8> bit2(compressed[i+1]);
        bitset<16> concat(bit1.to_string() + bit2.to_string());
        bitset<4> length_b(concat.to_string().substr(0,4));
        int length = length_b.to_ulong();

        if(length == 0)
        {
            bitset<8> c(concat.to_string().substr(8, 8));
            s += (char)c.to_ulong();
        }
        else
        {
            bitset<12> pos(concat.to_string().substr(4, 12));

            s += s.substr(max(0, i - window_size) + pos.to_ulong(), length);
        }
   }

    return s;
}

Bug

The bug you mentioned about not handled files larger than 4K is here:

        s += s.substr(max(0, i - window_size) + pos.to_ulong(), length);

Here, i is the amount of compressed file you have read so far.  But you want to use the size of the decompressed file instead.  So that line should be changed to:

        s += s.substr(max(0, (int) s.size() - window_size) +
                pos.to_ulong(), length);

I tested this change against a large file and it fixed the problem.

Substrings are costly

In the main loop of lz_compress(), you set call s.substr() for every byte of the input file, and the substring can be as large as your window size (4KB).  Each time you do that, you are making a copy of a portion of the original string.  So for a 1 MB file, you end up copying 4 GB of data during the course of your compression loop.  You could do a lot better if you just passed the original string into your longest_prefix() function along with the start index and length of the substring you would have created.  That way you wouldn't need to make unneeded copies of your input string.

Simplification of bit packing/unpacking

It looks like you did something really complicated here with bitsets in the compress function:

        bitset<4> length(match.second);
        bitset<12> pos(match.first);
        bitset<16> concat(length.to_string() + pos.to_string()); // change a 4-bit and a 12-bit number into two 8-bits numbers
        bitset<8> bit1(concat.to_string().substr(0,8));
        bitset<8> bit2(concat.to_string().substr(8,8));

        i += match.second;

This could be simplified to:

        int pos    = match.first;
        int length = match.second;
        compressed += (char) ((length << 4) | (pos >> 8));
        compressed += (char) (pos & 0xff);

        i += length;

Similarly, the decompress function:

for(int i = 0; i < compressed.size(); i += 2)
{
    bitset<8> bit1(compressed[i]);
    bitset<8> bit2(compressed[i+1]);
    bitset<16> concat(bit1.to_string() + bit2.to_string());
    bitset<4> length_b(concat.to_string().substr(0,4));
    int length = length_b.to_ulong();

    if(length == 0)
    {
        bitset<8> c(concat.to_string().substr(8, 8));
        s += (char)c.to_ulong();
    }
    else
    {
        bitset<12> pos(concat.to_string().substr(4, 12));

        s += s.substr(max(0, (int) s.size() - window_size) +
                pos.to_ulong(), length);
    }
 }

could be simplified to:

for(int i = 0; i < compressed.size(); i += 2)
{
    unsigned char byte1 = compressed[i];
    unsigned char byte2 = compressed[i+1];
    int length = (byte1 >> 4);

    if(length == 0)
    {
        s += byte2;
    }
    else
    {
        int pos = ((byte1 & 0xf) << 8) | byte2;

        s += s.substr(max(0, (int) s.size() - window_size) + pos, length);
    }

}

Speed of compression

One of the slowest parts of your program is the longest_prefix() function.  This function searches a 4KB window for the longest match to a 16 byte pattern.  Your brute force search takes \$O(n*m)\$ time, where \$n\$ = 4KB and \$m\$ = 16.  To achieve faster compression speeds, you will need to use a faster search algorithm.  Some alternatives are: Knuth-Morris-Pratt searching or hash tables.  I found this webpage that discusses some of the various alternatives with sample implementations, if you are interested in improving your search algorithm.


Determining if the kangaroos will land in the same position Problem

  There are two kangaroos on an x-axis ready to jump in the positive
  direction (i.e, toward positive infinity). The first kangaroo starts
  at location x1 and moves at a rate of v1 meters per jump. The second
  kangaroo starts at location x2 and moves at a rate of v2 meters per
  jump. Given the starting locations and movement rates for each
  kangaroo, can you determine if they'll ever land at the same location
  at the same time?

My Solution

#include <iostream>
using namespace std;

int main(){
    int x1, v1, x2, v2;
    cin >> x1 >> v1 >> x2 >> v2;

    // If one kangaroo is behind the other AND moving slower,
    //    he/she will never catch up to the other one
    if ((x1 < x2) && (v1 < v2)) cout << "NO";
    else if ((x2 < x1) && (v2 < v1)) cout << "NO";

    // Otherwise, move each kangaroo one jump at a time until
    //     the one behind is no longer behind.
    else {
        if (x1 < x2) {
            while (x1 < x2) {
                x1 += v1;
                x2 += v2;
            }
        } else {
            while (x2 < x1) {
                x1 += v1;
                x2 += v2;
            }
        }

        // Once he/she is no longer behind the other, check to see
        //    if he/she is in the same position, or if he/she has passed
        if (x1 == x2) cout << "YES";
        else cout << "NO";
    }
    return 0;
}

I am wondering if there is a way to optimize my code for this particular HackerRank problem. I have gotten all of the correct answers, and none of them timeout so for the problem it's 'good enough,' but I am curious if there is a better way of doing it.

Do I use too many conditionals, loops, etc? I have a bad habit of "do everything in for loops, if statements, etc."

Closed form solution

A better approach is to calculate the intercept point directly and then check whether or not it is an integer. The time of intercept is:

$$t = \frac{x_2 - x_1}{v_1 - v_2}$$

where both numerator and denominator are integers, but their ratio may not. We can also handle all special cases from the ratio.

bool kangaroos_meet(int x1, int v1, int x2, int v2) {
    int numerator = x2 - x1;
    int denominator = v1 - v2;
    if (denominator == 0)                // same velocity
    {
        return numerator == 0;           // they meet always or never
    }
    if (numerator % denominator != 0)    // intercept point not an integer
    {
        return false;
    }
    int t = numerator / denominator;     // calculate intercept point
    return t >= 0;                       // intercept point lies in the past
}


Reading from text file with RegexMatch Below is the method that I have written for reading from a text file.  While reading, I need to match a line string to a given regex, and if it matches, I need to add the line string to a collection.

private static void GetOrigionalRGBColours(string txtFile)
{
    string tempLineValue;
    Regex regex = new Regex(@"^\d+.?\d* \d+.?\d* \d+.?\d* SRGB$");

    using (StreamReader inputReader = new StreamReader(txtFile))
    {
        while (null != (tempLineValue = inputReader.ReadLine()))
        {
            if (regex.Match(tempLineValue).Success
                && tempLineValue != "1 1 1 SRGB"
                && tempLineValue != "0 0 0 SRGB")
            {
                string[] rgbArray = tempLineValue.Split(' ');
                RGBColour rgbColour = new RGBColour() { Red = Convert.ToDecimal(rgbArray[0]), Green = Convert.ToDecimal(rgbArray[1]), Blue = Convert.ToDecimal(rgbArray[2]) };
                originalColourList.Add(rgbColour);
            }
        }
    }
}

When this method is run for a text file of 4MB having 28653 lines, it takes around 3 minutes just to finish the above method. Also, as a result of the above run, originalColourList is populated with 582 items.

How can I improve the performance of this method?  I had truncated the original text file for testing purpose.  The actually text file size may go up to 60MB.

One option is to not perform any regular expression check at all. You can perform a quick match by checking if the line ends with SRGB and the try the split and convert and simply continue if it fails. Something along these lines:

private static void GetOrigionalRGBColours(string txtFile)
{
    using (StreamReader inputReader = new StreamReader(txtFile))
    {
        while (!inputReader.EndOfStream)
        {
            var line = inputReader.ReadLine();

            if (line.EndsWith(" SRGB")
                && line != "1 1 1 SRGB"
                && line != "0 0 0 SRGB")
            {
                try
                {
                    string[] rgbArray = line.Split(new char[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
                    if (rgbArray.Length == 4)
                    {
                        RGBColour rgbColour = new RGBColour() { Red = Convert.ToDecimal(rgbArray[0]), Green = Convert.ToDecimal(rgbArray[1]), Blue = Convert.ToDecimal(rgbArray[2]) };
                        originalColourList.Add(rgbColour);
                    }
                }
                catch (FormatException) {}
                catch (OverflowException) {}
            }
        }
    }
}

Alternatively to the try-catch you can use decimal.TryParse() and ignore the line if it returns false for any of the 3 entries.

Update: Version with decimal.TryParse.

private static void GetOrigionalRGBColours(string txtFile)
{
    using (StreamReader inputReader = new StreamReader(txtFile))
    {
        while (!inputReader.EndOfStream)
        {
            var line = inputReader.ReadLine();

            if (line.EndsWith(" SRGB")
                && line != "1 1 1 SRGB"
                && line != "0 0 0 SRGB")
            {
                string[] rgbArray = line.Split(new char[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
                if (rgbArray.Length != 4) { continue; }

                decimal red;
                var hasRed = decimal.TryParse(rgbArray[0], out red);
                decimal green;
                var hasGreen = decimal.TryParse(rgbArray[1], out green);
                decimal blue;
                var hasBlue = decimal.TryParse(rgbArray[2], out blue);

                if (hasRed && hasGreen && hasBlue)
                {
                    RGBColour rgbColour = new RGBColour() { Red = red, Green = green, Blue = blue };
                    originalColourList.Add(rgbColour);
                }
            }
        }
    }
}

I like that one better because I find try {} catch {} block always a bit ugly . It also conveys the intend of the code better.


Describe how you could use a single array to implement three stacks I'm preparing for an interview so I'm trying to solve some problems to stretch my mind. Here is one:

  Describe how you could use a single array to implement three stacks.

And here is my implementation:

class StackUnderflowError extends RuntimeException {
    public StackUnderflowError(String msg) {
        super(msg);
    }
}

class ThreeStacks<T>{

private T[] array;
private int firstIndex = 0;
private int secondIndex = 1;
private int thirdIndex = 2;

public ThreeStacks(Class<T> classT, int capacity) {
    array = (T[])Array.newInstance(classT, capacity);
}

void push(T data, int stack){ //enum to select the stack
    switch(stack)
    {
        case 1: pushOne(data);
            break;
        case 2: pushTwo(data);
            break;
        case 3: pushThree(data);
            break;
        default: throw new IllegalArgumentException("stack");
    }
}

T pop(int stack){
    switch(stack){
        case 1: return popOne();
        case 2: return popTwo();
        case 3: return popThree();
        default: throw new IllegalArgumentException("stack");
    }
}

private void pushOne(T data) {
    if (firstIndex > array.length - 1) {
        throw new StackOverflowError("firstStack");
    } else{
        array[firstIndex] = data;
        firstIndex += 3;
    }
}

private void pushTwo(T data) {
    if (secondIndex > array.length - 1) {
        throw new StackOverflowError("secondStack");
    } else{
        array[secondIndex] = data;
        secondIndex += 3;
    }
}

private void pushThree(T data) {
    if (thirdIndex > array.length - 1) {
        throw new StackOverflowError("thirdStack");
    } else{
        array[thirdIndex] = data;
        thirdIndex += 3;
    }
}

private T popOne() {
    firstIndex -= 3;
    if(firstIndex < 0){
        throw new StackUnderflowError("fristStack");
    }
    return array[firstIndex];
}

private T popTwo() {
    secondIndex -= 3;
    if(secondIndex < 0){
        throw new StackUnderflowError("fristStack");
    }
    return array[secondIndex];
}

private T popThree() {
    thirdIndex -= 3;
    if(thirdIndex < 0){
        throw new StackUnderflowError("fristStack");
    }
    return array[thirdIndex];
}

}

And test:

    ThreeStacks<String> threeStacks = new ThreeStacks<>(String.class, 1024);
    threeStacks.push("1", 1);
    threeStacks.push("2", 2);
    threeStacks.push("3", 3);
    threeStacks.push("4", 1);
    threeStacks.push("5", 2);
    threeStacks.push("6", 3);

    System.out.println( threeStacks.pop(1) );
    System.out.println( threeStacks.pop(2) );
    System.out.println( threeStacks.pop(2) );
    System.out.println( threeStacks.pop(1) );
    //System.out.println( threeStacks.pop(1) );

How can I improve this? A few thoughts:

use array instead of switch (stacks' indexes)
use enum to choose stack
use ArrayList (if interviewer agrees)

Solution that I found

In this approach, any stack can grow as long as there is any free space in the array.
We sequentially allocate space to the stacks and we link new blocks to the previous block. This means any new element in a stack keeps a pointer to the previous top element of that particular stack.

In this implementation, we face a problem of unused space. For example, if a stack deletes some of its elements, the deleted elements may not necessarily appear at the end of the array. So, in that case, we would not be able to use those newly freed spaces.
To overcome this deficiency, we can maintain a free list and the whole array space would be given initially to the free list. For every insertion, we would delete an entry from the free list. In case of deletion, we would simply add the index of the free cell to the free list.

In this implementation we would be able to have flexibility in terms of variable space utilization but we would need to increase the space complexity.

 int stackSize = 300;
 int indexUsed = 0;
 int[] stackPointer = {-1,-1,-1};
 StackNode[] buffer = new StackNode[stackSize * 3];
 void push(int stackNum, int value) {
    int lastIndex = stackPointer[stackNum];
    stackPointer[stackNum] = indexUsed;
    indexUsed++;
    buffer[stackPointer[stackNum]]=new StackNode(lastIndex,value);
 }

int pop(int stackNum) {
    int value = buffer[stackPointer[stackNum]].value;
    int lastIndex = stackPointer[stackNum];
    stackPointer[stackNum] = buffer[stackPointer[stackNum]].previous;
    buffer[lastIndex] = null;
    indexUsed--;
    return value;
 }
 int peek(int stack) { return buffer[stackPointer[stack]].value; }
 boolean isEmpty(int stackNum) { return stackPointer[stackNum] == -1; }

class StackNode {
 public int previous;
 public int value;
 public StackNode(int p, int v){
    value = v;
    previous = p;
 }
}


Counting adjacent swaps to sort an array with 3 different values This is the Kindergarten Excursion problem from kattis.com:

  The kindergarten teachers had finally managed to get all the kids in a line for the walk to the bus station and the weekly excursion. What they hadn’t thought of was the fact that today different kids were supposed to go to different excursions. They should walk as one line to the bus station, but to avoid total chaos when they arrive there, all the kids going to the zoo should be in the beginning of the line, the ones going to the lake should be in the middle and the rest, going to the science museum, should be in the end.

  Since it takes a lot of time to get all the kids to stand in a line no kid may step out of the line. To get the line organized after excursion group, kids standing next to each other can swap places. Now the kindergarten teachers wonder, if they will have time to do this and still catch their bus.

  Task

  You will be given a sequence of numbers 0, 1, and 2, denoting the excursion destinations of each kid from first to last in the line. Pairs of adjacent kids can swap positions, and the line should be organized after destination number starting with 0 and ending with 2. What is the minimum number of swaps required to organize the line?

  Input

  The only line of input contains a string consisting of characters 0, 1 or 2, denoting the destinations of kids. The length of the string will be at most 1,000,000 characters.

  Output

  Output one line with one integer – the minimum number of swaps needed to get the kids in order.

  Sample Input 1

10210

  Sample Output 1

5

This is my code. My solution is too slow and I need help with optimization or a different approach for this problem.

def excursion(str):
    digits = [0, 0, 0]
    x = 0
    for i in range(len(str)):
        for j in range((int(str[i])) + 1, 3):
            x += digits[j]
        digits[int(str[i])] += 1
    return x

print(excursion(input()))

The code in the post runs in linear time, so we can't hope to improve the asymptotic complexity, but we might be able to make some piecewise optimizations. Let's start by making a random test case:

>>> import random
>>> CASE = ''.join(random.choice('012') for _ in range(10**6))

Timing the performance on this test case establishes a baseline for any improvements.

>>> from timeit import timeit
>>> timeit(lambda:excursion(CASE), number=1)
1.1623761800583452

The expression int(str[i])) appears twice, so let's remember it in a local variable:

def excursion2(str):
    digits = [0, 0, 0]
    result = 0
    for i in range(len(str)):
        digit = int(str[i])
        for j in range(digit + 1, 3):
            result += digits[j]
        digits[digit] += 1
    return result

This saves about 25% of the runtime:

>>> timeit(lambda:excursion2(CASE), number=1)
0.867930110078305

The code only uses the index i to look up the corresponding character of str. So instead of iterating over the indexes, iterate over the characters themselves:

def excursion3(str):
    digits = [0, 0, 0]
    result = 0
    for c in str:
        digit = int(c)
        for j in range(digit + 1, 3):
            result += digits[j]
        digits[digit] += 1
    return result

This saves about 3%:

>>> timeit(lambda:excursion3(CASE), number=1)
0.8365003759972751

We can save an assignment statement by using map to apply the function int to the characters of str:

def excursion4(str):
    digits = [0, 0, 0]
    result = 0
    for digit in map(int, str):
        for j in range(digit + 1, 3):
            result += digits[j]
        digits[digit] += 1
    return result

This saves about 4%:

>>> timeit(lambda:excursion4(CASE), number=1)
0.7819818791467696

We could cache the range objects, to avoid rebuilding them on each loop:

def excursion5(str):
    n = 3
    digits = [0] * n
    ranges = [range(i + 1, n) for i in range(n)]
    result = 0
    for digit in map(int, str):
        for j in ranges[digit]:
            result += digits[j]
        digits[digit] += 1
    return result

This saves about 25%:

>>> timeit(lambda:excursion5(CASE), number=1)
0.497486931970343

The cumulative impact of these changes reduced the overall runtime by about 60%. Is that good enough to pass the time limit?


Handling Parent-Child Relationship using MVP within a WinForms Application I recently asked on Programmers:

How should one handle a Parent-Child relationship using MVP?

Based on the comments received, I tried this:

IParentView

public interface IParentView { }

IChildView

public interface IChildView<V> where V : IParentView {
    void Show(V parentView);
}

IChildPresenter

public interface IChildPresenter<V> where V : IParentView {
    void ShowView(V parentView);
}

CustomerManagementPresenter

public class CustomerManagementPresenter : Presenter<ICustomerManagementView>
    , ICustomerManagementUiHandler
    , IChildViewPresenter<IMainView> {
    public CustomerManagementPresenter(ICustomerManagementView view
        , ICustomerManagementService service) : base(view) {
        view.Handler = this;
        if (service == null) throw new ArgumentNullException("service");
        customerManagementService = service;
    }

    public void LoadCustomers() {
        var customers = customerManagementService.GetCustomers();
        View.ViewModel.Customers = customers;
    }

    public void ShowView(IMainView parentView) { View.Show(parentView); }

    private readonly ICustomerManagementService customerManagementService;
}

IMainView

public interface IMainView : IView, IParentView, IHasUiHandler<IMainViewUiHandler> { }

MainPresenter

public class MainPresenter : Presenter<IMainView>, IMainViewUiHandler {
    public MainPresenter(IMainView view, ICustomerManagementPresenterFactory factory)
        : base(view) {
        view.Handler = this;
        if (factory == null) throw new ArgumentNullException("factory");
        this.customerManagementPresenterFactory = factory;
    }

    public void ManageCustomers() {
        var p = customerManagementPresenterFactory.Create();
        var parent = this.View;
        p.ShowView(parent);
    }
}

CustomerManagementForm

public partial class CustomerManagementForm : Form, IMainView {
    public CustomerManagementForm(CustomerManagementViewModel viewModel) {
        InitializeComponent();

        if (viewModel == null) throw new ArgumentNullException("viewModel");

        customerManagementViewModel = viewModel;
        customerManagementViewModel.PropertyChanged +=
            new PropertyChangedEventHandler(
                customerManagementViewModel_PropertyChanged);
    }

    public ICustomerManagementUiHandler Handler { get; set; }
    public string Message { get; set; }
    public string Title { get { return Text; } set { Text = value; } }
    public CustomerManagementViewModel ViewModel {
        get { return customerManagementViewModel; }
    }

    public void Show(IMainView parent) {
        MdiParent = (Form)parent;
        Show();
    }

    private void CustomerManagementForm_Load(object sender, EventArgs e) {
        Handler.LoadCustomers();
    }

    private void CustomerManagementViewModel_PropertyChanged(
        object sender
        , PropertyChangedEventArgs e) {
        customerManagementViewModelBindingSource.DataSource = ViewModel;
    }

    private readonly CustomerManagementViewModel customerManagementViewModel;
}

I believe I could reduce complexity or repetition with my IChildPresenter and IChildView which are pretty similar. Besides, The Show() methods belongs expressly to the views, whilst ShowView() methods actually belongs to presenters. I tend to follow some such basic nomenclature to make it more understandable.

So now I wonder, what do you guys think of this approach?

Did I miss the point?

Any possible imporvement?

I'm no expert in design patterns, but I fail to see the benefit in having a completely empty interface.

public interface IParentView { }

Interfaces do two things for us.

They enforce a contract. Anything implementing this interface must have this specific public api. Which allows for...
The ability to code against an expected api, not an implementation.

While you've done a good job of coding against IParentView instead of IChildView, what does the parent view get us? Nothing as far as I can tell. There's no real contract being made here, so there's no reason you couldn't have coded directly against IChildView. (Which, at that point, would not be a good name.)

But don't take just my word for it, 24 people on stack overflow agree that this pattern is a code smell.


Flex panels in CSS and JS I was following Wes Bos JS 30-day challenge, so HTML and CSS are mostly are copy-paste,
I'd like feedback on JS (mostly).
Thanks.

const panels = document.querySelectorAll('.panel');

panels.forEach(panel => panel.addEventListener('click', () => {
  const isOpen = panel.classList.contains('open');
  panels.forEach(panel => panel.classList.remove('open'));
  if(!isOpen) {
    panel.classList.add('open');
  }
}));

panels.forEach(panel => panel.addEventListener('transitionend', e => {
  if(e.propertyName.includes('flex')) {
    panels.forEach(panel => {
      if(panel.classList.contains('open')) {
        panel.classList.add('open-active');
      } else {
        panel.classList.remove('open-active');
      }
    });
  }
}));
html {
  box-sizing: border-box;
  background: #ffc600;
  font-family: 'helvetica neue';
  font-size: 20px;
  font-weight: 200;
}

body {
  margin: 0;
}

*,
*:before,
*:after {
  box-sizing: inherit;
}

.panels {
  min-height: 100vh;
  overflow: hidden;
  display: flex;
}

.panel {
  background: #6B0F9C;
  box-shadow: inset 0 0 0 5px rgba(255, 255, 255, 0.1);
  color: white;
  text-align: center;
  align-items: center;
  /* Safari transitionend event.propertyName === flex */
  /* Chrome + FF transitionend event.propertyName === flex-grow */
  transition:
    font-size 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11),
    flex 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11), background 0.2s;
  font-size: 20px;
  background-size: cover;
  background-position: center;

  flex: 1;
  display: flex;
  justify-content: center;
  flex-direction: column;
}

.panel1 {
  background-image: url(https://source.unsplash.com/gYl-UtwNg_I/1500x1500);
}

.panel2 {
  background-image: url(https://source.unsplash.com/rFKUFzjPYiQ/1500x1500);
}

.panel3 {
  background-image: url(https://images.unsplash.com/photo-1465188162913-8fb5709d6d57?ixlib=rb-0.3.5&q=80&fm=jpg&crop=faces&cs=tinysrgb&w=1500&h=1500&fit=crop&s=967e8a713a4e395260793fc8c802901d);
}

.panel4 {
  background-image: url(https://source.unsplash.com/ITjiVXcwVng/1500x1500);
}

.panel5 {
  background-image: url(https://source.unsplash.com/3MNzGlQM7qs/1500x1500);
}

/* Flex Children */
.panel>* {
  margin: 0;
  width: 100%;
  transition: transform 0.5s;
  flex: 1 0 auto;
  display: flex;
  align-items: center;
  justify-content: center;
}

.panel *:first-child {
  transform: translateY(-100%);
}

.panel *:last-child {
  transform: translateY(100%);
}

.panel.open-active *:first-child,
.panel.open-active *:last-child {
  transform: translateY(0);
}

.panel p {
  text-transform: uppercase;
  font-family: 'Amatic SC', cursive;
  text-shadow: 0 0 4px rgba(0, 0, 0, 0.72), 0 0 14px rgba(0, 0, 0, 0.45);
  font-size: 2em;
}

.panel p:nth-child(2) {
  font-size: 4em;
}

.panel.open {
  font-size: 40px;
  flex: 5;
}
<!DOCTYPE html>

  <meta charset="UTF-8">
  <title>Flex Panels </title>
  <link href="https://fonts.googleapis.com/css?family=Amatic+SC" rel="stylesheet" type="text/css">

Hey

Let's

Dance

Give

Take

Receive

Experience

It

Today

Give

All

You can

Life

In

Motion

It looks pretty good to me. I can only see a few things to consider:
More precise propertyName check You have if(e.propertyName.includes('flex')) { because Safari uses flex and others use flex-grow. Are you sure that the flex substring won't be present in any other possible CSS transitions? Even if you're sure, will readers of the code be sure? I'd change to an === test against both possibilities, or at least use startsWith (which is a bit more appropriate than .includes here, since both possibilities start with flex).
You can also move the comment about the transition event name to the JS as well as the CSS.
Concise classList setting When you want to either add a class name, or remove a class name, based on a condition, you can condense an if(...) classList.add(...) else(...) classList.remove into a single classList.toggle with a second argument that indicates whether to add or remove the class. Your
if(panel.classList.contains('open')) {
  panel.classList.add('open-active');
} else {
  panel.classList.remove('open-active');
}

simplifies to
const { classList } = panel;
classList.toggle('open-active', classList.contains('open'));

Browser compatibility Though, some ancient browsers don't support the 2nd argument, so consider what sort of browsers you need to support. If you only want to support reasonably up-to-date browsers, it's just fine. Another thing to keep in mind is that NodeList.prototype.forEach was only introduced a few years ago, around 2016 or 2017 IIRC; like startsWith, it's newer than ES6, so either use a polyfill or use iterators and Babel instead, eg:
for (const panel of panels) {
  // do stuff with panel

(if you want to support IE, you should be using Babel anyway, to transpile your code to ES5 syntax)
Void return? panels.forEach(panel => panel.addEventListener returns the value of calling addEventListener to the caller of forEach. Since forEach doesn't look at what its callbacks return, this doesn't do anything. It's not a real problem, but some might consider the code to make a bit more sense if the forEach callback returned void (no return statement or implicit return at all). (Described in TypeScript's TSLint here)
Clickable panels Since the panels are clickable, maybe change from the default cursor to cursor: pointer to make it more obvious to the user that they're meant to be clicked?
Space between elements in selectors I'd change .panel>* to .panel > * - it makes it a bit easier to read when separate elements are separated by spaces.
Repetitive panels Rather than

.panel1 {
  background-image: url(https://source.unsplash.com/gYl-UtwNg_I/1500x1500);
}

.panel2 {
  background-image: url(https://source.unsplash.com/rFKUFzjPYiQ/1500x1500);
}

Consider using :nth-child instead, allowing you to remove the extra panel# classes entirely.
.panel:nth-child(1) {
  background-image: url(https://source.unsplash.com/gYl-UtwNg_I/1500x1500);
}

.panel:nth-child(2) {
  background-image: url(https://source.unsplash.com/rFKUFzjPYiQ/1500x1500);
}

const panels = document.querySelectorAll('.panel');

panels.forEach((panel) => {
  panel.addEventListener('click', () => {
    const isOpen = panel.classList.contains('open');
    panels.forEach(panel => panel.classList.remove('open'));
    if (!isOpen) {
      panel.classList.add('open');
    }
  });
});

panels.forEach((panel) => {
  panel.addEventListener('transitionend', e => {
    /* Safari transitionend event.propertyName === flex */
    /* Chrome + FF transitionend event.propertyName === flex-grow */
    if (e.propertyName === 'flex' || e.propertyName === 'flex-grow') {
      panels.forEach(panel => {
        const { classList } = panel;
        classList.toggle('open-active', classList.contains('open'));
      });
    }
  })
});
html {
  box-sizing: border-box;
  background: #ffc600;
  font-family: 'helvetica neue';
  font-size: 20px;
  font-weight: 200;
}

body {
  margin: 0;
}

*,
*:before,
*:after {
  box-sizing: inherit;
}

.panels {
  min-height: 100vh;
  overflow: hidden;
  display: flex;
}

.panel {
  background: #6B0F9C;
  box-shadow: inset 0 0 0 5px rgba(255, 255, 255, 0.1);
  color: white;
  text-align: center;
  align-items: center;
  /* Safari transitionend event.propertyName === flex */
  /* Chrome + FF transitionend event.propertyName === flex-grow */
  transition:
    font-size 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11),
    flex 0.7s cubic-bezier(0.61, -0.19, 0.7, -0.11), background 0.2s;
  font-size: 20px;
  background-size: cover;
  background-position: center;

  flex: 1;
  display: flex;
  justify-content: center;
  flex-direction: column;

  cursor: pointer;
}

.panel:nth-child(1) {
  background-image: url(https://source.unsplash.com/gYl-UtwNg_I/1500x1500);
}

.panel:nth-child(2) {
  background-image: url(https://source.unsplash.com/rFKUFzjPYiQ/1500x1500);
}

.panel:nth-child(3) {
  background-image: url(https://images.unsplash.com/photo-1465188162913-8fb5709d6d57?ixlib=rb-0.3.5&q=80&fm=jpg&crop=faces&cs=tinysrgb&w=1500&h=1500&fit=crop&s=967e8a713a4e395260793fc8c802901d);
}

.panel:nth-child(4) {
  background-image: url(https://source.unsplash.com/ITjiVXcwVng/1500x1500);
}

.panel:nth-child(5) {
  background-image: url(https://source.unsplash.com/3MNzGlQM7qs/1500x1500);
}

/* Flex Children */
.panel > * {
  margin: 0;
  width: 100%;
  transition: transform 0.5s;
  flex: 1 0 auto;
  display: flex;
  align-items: center;
  justify-content: center;
}

.panel *:first-child {
  transform: translateY(-100%);
}

.panel *:last-child {
  transform: translateY(100%);
}

.panel.open-active *:first-child,
.panel.open-active *:last-child {
  transform: translateY(0);
}

.panel p {
  text-transform: uppercase;
  font-family: 'Amatic SC', cursive;
  text-shadow: 0 0 4px rgba(0, 0, 0, 0.72), 0 0 14px rgba(0, 0, 0, 0.45);
  font-size: 2em;
}

.panel p:nth-child(2) {
  font-size: 4em;
}

.panel.open {
  font-size: 40px;
  flex: 5;
}
<link href="https://fonts.googleapis.com/css?family=Amatic+SC" rel="stylesheet" type="text/css">

Hey

Let's

Dance

Give

Take

Receive

Experience

It

Today

Give

All

You can

Life

In

Motion


Backbone Marionette code improvement 2 How could this code be improved and have fewer repeated lines?

vent.on("createAccountLayout:rendered", function() {
    logger.info('createAccountLayout:rendered => CreateAccountLayoutController');
    showRegion(['phoneNumberRegion', 'keyboardRegion', 'nextRegion', 'quitRegion']);

    vent.trigger('createAccountLayout:setToNumericKeyboard');
});

vent.on('createAccountPhoneNumberView:nextStep', function() {
    closeRegion(['phoneNumberRegion', 'keyboardRegion', 'nextRegion']);
    showRegion(['nameRegion', 'keyboardRegion', 'nextRegion']);

    vent.trigger('createAccountLayout:setToAlphaNumericKeyboard');
    vent.trigger('createAccountLayout:disableNextButton');
});

Answerifying what Joseph the Dreamer commented on,

there is really not enough code to build a general routine out of, the code is fine as is. Though ideally you could add comments.


Pyg Latin Translator I have a code for a short Python 2 Pig Latin translator. Is it a good code, or should it be revised?

pyg = 'ay'
original = raw_input('Enter a word:')
# allows user input ^

if len(original) > 0 and original.isalpha():
word = original.lower()
first = word[0]
new_word = word + first + pyg
new_word = new_word[1:len(new_word)]
else:
print 'empty'

# prints the inputted word. If someone typed 'Hello', it will translate into Pig Latin.
print new_word

It works, but a shorter code may or may not be a good thing.

First, note how you're doing a if len(original) > 0 and original.isalpha(): check, then printing "empty" if it fails. This means though that "empty" will be printed out if you input a non-alphabetical word, which is confusing. I'd either handle those two checks separately, or print out a generic error message instead like "Invalid Input".

You have

new_word[1:len(new_word)]

To discard the first letter. There's actually a short-cut for this though:

new_word[1:]

If you omit the second argument to the slice operator, it defaults to the length of the collection. If you omit the first, it defaults to the beginning of the collection. Combined, that's why my_list[:] makes a shallow copy of my_list.

first = word[0]
new_word = word + first + pyg
new_word = new_word[1:]

This chunk breaks off the first letter, does some concatenation, then chops off the first character of the new string. Note though that word will be on the front of new_word, so you can just chop off the first character of word at the same time you do word[0].

If you were using Python 3, you could simply write:

first, *rest_word = word
new_word = ''.join(rest_word) + first + pyg

It's just deconstructing the string, except that instead of the second character being stored in rest_word, everything that's left is stored in it. This essentially just separates the "head" of the string from the "tail".

Unfortunately, I came back a few hours later and realized that my original code that I had posted was wrong. rest_word is a list of strings, not a string itself. This necessitated the ''.join(rest_word) bit. This method isn't quite as efficient in this case, but you may find that it's useful if you don't care what exact type the rest * variable is. If you just want to iterate it, it wouldn't matter.

You overwrite new_word instead of creating a new variable. I would have created a new one to preserve the old new_word. That can be helpful when debugging.

On success, you print out new_word outside of the if. That makes less sense than printing inside of it, and it causes an error to be raised if the if was False since new_word isn't defined.

At the top you have pyg. It's a constant though, and according to Python's style guide, constants should be uppercase, and separated by underscores. I'd also make it more descriptive:

PIG_LATIN_SUFFIX = 'ay'

You stated that the goal was to be short, so I won't go too into this, but you should make use of functions here. You have a couple discreet things going on here:

Handling input from the user
Doing the conversion on the input

And you have both of those things mixed up together.

I'd separate things out properly, and do some basic input checking:

PIG_LATIN_SUFFIX = 'ay'

def convert_to_pig_latin(original_word):
    word = original_word.lower()
    first = word[0]
    rest_word = word[1:]

    return rest_word + first + PIG_LATIN_SUFFIX

def ask_for_input():
    while True:
        word = raw_input('Enter a word:')

        if len(word) > 0 and word.isalpha():
            return word

        else:
            print("Invalid Input. Please try again.")

def main():
    original_word = ask_for_input()
    pig_word = convert_to_pig_latin(original_word)
    print pig_word

ask_for_input will ask until it gets valid input now instead of just ending the program.

Oh ya, and switch to Python 3! Python 2 is about to have support for it discontinued.


Immutable pure data classes with public setters on properties I'm putting together some classes as a model for some information (that I'm currently pulling from a website).

The classes are implemented in C# - because in the current version of F# there are no autoimplemented properties. The logic to fill these classes from the website is going to be writen in F#. (Because I like F#, and it works nice for webscraping.)

So since I'm working functionally (I seems to always work functionally there days even in C#).
I don't want these objects to be mutable. Setting mutable fields is ugly in F# (and for good reason, mutable objects are evil).

So all there data fields have only private setters but this makes my constructors long.
I planned to be using named arguments in them anyway, but still 5 arguments is a lot.
And I don't think F# supports object property initialisers anyway.

public class PopularitySplitClassOptionSet : IClassOptionsSet
{
    public PopularitySplitClassOptionSet (string description, IEnumerable<ClassOption> popularClasses, IEnumerable<ClassOption> unpopularClasses, int reqPreferences, int minUnpopularPreferences)
    {
        PopularClasses  = popularClasses;
        UnpopularClasses = unpopularClasses;
        RequiredPrefereces = reqPreferences;
        MinUnpopularPrefereces = minUnpopularPreferences;
    }

    public IEnumerable<ClassOption> Classes
    {
        get
        {
            return PopularClasses.Concat(UnpopularClasses);
        }
    }

    public int RequiredPrefereces { get; private set; }
    public int MinUnpopularPrefereces { get; private set; }

    public int MaxPopularPrefereces {
        get
        {
            return RequiredPrefereces - MinUnpopularPrefereces;
        }
    }

    public  IEnumerable<ClassOption> PopularClasses { get; private set; }
    public  IEnumerable<ClassOption> UnpopularClasses { get; private set; }

}

Edit: Is this good practice? Am I thinking this right?

Also: I wonder if:

public readonly int RequiredPrefereces { get; private set; }

would be better? Or is that not actually a thing you can do?

Related questions:

C#, immutability and public readonly fields
How to avoid “too many parameters” problem in API design?

This is what I program when I want "immutable" classes in C#. You do have an access leak right now though, in that PopularClasses and UnpopularClasses both set and get the underlying object, allowing modification of that object, thus making the class immutable. (This is a bigger problem for set than get, as get would require figuring out which collection type is actually being held. Set you already know because you set it.)

List<string> popularClasses = new List<string>;
popularClasses.add("hello");

pscos = new PopularitySplitClassOptionSet(..., popularClasses, ...);
popularClasses.add("sup"); // This is now added to pscos.

List<string> getAlsoLeaks = (List<string>)(pscos.PopularClasses);
getAlsoLeaks.add("yo"); // This is also added.

You will need to copy on get and set to avoid this. Usually I use LinkedList<T> for this purpose, unless I will need random access, in which case I use List<T>. Since you're only currently using IEnumerable<T>, I'm assuming that random access isn't an issue and using LinkedList<T>.

private LinkedList<ClassOption> popularClasses;
public IEnumerable<ClassOption> PopularClasses
{
    get
    {
        foreach (ClassOption classOption in list.popularClasses)
            yield return classOption;
        // OR:
        // return new LinkedList<ClassOption>(this.popularClasses);
    }
    set
    {
        this.popularClasses = new LinkedList<ClassOption>(value);
    }
}