A text editor driven file renamer in Ruby I wrote the following Ruby script several years ago and have been using it often ever since.

It opens a text editor with the list of files in current directory. You can then edit the file names as text. Once you save and exit the files are renamed.

The renamer allowed me to maintain spreadsheet-like file names thanks to the powerful column editing capabilities of vi (emacs also has those).

Here is how one such directory looks:

I would like to use it as an example for a Ruby workshop. Could you please suggest best-practice and design pattern improvements?

#!/usr/bin/ruby

RM = '/bin/rm'
MV = '/bin/mv'

from = Dir.entries('.').sort; from.delete('.'); from.delete('..')
from.sort!

from.delete_if {|i| i =~ /^\./} # Hidden files

tmp = "/tmp/renamer.#{Time.now.to_i}.#{(rand * 1000).to_i}"

File.open(tmp, 'w') do |f|
  from.each {|i| f.puts i}
end

ENV['EDITOR'] = 'vi' if ENV['EDITOR'].nil?
system("#{ENV['EDITOR']} #{tmp}")

to = File.open(tmp) {|f| f.readlines.collect{|l| l.chomp}}
`#{RM} #{tmp}`

if to.size != from.size
  STDERR.puts "renamer: ERROR: number of lines changed"
  exit(1)
end

from.each_with_index do |f, i|
  puts `#{MV} -v --interactive "#{f}" "#{to[i]}"` unless f == to[i]
end

RM = '/bin/rm'
MV = '/bin/mv'

In general it's preferable to use the FileUtils class rather than relying on shell utilities. Though in this particular case, you might want to stick at least with mv since the FileUtils.mv method does not have an :interactive option.

tmp = "/tmp/renamer.#{Time.now.to_i}.#{(rand * 1000).to_i}"

Ruby has a Tempfile class which can generate a unique temporary file more reliably than this. You should use it.

from.each {|i| f.puts i}

Calling puts on an array will puts each line individually, so the above can just be shortened to f.puts from.

ENV['EDITOR'] = 'vi' if ENV['EDITOR'].nil?

Can be shortened to ENV['EDITOR'] ||= 'vi'. Though what you have isn't particularly verbose either, so it doesn't really matter much which one you choose.

system("#{ENV['EDITOR']} #{tmp}")

Use system(ENV[EDITOR], tmp) instead. This way you get rid of the string interpolation and the code still works if either ENV['EDITOR'] or tmp should contain a space or other shell meta-character (not that they're particularly likely to, but it's a good idea to use the multiple-argument-form of system where ever possible).

to = File.open(tmp) {|f| f.readlines.collect{|l| l.chomp}}

Usually this could be replaced with to = File.readlines(tmp).collect {|l| l.chomp}. However if you follow my suggestion of using Tempfile, that won't be an option any more.

`#{RM} #{tmp}`

If you use fileutils, this will just be FileUtils.rm(tmp) (or rm(tmp) if you include FileUtils). If you don't want to use FileUtils, you should at least use system(RM, tmp) for the same reasons as above.

However if you use Tempfile, which you should, this becomes redundant anyway.

from.each_with_index do |f, i|
  puts `#{MV} -v --interactive "#{f}" "#{to[i]}"` unless f == to[i]
end

To iterate over two arrays in parallel, use zip:

from.zip(to) do |f, t|
  system(MV, "-v", "--interactive", f, t) unless f == t
end

Note that here using system instead of backticks is especially important since one of the files in from or to containing spaces is actually somewhat likely.

So with all my suggestions, your code should now look like this:

#!/usr/bin/env ruby
require 'tempfile'

MV = '/bin/mv'

from = Dir.glob('*').sort

ENV['EDITOR'] ||= 'vi'

to = nil
Tempfile.open("renamer") do |f|
  f.puts from
  f.close
  system(ENV['EDITOR'], f.path)
  f.open
  to = f.readlines.collect {|l| l.chomp}
end

if to.size != from.size
  STDERR.puts "renamer: ERROR: number of lines changed"
  exit(1)
end

from.zip(to) do |f, t|
  system(MV, "-v", "--interactive", f, t) unless f == t
end


Comparing two large binary files in C I'm trying to read two sufficiently large binary files, comparing them and printing the offset at which they differ. I'm using fread to read the binary files and memcmp to compare them.

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

clock_t start, end;
int main(int argc, char *argv[])
{
    double cpu_time_taken;
    FILE *fp1, *fp2;
    printf("\nArgument count:  %d", argc);
    printf("\nFile 1 is: %s", argv[1]);
    printf("\nFile 2 is: %s\n", argv[2]);
    if (argc < 3)
    {
        printf("\nInsufficient Arguments: \n");
        printf("\nHelp:./executable <filename1> <filename2>\n");
        return 0;
    }
    else
    {
        fp1 = fopen(argv[1],  "rb");
        if (fp1 == NULL)
        {
            printf("\nError in opening file %s", argv[1]);
            return 0;
        }

        fp2 = fopen(argv[2], "rb");

        if (fp2 == NULL)
        {
            printf("\nError in opening file %s", argv[2]);
            return 0;
        }

        if ((fp1 != NULL) && (fp2 != NULL))
        {
            start = clock();
            compare_two_binary_files(fp1, fp2);
            end = clock();
            cpu_time_taken = ((double) (end - start)) / CLOCKS_PER_SEC;
            printf("\nTime taken to compare: %f", cpu_time_taken*1000);
        }
    }
}

int compare_two_binary_files(FILE *fp1, FILE *fp2)
{
    char tmp1[16], tmp2[16];

    size_t bytes = 0, readsz = sizeof tmp1;
    int count = 0;
    while (!feof(fp1) || !feof(fp2)){
        fread (tmp1, sizeof *tmp1, readsz, fp1);
        fread (tmp2, sizeof *tmp2, readsz, fp2);
        count += 16;
        if(memcmp(tmp1, tmp2, readsz)){
            for(int i=0; i < readsz; i++){
                printf ("%d: 0x%02x ",i, tmp1[i]);
            }
            printf("\n%x", count);
            return 0;
        }
    }
}

fread (tmp1, sizeof *tmp1, readsz, fp1);
fread (tmp2, sizeof *tmp2, readsz, fp2);
count += 16;
if(memcmp(tmp1, tmp2, readsz)){
    …
}

You are discarding the return values of the fread() calls, blindly assuming that they both successfully read 16 bytes.

It is unclear what the return value of compare_two_binary_files(…, …) means.  In fact, you sometimes don't return a value at all.  Your compiler should have warned you about that problem.

File I/O should be done a block at a time: 512 bytes, 1024 bytes, or 2048 bytes would be a more reasonable chunk size than 16.

Technically, most of the time is spent waiting for I/O.  cpu_time_taken is a misnomer: what you're measuring is called "wall clock time".


Project Euler problem 79: deducing a passcode For one place that I interviewed at (for a Python developer position) I worked with one of the devs on two Project Euler problems, one being problem 79. We talked through different approaches and came up with solutions together, but we coded separately. He seemed impressed that we were able to get through two problems. After the interview, he asked me to clean up the code and submit it, which you can see below. He never got back to me after I sent him the code. I'd like to know what's wrong with my code and not make the same mistakes again.
Problem 79:

A common security method used for online banking is to ask the user for three random characters from a passcode. For example, if the passcode was 531278, they may ask for the 2nd, 3rd, and 5th characters; the expected reply would be: 317.
The text file, keylog.txt, contains fifty successful login attempts.
Given that the three characters are always asked for in order, analyse the file so as to determine the shortest possible secret passcode of unknown length.

# problem79.py
# Project Euler
# Nicolas Hahn

# list of keys -> passcode string
# at each iteration through list,
def passcodeDerivation(keys):
    answer = ''
    while len(keys) > 0:
        firsts = []
        rests = []
        for key in keys:
            # split first digits, rest of the key into two lists
            f, r = key[0], key[1:]
            if f not in firsts:
                firsts += f
            rests += r
        for f in firsts:
            # if f has never been seen except as the first digit of a key
            # it must be the next one in the passcode
            if f not in rests:
                newkeys = []
                # take out digit we just found from all keys
                for key in keys:
                    newkeys.append(key.replace(f,''))
                # remove empty keys
                keys = [k for k in newkeys if k != '']
                answer += f
                break
    return answer

# filename -> list of keys
def loadKeys(keyFile):
    keys = []
    with open(keyFile,'r') as f:
        for line in f:
            keys.append(line.replace('\n',''))
    return keys

def main():
    keys = loadKeys('problem79keylog.txt')
    # correct answer = 73162890
    print(passcodeDerivation(keys))

if __name__ == "__main__":
    main()

The thing wrong isn't so much the code as the algorithm. Your approach to finding the next first digit is inefficient. What is the right way to approach this problem?

This is actually one of the few Euler problems that I've been able to do on paper. Let's start with just establishing the constraints. We know what digits have to go before other digits and thankfully there aren't any repeats. We can turn the input text into a graph, where a directed edge indicates that a digit has to precede another digit. Behold, the power of pictures:

Just looking at the graph makes the answer obvious: 73162890. Why is it obvious and what work did we do in our head to determine this answer? We sorted the graph. Specifically, we performed a topological sort by following the edges. The algorithm is very straightforward assuming no cycles:

L <- Empty list that will contain the sorted nodes
while there are unmarked nodes do
    select an unmarked node n
    visit(n)

function visit(node n)
    if n is not marked then
        for each node m with an edge from n to m do
            visit(m)
        mark n
        add n to the head of L

Since we're dealing with digits, we can keep our edges as just an array of sets:

class DigitGraph(object):
    def __init__(self):
        self.digits = set()
        self.edges = [set() for _ in xrange(10)]

    def add_edge(self, i, j):
        self.digits.add(i)
        self.digits.add(j)
        self.edges[i].add(j)

And then topo-sort can be written by directly translating the algorithm. I'll leave the rest as an exercise. I time it at about 2x faster.


Random Topic Generator I've written a python module that randomly generates a list of ideas that
can be used as the premises for furthur research. For instance, it would be useful in situations where a new thesis topic is required, either for a masters or a doctorate program. Or if someone is just simply bored with their lives and wants some brainstorming ideas about the kind of project that they would like to be involved in, this python module can be used.

The output is a list of random ideas or technologies. It is up to the user as to how they interpret the output. Each idea in the list can be assessed individually, or the ideas can be combined in ways never thought off before to create something new.

I would like some input on how else I can improve the project or what other features I may add. The project is hosted on github. The name of the project is ranto for lack of a better name.

For the impatient, here is the code from the sole python file in the project...

import random
import sys

def topics(f):
    f = open(f, 'r')
    wordlist = []

    for i in f:
        wordlist.append(i.strip())
    return wordlist

def mainp():
    wordlist = topics('../data/' + sys.argv[1])

    while True:
        print random.sample(wordlist, int(sys.argv[2]))
        if raw_input() == '':
            continue
        else:
            break

mainp()

If you want to test it out, you need to download one of the data files from the github repo linked earlier.

Any feedback would be appreciated.

I can't tell from your question whether you are wanting ideas to improve this python code or the project itself.  Your code above works and is readable, so take the following re-spelling of it as minor nitpicks.

def topics(f):
    for i in open(f):   # inline the call to open(), and drop the implicit 'r'.
        yield i.strip() # "yield" turns this into a generator function that will only
                        # read as many lines of the file as are requested.

def mainp():  # consider renaming to something more descriptive (generate_topics?)
    wordlist = topics('../data/' + sys.argv[1])
    response = True # Or any "truthy" value that drops us into the while loop one time.
    while response:
        print random.sample(wordlist, int(sys.argv[2]))
        response = raw_input()

if __name__ == "__main__":  # Putting in this if guard allows you to import your
    mainp()                 # functions above from anywhere else you might want them.

You may want to peruse the HOWTO for generator functions.


Yield statement in a using block from a clarity point of view I want an opinion of the community about readability and maintainability of the following code.
This is quite a rare case when yield statement included in a using construction.

I know that this code will work as intended and dispose will be called eventually.
But I'm worried it might be difficult for other developers to understand and reuse it.

    public IEnumerable<string> GetUsers()
    {
        using (SqlDataAdapter adapter = new SqlDataAdapter())
        {
            DataSet ds = new DataSet();
            string sql = @"SELECT lead  FROM teams";

            using (var sqlCnn = new SqlConnection(connetionString))
            {
                sqlCnn.Open();

                using (var sqlCmd = new SqlCommand(sql, sqlCnn))
                {
                    adapter.SelectCommand = sqlCmd;
                    adapter.Fill(ds);

                    foreach (DataRow row in ds.Tables[0].Rows)
                    {
                        var login = row[0].ToString();
                        yield return login;
                    }
                }
            }
        }
    }

public IEnumerable<string> GetUsers()
{
    using (SqlDataAdapter adapter = new SqlDataAdapter())
    {
        DataSet ds = new DataSet();
        string sql = @"SELECT lead  FROM teams";

        using (var sqlCnn = new SqlConnection(connetionString))
        {
            sqlCnn.Open();

            using (var sqlCmd = new SqlCommand(sql, sqlCnn))
            {
                adapter.SelectCommand = sqlCmd;
                adapter.Fill(ds);

                foreach (DataRow row in ds.Tables[0].Rows)
                {
                    var login = row[0].ToString();
                    yield return login;
                }
            }
        }
    }
}

Usually you shouldn't worry about having a yield return inside a using block but this isn't needed here at all. This is because the DataSet is disconnected from the database. The only thing you would buy would be that for an exception the SqlDataAdapter, SqlConnection and SqlCommand will be properly disposed if for instance the row[0] contains DBNull.

I would extract the foreach loop over the Rows out of the using blocks which I would also use different. The most outer one should enclose the SqlConnection followed by the SqlCommand and the last should be the SqlDataAdapter.

By stacking the using blocks you reduce the horizontal spacing which makes it easier to read.

Because you are only interested in one DataTable you should use the overloaded adapter.Fill(DataTable) method.

Next to mention is that you should declare your variables as near to their usage as possible.

Applying this points will lead to

public IEnumerable<string> GetUsers()
{
    DataTable dataTable = new DataTable();
    string sql = "SELECT lead FROM teams";

    using (var sqlCnn = new SqlConnection(connetionString))
    using (var sqlCmd = new SqlCommand(sql, sqlCnn))
    using (SqlDataAdapter adapter = new SqlDataAdapter())
    {

        sqlCnn.Open();

        adapter.SelectCommand = sqlCmd;
        adapter.Fill(dataTable);

    }

    using (dataTable)
    {
        foreach (DataRow row in dataTable.Rows)
        {
            var login = row[0].ToString();
            yield return login;
        }
    }
}

Edit based on @t3chb0t comment

  The data table is being created before the sql connection, if something goes wrong there, using(dataTable) will not be executed

nevertheless I am not sure it is needed.

public IEnumerable<string> GetUsers()
{

    string sql = "SELECT lead FROM teams";

    using (var sqlCnn = new SqlConnection(connetionString))
    using (var sqlCmd = new SqlCommand(sql, sqlCnn))
    using (SqlDataAdapter adapter = new SqlDataAdapter())
    using (DataTable dataTable = new DataTable())
    {

        sqlCnn.Open();

        adapter.SelectCommand = sqlCmd;
        adapter.Fill(dataTable);

        sqlCnn.Close();

        foreach (DataRow row in dataTable.Rows)
        {
            var login = row[0].ToString();
            yield return login;
        }
    }
}


Efficiently read a file into a (C) string using POSIX APIs Exactly what the title says, this is a C++ class that attempts to read a file into a null-terminated string as efficiently as possible, using POSIX APIs. Obviously this is not intended to be portable code (other than between POSIX-compliant operating systems and GCC or clang), and the Slurp class would have a more extensive API in "real" code.
But, is there any room for improvement in terms of correctness or efficiency?
#ifndef SLURP_H
#define SLURP_H

class Slurp {
    void *begin_{nullptr};
    void *end_;
public:
    Slurp() = delete;
    Slurp(Slurp&) = delete;
    Slurp &operator=(Slurp) = delete;

    [[gnu::nonnull]] Slurp(const char path[]);

    ~Slurp();

    [[gnu::const, gnu::returns_nonnull]] const char *begin() const noexcept;
    [[gnu::const, gnu::returns_nonnull]] const char *end() const noexcept;
};

#endif //SLURP_H

// Slurp.cc
#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <unistd.h>

#include <cerrno>
#include <cstdlib>
#include <cstring>
#include <system_error>

using namespace std;

#define ERROR_IF(CONDITION) do {\
        if (__builtin_expect((CONDITION), false))\
            throw system_error(errno, system_category());\
    } while (false)

#define ERROR_IF_POSIX(POSIX_PREFIXED_FUNCTION_CALL) do {\
        const int error_number = posix_##POSIX_PREFIXED_FUNCTION_CALL;\
        if (__builtin_expect(error_number, 0))\
            throw system_error(error_number, system_category());\
    } while (false)

Slurp::Slurp(const char path[]) {
    class FileDescriptor {
        const int fd_;
    public:
        FileDescriptor(int fd) : fd_{fd} {ERROR_IF(fd_ == -1);}
        ~FileDescriptor() {close(fd_);}
        [[gnu::const]] operator int() const noexcept {return fd_;}
    };

    FileDescriptor fd{open(path, O_RDONLY)};

    ERROR_IF_POSIX(fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL));

    struct stat file_statistics;

    ERROR_IF(fstat(fd, &file_statistics));

    constexpr char32_t NullChar = U'\0';

    const size_t fsize = static_cast<size_t>(file_statistics.st_size),
        blksize = static_cast<size_t>(file_statistics.st_blksize),
        bufsize = fsize + sizeof NullChar;

    ERROR_IF_POSIX(memalign(&begin_, blksize, bufsize));

    end_ = static_cast<char*>(begin_) + fsize;

    ERROR_IF_POSIX(madvise(begin_, bufsize, POSIX_MADV_SEQUENTIAL));

    ERROR_IF(read(fd, begin_, fsize) == -1);

    memcpy(end_, &NullChar, sizeof NullChar); // ensure null termination
}

Slurp::~Slurp() {free(begin_);}

const char *Slurp::begin() const noexcept {return static_cast<char*>(begin_);}
const char *Slurp::end() const noexcept {return static_cast<char*>(end_);}

This is a good project, one that could be very useful in practice. I’ve had two or three C++ blogs over the years, and every time I spin up a new one, I include an updated version of a very old post I wrote about reading an entire file into a string. It’s not an easy problem to solve efficiently, for a number of complicated reasons. Eschewing the standard library and going directly to underlying POSIX API is not a bad idea; one of the sources of inefficiency is the fact that the standard library will buffer everything, meaning you end up with an unnecessary “middle man” between your code and the actual file data.
I am not an expert in the POSIX APIs, so take everything I say in that area with a grain of salt. However, the algorithm itself looks okay; I don’t see anything wrong with it. There are a few things I think are probably unnecessary and useless, but nothing that introduces potential dangers. I’ll get into details about the algorithm as we come to them in the code.
Normally I would start with a design review, but… well, frankly, there doesn’t really look like there’s much in the way of design here. Your focus appears to be on the reading algorithm, and very little else. Which… not a good thing. That’s actually the opposite of how you write good code. And it shows here. To word it another way, the actual file-reading code is probably fine… but to actually use it in practical code (the way it’s currently written) would be difficult, at best.
For example, the iterators return char const*, and there looks like a lot of effort to NUL-terminate the data. So… is it meant to be string data? If so, why isn’t there a c_str() function? Or a conversion to string_view? On the other hand, if it’s not meant to be a string, just raw byte data read from a file, why not use std::byte? Why isn’t there a data() function?
Better yet, why not just read the data directly into an existing container, like std::string (for text data) or std::vector<std::byte> (for binary data)?
But I get it; your focus was on the read algorithm, not making an actually useful interface. That’s fine for experimentation, but when you’re eventually putting this code to use in a real-world library, the place that really needs focus is the public interface… not the low-level reading code. If your public interface sucks, then even if the reading code is the most brilliant code ever, the whole library still sucks for the user. But if your public interface is really good, then even if the reading code sucks… well, you can fix that under the hood, and users of your public interface won’t notice (except for maybe seeing performance gains).
Also, you should really get into the habit of testing your code. And by that I don’t mean “write it, then write a main() function that tries it out and if it all compiles and appears to run and produce correct-looking output, that’s that”. I mean fully unit-testing the code.
So, let’s get into the actual code.
Code review
Slurp.hh
    void *begin_{nullptr};
    void *end_;

The convention in C++ is to put all the type information together. In other words:

void *end_;: This is C style.
void* end_;: This is C++ style. (Well, other than that we try to avoid void* in C++.)

There is an alternative design to consider. Rather than holding two pointers, it might make more sense to hold a pointer and a size. Why? It makes it much easier to maintain and check the class invariant. If you use a pair of pointers, you have to make sure that end_ comes after begin_. But there is no portable way to ensure that (comparing random pointers is UB). On the other hand, if you use a pointer and a size, there is no possible way that the “end” can be before the beginning (assuming the size is unsigned, which is standard practice, but even if it’s signed, it’s trivial to check that the size is greater than zero).
It’s not really a big deal, and two pointers does work just fine in practice. It’s just something I’ve seen experts have opinions on.
I also don’t get why these are void*. void* makes a lot less sense in C++ than it does in C, and in this case… I can’t see the point of it at all. If this is supposed to be character data—a string—then why aren’t these char*? That sure seems to be the intention, what with all the NUL terminating and casting in begin() and end(). If this is supposed to be binary data, then unsigned char* or, better, std::byte* make more sense. void* just doesn’t make sense, not least because you’re just casting it to char* before using it anyway.
Anywho, aside from all that, there doesn’t seem to be a reason why end_ couldn’t also have an initializer.
    Slurp() = delete;

This line is technically unnecessary, because the fact that you have the constructor Slurp(char const[]) means that you have already disabled the default constructor.
Still, it doesn’t hurt, and it does express intent, so I’d leave it.
    Slurp(Slurp&) = delete;
    Slurp &operator=(Slurp) = delete;

These will probably “work”, but if your intention is to disable copying, then they are malformed. They should be:
    Slurp(Slurp const&) = delete;
    auto operator=(Slurp const&) -> Slurp& = delete;
    // or:
    //  Slurp& operator=(Slurp const&) = delete;

Disabling copying makes sense, given that copying will probably be fairly expensive. If someone really wants to copy the data, they can always do something like:
auto const data = Slurp{"input.data"};

auto const data_copy = std::vector(data.begin(), data.end());

But you might consider making the type movable. That could be very handy. It wouldn’t even be hard to do; you just need to make the class aware of a “null” or moved-from state, perhaps like so:
class Slurp
{
private:
    void* begin_ = nullptr;
    void* end_ = nullptr;

public:
    [[gnu::nonnull]] explicit Slurp(char const* path)
    {
        // your existing constructor code...
    }

    Slurp(Slurp&& other) noexcept
    {
        std::ranges::swap(begin_, other.begin_);
        std::ranges::swap(end_, other.end_);
    }

    ~Slurp()
    {
        if (begin_) // note the if test
            std::free(begin_);
    }

    auto operator=(Slurp&& other) noexcept -> Slurp&
    {
        std::ranges::swap(begin_, other.begin_);
        std::ranges::swap(end_, other.end_);
        return *this;
    }

    Slurp(Slurp const&) = delete;
    auto operator=(Slurp const&) -> Slurp& = delete;
};

Being able to very cheaply moved the slurped data around could be very handy indeed.
    [[gnu::nonnull]] Slurp(const char path[]);

Writing functions that look like they take arrays like this is a really bad idea. (Except for main(), which is just special in a lot of ways.) I’ve lost count of the number of newbies who think that this actually takes an array argument, and then think they can do sizeof(path) / sizeof(path[0]) to get the number of elements. Yes, you and I and other experts know what’s really going on, but you shouldn’t be writing code that’s only readable by experts. Down that path lies madness.
On the other hand Slurp(const char* path) or Slurp(char const* path) are unlikely to confuse beginners, and, really, tell the truth of what’s going on much better.
As a general rule, all constructors that take a single argument should be marked explicit (unless you have a damn good reason not to). Yes, explicit should really be the default, but that ship sailed a long time ago.
In addition to this constructor, it would help usability if you could use strings or—even better—actual paths with this class. It would be trivial to do, too:
explicit Slurp(std::string const& path) :
    Slurp{path.c_str()}
{}

explicit Slurp(std::filesystem::path const& path) :
    Slurp{path.c_str()}
{}

Now this next bit is going to get really complicated. Brace yourself.
gnu::const on non-static member functions
    [[gnu::const, gnu::returns_nonnull]] const char *begin() const noexcept;

The issue here the gnu::const attribute. I am not a GCC expert, but from my understanding of gnu::const, it sure seems like using it for a member function is an absolutely terrible, and dangerous, idea.
Here’s why.
gnu::const means that a function’s output depends ONLY on the function’s arguments. And I mean ONLY. The function’s behaviour cannot be affected by anything else; it doesn’t matter what state the rest of the program happens to be in (well, I mean, the state has to be valid… if the abstract machine’s state is kerfuckt, then all bets are off in any case), the function’s return value will ALWAYS be the same, given the same arguments.
This makes sense for, say, a function like: auto twice(int i) { return i * 2; }. It doesn’t matter what’s going on with the rest of the program, like how many exceptions are in flight or how many files are open or how much memory has been allocated. So long as the function can complete without UB (for example, there is no stack overflow), then it will ALWAYS return the same result for the same arguments. twice(32) will ALWAYS return 64. ALWAYS. GCC can use that information to optimize. For example, if you call twice(14) a million times in your program, GCC might cache the result of the first call, and then replace every other call with just loading that cached value.
So far none of this sounds problematic, I suppose. Well, now let’s talk about non-static member functions.
There is a very important key feature of non-static class member functions: the implicit this.
You see, every non-static class member function has a hidden this argument. We usually pretend it’s the first argument (but it doesn’t actually have to be, in reality, it’s usually passed in a specific register), so your begin() function actually looks like begin(Slurp const*):
// what you write:
auto Slurp::begin() const noexcept -> char const*
{
    return begin_;
}

// what the compiler actually implements:
auto Slurp__begin(Slurp const* this) noexcept -> char const*
{
    return this->begin_;
}

Normally, this isn’t something you need to think about.
However… remember what gnu::const means. It means that begin() will ALWAYS return the same result, when called with the same argument.
So let’s imagine you write this:
auto const s = Slurp{"data.file"};

auto p1 = s.begin();
auto p2 = s.begin();
auto p3 = s.begin();
auto p4 = s.begin();
auto p5 = s.begin();

The above is theoretically:
auto const s = Slurp{"data.file"};

auto p1 = Slurp__begin(&s);
auto p2 = Slurp__begin(&s);
auto p3 = Slurp__begin(&s);
auto p4 = Slurp__begin(&s);
auto p5 = Slurp__begin(&s);

But since begin() is gnu::const, once we know the result of calling it with address of s, we can assume that will ALWAYS be the result of calling it with that address. So the compiler could do:
auto p1 = Slurp__begin(&s);
auto p2 = p1;
auto p3 = p1;
auto p4 = p1;
auto p5 = p1;

(And it probably will.)
So far no problems. There is currently no way for the value of begin_ to change once a Slurp object has been constructed, so the above code is a perfectly valid optimization.
However, suppose you implemented moving—or copying, or any way to change the data in a Slurp object. Then you wrote this:
auto const s = Slurp{"data.file"};

auto p1 = s.begin();
auto p2 = s.begin();

s = Slurp{"other.file"};    // this line will construct a new Slurp,
                            // and REPLACE the internals of `s` with the
                            // new data

auto p3 = s.begin();
auto p4 = s.begin();

Now the address of s hasn’t changed… but it’s internal data—in particular, the value of begin_—has changed. The compiler sees the above code as:
auto const s = Slurp{"data.file"};

auto p1 = Slurp__begin(&s);
auto p2 = Slurp__begin(&s);

s = Slurp{"other.file"};

auto p3 = Slurp__begin(&s);
auto p4 = Slurp__begin(&s);

And it does the optimization:
auto const s = Slurp{"data.file"};

auto p1 = Slurp__begin(&s);
auto p2 = p1;

s = Slurp{"other.file"};

auto p3 = p1;
auto p4 = p1;

Except after the move assignment, p1 is now a dangling pointer. Which means p3 and p4 are dangling pointers, too.
This is a disaster.
Now you may be thinking: “Meh, no problem; my class is non-copyable, non-movable, and immutable. This problem doesn’t affect me.”
Wanna bet?
Remember, gnu::const isn’t about the object. It’s about the arguments to the function. In the example above, the object’s internals changed, but the address of the object didn’t, so the argument to begin() didn’t either, and that was the problem. So if the object’s internals can’t possibly change… no problem, right?
Wrong.
Because even if it is impossible to change the internals of an object, it is still possible to create an object… destroy it… then create a whole new object at the same memory address… and it is the address that matters. Because that is what is being used as the function argument to begin().
Here’s just one example of how that could happen:
auto s = std::optional<Slurp>{std::in_place, "data.file"};

auto p1 = s->begin();
auto p2 = s->begin();

s.emplace("other.file");    // destroys contained Slurp object, then
                            // constructs a new one in the same place... so it
                            // has the same address

auto p3 = s->begin();
auto p4 = s->begin();

Which eventually optimizes to:
auto s = std::optional<Slurp>{std::in_place, "data.file"};

auto p1 = s->begin();
auto p2 = p1;

s.emplace("other.file");

auto p3 = p1;
auto p4 = p1;

And ends up with the same problem.
But you don’t even need std::optional! This problem could be triggered with even this code:
auto f()
{
    auto s = Slurp{"data.file"};

    auto p = s.begin();
}

auto some_other_function()
{
    f();
    f();
}

What might happen above is the first call to f() happens to place s at some address on the stack… then when f() ends, s is destroyed… then f() is called again and just happens to place the new s at the same address on the stack. But of course, the allocation done within the Slurp constructor (the posix_memalign() call) may be entirely different. So p SHOULD have two entirely different values with the two successive calls to f() even though the address of s (and thus, the argument passed to begin()) is the same for both calls. But gnu::const says it won’t; gnu::const says that if the argument is the same, the result is the same.
Once again: catastrophe.
To get a clearer view of why, imagine the calls to f() are inlined. Inlining isn’t necessary to trigger the problem, but it helps illustrate. If the calls are inlined, some_other_function() will look like:
auto some_other_function()
{
    {
        auto s = Slurp{"data.file"};

        auto p = s.begin();

        // s is destroyed here
    }

    {
        auto s = Slurp{"data.file"};

        auto p = s.begin();

        // s is destroyed here
    }
}

After the first s is destroyed, the second s may very well be constructed at the exact same location… let’s call it STACK_ADDRESS. So:
auto some_other_function()
{
    {
        auto s = Slurp{"data.file"};

        auto p = Slurp__begin(STACK_ADDRESS);

        // s is destroyed here
    }

    {
        auto s = Slurp{"data.file"};

        auto p = Slurp__begin(STACK_ADDRESS);

        // s is destroyed here
    }
}

The two calls to begin() are identical, and gnu::const promises that that means the result will be identical, so the compiler might do:
auto some_other_function()
{
    auto __p_inlined = static_cast<char const*>(nullptr);

    {
        auto s = Slurp{"data.file"};

        __p_inlined = Slurp__begin(STACK_ADDRESS);
        auto p = __p_inlined;

        // s is destroyed here
    }

    {
        auto s = Slurp{"data.file"};

        auto p = __p_inlined;

        // s is destroyed here
    }
}

And, boom.
Whew, that was a long digression. The tl;dr is “don’t use gnu::const for (non-static) member functions”… but I think it’s important to understand why.
And, really, it seems kinda pointless in modern C++. If it is possible for a function to be gnu::const, then it is almost certainly also possible for it to be constexpr… in which case the compiler can optimize it far more aggressively than it could possibly optimize a function that is merely gnu::const (but not wholly visible).
Slurp.cc
using namespace std;

Never, ever do this at file scope. No, not even in a non-header file. Just use std::.
#define ERROR_IF(CONDITION) do {\
        if (__builtin_expect((CONDITION), false))\
            throw system_error(errno, system_category());\
    } while (false)

#define ERROR_IF_POSIX(POSIX_PREFIXED_FUNCTION_CALL) do {\
        const int error_number = posix_##POSIX_PREFIXED_FUNCTION_CALL;\
        if (__builtin_expect(error_number, 0))\
            throw system_error(error_number, system_category());\
    } while (false)

Avoid macros. There’s no good reason for them here. Inlined functions will be just as efficient… not that it really matters, because these are error cases after all. For example:
namespace {

auto error_if(int result)
{
    if (result != 0) [[unlikely]]
        throw std::system_error{errno, std::system_category()};
}

auto error_if_posix(int error_number)
{
    if (error_number != 0) [[unlikely]]
        throw std::system_error{error_number, std::system_category()};
}

} // anonymous namespace

Slurp::Slurp(char const* path)
{
    // ... [snip] ...

    error_if_posix(::posix_fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL));

    struct ::stat file_statistics{};
    error_if(::fstat(fd, &file_statistics));

    // ... etc.

But even that’s not really great, because it just obfuscates the function’s logic. I don’t see how it’s really an improvement over:
Slurp::Slurp(char const* path)
{
    // ... [snip] ...

    if (auto const res = ::posix_fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL); res != 0) [[unlikely]]
        throw std::system_error{res, std::system_category()};

    struct ::stat file_statistics{};
    if (auto const res = ::fstat(fd, &file_statistics); res != 0) [[unlikely]]
        throw std::system_error{errno, std::system_category()};

    // ... etc.

The only thing I might do is maybe avoid repeating std::system_category by doing:
namespace {

auto posix_error(int err)
{
    return std::system_error{err, std::system_category()};
}

} // anonymous namespace

Slurp::Slurp(char const* path)
{
    // ... [snip] ...

    if (auto const res = ::posix_fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL); res != 0) [[unlikely]]
        throw posix_error(res);

    auto file_statistics = ::stat{};
    if (auto const res = ::fstat(fd, &file_statistics); res != 0) [[unlikely]]
        throw posix_error(errno);

    // ... etc.

I just don’t see the benefit of hiding the throws. Yes, I get that it is somewhat ugly to have to check the return code every function call… but this is a C API; that’s just the nature of the game.
If you really don’t want all that ugly C-ish code, the right move is to properly wrap the API… not to obfuscate your logic by hiding it behind macros. For example:
Slurp::Slurp(char const* path)
{
    auto const fd = posix::open(path, O_RDONLY);

    posix::fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL);

    auto const file_statistics = posix::fstat(fd);

    // ... etc.

(Note that implied is that each call above throws on failure, like a good C++ API should.)
But it’s really an all-or-nothing deal. Either completely wrap the API into a proper C++ API, so the usage just looks like regular C++… which maintainers of the code should be familiar with. Or use the C API as-is—don’t hide it behind macros—so people who actually know the POSIX API inside and out can look at your code and immediately spot what’s going on (and any potential problems). Going half-way just annoys both parties, with no real benefits.
    class FileDescriptor {
        const int fd_;
    public:
        FileDescriptor(int fd) : fd_{fd} {ERROR_IF(fd_ == -1);}
        ~FileDescriptor() {close(fd_);}
        [[gnu::const]] operator int() const noexcept {return fd_;}
    };

This is a really useful class! Rather than burying it as an implementation detail, it could be part of a public API. You’d need to flesh it out a bit (and make it non-copyable, but perhaps movable), but it’s pretty cool.
FileDescriptor fd{open(path, O_RDONLY)};

This goes back to the “never use using namespace std;” rule: function names like open() are extremely common, and there is a very high chance of conflict between the POSIX function, and some function you might put in your class in the future. If that were to happen, if you are lucky, you’ll get a compile error… but I’ve seen cases where behaviour subtly changes because the wrong function is called.
To make sure you get the right function, you should explicitly qualify it. In this case, because it’s a POSIX function that doesn’t exist in a namespace, you have to use ::open().
You should do this for all the POSIX functions.
constexpr char32_t NullChar = U'\0';

I’m a little baffled by the logic here.
Okay, so you want the data to be null-terminated. Fine, sure. But… you’re treating the data as an array of char… not char32_t. So… huh?
The only thing I can figure is that you’re imagining that maybe someone might possibly want to reinterpret_cast the char const* to char32_t const*, and treat the data as a UTF-32 NUL-terminated string. That seems a pretty odd thing to presume. And it’s even odder when you consider that the Slurp class doesn’t even model a NUL-terminated string in any sense. There is no c_str() function, for example. Even if there were, it’s hard to see why it should return a char32_t const* and not a char const*.
Aside from being odd, it may also be wrong. You are aligning the memory according to file_statistics.st_blksize… not alignof(char32_t). In other words, if you’re thinking that you can just reinterpret_cast the result of begin() to a char32_t const* and treat it as a UTF-32 string… you may be in for a nasty surprise when your program crashes due to a a misaligned read. Or, possibly, just garbled text (that is probably illegal Unicode). (And all this is assuming the endianness of the UTF-32 data is correct.)
I mean, if you really wanted to read file data as a UTF-32 string, why wouldn’t you read it into a std::u32string? Similarly, if you want a regular string, why not read it directly into a std::string? In either case, all the allocation and alignment headaches are taken care of, and the NUL termination is automatic. And if you want binary data, why not read directly into a std::vector<std::byte>?
I’ll be getting back to that idea later.
ERROR_IF_POSIX(memalign(&begin_, blksize, bufsize));

Honestly, I don’t see the point of this.
Okay, I get that the file you’re reading probably has a preferred block size, and if you were mem-mapping the file, or reading it directly from storage, then yes, you’d need aligned memory. But you’re doing neither.
Internally, when you open the file (and specify the access characteristics via posix_fadvise()) a buffer gets created. (I think you can avoid this by using direct input, but that’s beyond the scope here. mmap() is another way to avoid the buffer, but that’s another beast altogether.) Then you are reading from that buffer into your program’s memory space. Now, the buffer needs to be aligned to the block size of the file, so that the file can be read efficiently a block at a time. But your memory… the memory you allocate to read the file data (from the buffer) into… that does not need to be aligned to the block size of the file… because with that memory, you are not reading directly from the file, you are reading memory-to-memory (from the kernel’s buffer, to your array).
Follow? Block-alignment only matters for the initial read of the file from the device. THAT needs to be block-aligned. But unless you’re doing direct input, that’s happening in the kernel (usually), and then read() copies from that buffer to your desired location. Once the data is already in RAM, it’s just a memory-to-memory copy… which is just a rep stosb or some SSE or AVX operation. That doesn’t need to be aligned.⁕
⁕ (Ugh, in order to make this not too absurdly long, I have to gloss over or outright ignore a lot of hyper-technical details, so I know there are nerds out there just champing at the bit to “akshually…” me in the comments. Okay, yes, the data does need to be aligned for super-fast memory-to-memory transfer… both the source and destination data, with penalties for not being aligned varying depending on your processor architecture. HOWEVER… the default C++ allocator in any serious-grade standard library is almost certainly already aligning every allocation to take advantage of those efficiencies by default. So unless you’re doing something “clever”, you don’t need to worry about it.)
In other words, you’re wasting your time using posix_memalign(). You gain nothing from it. You could just as well be using malloc() or new.
Which brings us back again to the idea of reading directly into std::string or std::vector<std::byte>. But let me postpone that for a bit more, because we’re almost at the end of the code, so let’s get through that first.
ERROR_IF_POSIX(madvise(begin_, bufsize, POSIX_MADV_SEQUENTIAL));

I’ll admit that I don’t understand how this call isn’t failing. I thought posix_madvise() was only for memory-mapped memory. I would have guessed you’d get a EBADF from trying to use it on random memory. TIL, I guess.
But I do suspect the call is just being ignored. My understanding of POSIX_MADV_SEQUENTIAL is that it signals to the kernel to make a larger look-ahead buffer… which makes sense if you’re mem-mapping a file into memory, but is just nonsense for RAM. (Where you gonna put the look-ahead buffer for RAM? Super-duper-RAM?)
ERROR_IF(read(fd, begin_, fsize) == -1);

read() can fail spuriously for a number of reasons, like a signal interrupting it, and end up doing only a partial read. You can’t just write a read() call like this; you need to call it in a loop and keep doing reads until you’ve read fsize bytes in total or until read() returns zero.
memcpy(end_, &NullChar, sizeof NullChar); // ensure null termination

I mean… why not just std::fill_n(end, 4, 0);? It would probably be faster. (Unless your optimizer is smart enough to transform that memcpy() into a memset(), in which case it will probably be equally fast.)
Reading directly into containers
Okay, the biggest problem with Slurp is that it’s a specialized type. Even if it had a perfect interface, it’s still not a string. If I need a std::string, which is very common, then I’ll be forced to copy the entire Slurp object into a std::string. What a waste.
What if you just read the file’s contents directly into a std::string. Or, if it’s binary data, into a std::vector<std::byte>. Remember, there is no need to use posix_memalign() to specially align the memory to the original file’s preferred block size. The default allocator will align things just fine. So you can just use standard containers.
(But, FYI, even if the default allocator didn’t align things just fine, it would still be a better solution to use a custom allocator, and standard containers, rather that rolling your own, half-assed container.)
For example, to read into a string (note: untested, rough code):
auto read_into_string(std::filesystem::path const& path)
{
    // file_descriptor is a class basically identical to yours
    auto const fd = file_descriptor{::open(path.c_str(), O_RDONLY)};

    // get the file size from the file descriptor
    auto const file_size = [&]
    {
        struct ::stat fstats{};
        if (auto const res = ::fstat(fd, &fstats); res != 0)
            throw std::system_error{errno, std::system_category()};
        return fstats.st_size;
    }();

    // create the string we'll be reading into
    auto s = std::string{};
    s.resize(file_size);

    // this is the read loop
    auto pos = std::string::size_type{};
    while (true)
    {
        auto const last_read = ::read(fd, s.data() + pos, s.size() - pos);
        if (last_read < 0)
            throw std::system_error{errno, std::system_category()};
        else if (last_read == 0)
            break;

        // this should never happen
        if (last_read > (s.size() - pos)) [[unlikely]]
            throw std::runtime_error{"more data in file than file size"};

        pos += last_read;
    }

    // this should never happen
    if (pos != s.size()) [[unlikely]]
        throw std::runtime_error{"failed to read entire file"};

    return s;
}

And the usage:
auto const string_data = read_into_string("file.txt");

And string_data is a std::string.
The only thing that I suspect might make this code slower than your code, and only for very, very large files, is that there is no way (AFAIK) to allocate a std::string without initializing its contents. In other words, s.resize(n) doesn’t does just allocate n bytes (internally), it also zeros those bytes… which is a total waste of time for us. You shouldn’t have this inefficiency if you’re reading binary data into a std::vector<std::byte> (for which the code is otherwise identical).
But even that std::string inefficiency should be tolerable (unless your files are huge), because you get the benefit of actually having an honest-to-goodness string. Which basically works everywhere, and which many APIs specifically need, in one form or another.
And, frankly, I seriously doubt you’ll even notice it, because, again, unless you’re dealing with huge files, the cost of that zeroing will be completely dwarfed by the I/O costs.
Anywho, this is getting way too long, and I’ve been sitting on it for days, so, I’ll just go ahead and publish what I’ve got.


An approach to the network messaging service I'm working on the communication module of an ecommerce application. This module takes care of handling messages that come from other apps over the wire.
The awful switch case is never an option so I decided to look for some pattern. While reading about this topic found that Message Factory and Double Dispatch patterns are the very useful in this scenario. I created my own version following those, but there is one issue I cannot figure out (maybe is ok the way I'm doing it). The things is: as I have one class responsible for handling each message, where should I put the code that register each message handler? In the code I've seen that logic is putted together in some method like in the code, but that will imply one line for each message handler, and maybe there is a shortcut for this. I thought about using reflection to discover Message Handlers at runtime but that would be a performance penalty just to avoid a couple lines of code that I can write I forget about them.
Any other idea?

Interface

/// <summary>
/// Represents the entity in charge of handling messages from other applications
/// </summary>
interface ICommunicationService
{
    void RegisterHandler(IMessageHandler handler);

    void ProcessMessage(Message message);
}

Concrete implementation

class CommunicationService : ICommunicationService
{
    private readonly Dictionary<int, IMessageHandler> handlers = new Dictionary<int, IMessageHandler>();

    public void CommunicationService()
    {
        // this is what I want to avoid or improve.
        RegisterHandler(new LoginResponseMessageHandler());
        // ... a line for each handler should go next
    }

    public void RegisterHandler(IMessageHandler handler)
    {
        if (!handlers.ContainsKey(handler.Command))
            handlers.Add(handler.Command, handler);
        else
            handlers[handler.Command] = handler;
    }

    public void ProcessMessage(Message message)
    {
        // Incoming messages that does not have a handler registered will be ignored. This could be or not
        // the desired behavior.
        if (handlers.ContainsKey(message.Command))
            handlers[message.Command].ProcessMessage(message);
    }
}

Message Handlers

interface IMessageHandler
{
    /// <summary>
    /// Gets the identifier of the Command this handler will process.
    /// </summary>
    int Command { get; }

    /// <summary>
    /// When implemented, should contain the logic for handling the requested command.
    /// </summary>
    void ProcessMessage(Message message);
}

Example message handler

class LoginResponseMessageHandler : IMessageHandler
{
    public int MessageId
    {
        get { return 1; } // set the id of the message. this will identify the message across all apps.
    }

    public void ProcessMessage(Message message)
    {
        // here we receive the incomming message and we take care of it...
    }
}

PD: Looking at the code another question came to mind. As you can see I'm using an integer value to identify each message between all apps. Is that a good choice?

In addition to RubberDuck's answer, I'd also propose the following improvements.

I don't really see the need to do this:

if (!handlers.ContainsKey(handler.Command))
    handlers.Add(handler.Command, handler);
else
    handlers[handler.Command] = handler;

You can omit the if/else logic and simply assign the value:

handlers[handler.Command] = handler;

I also would advise to avoid a "negative" check in the if-clause. IMHO this is much easier to understand:

if (handlers.ContainsKey(handler.Command))
    handlers[handler.Command] = handler;
else
    handlers.Add(handler.Command, handler);

But like I said, the whole if/else logic isn't necessary in this case anyway; I'm just using it as an example.

Do not that your code currently doesn't prevent overwrites of existing key/value pairs. You might not want this; if you'd use this (instead of your four line if/else block):

handlers.Add(handler.Command, handler);

... then there'd be an exception if the same command is used more than once as a key.

I also advise against this:

if (handlers.ContainsKey(message.Command))
    handlers[message.Command].ProcessMessage(message);

And instead use TryGetValue to avoid unnecessary lookups:

IMessageHandler handler;
if(handlers.TryGetValue(message.Command, out handler)
{
    handler.ProcessMessage(message);
}


Find all pairs of numbers such that the number of binary 1's in their OR is maximum So, I tried to solve this question and my code did not execute within time-limits.
https://www.hackerrank.com/challenges/acm-icpc-team

Here's my code,

from itertools import combinations
nm =input().split()
n = int(nm[0])
m = int(nm[1])
mat=[]
z=[]
for i in range(n):
    mat.append(input())
a=combinations(range(1,n+1),2)

for i in a:
    count=0
    for j in range(m):
        if mat[i[0]-1][j]=="0" and mat[i[1]-1][j]=="0":
            count=count+1
    z.append(m-count)
print(max(z))
print(z.count(max(z)))

How can I improve it?

I don't have much time, and don't see any quick performance suggestions, but,

mat = []
for i in range(n):
    mat.append(input())

Can be written more terse as a list comprehension:

mat = [input() for _ in range(n)]

And note how you never use i. If you don't use a variable, you can indicate that it isn't needed by calling it _. That's a placeholder convention to say that "I needed to create a variable, but I don't need it".

count=count+1

can be

count += 1

Also, please be more careful with spaces around operators. In some places, you use no spaces, in some places you use one on each side, and then in one case, you have

nm =input().split()

I prefer one per side, but regardless of what you choose, you should be consistent.


Serialize and deserialize binary tree Here is my code of serialize and de-serialize a binary tree, looking for advice. One issue in my mind which I cannot resolve is, I am using a global variable index, wondering if more elegant solutions in Python 2.7 -- since in Python there is no solution to pass a reference as Java/C++ did.

index = 0
class Node:
    def __init__(self, value, left, right):
        self.value = value
        self.left = left
        self.right = right
    @staticmethod
    def serialize(root, output):
        if not root:
            output.append('#')
            return
        output.append(root.value)
        Node.serialize(root.left, output)
        Node.serialize(root.right, output)
        return output
    @staticmethod
    def deserialize(source):
        global index
        if source[index] == '#':
            index += 1
            return None
        value = source[index]
        index += 1
        left = Node.deserialize(source)
        right = Node.deserialize(source)
        return Node(value, left, right)

if __name__ == "__main__":
    root = Node('a', Node('b', None, None), Node('c', Node('e', Node('g', None, None), None), Node('f', None, None)))
    result = []
    Node.serialize(root, result)
    print result
    new_root = Node.deserialize(result)
    result2 = []
    Node.serialize(new_root, result2)
    print result2

For starter, you don't need to use a global index. Python tuples make it easy to return several values at once, you can return the new index with the deserialized value:

    @staticmethod
    def deserialize(source):
        def _helper(index):
            if source[index] == '#':
                return None, index + 1

            value = source[index]
            left, index = _helper(index + 1)
            right, index = _helper(index)
            return Node(value, left, right), index
        return _helper(0)[0]

Now, what if I what to serialize the node Node('#', None, None)? I will get ['#', '#', '#'] as output that will get deserialized as None. Not that ideal. If you want to keep this intermediate format, you should at least allow the user to chose its sentinel value. Using a parameter with a default value in both serialize and `deserialize is a good option.

I would also improve the interface of your class: turning serialize into a method and deserialize into a classmethod. This allow for ease of use and easy subclassing:

class Node(object):
    ...

    def serialize(self, sentinel='#'):
        serial = [self.value]
        if self.left is None:
            serial.append(sentinel)
        else:
            serial.extend(self.left.serialize(sentinel))
        if self.right is None:
            serial.append(sentinel)
        else:
            serial.extend(self.right.serializr(sentinel))
        return serial

    @classmethod
    def deserialize(cls, source, sentinel='#'):
        def _helper(index):
            if source[index] == sentinel:
                return None, index + 1

            value = source[index]
            left, index = _helper(index + 1)
            right, index = _helper(index)
            return cls(value, left, right), index
        return _helper(0)[0]

And at the very last, I would provide default values for the left and right parameters:

class Node(object):
    def __init__(self, value, left=None, right=None):
        self.value = value
        self.left = left
        self.right = right

So I can more easily write trees:

t = Node(13, Node(8), Node(15, right=Node(18)))
t.serialize()
# [13, 8, '#', '#', 15, '#', 18, '#', '#']

An other way to look at the problem is to take inspiration from repr. If we were to write an official representation for such objects, we could end up with:

    def __repr__(self):
        return '{}(value={!r}, left={!r}, right={!r})'.format(self.__class__.__name__, self.value, self.left, self.right)

And repr(t) would be the string:

Node(value=13, left=Node(value=8, left=None, right=None), right=Node(value=15, left=None, right=Node(value=18, left=None, right=None)))

It's a bit verbose, but it is serialized as a string. Using eval on this string would even deserialize it and return a Node equivalent to t. I however advise against the use of eval for deserialization purposes as it is too generic and thus dangerous.

Instead, serialization could return a more stripped form such as (8,,) for Node(8) or ('#',(5,,),) for Node('#', Node(5)) and deserialization would be a parser tailored for this representation.

Serialization is easy:

    def serialize(self):
        return '({!r},{},{})'.format(
            self.value,
            '' if self.left is None else repr(self.left),
            '' if self.right is None else repr(self.right))

Deserialization is left as an exercise for the reader.


Collapsing Folder Trees with Ruby This is a quick script I whipped up to collapse some folder trees down to individual folders. The specific situation that arose was my formerly-itunes collection (it's currently organized Music\artist\album\songs.(mp3|m4a) and I'd really prefer it to be Music\artist\song.(mp3|m4a)).

#!/usr/bin/ruby

require 'optparse'
require 'pp'
require 'fileutils'

### parsing command line options via optparse, nothing to see here
options={}
optparse = OptionParser.new do|opts|
  opts.on('-h', '--help',
          'Display this screen') do
    puts opts
    exit
  end
  opts.on('-t', '--type a,b,c',
          Array, 'Specify filetypes to flatten out (defaults to mp3,m4a)') do|t|
    options[:types] = t
  end
end
optparse.parse!

### the setup (ideally, this would be in a `let` statement)
origin = Dir.pwd
if not options[:types]
  options[:types] = ["mp3", "m4a"]
end

### the actual work
ARGV.each do |target|
  FileUtils.cd("#{origin}/#{target}")
  options[:types].each{|ext| `find -iname '*.#{ext}' -exec mv {} ./ \\\;`}
  `find -depth -type d -empty -exec rmdir {} \\\;`
end
FileUtils.cd(origin)

Am I doing anything eye-stabbingly wrong? Is there a better way of handling any of the above?

Specifically, I suspect that I could replace the find calls with native Ruby methods, but I can't see how without complicating it significantly. I also suspect that there's a better way of writing FileUtils.cd("#{origin}/#{target}"), but a cursory reading of Dir doesn't reveal anything. Any tips?

#!/usr/bin/ruby

require 'optparse'
require 'pp'
require 'fileutils'

### parsing command line options via optparse
options={:types => ["mp3", "m4a"]}
optparse = OptionParser.new do|opts|
  opts.on('-h', '--help', 'Display this screen') do
    puts opts
    exit
  end
  opts.on('-t', '--type a,b,c', Array, 'Specify filetypes to flatten out (comma separated, defaults to mp3,m4a)') do|t|
    options[:types] = t
  end
end
optparse.parse!

### the actual work
ARGV.each do |target|
  FileUtils.cd(target) do
    options[:types].each{|ext| `find -iname '*.#{ext}' -exec mv {} ./ \\\;`} ##move all matched files into [target]
    `find -depth -type d -empty -exec rmdir {} \\\;` ##recursively delete all empty folders in [target]
  end
end

options is now initialized beforehand rather than being checked after .parse!
using the block option on FileUtils.cd
removed useless final cd back
tried the glob option, but it just errors without continuing if it hits a matched file in target (ie. one that needs to stay where it is). This seems to happen when I pass :force => TRUE too. The find call notes such files, but still flattens what it can. The glob route also doesn't seem to be case-insensitive.

options={}
# ...
if not options[:types]
  options[:types] = ["mp3", "m4a"]
end

I would initialize options with the default values first, instead of checking whether the option is set afterwards. I.e.

options = {:types => ["mp3", "m4a"]}

options[:types].each{|ext| `find -iname '*.#{ext}' -exec mv {} ./ \\\;`}

This can be done with a glob. For this I'd first change options[:types] to be string (i.e. replace Array with String in the optparse code and use "mp3,m4a" instead of ["mp3", "m4a"] as the default. Then you can use Dir.glob with FileUtils.mv like this:

Dir.glob("**/*.{#{ ext }}") {|f| FileUtils.mv(f, ".")}

  I suspect that I could replace the find calls with native Ruby methods, but I can't see how without complicating it significantly.

For the first one see above. However the second one would indeed be more complicated to replace, so I'd just stick with the find call.

  I also suspect that there's a better way of writing FileUtils.cd("#{origin}/#{target}")

Yes, there is: cd optionally takes a block. When given a block, cd will cd into the given directory, execute the block, and then cd back. So you can get rid of origin and just do

FileUtils.cd(target) do
  ...
end

FileUtils.cd(origin)

Even without using the block version of cd the above line would be completely unnecessary. Changing the directory at the end of a script has no effect. Any cd performed in a process will never affect the parent process.


Case insensitively removing a substring, efficiently I am going through the CodingBat exercises for Java. I have just completed this one:

  Given two strings, base and remove, return a version of the base string where all instances of the remove string have been removed (not case sensitive). You may assume that the remove string is length 1 or more. Remove only non-overlapping instances, so with xxx removing xx leaves x.

I wanted to try using a StringBuilder to solve this because I have not done so before. Here is my code:

public String withoutString(String base, String remove){

    String removeLo = remove.toLowerCase();
    String removeHi = remove.toUpperCase();

    int rL = remove.length();

    StringBuilder s = new StringBuilder(base);

    for (int i = 0; i < s.length(); i++) {
        int j = s.indexOf(remove, i);

        if (j < 0) {
            break;
        } else {
            s.delete(j, j + rL);
        }
    }

    for (int i = 0; i < s.length(); i++) {
        int j = s.indexOf(removeLo, i);

        if (j < 0) {
            break;
        } else {
            s.delete(j, j + rL);
        }
    }

    for (int i = 0; i < s.length(); i++) {
        int j = s.indexOf(removeHi, i);

        if (j < 0) {
            break;
        } else {
            s.delete(j, j + rL);
        }
    }
    return s.toString();
}

The code repeats the for loop 3 times in order to search for the unaltered, lower case, and upper case versions of remove. My questions are:

Is it possible to reduce this down to one for loop, and is that practical (with good readability)?
The test strings involve digits. Is it problematic to be 'converting' these to lower and/or upper cases? Should I deal with those before testing the alphabetic strings?
The following test cases are the ones that require all three for loops:

withoutString("xxx", "x")
withoutString("1111", "1")
withoutString("MkjtMkx", "Mk")
withoutString("Hi HoHo", "Ho")

The rest of the tests work without the first for loop (unaltered remove). The second two make sense, because the remove string uses upper and lower case, but the first two don't follow this. I have used the debugger in Eclipse but I still can't figure this out. Can you please explain?

Regular expressions make your life really easy when solving such exercises:

public static String withoutString(String base, String remove) {
    return Pattern.compile(Pattern.quote(remove), Pattern.CASE_INSENSITIVE).matcher(base).replaceAll("");
}

I explain this code a little:

Pattern.compile(Pattern.quote(remove), Pattern.CASE_INSENSITIVE): Creates a regular expression pattern containing the string you want to have removed. Pattern.quote(remove) takes care of special characters that may be interpreted as a regular expression (such as: *, \, +, (), [] etc.). The flag Pattern.CASE_INSENSITIVE makes sure, that the case of the characters don't matter.
.matcher(base): returns a Matcher that holds all matches of the regular expression in the string base.
.replaceAll("");: replaces all matches that have been found with an empty string, effectively removing them.

You may have noticed, that I made this method static because it does not access any fields or methods that are non-static. This is always advised unless it really needs to be non-static for some reason.


Setting up longpolling JavaScript is the language everyone uses without bothering to learn it first, and I'm no exception. I'm trying to set up autorefresh with longpolling. What I have on the client side is the following:

var getAjaxer = function(element) {

    return function() {
        $.ajax({
            url: "/path/to/resource",
            success: function(response) {
                element.html(response);
            },
            dataType: "html",
            timeout: 10000,
            type: "POST",
            complete: arguments.callee
        });
    };
}

var startAjaxing = function() {
    myajaxer = getAjaxer($("#myid"));
    myajaxer();
}

$(startAjaxing);

This works, but I'm worried I might be doing it all wrong. Could I get some pointers on how I could use the language more effectively?

I can't say whether yours is right or wrong but there were a couple of things that seemed odd to me.

I wouldn't add functions to variables unless you intend to use them more than once. (or it adds to the readability)

var startAjaxing = function() {
    myajaxer = getAjaxer($("#myid"));
    myajaxer();
}

$(startAjaxing);

then becomes:

$(function() {
    getAjaxer($("#myid"))();
}); // Start ajaxing

Also I would get rid of arguments.callee and get the element every time:

var getAjaxer = null;
getAjaxer = function(elementid) {
    return function() {
        $.ajax({
            url: "/path/to/resource",
            success: function(response) {
                $("#" + elementid).html(response);
            },
            dataType: "html",
            timeout: 10000,
            type: "POST",
            complete: getAjaxer(elementid);
        });
    };
}

To ME this seems easier to read butt it may be only a matter of taste. (PS. I didn't check this for syntax errors etc.)

If you don't want to create functions every time?

getAjaxer = function(elementid) {
    var elementAjaxer = $("#" + elementid).data("ajaxer");
    if(elementAjaxer == null){
        elementAjaxer = function() {
            $.ajax({
            // etc etc
            });
        };
        $("#" + elementid).data("ajaxer", elementAjaxer);
    }
    return elementAjaxer;
}


Count the number of discrete values in a slice I'm using this right now to count the number of discrete values in a given []string. Is there a faster way to do this?

func CountDiscrete(x []string) int {
    discrete := 0

    for i,xi := range x {
        is_discrete := true
        for j,xj := range x {
            if i != j {
                if xi == xj {
                    is_discrete = false
                    break
                }
            }
        }

        if is_discrete {
            discrete = discrete + 1
        }
    }

    return discrete
}

Code review:
1. Use  discrete++ instead of discrete = discrete + 1
2. Use if i != j && xi == xj instead of

if i != j {
    if xi == xj {
        is_discrete = false
        break
     }
}

Don't use underscores in Go names: is_discrete, simply use discrete or isDiscrete.
You may use slice ...string instead of slice []string like the following code:

// O(n**2)
func distincts(x ...string) int {
    result := 0
    for i, xi := range x {
        unique := 1
        for j, xj := range x {
            if i != j && xi == xj {
                unique = 0
                break
            }
        }
        result += unique
    }
    return result
}

Also note the result += unique instead of if ....

Using map, the following code has the time complexity (asymptotic notation): O(n)

package main

import "fmt"

func main() {
    r := distincts("a", "b", "c", "a")
    fmt.Println(r) // 2

}

// O(n)
func distincts(slice ...string) int {
    result := 0
    m := map[string]int{}
    for _, str := range slice {
        m[str]++
    }
    for _, v := range m {
        if v == 1 {
            result++
        }
    }
    return result
}


Tracking stock prices using Python and Google Sheets The purpose of this program is to keep track of some stocks that we bought in a game and every week we must list the closing prices of each stock in a Google Sheet. I thought since I had a lot of different investments to keep track of I would write a quick script to do it for me. And I could have my friends in class send me their stocks as well to save them time.

I am just looking for any bad practices I might have in my code as well as any more efficient ways of carrying out some of the functions.

from yahoofinancials import YahooFinancials
import re
import pygsheets
import pandas as pd
import datetime

now = datetime.datetime.now()
time_now = (now.year, now.month, now.day)
time_now = str(time_now)

data = []
formatedData = []
shortData = []
everything = []
tickers = ['XOM', 'JNJ', 'TR', 'CRON', 'HSY', 'FL', 'PLNT', 'MCD', 'ARLP', 'LULU', 'RCII', 'DELL', 'DNKN', 'DIS']

def main(sheet, everything,place):
    gc = pygsheets.authorize(service_file='creds.json')

    # Create empty dataframe
    df = pd.DataFrame()

    # Create a column
    df['Closings: ' + time_now] = everything
    # open the google spreadsheet (where 'PY to Gsheet Test' is the name of my sheet)
    sh = gc.open('PY to Gsheet Test')

    # select the first sheet
    wks = sh[sheet]

    # update the first sheet with df, starting at cell A2.
    wks.set_dataframe(df, (1, place))  # up/down,left right

for i in tickers:
    tick = YahooFinancials(i)
    history = tick.get_historical_price_data('2019-03-07', '2019-03-08', 'daily')
    y = str(history)
    data.append(y)

for i in data:
    i = i[i.index('adjclose'):]
    i = re.findall("\d+\.-?\d*", i)
    i = str(i)
    if len(i) > 7:
        formatedData.append(i[2:9]) #the decimals were really long so this shortens them down a bit
    else:
        formatedData.append(i)

y = 0
for i in tickers:
    x = i
    x = x + " : " + formatedData[y]
    y = y + 1
    everything.append(x)

def get_name(person):
    if person == 'tommy':
        sheet = 0
    elif person == 'chad':
        sheet = 1
    elif person == 'kaya':
        sheet = 2
    elif person == 'twohey':
        sheet = 3
    elif person == 'majers':
        sheet = 4
    elif person == 'tori':
        sheet = 5
    elif person == 'kayla':
        sheet = 6
    place = input("What Week Is This? 1,2,3,4,5?: ") # This will move the entry one place over for each week
    place = int(place)
    main(sheet, everything,place)

print('DONT FORGET TO CHANGE TICKER')
person = input("Whos Data Is Going In: ")
get_name(person)

This looks very interesting. Well done!

Here is some nitpicking:

from yahoofinancials import YahooFinancials
import re
import pygsheets
import pandas as pd
import datetime

Organise these imports in following way: builtins, third-party then first-party.

 def main(sheet, everything,place):

Name of this function is sloppy. Use main function as an entry-point.
Name the function as a verb phrase or that explains what it is doing.
What is everything here? I can see that it is defined outside as well.
Avoid using everything as a name. It is very vague.

for i in tickers:
    tick = YahooFinancials(i)

What is i here? Reserve i for integer indexes. These are ticker_names.
for ticker_name in tickers is more readable.

tickers = ['XOM', 'JNJ', 'TR', 'CRON', 'HSY', 'FL', 'PLNT', 'MCD', 'ARLP', 'LULU', 'RCII', 'DELL', 'DNKN', 'DIS']

Load this from a JSON file.

def get_name(person):

This function doesn't get you anything. There is no return.
Why does this call main?
Name to sheet mapping can probably be loaded from a JSON as well.

adjclose, PY to Gsheet Test

Perform extract constant refactoring on strings like these and remove magic values.

  A magic number is a direct usage of a number in the code.

  For example, if you have (in Java):

  public class Foo {
    public void setPassword(String password) {
         // don't do this
         if (password.length() > 7) {
              throw new InvalidArgumentException("password");
         }
    }
}

  This should be refactored to:

  public class Foo {
    public static final int MAX_PASSWORD_SIZE = 7;

    public void setPassword(String password) {
         if (password.length() > MAX_PASSWORD_SIZE) {
              throw new InvalidArgumentException("password");
         }
    }
}

  From: https://stackoverflow.com/questions/47882/what-is-a-magic-number-and-why-is-it-bad

Summary - This code is very hard to understand. Functions do things that are completely different from function-name. Variable names are vague. Contains lot of hard-coded values that can be extracted to a JSON or some other kind of config. Contains magic values that should be converted to meaningful constants.


Display catalog product quantity I have a simple quantity display function that runs for every product displayed in our catalog.  The MaxQtyDisplay value is set in our web.config file.  Part of me thinks the function is as streamlined as possible and part of me thinks it can be improved upon somewhere.  Because this function is called many, many times in our web application I would like it be optimized as best it could.

What could be done to improve it?

Private Shared Function CalculateQtyDisplay(ByVal x As Double, ByVal y As Double) As Double
    Dim q As Double = x - y
    If q > 0 And q <= My.Settings.MaxQtyDisplay Then
        Return q
    ElseIf q > My.Settings.MaxQtyDisplay Then
        Return My.Settings.MaxQtyDisplay
    Else
        Return 0
    End If
End Function

I'm sure there will be elements of your application that are of much more concern, performance-wise, than this, but it could certainly be a little more clear, or maybe a little less superfluous...

Evaluation of and work within conditions is only required if q > 0
You really want to use AndAlso to apply short-circuiting in the first condition
q > MaxQtyDisplay is redundant since this condition wouldn't hit otherwise (forgetting negative quantities but we address that by addressing my first point)
Else is likewise not required.
Only get the value of MaxQtyDisplay from the settings context once (negligible in terms of performance really, since settings are loaded from file into memory on request)
Naming variables with single character identifiers doesn't optimise anything

So, for example, you can correct these as such:

Private Function CalculateQtyDisplay(ByVal x As Double, ByVal y As Double) As Double
    Dim result = 0.0
    Dim desiredQuantity As Double = x - y
    Dim maxQuantity = My.Settings.MaxQtyDisplay
    If (desiredQuantity > 0) Then
        If (desiredQuantity <= maxQuantity) Then
            result = maxQuantity
        Else
            result = desiredQuantity
        End If
    End If
    Return result
End Function

Any other modifications to this code are simply going to be aesthetic and preferential really, as far as I can see. For instance you could do away with maintaining result, this goes against what I recommend in another answer but it clearly boils down to context):

Dim desiredQuantity As Double = x - y
Dim maxQuantity = My.Settings.MaxQtyDisplay
If (desiredQuantity > 0) Then
    If (desiredQuantity <= maxQuantity) Then
        Return maxQuantity
    End If
    Return desiredQuantity
End If
Return 0

A modification that comes to mind which could potentially impact performance, if anything might, is to use the IIf function available to VB.NET (both TruePart and FalsePart of this call are evaluated, though I highly doubt you'll ever have any noticeable degradation with this method anyway, even if you used this method):

Dim quantity As Double = x - y
Dim maxQtyDisplay = 1800 'My.Settings.MaxQtyDisplay
If (quantity > 0) Then
    Return IIf(quantity <= maxQtyDisplay, quantity, maxQtyDisplay)
End If
Return 0


Creating and referencing markup....cloning, etc. efficient system for this plugin I am pulling multiple feeds from youtube that have a callback function which makes markup from each feed. They add that markup to a documentFragment stored in var data. When all the feeds have contributed what they should, a final callback runs. Each feed can deal with its own thumbnails and page menu, so it makes sense to get that started without waiting for all of the feeds to be done. Some markup requires all of them to be done, so I do that in the final callback. To complicate matters, the markup needs to be appended in an order dictated by the user and some sets of markup may need to be cloned if requested. This allows the user to easily build the plugin as they see fit. Here is the option for that.

build:['listsMenu', 'listTitle', 'listDesc', 'player', 'pageMenu', 'thumbnails']

or another possibility...something like this:

build:['player', 'listsMenu', 'pageMenu', 'thumbnails', 'pageMenu']

This is the best solution I have been able to come up with so far. Any suggestions are appreciated!

//in the function that makes the ajax calls -
    data = {};

namespace.thumbs.prototype.feedCallback = function(context, options, data, optList, listNum, feed) {
    if (options.build.indexOf('pageMenu') !== -1 || options.build.indexOf('thumbnails') !== -1) {
        var feedVideosInPages = this.return_feedVideosInPages(options.thumbCount, options.thumbsPerPage, feed.videos);
        var pagesMenu = this.return_PagesMenu(options, listNum, feedVideosInPages);
        var thumbnails = this.return_thumbsMarkup(options, optList, listNum, feedVideosInPages);
        data.pagesMenus = data.pagesMenus || document.createDocumentFragment();
        data.thumbnails = data.thumbnails || document.createDocumentFragment();
        if (pagesMenu) { data.pagesMenus.appendChild(pagesMenu); }
        data.thumbnails.appendChild(thumbnails);
    }

    data.each = data.each || [];
    data.each.push({'feed':feed, 'optList':optList, 'listNum':listNum});

    if (data.each.length === options.lists.length) { this.allFeedsReady(context, options, data); }
};
namespace.thumbs.prototype.allFeedsReady = function(context, options, data) {
    this.sort_dataEach(data);

    elems = document.createDocumentFragment();
    addedElemNames = [];
    clonedElems = {};
    toAdd = options.build.slice();
    var buildLength = options.build.length;
    for (var i=0; i<buildLength; ++i) {
        var name = options.build[i];
        if (addedElemNames.indexOf(name) === -1) { //this is the first time the element has been added.
            switch (name) {
                case 'pageMenu': elem = data.pagesMenus; break;
                case 'thumbnails': elem = data.thumbnails; break;
                case 'player': elem = this.return_playerDiv(context); break;
                case 'listMenu': elem = this.return_listsMenuMarkup(data.each); break;
                //case 'listTitle': elem = allData.markup.listsTitles[0]; break;
                case 'listDesc': elem = this.return_listsDescsMarkup(data.each); break;
                case 'videoTitle': elem = this.return_videoTitleMarkup(data.each); break;
                    default: elem = false;
            }
            addedElemNames.push(name);
        }
        else {
            elem = clonedElems[name];
        }
        toAdd.splice(toAdd.indexOf(name), 1); //remove this item from array of things to be added
        if (elem) {
            if (toAdd.indexOf(name) !== -1) { clonedElems[name] = elem.cloneNode(true); }//the build will contain another copy, so clone it
            elems.appendChild(elem);
        }
    }
    context.appendChild(elems);
}

From a once over:

data should be declared with var, so should a number of other variables. Please paste your code into the JSHint.com site and have a look
It is considered better form to have 1 comma separated var statement:

var feedVideosInPages = this.return_feedVideosInPages(options.thumbCount, options.thumbsPerPage, feed.videos),
    pagesMenu = this.return_PagesMenu(options, listNum, feedVideosInPages),
    thumbnails = this.return_thumbsMarkup(options, optList, listNum, feedVideosInPages);

return_feedVideosInPages is a terrible name, I assume you chose it because feedVideosInPages was already taken ;) I would for something shorter:

var feedVideos = this.getFeedVideosInPages(options.thumbCount, options.thumbsPerPage, feed.videos),

which is still horizontally challenged, since you call that function only once, and since thumbCount and thumbsPerPage are in the same convenient options object you could improve even further to:

var feedVideos = this.getFeedVideos(options, feed.videos),

Finally, if you think about it, since you get the videos from the feed, let getFeedVidoes decide/worry about which property of feed contains the videos:

var feedVideos = this.getFeedVideos(options, feed),

Bad (TMI):

if (toAdd.indexOf(name) !== -1) { clonedElems[name] = elem.cloneNode(true); }//the build will contain another copy, so clone it

Better ( Comment on top ):

//The build will contain another copy, so clone it
if (toAdd.indexOf(name) !== -1) { clonedElems[name] = elem.cloneNode(true); }

Even better ( never drop the newline, but you can drop the curlies! )

//The build will contain another copy, so clone it
if (toAdd.indexOf(name) !== -1)
  clonedElems[name] = elem.cloneNode(true);

The ~ operator ( Bitwise NOT ) is great for comparing to -1:

if (!~toAdd.indexOf(name))
  clonedElems[name] = elem.cloneNode(true);


Counting terms in Collatz sequences until 1 is reached or a limit is exceeded My friend and I are into competitive programming and we wrote two very similar programs for a problem. His got accepted with a runtime of 0.6 s while mine didn't get accepted even after 3 s.

I was hoping somebody could explain to me as to why the addition of one function slowed down my code so much.

Both programs produce the correct output. No issues on that front.

The Question: Given A, the initial value of a Collatz sequence, and some limit L, count the number of terms until either 1 is reached or L is exceeded.  A and L are both smaller than 231.

His Code

#include<iostream>

using namespace std;

int main(){
    long long A,L;
    int cont,caso=1;

    while(1){
        cin>>A>>L;
        if(A<0 && L<0) break;

        cout<<"Case "<<caso<<": A = "<<A<<", limit = "<<L<<", number of terms = ";
        caso++;

        cont=0;

        while(A<=L && A!=1){
            if(A%2==0) A/=2;
            else A=3*A+1;

            cont++;
        }

        if(A==1) cont++;

        cout<<cont<<endl;
    }

    return 0;
}

My Code

#include<iostream>

using namespace std;

bool even(int a)
{
    if(a%2==0)
        return true;
    else
        return false;
}

int main()
{
    int n=0;
    while(1)
    {
        long long int a, limit;
        cin>>a>>limit;
        if((a==-1) && (limit==-1))
            break;
        cout<<"Case "<<n<<": A = "<<a<<", limit = "<<limit<<", number of terms = ";
        int val=a;
        int count=0;
        while((val<=limit) && (val!=1))
        {
            if(even(val))
                val/=2;
            else
                val=(val*3) + 1;
            count++;
        }
        if(a==1)
            count++;
        cout<<count<<endl;
    }
    return 0;
}

Overflow Bug

The reason your program is timing out is because your variable val is defined as an int instead of a long long.  Because of this, val can become negative during the line val=(val*3) + 1, which leads to an infinite loop.  You should change the type of val to long long.  If you want to test the infinite loop yourself, try this test case and see what happens:

113383 2147483647

Output Bug

Your program has two output related errors:

You should initialize n=1 instead of n=0.
Your code is missing the line n++.

The above two problems mean that your program is outputting something like:

Case 0: A = 100, limit = 100000, number of terms = 25
Case 0: A = 101, limit = 100000, number of terms = 25

instead of the correct format of:

Case 1: A = 100, limit = 100000, number of terms = 25
Case 2: A = 101, limit = 100000, number of terms = 25


LRU Cache Design using LinkedHashMap This is a follow-on qestion from: LRU cache design

Revised Implementation

package lrucache;

import java.util.LinkedHashMap;

public class LRUCacheV2<K, V> extends LinkedHashMap<K, V>{

    private static final int MAX_ENTRIES = 3;
         /**
     * super class constructor arguments are
     * cache size = MAX_ENTRIES
     * load factor = 0.75F
     * access order = true
     */
    public LRUCacheV2(){
        super(MAX_ENTRIES,0.75F,true);
    }

    @Override
    protected boolean removeEldestEntry(java.util.Map.Entry<K, V> entry) {
        return size() > MAX_ENTRIES;
    }

    public static void main(String[] args) {
        LRUCacheV2<String, Integer> cache = new LRUCacheV2<String, Integer>();
        cache.put("abc", 1);
        cache.put("def", 2);
        cache.put("ghi", 3);
        cache.put("jkl", 4);
        cache.put("mno", 5);
        cache.put("abc", 1);
        System.out.println(cache);
    }
}

Output

{jkl=4, mno=5, abc=1}

Your code looks fine, depending on the use case in which it is applied. A few people have mentioned "composition over inheritance". This is a good point, but it is not directly practical in this situation. You need inheritance in order to get access to the functionality you use in the LinkedHashMap. But, there is an approach where you use inheritance to access the functionality, and composition to expose it.

If you assume the direct approach you have taken is useful for accessing additional functionality from the LinkedHashMap, then your code is good. The variables are well named, the code is consistent (there's a glitch on the one comment line...), and it's all fine.

Whether the hybrid approach is useful to you depends on whether your use case requires just a smallish subset of the the LinkedHashMap functionality. For example, if you needed to access the entries on the content as Map.Entry instances, or if you need to do more complicated iterations, then you start to lose the benefit of the composition.

In this case, the first approach you have using straight inheritance has a place. Th secnd approach requires wrapping the functionality you need from the private inner cache.

This second approach requires tailoring your LRU cache to its specific use cases. At the moment, you have a very general LRU cache. On the other hand, you are only using the put method (and presumably the get()). You can create a class with just these methods (and a selection of other ones that make sense) and gave an internal inherited instance for access to the LinkedHashMap. Consider the following:

public final LRUCacheVC<K,V>() {

    // use inheritence to access LinkedHashMap functionality.
    private static class AccessOrderCache<T,U> extends LinkedHashMap<T,U> {
        private final int capacity;
        AccessOrderCache(final int capacity) {
            super(capacity, 0.75f, true);
            this.capacity = capacity;
        }
        @Override
        protected boolean removeEldestEntry(java.util.Map.Entry<T, U> entry) {
            return size() > MAX_ENTRIES;
        }
    }

    // Use composition to access the Inherited Cache.
    private final AccessOrderCache<K,V> cache;

    public LRUCacheVC(final int capacity) {
        cache = new AccessOrderCache<>(capacity);
    }

    public V get(K key) {
        return cache.get(key);
    }

    public V put(K key, V value) {
        return cache.put(key, value)
    }

    @Override
    public int hashCode() {
        return cache.hashCode();
    }

    @Override
    public boolean equals(Object other) {
        return other != null
            && (other instanceof LRUCacheVC)
            && cache.equals(((LRUCacheVC)other).cache);
    }

    @Override
    public String toString() {
        return cache.toString();
    }
}

Note how you use inheritence to access the LinkedHashMap concepts, then use composition to incorporate that new class in to your code.

You only expose that limited functionality that you want (get and put), as well as a few other standard methods that make life easier.


Python Guess the Word Game Can the following code be simplified/ improved? (NOTE: I am yet to learn OOP/ def functions). I have not used OOP and def functions on purpose, as I am yet to learn this aspect of Python. I have listed some things I would like to improve on below.

Could/ Should I implement for loops?
Is the structure of the code of a good standard?
How can I simplify the code (using less loops/ less code in general)?

# Python 3.4.3 // MacOS (Latest)

import random

WORD = ('apple', 'oracle', 'amazon', 'microsoft')
word = random.choice(WORD)
correct = word
clue = word[0] + word[(len(word)-1):(len(word))]
letter_guess = ''
word_guess = ''
store_letter = ''
count = 0
limit = 5

print('Welcome to "Guess the Word Game!"')
print('You have 5 attempts at guessing letters in a word')
print('Let\'s begin!')
print('\n')

while count < limit:
    letter_guess = input('Guess a letter: ')

    if letter_guess in word:
        print('yes!')
        store_letter += letter_guess
        count += 1

    if letter_guess not in word:
        print('no!')
        count += 1

    if count == 2:
        print('\n')
        clue_request = input('Would you like a clue?')
        if clue_request == 'y':
            print('\n')
            print('CLUE: The first and last letter of the word is: ', clue)
        if clue_request == 'n':
            print('You\'re very brave!')

print('\n')
print('Now its time to guess. You have guessed',len(store_letter),'letters correctly.')
print('These letters are: ', store_letter)

word_guess = input('Guess the whole word: ')
while word_guess:
    if word_guess.lower() == correct:
        print('Congrats!')
        break

    elif word_guess.lower() != correct:
        print('Unlucky! The answer was,', word)
        break

print('\n')
input('Press Enter to leave the program')

In this case it's totally fine to not define any functions or classes. It does not always make sense to use those things just because you can.

In this case for loops do indeed make sense.

The structure is pretty straight forward, but you should revise what if, else and elif do.

correct = word

Not quite sure what you are doing here by having two variables with the same content.

clue = word[0] + word[(len(word)-1):(len(word))]

You can use negative indexes to count from the end of the sequence:

That would make this line:

clue = word[0] + word[-1]

letter_guess = ''
word_guess = ''
store_letter = ''
count = 0
limit = 5

you could use more descriptive names like: guess_limit, guess_count etc. Some of these variables don't need to be set here yet, e.g. letter_guess.

while count < limit:

Here you would use a for loop (for guess_count in range(limit)) like you suspected.

    letter_guess = input('Guess a letter: ')

I recommend you add some checking here to see if the letter is actually valid.

    if letter_guess in word:
        ...

    if letter_guess not in word:

If it was in the word previously, it still is. No need to re-check, you can just use else

            print('\n')

you can just use an empty print() to print a new line

        if clue_request == 'y':
            ...

        if clue_request == 'n':
            ...

what happens here if clue_request is neither 'y' nor 'n'?

while word_guess:

what is the purpose of this while loop? All cases hit break

    if word_guess.lower() == correct:
        ...

    elif word_guess.lower() != correct:
        ...

You can just use else here again

Full program:

"""
Word Guess Game

by Coal_
"""
import random

WORDS = ('apple', 'oracle', 'amazon', 'microsoft')
correct_word = random.choice(WORDS)
clue = correct_word[0] + correct_word[-1]

store_letter = ''
limit = 5

print('Welcome to "Guess the Word Game!"')
print('You have 5 attempts at guessing letters in a word')
print('Let\'s begin!')
print()

for guess_count in range(limit):
    while True:
        letter_guess = input('Guess a letter: ')

        if len(letter_guess) == 1:
            break
        else:
            print("Oops! Guess a letter!")

    if letter_guess in correct_word:
        print('yes!')
        store_letter += letter_guess
    else:
        print('no!')

    if guess_count == 2:
        print()
        clue_request = input('Would you like a clue?')
        if clue_request.lower().startswith('y'):
            print()
            print('CLUE: The first and last letter of the word is: ', clue)
        else:
            print('You\'re very brave!')

print()
print('Now its time to guess. You have guessed', len(store_letter), 'letters correctly.')
print('These letters are: ', store_letter)

word_guess = input('Guess the whole word: ')
if word_guess.lower() == correct_word:
    print('Congrats!')
else:
    print('Unlucky! The answer was,', correct_word)

print()
input('Press Enter to leave the program')


Is this quicksort efficient and acceptable? In order to complete one of my assignment I needed to include a quicksort so I was wondering if this is correct and acceptable.

The code:

    private ArrayList<String> sort(ArrayList<String> ar, int lo, int hi){
        if (lo < hi){
            int splitPoint = partition(ar, lo, hi);
            sort(ar, lo, splitPoint);
            sort(ar, splitPoint +1, hi);
        }
        return ar;
    }

    private int partition(ArrayList<String> ar, int lo, int hi){
        String pivot = ar.get(lo);
        lo--;
        hi++;
        while (true){
            lo++;
            while (lo<hi && ar.get(lo).compareTo(pivot) < 0){
                lo++;
            }
            hi--;
            while (hi>lo && ar.get(hi).compareTo(pivot) >= 0){
                hi--;
            }
            if (lo<hi){
                swap(ar, lo, hi);
            }else {
                return hi;
            }
        }
    }

The additional swap method:

    private ArrayList<String> swap(ArrayList<String> ar, int a, int b){
        String temp = ar.get(a);
        ar.set(a, ar.get(b));
        ar.set(b, temp);
        return ar;
    }

If this is incorrectly done can you please give me some pointers and tell me why. Otherwise I would gladly accept any suggestions you have.

Those are pure functions that don't access any instance variables.  You can therefore make them static.

You could use generics more generically.  Your code should work with any List, as long as it supports an efficient .get(i) for arbitrary i.  The elements of the list can be any kind of Comparable object.

The .sort() function should have a void return type to avoid giving the false impression that it returns a copy of the original array.

private static <E extends Comparable<E>, L extends List<E> & RandomAccess>
void sort(L ar, int lo, int hi) {
    if (lo < hi) {
        int splitPoint = partition(ar, lo, hi);
        sort(ar, lo, splitPoint);
        sort(ar, splitPoint +1, hi);
    }
}

private static <E extends Comparable<E>, L extends List<E> & RandomAccess>
int partition(L ar, int lo, int hi){
    E pivot = ar.get(lo);
    lo--;
    hi++;
    while (true){
        lo++;
        while (lo<hi && ar.get(lo).compareTo(pivot) < 0){
            lo++;
        }
        hi--;
        while (hi>lo && ar.get(hi).compareTo(pivot) >= 0){
            hi--;
        }
        if (lo<hi){
            swap(ar, lo, hi);
        }else {
            return hi;
        }
    }
}

private static <E extends Comparable<E>, L extends List<E> & RandomAccess>
void swap(L ar, int a, int b){
    E temp = ar.get(a);
    ar.set(a, ar.get(b));
    ar.set(b, temp);
}