Return sizeof a struct containing members ordered like given template parameters template<typename... T>
constexpr std::size_t sizeof_struct()
{
    // Helper
    auto alignto = [](auto x, auto a) { return (x + a - 1) / a * a; };
    // Algorithm
    std::size_t max_align{0};
    std::array<std::pair<std::size_t, std::size_t>, sizeof...(T)> arr{{
        {alignof(T), sizeof(T)}...
        }};
    auto sum = std::accumulate(
        std::begin(arr), std::end(arr), std::size_t{0},
        [&max_align, alignto] (auto s, auto&& it) {
            max_align = std::max(it.first, max_align);
            return alignto(s, it.first) + it.second;
        });
    return max_align ? alignto(sum, max_align) : 1;
}

Is there any type I can put in the parameter pack T... that can break this code?  Anything else that doesn't look right?  Anything I can clean or make more concise?

Looks reasonable to me.

I don't understand why you keep sum.first and sum.second as separate values; IIUC, sum.first is "the size of the struct so far, minus its last element", and sum.second is "the size of the last element". It seems like you could just keep "the size of the struct so far" as a single value. That is, isn't your code equivalent to the following?

template<typename... T>
constexpr std::size_t sizeof_struct()
{
    using meta_t = std::pair<std::size_t, std::size_t>;
    auto alignto = [](auto x, auto a) { return (x + a - 1) / a * a; };
    size_t sum{0};
    std::size_t max_align{0};
    std::array<meta_t, sizeof...(T)> arr{{ {alignof(T), sizeof(T)}... }};
    for (auto&& it : arr) {
        sum = alignto(sum, it.first) + it.second;
        max_align = std::max(it.first, max_align);
    }
    return sum ? alignto(sum, max_align) : 1;
}

Or if not, why not?

In the above version I also "corrected" your brace placement on the std::array initializer; I think it's more idiomatic (and in this case specifically, more readable) to bundle together the outer pair of braces {{ {x}... }} as opposed to staggering them out as { {{x}...} }.

Also for (x : y) is more idiomatic than for (x: y), and you had a trailing redundant semicolon on the for-loop's body.

If you're looking for template type parameters that break the above code, try sizeof_struct<char&>. Your code returns 1, but obviously it should return sizeof(char*). You can solve that easily, though.

And of course you have no guarantee that the compiler's struct layout algorithm is equivalent to the one you've written. It almost certainly is (modulo base classes, vptrs, bitfields, and most importantly the fact that the compiler can reorder public and private data members)... but technically you have no portable way to know that.


Collatz sequence using Python 3.x I created this program originally for Project Euler 14.

Here's the code:

from sys import setrecursionlimit
setrecursionlimit(10 ** 9)

def memo(f):
    f.cache = {}

    def _f(*args):
        if args not in f.cache:
            f.cache[args] = f(*args)
        return f.cache[args]

    return _f

@memo
def collatz(n):
    if n == 1:
        return 1

    if n % 2:
        print(3 * n + 1)
        return 1 + collatz(3 * n + 1)

    if not n % 2:
        print(n // 2)
        return 1 + collatz(n // 2)

if __name__ == '__main__':
    number = None

    while True:
        try:
            number = input('Please enter a number: ')
            number = int(number)
            break

        except ValueError:
            print(f'Value has to be an interger instead of "{number}"\n')

    print('\nThe collatz sequence: ')

    length = collatz(number)

    print(f'\nLength of the collatz sequence for {number}: {length}')

I would like to make this faster and neater if possible.

Useless Code

Your @memo-ization doesn't do anything of value:

Your mainline calls collatz() exactly once.  Unless given the value 1, the collatz() function calls itself with 3*n + 1 or n // 2, and since the Collatz sequence doesn't have any loops until the value 1 is reached, it will never call itself with a value it has already been called with.  So, you are memoizing values which will never, ever be used.
Your collatz() function doesn't just return a value; it has a side-effect: printing!  If you did call collatz() with a value that has already been memoized, nothing will be printed.

Tail Call, without Tail Call Optimization

You've increased the recursion limit to \$10^9\$ stack levels, which is impressive.  But the algorithm doesn't need to be recursive.  A simple loop would work.  And since Python cannot do Tail Call Optimization, you should replace recursion with loops wherever possible.  Doing so eliminates the need for the increased stack limit:

def collatz(n):
    count = 1

    while n > 1:
        if n % 2 != 0:
            n = n * 3 + 1
        else:
            n //= 2

        print(n)
        count += 1

    return count

Separate Sequence Generation from Printing

You can create a very simple Collatz sequence generator:

def collatz(n):
    while n > 1:
        yield n
        n = n * 3 + 1 if n % 2 else n // 2
    yield 1

Using this generator, the caller can print out the Collatz sequence with a simple loop:

for x in collatz(13):
    print(x)

Or, if the caller just wants the length of the Collatz sequence, without printing out each item in the sequence, you can determine the length of the sequence with len(list(collatz(13))).  Better would be to count the items returned by the generator without realizing the list in memory: sum(1 for _ in collatz(13)).

Project Euler 14

The above generator works great for determining the sequence from any arbitrary value.  If you want to compute the length of collatz(n) for \$1 \le n \le 1,000,000\$, you may want to return to memoization.  However, this is Code Review, and while you alluded to PE14, you didn't actually provide code for that, so that cannot be reviewed here.


Check if images are loaded (ES6 Promises) I wrote a small function that checks if an image or multiple images are loaded. For that purpose, I decided to use ES6 promises, but I'm not really sure if my way of handling errors is the best way.

const loadImg = function loadImg(src) {
    'use strict';

    const paths = Array.isArray(src) ? src : [src];
    const promise = [];

    paths.forEach((path) => {
        promise.push(new Promise((resolve, reject) => {
            const img = new Image();

            img.onload = () => {
                resolve({
                    path,
                    status: 'ok',
                });
            };

            // Call `resolve` even if the image fails to load. If we were to
            // call `reject`, the whole "system" would break
            img.onerror = () => {
                resolve({
                    path,
                    status: 'error',
                });
            };

            img.src = path;
        }));
    });

    return Promise.all(promise);
};

// Usage
loadImg([
    'IMG.png',
    'IMG.jpg',
    'IMG.jpg',
    'IMG.jpg',
]).then(function(e) {
    console.log(e);
});

It works, but I'm worried that using objects as error messages rather than reject has some disadvantages that I'm not aware of (e.g. potential memory leaks or people don't expect to see errors in then())? Is there any way I can improve this function?

Well, you normally don't want to use the Promise constructor in your higher level code, you want to Promisify as low as possible. So let's create a function that checks for a single image, and resolves whenever you know the status of that image:

// When there's only one statement, you can drop the {} and the return
// x => y is equivalent to x => { return y; }
const checkImage = path =>
    new Promise(resolve => {
        const img = new Image();
        img.onload = () => resolve({path, status: 'ok'});
        img.onerror = () => resolve({path, status: 'error'});

        img.src = path;
    });

Now, with that Promise returning action, we can do some fancy things:

const loadImg = paths => Promise.all(paths.map(checkImage))

Or, if you want to get fancier

const loadImg = (...paths) => Promise.all(paths.map(checkImage));
// and call with
loadImg('path1', 'path2', 'path3');
// or
loadImg(...arrayOfPaths)

It's perfectly fine to not reject if there's no error. Yes, there was an error loading the image, but it isn't an exceptional situation, it's not something you'd throw because of. Your function is actually there to check if the image loaded fine or not.


Simple recursive transformation solution in Python This simple Python code I wrote for a problem asked during an interview. The problem statement is:

  Given a tuple (a, b), transform this into (c, d) by applying any of the two below operations multiple time in any order you like:

  (a, b) -> (a+b, b)
  (a, b) -> (a, a+b)

  The program should take in a, b, c, and d. And tells us if it is possible to transform (a, b) to (c, d).

  Edit: a, b, c and d can be negative integers.

This recursive solution seems correct but recursion depth increases very fast for even slight increase in input size. I feel I am missing some easy optimization here. I would be glad for any kind of inputs be it style, algorithm choice, speed etc.

def intro():
  print "This program will check if transformation (a, b) -> (c, d) is possible by recursive application of any of the below rules:"
  print "1. (a, b) => (a+b, b) \n2. (a, b) => (a, a+b)\n"

def user_input():
  print "Enter space separated four integers a, b, c and d:\n"
  a, b, c, d = map(int, raw_input().split(' '))
  return a, b, c, d
  # print 'a = ' + a + ' b = ' + b + ' c = ' + c + ' d = ' + d

def solve(a, b, c, d):
  print '(a: {}, b: {})  (c: {}, d:{})'.format(a,b,c,d)
  if a == c and b == d:
    return True
  elif a > c or b > d:
    return False
  elif a == c and b != d:
    return solve(a, a+b, c, d)
  elif a !=c and b == d:
    return solve(a+b, b, c, d)
  else:
    return solve(a+b, b, c, d) or solve(a, a+b, c, d)

if __name__ == '__main__':
  intro()
  a, b, c, d = user_input()
  if solve(a, b, c, d):
    print "Transformation possible"
  else:
    print "Transformation not possible"

Your code works correctly – as far as I can see – for strictly positive
integers \$ a, b, c, d \$. It fails if the integers are allowed to
be zero or negative. For example:

solve(-1, -1, -2, -3) returns False although \$(-1, -1) \$ can
be transformed to \$ (-2, -3) \$.
solve(0, 0, 1, 1) fails with a “maximum recursion depth exceeded”
because it calls itself with the same arguments.

The remainder of this review is based on the assumption that
\$ a, b, c, d > 0\$.

The number of recursive calls increases quickly with larger input values
because in most cases you have two possible branches at

return solve(a+b, b, c, d) or solve(a, a+b, c, d)

As an example, solve() is called

\$29\$ times to compute that \$(4, 6)\$ is not
reachable from \$(1, 1)\$, and
\$8189\$ times to compute that \$(127, 99)\$ is reachable
from \$(1, 1)\$.

The problem is that one does not know if \$(a, b)\$ should become \$(a+b, b)\$
or \$(a, a+b)\$ in order to reach \$(c, d)\$.

The “trick” to do the transformations backwards. If \$(c, d)\$ is
reached eventually, then the previous pair must be either
\$(c-d, d)\$ or \$(c, d-c)\$. But only one of these is possible, depending on the
sign of \$ c-d \$.

That leads to the following implementation:

def solve(a, b, c, d):
  if a == c and b == d:
    return True
  elif a > c or b > d:
    return False
  elif c >= d:
    return solve(a, b, c - d, d)
  else:
    return solve(a, b, c, d - c)

which is considerable faster: Now solve() is called

\$4\$ times to compute that \$(4, 6)\$ is not
reachable from \$(1, 1)\$, and
\$14\$ times to compute that \$(127, 99)\$ is reachable from \$(1, 1)\$.

The method can still fail with a “maximum recursion depth exceeded” runtime
error. But with this changed algorithm it is easy to replace the
recursion by an iteration:

def solve(a, b, c, d):
  while c >= a and d >= b:
    if c == a and d == b:
        return True
    (c, d) = (c - d, d) if c >= d else (c, d - c)
  return False

The algorithm itself can be improved further by replacing multiple transformations
$$
(c, d) \to (c-d, d) \to (c-2d, d) \to \ldots
$$
by a single transformation
$$
   (c, d) \to (c - kd, d)
$$
with a suitable integer \$k\$. How large can \$k\$ be chosen?
If that reminds you of the Euclidean algorithm for computing the greatest common divisor then you are on the right
track to implement an optimal method.


Better way to merge pdfs I am trying to merge two pdfs - one from a razor view and one from a file on the server.  To create the pdf from the razor view I have used the following nuget package: MVCRazorToPDF.

Unfortunately this didn't allow me to merge an existing file with the pdf so I have downloaded the project and changed the PdfActionResult to accept a mergeFilename.

I then generate the new pdf using the following:

public byte[] GeneratePdfOutput(ControllerContext context, object model = null, string viewName = null,
    Action<PdfWriter, Document> configureSettings = null, string filename = null)
{
    byte[] output;

    if (!string.IsNullOrEmpty(filename) && File.Exists(filename))
    {
        using (var copyDoc = new Document())
        {
            using (var copyStream = new MemoryStream())
            {
                copyDoc.Open();

                PdfCopy copy = new PdfCopy(copyDoc, copyStream);
                copy.CloseStream = false;
                copy.AddDocument(new PdfReader(GetBytes(context, model, viewName, configureSettings)));

                using (var reader = new PdfReader(filename))
                {
                    reader.ConsolidateNamedDestinations();

                    for (int i = 1; i <= reader.NumberOfPages; i++)
                    {
                        PdfImportedPage page = copy.GetImportedPage(reader, i);
                        copy.AddPage(page);
                    }
                }

                copy.Close();
                copyDoc.Close();
                output = copyStream.ToArray();
            }
        }
    }
    else
    {
        output = GetBytes(context, model, viewName, configureSettings);
    }

    return output;
}

private byte[] GetBytes(ControllerContext context, object model = null, string viewName = null, Action<PdfWriter, Document> configureSettings = null)
{
    if (viewName == null)
    {
        viewName = context.RouteData.GetRequiredString("action");
    }

    context.Controller.ViewData.Model = model;

    using (var document = new Document())
    {
        using (var workStream = new MemoryStream())
        {
            PdfWriter writer = PdfWriter.GetInstance(document, workStream);
            writer.CloseStream = false;

            if (configureSettings != null)
            {
                configureSettings(writer, document);
            }

            document.Open();

            using (var reader = new StringReader(RenderRazorView(context, viewName)))
            {
                XMLWorkerHelper.GetInstance().ParseXHtml(writer, document, reader);
                document.Close();
                return workStream.ToArray();
            }
        }
    }
}

I was wondering if there was anyway to do this so that the GetBytes was part of the first function - ie I open a new document and then add the razor and if there is a file on the server, I add that using the same memory stream and document (rather than having to create new ones), without the need for the else

If you change the first line from

byte[] output;

to

byte[] output = GetBytes(context, model, viewName, configureSettings);

and then revert the if condition you can return early.

So the remaining code (if the filename is valid and the file exists) could become

by stacking the usings which saves some horizontal spacing
by using the byte[] output for the PdfReader ctor
and using copyStream.GetBuffer() instead of calling its ToArray() method which eleminates the creation of an additional array

like so

public byte[] GeneratePdfOutput(ControllerContext context, object model = null, string viewName = null,
    Action<PdfWriter, Document> configureSettings = null, string filename = null)
{
    byte[] output = GetBytes(context, model, viewName, configureSettings);

    if (string.IsNullOrEmpty(filename) || !File.Exists(filename))
    {
        return output;
    }

    using (var copyDoc = new Document())
    using (var copyStream = new MemoryStream())
    {
        copyDoc.Open();

        PdfCopy copy = new PdfCopy(copyDoc, copyStream);
        copy.CloseStream = false;
        copy.AddDocument(new PdfReader(output));

        using (var reader = new PdfReader(filename))
        {
            reader.ConsolidateNamedDestinations();

            for (int i = 1; i <= reader.NumberOfPages; i++)
            {
                PdfImportedPage page = copy.GetImportedPage(reader, i);
                copy.AddPage(page);
            }
        }

        copy.Close();
        copyDoc.Close();
        return copyStream.GetBuffer();
    }
}


Increment value in a list of dictionaries I have a list of dictionary let's say : l=[{'a':1,'b':2},{'a':5,'b':6},{'a':3,'b':2}]
and I want to have another list l2 and add 4 to the 'a' in the all dictionaries
l2=[{'a':5,'b':2},{'a':9,'b':6},{'a':7,'b':2}]
(the first list l shouldn't change)
so I am doing it this way but i feel there is better, it is a bit hard coded I think :
 l=[{'a':1,'b':2},{'a':5,'b':6},{'a':3,'b':2}]
l2=[d.copy() for d in l]
for v in l2:
    v['a']=v['a']+4

Should have spaces around =, and += would shorten it a bit. And I'd be consistent with the variable name (not switch from d to v) and use better names for the lists (l looks too much like 1 and is just too short for something presumably living longer (unlike d, which I find ok in such small contexts)):
lst2 = [d.copy() for d in lst]
for d in lst2:
    d['a'] += 4

I'd say that's alright then. But here are two alternatives:
>>> [d | {'a': d['a'] + 4} for d in lst]
[{'a': 5, 'b': 2}, {'a': 9, 'b': 6}, {'a': 7, 'b': 2}]

>>> [{**d, 'a': d['a'] + 4} for d in lst]
[{'a': 5, 'b': 2}, {'a': 9, 'b': 6}, {'a': 7, 'b': 2}]


My favorite privacy settings I'm looking for general feedback on this module.  Here is what it handles on the page:

This is a private "class" meaning none of its properties are available outside of it.

init is run at run-time.  pre sets up parameters to be sent to the server via ajax and post receives the ajax response.

I can post the code for Class.create if needed, but I wanted to focus on the class itself.

$A.Class.create('private', {
    Name: 'Settings',
    E: {
        save_button:  '#save_button',
        cancel_button:  '#cancel_button',
        se_form: '#se_form'
    },
    init: function () {
        var self = this;
        $(this.E.save_button).on("click", function () {
            $A.Packet.push({model: self.Name});
        });
        $(this.E.cancel_button).on("click", function () {
            Page.flip('fave');
        });
    },
    pre: function (packet) {
        packet.h_token = Storage.getRemembered();
        packet.privacy = this.getSaved();
        packet.state = true;
        return packet;
    },
    post: function (packet) {
        Page.flip('fave_custom');
        return packet;
    },
    getSaved: function () {
        var form_elements = this.E.se_form.elements,
            send_value = '',
            iterator;
        for (iterator = 0; iterator < form_elements.length; iterator++) {
            if (form_elements[iterator].checked) {
                send_value = form_elements[iterator].value;
            }
        }
        return send_value;
    }
});

The main improvement with this code would be your handling of jQuery elements.  You should save them in the object literal like you did with plain elements.

Secondly, you can shorten up your code, using jQuery to pull the radio button value.

Your code might be a bit faster, as it works at a lower abstraction level, but if you do choose to use it, it might be better to abstract it out into a DOM function.

Below is the modified code.

$A.Class.create('private', {
    Name: 'Settings',
    J: {
        save_button:          $('#save_button'),
        cancel_button:        $('#cancel_button')
    },
    init: function () {
        var self = this;
        this.J.save_button.on("click", function () {
            $A.Packet.push({model: self.Name});
        });
        this.J.cancel_button.on("click", function () {
            Page.flip('fave');
        });
    },
    pre: function (packet) {
        packet.h_token = Storage.getRemembered();
        packet.privacy = this.getSaved();
        packet.state = true;
        return packet;
    },
    post: function (packet) {
        Page.flip('fave_custom');
        return packet;
    },
    getSaved: function () {
        return $('input[name=privacy]:checked', '#se_form').val();
    }
});

/* Put in a DOM abstraction library

            var form_elements = this.E.se_form.elements,
                send_value = '',
                iterator;
            for (iterator = 0; iterator < form_elements.length; iterator++) {
                if (form_elements[iterator].checked) {
                    send_value = form_elements[iterator].value;
                }
            }
            return send_value;
        }
*/


FizzBuzz in Common Lisp Where I stand on Lisp

So, Lisp. It's been on my radar for years and I'm finally getting around to learning it. The syntax is foreign but I find I'm having the most difficulty in the areas of semantics and the standard library. The symbol semantics are giving me tremendous trouble for sure, mainly because I find it hard to tell when a symbol is needed as opposed to a value. It probably sounds silly, but I find C pointers to be more intuitive and straightforward, and this is probably owed to the fact that C is exclusively pass-by-value, whereas Lisp is ostensibly both pass-by-value and pass-by-reference, and the semantics therein--which behavior is chosen and when--is unknown to me. For example, why is (1 2 3) inappropriate in a situation where '(1 2 3) and list--assuming its value is (1 2 3)--are perfectly okay? Ostensibly, this is an ambiguity in the syntax and Lisp thinks I'm trying to call a function called 1. This type of situation stresses me out precisely because I start thinking of the unknown unknowns--what other situations like this are going to come along and derail me or cause bugs that remain hidden for extended periods of time? I imagine, perhaps without warrant, disasters on par with Undefined Behavior in C.

The code to be reviewed

Long intro, but since this is a code review, I feel it should probably be made clear where I stand so that reviews can take it into account and use it to more effectively communicate the review. I.e. I am a seasoned programmer, I dabble in "language lawyering", and I'm a complete beginner in Lisp.

That said, my first Common Lisp project is FizzBuzz. And what better way to get more comfortable with the language than to put my newbie code out for other people to systematically tear apart?

#|| divisible()
 || I don't usually write functions for something as obvious as
 || "is X divisible by Y", but in this case, I find
 ||
 ||     (divisible 15 5)
 ||
 || easier to read than
 ||
 ||     (= 0 (mod 15 5))
 ||
 || feel free to refute if you disagree and think this is a
 || code smell or you know of a more idiomatic way
 ||
 || Also feel free to say something if you feel
 || this C-like multi-line style is in bad taste
 ||#
(defun divisible (a b) (= 0 (mod a b)))

#|| range()
 || Copy/pasted from somewhere else. I don't know what cons does
 || I do understand that the function is recursive
 || I do not think I would have thought of this on my own
 ||#
(defun range (min max)
  (when (<= min max)
    (cons min (range (+ min 1) max))))

(defun fizzbuzz (n)
  (setf out (string ""))

  (unless (or (divisible n 3) (divisible n 5))
    (return-from fizzbuzz n))

  (when (divisible n 3)
    (setf out (format nil "~d~d" out "Fizz")))

  (when (divisible n 5)
    (setf out (format nil "~d~d" out "Buzz")))

  out)

(setf 1to100 (range 1 100))
(setf 1to100fb (map 'list #'fizzbuzz 1to100))
(mapcar #'(lambda (str) (format t "~d~%" str)) 1to100fb)

I admit, it's hard to break away from imperative programming.

The style isn't completely foreign to me, actually. I've already used (and am very much a fan of) style similar to this in C-like languages...

while(condition)
    if(condition)
        do_something;

And I'm already a fan of functional programming...

return list.map(x => (x * x) % 256).filter(x => x >= 10);

So I believe I'm on the right track. Although Dunning-Kruger is always in effect.

1. Divisible function

It is perfectly fine to define a function like divisible to improve the reading of a program. In general, when defining a function that could be reused, it is a good practice to comment its meaning (and this is encouraged by the syntax of the language):

(defun divisible (a b)
  "a is evenly divisible by b"
  (zerop (mod a b)))

Note the idiomatic use of the predicate (zerop x) instead of (= 0 x).

2. Introduction of variables

The use of setf (or setq) to introduce new variables is wrong (you should have received a warning, something like In FIZZBUZZ: Undeclared free variable OUT). The setf and setq operators should be used only to modify them.

Instead, you can introduce local variables with the let syntax:

(let ((new-local-variable1 initial-value)
      (new-local-variable2 initial-value)
      ...)
  body-in-which-the-new-local-variables-are-bound)

So fizzbuzz could be redefined as:

(defun fizzbuzz (n)
  "return n or Fizz or Buzz or FizzBuzz"
  (let ((out (string "")))

    (unless (or (divisible n 3) (divisible n 5))
      (return-from fizzbuzz n))

    (when (divisible n 3)
      (setf out (format nil "~d~d" out "Fizz")))

    (when (divisible n 5)
      (setf out (format nil "~d~d" out "Buzz")))

  out))

The global variables are instead introduced by defvar, so you could write for instance:

(defvar 1to100 (range 1 100))
(defvar 1to100fb (map 'list #'fizzbuzz 1to100))

3. The algorithm

The function range actually builds a list of (max-min+1) elements, filled with the values min, min+1, min+2, ..., max. The use of map over 1to100 is quite an expensive way of performing a simple iteration from the integer min to the integer max! Others, much more efficient ways, exist. For instance, you could do a classical loop (think of a for in a C-like language):

(loop for i from 1 to 100
  do (format t "~d~%" (fizzbuzz i)))

Another possibility is to use the dotimes operator:

(dotimes (i 100)
   (format t "~d~%" (fizzbuzz (1+ i))))

that iterates from 0 to (but not included) 100 (so there is the need of use the 1+ function).

If, on the other hand, you want to go for recursion, here is a possible solution (which is tail-recursive, so compiled by most systems as an iterative loop):

(defun fizzbuzz-to (limit &optional (start 1))
  (when (<= start limit)
    (format t "~d~%" (fizzbuzz start))
    (fizzbuzz-to limit (1+ start))))

4. Types

In a format string, for a general printing ~a should be used instead of ~d, which mainly is used to print integer (decimal) numbers. It can be used also for strings, but this is due to the fact that in the specification it is said that:

  if arg is not an integer, it is printed in ~A format and decimal base.

What I do not like more, however, is the fact that the function fizzbuzz sometimes returns an integer, sometimes a string. The fact that Common Lisp has dynamic typing in some way hides this fact, but I think that the program would be more elegant (and in some sense more correct) if the result has always the same type.

One way is to redefine the function in this way:

(defun fizzbuzz (n)
  (cond ((divisible n 3)
         (if (divisible n 5)
             "FizzBuzz"
             "Fizz"))
        ((divisible n 5) "Buzz")
        (t (format nil "~d" n))))

For other solutions, you could for instance look at this CR question.


Produce product of lists I want to combine elements in different lists.
For example, here are 3 lists.

list_a : [ 1,2,3 ]
list_b : [ 4,5,6 ]
list_c : [ 7,8 ]

Selecting elements from list_b and list_c is mutually exclusive.  When including an element from list_b, I'd like to also include an element from list_bb; when including an element from list_c, I'd like to also include an element from list_cc.

list_bb : [ 14,15,16 ]
list_cc : [ 17,18 ]

I wrote this code:

global sum_list=[]
for a in list_a:
    for b in list_b:
        for bb in list_bb:
            sum_list.append([a,b,bb])
for a in list_a:
    for c in list_c:
        for cc in list_cc:
           sum_list.append([a,c,cc])

Is there any better way to get the same result?

The itertools library has a function called product. You can rewrite your snippet as:

global sum_list=itertools.product(list_a, list_b, list_bb) + itertools.product(list_a, list_c, list_cc)

If you don't want to use itertools (e.g. because you're stuck on python 2.4), you can also use a list comprehension:

global sum_list=[]
sum_list.extend([a,b,bb] for a in list_a for b in list_b for bb in list_bb)
sum_list.extend([a,c,cc] for a in list_a for c in list_c for cc in list_cc)


Generate next combination in lexicographic order C++'s standard library has a std::next_permutation algorithm but no next_combination. The reason behind this absence is, I guess, that one of the easiest and fastest way to generate combinations one at a time is to rely on the permutations of a vector of boolean values, which is then used as a sieve to retain the elements in the combination. The drawback is that you need to maintain some state to compute the next permutation.

Here's a try at a STL style next_combination state-less algorithm. The range [begin, end) contains the whole set of elements, and [begin, begin+k) the current combination of k elements. The next combination is generated in place.

I'd be grateful for advice and improvement suggestions.

NB1: the algorithm depends on two invariants: [begin, begin+k] is sorted, and [begin+k, end) is sorted also.

NB2: I've tried to rely on existing STL algorithms as much as possible.

#include <algorithm>
#include <functional>

template <typename Iterator>
Iterator find_next_increase_position(Iterator begin, Iterator combination_end, Iterator end);

template <typename Iterator>
bool next_combination(Iterator begin, Iterator end, unsigned k) {
    const auto combination_end = begin+k;
    const auto next_move = find_next_increase_position(begin, combination_end, end);
    if (next_move == end) return false;
    const auto previous_value = *next_move;
    std::inplace_merge(next_move, combination_end, end);
    const auto next_rotation =
        std::rotate(next_move, std::upper_bound(next_move, end, previous_value), end);
    std::rotate(combination_end, next_rotation, end);
    return true;
}

template <typename Iterator>
Iterator find_next_increase_position(Iterator begin, Iterator combination_end, Iterator end) {
    auto pos = std::upper_bound(std::reverse_iterator(combination_end),
                                    std::reverse_iterator(begin),
                                    *--end,
                                    std::greater<typename Iterator::value_type>());
    if (pos.base() == begin)
        return ++end;
    return --pos.base();
}

An example:

#include <vector>

int main() {
    std::vector<int> test{1,2,3,4,5};
    while (next_combination(test.begin(), test.end(), 3)) {
        for (auto i : test) std::cout << i;
        std::cout << std::endl;
    }
}

With output:

12435
12534
13425
13524
14523
23415
23514
24513
34512

Edit: added #includes and example

My advice here is this: throw away your requirement of not having any state. The number sequence \$\binom{i}{\lceil i \rceil}\$ grows fast, so you won't be able to deal with combinations whose state is "large". Also, I see no point doing templates here. Instead you could write a simple class that manages indices in such a way that indirection is lexicographic:

#include <iostream>
#include <iterator>
#include <sstream>
#include <vector>

class lexicographical_combination_generator {
public:
    lexicographical_combination_generator(size_t set_size,
                                          size_t combination_size);
    bool increment();
    size_t get_set_size()                      { return m_set_size; }
    size_t get_combination_size()              { return m_combination_size; }
    size_t get_combination_index(size_t index) { return m_indices[index]; }
private:
    size_t              m_set_size;
    size_t              m_combination_size;
    std::vector<size_t> m_indices;

    void check_arguments(size_t set_size, size_t combination_size);
};

lexicographical_combination_generator::
lexicographical_combination_generator(size_t set_size, size_t combination_size)
{
    check_arguments(set_size, combination_size);
    m_set_size = set_size;
    m_combination_size = combination_size;

    for (size_t index = 0; index < m_combination_size; ++index)
    {
        m_indices.push_back(index);
    }
}

void lexicographical_combination_generator::
check_arguments(size_t set_size, size_t combination_size)
{
    if (combination_size > set_size)
    {
        std::stringstream ss;
        ss << "combination_size("
           << combination_size
           << ") > set_size("
           << set_size
           << ")";
        throw std::runtime_error{ss.str()};
    }

    if (set_size == 0)
    {
        std::stringstream ss;
        ss << "set_size is zero";
        throw std::runtime_error{ss.str()};
    }
}

bool lexicographical_combination_generator::increment()
{
    if (m_indices[m_combination_size - 1] < m_set_size - 1)
    {
        m_indices[m_combination_size - 1]++;
        return true;
    }

    for (int i = (int)(m_combination_size - 2); i >= 0; --i)
    {
        if (m_indices[i] < m_indices[i + 1] - 1)
        {
            m_indices[i]++;

            for (int j = i + 1; j < m_combination_size; ++j)
            {
                m_indices[j] = m_indices[j - 1] + 1;
            }

            return true;
        }
    }

    return false;
}

template<typename Iter>
void print(lexicographical_combination_generator& gen, Iter begin)
{
    for (size_t i = 0; i < gen.get_combination_size(); ++i)
    {
        std::cout << gen.get_combination_index(i);
    }

    std::cout << ": ";

    for (size_t i = 0; i < gen.get_combination_size(); ++i)
    {
        auto iter = begin;
        std::advance(iter, gen.get_combination_index(i));
        std::cout << *iter;
    }

    std::cout << "\n";
}

int main(int argc, const char * argv[]) {
    std::vector<char> alphabet = { 'a', 'b', 'c', 'd', 'e' };
    lexicographical_combination_generator gen(5, 3);
    do {
        print(gen, alphabet.begin());
    } while (gen.increment());
}


Dynamic image using PHP I need to create an image on the fly where one large image covers a small image. The large image is a mask which makes the underlying image visible. Here is a quick sketch of what I'm trying to accomplish:

It's important that Y lies beneath X.

I have created to code for this and it works, but I was wondering how I can make the code more efficient. This is what I have:

    // Create a blank image with the underlying image correctly positioned
    $blank = imagecreatetruecolor(403, 403);
    $profile = imagecreatefromjpeg('img.jpg');
    $w = imagesx($profile);
    $h = imagesy($profile);
    imagecopy($blank, $profile, 0, 140, 0, 0, $w, $h);
    imagejpeg($blank, 'tmp.jpg', 100);

    // Frame overlay
    $frame = imagecreatefrompng('frame.png');
    $photo = imagecreatefromjpeg('tmp.jpg');
    imagecopy($photo, $frame, 0, 0, 0, 0, 403, 403);
    imagejpeg($photo, 'output.jpg', 100);

    // Garbage collection
    unlink('tmp.jpg');
    imagedestroy($blank);
    imagedestroy($profile);
    imagedestroy($frame);
    imagedestroy($photo);

I just think that creating 2 different images is just overkill, but I just can't find a better solution.

First of all don't save your temporary file as tmp.jpg, think about what happends if there are 2 requests for an image at the same time, the first tmp.jpg will be overwritten by the second one (very unlikely, but still possible and very hard to debug / reproduce bug)..

Second saving to the disk is expensive and useless, avoid it whenever possible.

Third why do you need the blank image? I think you should have the frame image and overlay the profile image on top of it.

Given this, I think the code should look like this:

// Create a blank image with the underlying image correctly positioned
$base = imagecreatefrompng('frame.png');
$profile = imagecreatefromjpeg('img.jpg');
$w = imagesx($profile);
$h = imagesy($profile);
imagecopy($base, $profile, 0, 140, 0, 0, $w, $h);

imagejpeg($base, 'output.jpg', 100);

// Garbage collection
imagedestroy($base);
imagedestroy($profile);

Unless I miss something this code should do exactly the same thing, but faster and safer.


"Cool Guys" CodeChef challenge I was trying to solve a question on CodeChef. My code works perfectly fine in my IDE (Visual Studio here) but when I submit my code at CodeChef, it says "Time Limit Exceeded".

What the code does:

Input T number of test cases each with one input N
Let A and B be two numbers chosen randomly from 1,2,3...N
Output the probability that Greatest Common Divisor of A and B equals B for each test case.

int _tmain(int argc, _TCHAR* argv[])
{
    int T;
    cin>>T;
    int tests[1000];
    for(int k=0;k<T;k++) //  Take the test cases as input
        cin>>tests[k];

    for(int i=0;i<T;i++)   // the main loop that runs T times
    {
        int N=tests[i];

        int result=2*N-1;   //  the numerator of the result

        int square=N*N;   //  the denominator of the result

        for(int j=2;j<=(N/2);j++)
        {
            result=result+(N/j)-1;
        }

        for(int w=2;w<=result;w++)  //  Loop to convert fraction into irreducible form
        {
            if(result%w==0 && square%w==0)
            {
                result=result/w;
                square=square/w;
                w--;
            }
        }

        cout<<result<<"/"<<square<<endl;
    }
    return 0;
}

How can I optimize this code of mine so that it compiles within the specified time limit? Any optimization possible whatsoever?

Both _tmain() and _TCHAR are Microsoft-specific and are non-portable. Just make them main() and char respectively.

The name T is commonly used in template statements.  Even with the given explanation, it may still confuse others, especially if you end up adding templates somewhere.  You could just rename this to testCases or something similar.

It's good to have more whitespace between operands for readability.
For instance, this:

for(int k=0;k<T;k++)

can look like this:
  for (int k = 0; k < T; k++)

You have some not-so-useful comments:

//  Take the test cases as input

This is already obvious based on what it's doing.

// the main loop that runs T times

This is also already obvious, based on the loop statement.

int result=2*N-1;   //  the numerator of the result

int square=N*N;   //  the denominator of the result

Although not necessarily useless comments, they are an indication that those variables can be given more accurate names.  It'll also help give more meaning to the very last cout statement that uses those variables.


Some notes about your code:

(function () {. Is this old-style wrapping really needed? Isn't this a node.js script?
You parsed INPUT manually from the question. You should probably leave the input as similar as possible to how it's stated and do the parsing programatically.
const multiply = (acc, val) => acc * val;: The variable names reveal how the function is used; instead, keep it generic: const multiply = (x, y) => x * y;. Consider also creating the abstraction product, the product of all numbers in an array.
parser(latestMaxProduct, number.slice(1)); You should use recursive calls as a last resort, only when higher-level abstractions (map, filter, reduce, ...) are not enough.
I'd separate generic functions from the specific code used in the problem. Re-using generic abstractions is one the fundamental principles of FP.

I'd write:

const range = (start, end) => Array.from(new Array(end - start), (x, i) => i + start);
const groupsOf = (xs, n) => range(0, xs.length - n + 1).map(i =>  xs.slice(i, i + n));
const product = xs => xs.reduce((acc, x) => acc * x, 1);
const maximum = xs => Math.max(...xs);

const euler8 = () => {
  const unparsedDigits = `
    73167176531330624919225119674426574742355349194934
    96983520312774506326239578318016984801869478851843
    ...
  `;
  const groupSize = 13;

  const digits = unparsedDigits.replace(/[^\d]/g, "").split("").map(c => parseInt(c));
  return maximum(groupsOf(digits, groupSize).map(product));
}

console.log(euler8());


Beginner radian/degree converter program I'm a bit of a beginner when it comes to Java, and I created this small code to help me grasp a few small subjects. It allows its user to convert a radical to a degree measure and a degree measure to a radical. This is my final code:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.ArrayList;

public class MainClass {

    public static void dtr (BufferedReader dRead) throws IOException {

        ArrayList<Integer> divisors = new ArrayList<Integer>();
        ArrayList<Integer> gcd = new ArrayList<Integer>();

        int d = Integer.parseInt(dRead.readLine());
        for (int pd = 1; pd <= d; pd++) {
            if (d % pd == 0) {
                divisors.add(pd);
            }
        }

        for (int index = 0; index < divisors.size(); ++index) {
            if (180.0 % divisors.get(index) == 0) {
                gcd.add(divisors.get(index));
            }

        }

        int dem = (180 / gcd.get(gcd.size() - 1));
        if ((d / (gcd.get(gcd.size() - 1))) == 1) {
            System.out.print("Radical: pi / " + dem);
        } else {
            System.out.print("Radical: " + (d / (gcd.get(gcd.size() - 1))) + "pi / " + dem);
        }

    }

    public static void rtd (BufferedReader rnRead, BufferedReader rdRead) throws IOException {

        int rn = Integer.parseInt(rnRead.readLine());
        int rd = Integer.parseInt(rdRead.readLine());

        int dividend = rn * 180;

        System.out.print("Degrees: " + (dividend / rd));

    }

    public static void main(String[] args) throws IOException {

        System.out.println("Do you want to convert from radicals to degrees, or degrees to radicals?");
        System.out.print("Use \"R\" for radials to degrees, and \"D\" for degrees to radicals: ");
        BufferedReader userInput = new BufferedReader(new InputStreamReader(System.in));
        String userChoice = userInput.readLine();

        if (userChoice.contains("D") || userChoice.contains("d")) {

            System.out.print("Degrees: ");
            BufferedReader dRead = new BufferedReader(new InputStreamReader(System.in));
            dtr(dRead);

        } else if (userChoice.contains("R") || userChoice.contains("r")) {

            System.out.println("Radical numerator (omit pi): ");
            BufferedReader rnRead = new BufferedReader(new InputStreamReader(System.in));
            System.out.println("Radical denominator: ");
            BufferedReader rdRead = new BufferedReader(new InputStreamReader(System.in));
            rtd(rnRead, rdRead);

        } else {

            System.out.println("Invalid response. Please restart the program.");

        }

    }

}

I know how bad some of this code is, but I just want to know if there is any way I could have optimized what this does, because I'd like to apply those concepts to everything that I'll make in the future.

Nowadays is common to omit the generic type on the right hand side of an assignment so instead of

 ArrayList<Integer> divisors = new ArrayList<Integer>();

you can write

 ArrayList<Integer> divisors = new ArrayList<>();

Since you are not using any ArrayList specific functionality it is cleaner to declare divisors as List (same goes for gcd)

 List<Integer> divisors = new ArrayList<>();

The call gcd.get(gcd.size() deserves a separate method with a reasonable name.

Regarding BufferedReader userInput ... while in this example it is harmless to not close IO resources, it is good practice to do so in general. Check out "try with resources statement": https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html.

It is also common to reuse the Reader and not create a new one every time you need to perform a read operation. With this in mind you will have something like this:

public static void main(String[] args) throws IOException {
    System.out.println("Do you want to convert from radicals to degrees, or degrees to radicals?");
    System.out.print("Use \"R\" for radials to degrees, and \"D\" for degrees to radicals: ");
    try (BufferedReader userInput = new BufferedReader(new InputStreamReader(System.in))) {
        String userChoice = userInput.readLine();

        if (userChoice.contains("D") || userChoice.contains("d")) {

            System.out.print("Degrees: ");
            dtr(userInput);

        } else if (userChoice.contains("R") || userChoice.contains("r")) {

            System.out.println("Radical numerator (omit pi): ");
            System.out.println("Radical denominator: ");
            rtd(userInput);

        } else {

            System.out.println("Invalid response. Please restart the program.");
        }
    }
}

Note that now rtd requires only one parameter. Because you print before reading user input, the user gets to see

 Radical numerator (omit pi):
 Radical denominator:

before she has an opportunity to provide any input. Those statements should move into rtd interleaved with reading user input.

 if (userChoice.contains("D") || userChoice.contains("d"))

can be simplified like so:

if (userChoice.toLowerCase().contains("d")) {


Computing a hash value of a file I am trying to see if there is a more efficient way of creating a hash of a file of unknown size (could be gb). My code below works fine but I am sure there is a more efficient way of achieving this.

    private static string GetHash(FileInfo fileInfo, HashAlgorithm algorithm)
    {
        byte[] Block = ArrayPool<byte>.Shared.Rent(8192);
        string hashString = string.Empty;
        try
        {
            using (algorithm)
            {
                using (FileStream stream = fileInfo.OpenRead())
                {
                    int length;
                    while ((length = stream.Read(Block, 0, Block.Length)) > 0)
                    {
                        algorithm.TransformBlock(Block, 0, length, null, 0);
                    }
                    algorithm.TransformFinalBlock(Block, 0, 0);
                    hashString = algorithm.Hash.ToHex();
                    algorithm.Dispose();
                    stream.Flush();
                }
            }
            fileInfo = null;
            return hashString;
        }
        finally
        {
            ArrayPool<byte>.Shared.Return(Block);
        }
    }

    private static string ToHex(this byte[] bytes, bool upperCase = true)
    {
        StringBuilder result = new StringBuilder(bytes.Length * 2);

        foreach (var bit in bytes)
        {
            result.Append(bit.ToString(upperCase ? "X2" : "x2"));
        }
        bytes = null;
        return result.ToString();
    }

And the usage would be:

GetHash(fileInfo, SHA512.Create());

I think your overall approach is sound: read data into a buffer, update the hash, and repeat until the whole file has been processed.

Buffer size may have some impact on performance, so that may be worth experimenting with (I'd probably expose it as a parameter).

Having that said, there are a few odd things going on in your code:

Disposing algorithm is somewhat surprising - it's an argument, so I'd expect the calling code to be responsible for it. If anything, such behavior should be documented. You may also want to provide an overload that allows calling code to disable this - for example, StreamReader and StreamWriter have a constructor that takes a leaveOpen parameter.
Don't call Dispose when you're already using a using statement.
Successive using statements are usually not nested. Personally I think nesting them makes code a little less readable.
stream.Flush() isn't necessary because you're not writing to the stream.
Setting those local variables to null does nothing useful. It doesn't trigger the garbage collector, those variables will go out of scope shortly afterwards anyway, and since they're parameters they're also most likely not the only reference to the object they're referencing.
After removing those unnecessary lines, you can immediately return the result of algorithm.Hash.ToHex() - no need to store it in a local variable. If you do want to use a variable (to ease debugging or to improve readability of the code), then I would declare it as close to where it's used as possible. Currently, it appears at the top, middle and near the end of the method, so you have to look all over the place to see how it's used. That's no big deal for a small method like this, but it's not as easy to understand as it could be.


Permutations of string in Java This is a program about finding all the permutations of an string. I want to be able to make it faster and more efficient like eliminating the recursion maybe. Please advise.

What is intended is to also find the permutations of the sub-strings of the main string while repetitions should be omitted.

public class StrPerm {
    static int count = 0;

    public static void main(String[] args) {
        String str = "ABCD";
        permutation("", str);
        System.out.println("Total permutations of given string are : " + count);
    }

    private static void permutation(String prefix, String str) {
        int n = str.length();
        if (n == 0) {
            if (prefix != "")
                System.out.println(prefix);
            count++;
        } else {
            for (int i = 0; i < n; i++) {
                if (prefix != "") {
                    System.out.println(prefix);
                    count++;
                }
                permutation(prefix + str.charAt(i), str.substring(0, i) + str.substring(i + 1));
            }
        }
    }
}

The results are as follows:

  A
  AB
  ABC
  ABCD
  AB
  ABD
  ABDC
  A
  AC
  ACB
  ACBD
  AC
  ACD
  ACDB
  A
  AD
  ADB
  ADBC
  AD
  ADC
  ADCB
  B
  BA
  BAC
  BACD
  BA
  BAD
  BADC
  B
  BC
  BCA
  BCAD
  BC
  BCD
  BCDA
  B
  BD
  BDA
  BDAC
  BD
  BDC
  BDCA
  C
  CA
  CAB
  CABD
  CA
  CAD
  CADB
  C
  CB
  CBA
  CBAD
  CB
  CBD
  CBDA
  C
  CD
  CDA
  CDAB
  CD
  CDB
  CDBA
  D
  DA
  DAB
  DABC
  DA
  DAC
  DACB
  D
  DB
  DBA
  DBAC
  DB
  DBC
  DBCA
  D
  DC
  DCA
  DCAB
  DC
  DCB
  DCBA

  Total permutations of given strings are : 84

Normally we're not supposed to answer questions containing broken code. But I actually found this one too interesting not to fix it.

A side effect of fixing the algorithm actually greatly simplified the method as well. This is what I ended up with:

private static void permutation(String prefix, String str) {
    for (int i = 0; i < str.length(); i++) {
        String current = prefix+str.charAt(i);
        System.out.print(current+ " ");
        count++;
        permutation(current, str.substring(0, i) + str.substring(i + 1));
    }
}

The only thing wrong with the current implementation is the static count. What happens if you run the permutation method more than once? Perhaps on 2 different strings?

Let's make the count local to the method instead.

private static int permutation(String prefix, String str) {
    int count = 0;
    for (int i = 0; i < str.length(); i++) {
        String current = prefix+str.charAt(i);
        System.out.print(current+ " ");
        count++;
        count += permutation(current, str.substring(0, i) + str.substring(i + 1));
    }
    return count;
}

I think this is simple enough that I don't need to explain how this works.

Now for your concern about removing the recursion.

I don't think this is necessary. The only reason recursion in java is bad in general is that you add the method call to the stack each time, which might result in a stackoverflow.

Here the depth of the recursion is only equal to the length of the string. This will always be really low compared to the number of permutations that have to be calculated. For example: a string of length 10 would only go 10 recursions deep but has millions of permutations.

EDIT: implementation without string concatenations.

I had some more fun with this and tried to remove all string concatenations. Since we're only interested in printing the substrings, not necessarily return them we can optimise for this too.

All we need is a char[] to represent the string. The advantage is that we can really cheaply swap 2 letters in the string. Let's first create a helper function for swapping 2 letters in a string:

private static void swap(char[] string, int a, int b){
    if(a==b) return;
    char temp = string[a];
    string[a] = string[b];
    string[b] = temp;
}

I added the if(a==b) so we don't "swap" a letter with itself.

Let's also add another helper method to print out a single permutation. Since we pass the entire char[] we also need to add how many characters should be printed:

 private static void printUpTo(char[] string, int n){
    for (int i = 0; i < n; i++) {
        System.out.print(string[i]);
    }
    System.out.print(" ");
}

Now that we have the helper functions the actual method implementation becomes relatively easy. The main idea is to keep track of what the current prefix is. This is just an int pointing to an index in the char[]. Then for each char after that index we do the same steps as before, but adapted to using the array:

append it to the current prefix => swap to the first spot after the prefix.
print the current word => print the first prefix + 1 characters
increment count
recursively call for new prefix and remaining letters => call with the array and prefix + 1
reverse the swap so we don't mess up further iterations.

The actual implementation looks like this:

private static int printPermutations(char[] string, int prefix){
    int count = 0;
    for(int i = prefix; i< string.length; i++){
        swap(string, i, prefix);
        printUpTo(string, prefix+1);
        count ++;
        count += printPermutations(string, prefix + 1);
        swap(string, i, prefix);
    }
    return count;
}

It should be noted that since we swap the first letter after the prefix with the other letter, we lose the ordering. So for string "ABCD" this one doesn't print all permutations alphabetically, whereas the previous implementation did.

Fixing this is possible if we rotate instead of swapping. The implementation looks something like this:

private static int printPermutations(char[] string, int prefix) {
    int count = 0;
    for (int i = prefix; i < string.length; i++) {
        rotate(string, prefix, i);
        printUpTo(string, prefix + 1);
        count++;
        count += printPermutations(string, prefix + 1);
        reverseRotate(string, prefix, i);
    }
    return count;
}

private static void rotate(char[] string, int a, int b) {
    if (a == b) {
        return;
    }
    char temp = string[b];
    System.arraycopy(string, a, string, a + 1, b - a);
    string[a] = temp;
}

private static void reverseRotate(char[] string, int a, int b) {
    if (a == b) {
        return;
    }
    char temp = string[a];
    System.arraycopy(string, a + 1, string, a, b - a);
    string[b] = temp;
}

But in my opinion, this isn't worth it. The first implementation was good enough.


Doing #ifdef DEBUG and #define func() right? This doesn't seem quite right, but it's the best I could do. Is there a way to do this better? My goal is to use DEBUG when I'm developing by doing a 'cc -DDEBUG' at the prompt.

#ifndef DEBUG
#define ptrValues ptrvaluesRemote
#else
#define ptrValues ptrValuesLocal
#endif

ptrLinks = (struct sLinkDescriptionLayout*) ptrValues();

Perhaps use NDEBUG (as used in assert.h) instead of making up your own.  Also note that unless you put an ifdef around the two functions you might get a linkage warning (as one of the functions might be unused).  A warning is undesirable, but so is putting the ifdef around the functions, which will result in one of the functions not being compiled and in time the function may rot.

Note that you might achieve the same end using the tool-chain to modify the symbol table.

Another alternative would be to name the two functions the same in two separate files and link one or the other as required - that would be a cleaner solution, but would need documenting.

Also...

Perhaps make ptrLinks and ptrValuesEtc the same type so that a cast is unnecessary.

Consider whether ptrLinks and ptrValuesEtc are correctly named:

they appear to the uninitiated (me) to mean different things;
also, one is a function and the other a pointer but their form is the same.


Code that finds the largest palindrome from two three-digit factors My code is really ugly, but I don't know how to make it better.

#include "stdafx.h"
#include <iostream>
#include <math.h>

using namespace std;

bool palindromeTest(double numberVectorAddition){//tests if the factors of the palindrome are both below 3 digits each.
    double p = numberVectorAddition;
    double i = 999;
    double c;
    for (;; i--){
        if (fmod(p, i) != 0)
            continue;
        if (fmod(p, i) == 0)
            c = p / i;
        if (1000 <= c){
            cout << "One of the factors is too high. Looking for next largest palindrome..." << endl;
            cout << endl;
            return false;
            }
        else if (c <= 1000){
        cout << "Factors of the number both below 1000 are " << i << " and " << c;
        return true;
        }
    }
}

void nLargestPalindrome(double number){
    int i = 5; //subtract "1" because array starts at zero
    int intermediate = i; //intermediate value - equal to the array index;
    double *decimalDigit = new double[i + 1];
        while (0 <= i){ //acquire digits of number
            decimalDigit[i] = (number - fmod(number, pow(10, i))) / pow(10, i);
            cout << "Value of digit place " << i << " is " << decimalDigit[i] << endl;
            number = fmod(number, pow(10, i));
            i--;
        }
        for (int q = 0; q < 100; q++){ //decreases size of palindrome, and also tests after the decrease.
            double numberVectorAddition = pow(10, 5)*decimalDigit[0] + pow(10, 4)*decimalDigit[1] + pow(10, 3)*decimalDigit[2] + pow(10, 2)*decimalDigit[3] + pow(10, 1)*decimalDigit[4] + decimalDigit[5];
            cout << decimalDigit[0] << decimalDigit[1] << decimalDigit[2] << decimalDigit[3] << decimalDigit[4] << decimalDigit[5] << '\t';
            cout << numberVectorAddition << endl;
            if (palindromeTest(numberVectorAddition))
                break;
            decimalDigit[2]--;
            decimalDigit[3]--;
                if (decimalDigit[2] == 0){
                    numberVectorAddition = pow(10, 5)*decimalDigit[0] + pow(10, 4)*decimalDigit[1] + pow(10, 3)*decimalDigit[2] + pow(10, 2)*decimalDigit[3] + pow(10, 1)*decimalDigit[4] + decimalDigit[5];
                    cout << decimalDigit[0] << decimalDigit[1] << decimalDigit[2] << decimalDigit[3] << decimalDigit[4] << decimalDigit[5] << '\t';
                    cout << numberVectorAddition << endl;
                    if(palindromeTest(numberVectorAddition))
                        break;
                        if ((decimalDigit[1] == 0) && (decimalDigit[2] == 0)){
                            decimalDigit[1] = 9;
                            decimalDigit[4] = 9;
                            decimalDigit[2] = 9;
                            decimalDigit[3] = 9;
                            decimalDigit[0]--;
                            decimalDigit[5]--;
                            continue;
                        }
                    decimalDigit[2] = 9;
                    decimalDigit[3] = 9;
                    decimalDigit[1]--;
                    decimalDigit[4]--;
                }
        }
    delete decimalDigit;
}

int main(){
    double n = 997799; //997799 is equal to 999*999 - 2; palindrome is smaller.
    nLargestPalindrome(n); //finds the largest palindrome below "n" that has both factors of three digits.
    cin.get(); //stops the command window from closing prematurely
}

Code formatting

Your code seems to be poorly indented. I suggest you have a look at the documentation of your favorite text editor to see how to fix this.

Basic logic

It doesn't make much sense to check :

    if (fmod(p, i) != 0)
        continue;
    if (fmod(p, i) == 0)

Similarly,

        if (1000 <= c){
            return false;
        }
        else if (c <= 1000){

is quite redundant.

using namespace std;

Using using namespace std; is usually frowned upon, some will say that it is ok in a cpp file, some will say the opposite. I'll let you decide.

The right type

You are using double but long int are more than enough for what you are trying to do.

Right place to define variables

It is usually a good idea to define variables in the smallest possible scope. Among other things, it helps the reader not to have to keep too many things in mind. Also, it helps you see that intermediate is an unused variable. As a side-note, it is a good idea to activate all the warnings in your compiler to detect such a thing (and many other potential errors).

Define constants

Instead of having 5 in the middle of your code and need to comment it, you could store 6 in a constant named for instance NB_DIGITS and remove the need for any comment.

Use for loops when you can

Now, the loop to aquire digits can be rewritten :

for (int i = NB_DIGITS -1; 0 <= i; i--) { //acquire digits of number
    decimalDigit[i] = (number - fmod(number, pow(10, i))) / pow(10, i);
    cout << "Value of digit place " << i << " is " << decimalDigit[i] << endl;
    number = fmod(number, pow(10, i));
}

Compute things only once

You call pow(10, i) way too many times compared to what you actually need. You can simply write :

    double pow_ten = pow(10, i);
    decimalDigit[i] = (number - fmod(number, pow_ten)) / pow_ten;
    cout << "Value of digit place " << i << " is " << decimalDigit[i] << endl;
    number = fmod(number, pow_ten);

Smarter decomposition of a number

Instead of going through the hassle of getting digits starting with the one with the strongest weight, it is much easier to start by the end. Also, this makes you perform the iteration in the logicial order.

Better names

numberVectorAddition is a pretty bad name for a parameter. n conveys enough information as far as I can tell.

palindromeTests is also quite bad. hasDivisorsInRange is probably better.

Finally, DecimalDigit is a bit redundant are digits already include the notion of 10 but also, it probably should be plural to tell that it's a collection.

No need for new/delete here

You don't need to use new/delete here. If you just define it long int digits[NB_DIGITS];, it will be deleted when it does out of the scope.

Clearer responsability for functions

It could be interesting for your function to return values instead of printing them. This would lead to separation of concerns. One of the multiple advantages is to make your code easier to test.

At this point your code, looks like :

#include <iostream>
#include <math.h>

using namespace std;

const int NB_DIGITS = 6;

bool hasDivisorsInRange(long int n){//tests if the factors of the palindrome are both below 3 digits each.
    for (long int i = 999;; i--){
        if (n % i == 0)
        {
            long int c = n / i;
            if (1000 <= c){
                return false;
            }
            else if (c <= 1000){
                cout << "Factors of the number both below 1000 are " << i << " and " << c << endl;
                return true;
            }
        }
    }
}

long int LargestPalindrome(long int number){
    long int digits[NB_DIGITS];
    for (int i = 0; i < NB_DIGITS; i++) { //acquire digits of number
        digits[i] = number % 10;
        number /= 10;
        cout << "Value of digit place " << i << " is " << digits[i] << endl;
    }
    for (int q = 0; q < 100; q++){ //decreases size of palindrome, and also tests after the decrease.
        long int n = pow(10, 5)*digits[0] + pow(10, 4)*digits[1] + pow(10, 3)*digits[2] + pow(10, 2)*digits[3] + pow(10, 1)*digits[4] + digits[5];
        cout << digits[0] << digits[1] << digits[2] << digits[3] << digits[4] << digits[5] << '\t';
        cout << n << endl;
        if (hasDivisorsInRange(n))
            return n;
        digits[2]--;
        digits[3]--;
        if (digits[2] == 0){
            long int n2 = pow(10, 5)*digits[0] + pow(10, 4)*digits[1] + pow(10, 3)*digits[2] + pow(10, 2)*digits[3] + pow(10, 1)*digits[4] + digits[5];
            cout << digits[0] << digits[1] << digits[2] << digits[3] << digits[4] << digits[5] << '\t';
            cout << n2 << endl;
            if(hasDivisorsInRange(n2))
                return n;
            if ((digits[1] == 0) && (digits[2] == 0)){
                digits[1] = 9;
                digits[4] = 9;
                digits[2] = 9;
                digits[3] = 9;
                digits[0]--;
                digits[5]--;
                continue;
            }
            digits[2] = 9;
            digits[3] = 9;
            digits[1]--;
            digits[4]--;
        }
    }
    return 0;
}

int main(){
    long int n = 906608; //997799 is equal to 999*999 - 2; palindrome is smaller.
    int pal = LargestPalindrome(n); //finds the largest palindrome below "n" that has both factors of three digits.
    cout << "Solution is " << pal << endl;
}

Better algorithm

You are trying to iterate over palindromes in a quite complicated way (that I do not understand at all). To iterate over palindromes composed of 6 digits is nothing but iterating over values between 100 and 999 and adding the mirrored version of the number at the end.

This can be badly written :

long int LargestPalindrome(long int number){
    for (int beg = (number / 1000); beg > 100; beg--)
    {
        // Could be done in a clean way with array but I'm lazy
        int i = beg;
        int a = i % 10; i/=10;
        int b = i % 10; i/=10;
        int c = i % 10; i/=10;
        long int pal = beg * 1000 + 100 * a + 10 * b + c;
        if (pal < number && hasDivisorsInRange(pal))
            return pal;
    }
    return 0;
}

For n = 906608, this finds 888888 which happens to be a better solution that the one found by your code.

Also, you could perform a smarter research for divisors.

I have to go, I'll try to continue this answer at some point.


Indenting and dedenting ASP Classic code with Python I've created a Python script that tries to properly indent and dedent ASP code. Similar to http://www.aspindent.com/

Since I've began using it I've noticed some edge cases that I hadn't anticipated. The reason these edge-cases appear is because of the fact that I'm not parsing straight ASP code. There are instances of HTML and Javascript in the file, thus, some of this is causing indentations to occur sporadically throughout the final output. I realize that part of the problem stems from my regex use. I'm here to see if there is a cleaner way to go about this.

import re, sys

class Indenter:
    def __init__(self, string):
        self.space = 0
        self.count = 0
        self.string = string

    def print_ln(self, string):
        sys.stdout.write(" " * self.space + str(string))
        sys.stdout.flush()

    def indent(self):
        self.print_ln(self.string[self.count])
        self.space += 4

    def dedent(self):
        self.space -= 4
        self.print_ln(self.string[self.count])

    def dedent_indent(self):
        self.space -= 4
        self.print_ln(self.string[self.count])
        self.space += 4

    def main(self):
        while self.count < len(self.string):
            if re.search("^\s*if.*then", str(self.string[self.count]), re.IGNORECASE):
                self.indent()
            elif re.search("^\s*for", str(self.string[self.count]), re.IGNORECASE):
                self.indent()
            elif re.search("^\s*with", str(self.string[self.count]), re.IGNORECASE):
                self.indent()
            elif re.search("^\s*do until", str(self.string[self.count]), re.IGNORECASE):
                self.indent()
            elif re.search("^\s*do$", str(self.string[self.count]), re.IGNORECASE):
                self.indent()
            elif re.search("^\s*Select Case", str(self.string[self.count]), re.IGNORECASE):
                self.indent()

            elif re.search("^\s*End Select", str(self.string[self.count]), re.IGNORECASE):
                self.dedent()
            elif re.search("^\s*loop", str(self.string[self.count]), re.IGNORECASE):
                self.dedent()
            elif re.search("^\s*end with", str(self.string[self.count]), re.IGNORECASE):
                self.dedent()
            elif re.search("^\s*end if", str(self.string[self.count]), re.IGNORECASE):
                self.dedent()
            elif re.search("^\s*next", str(self.string[self.count]), re.IGNORECASE):
                self.dedent()

            elif re.search("^\s*Case", str(self.string[self.count]), re.IGNORECASE):
                self.dedent_indent()
            elif re.search("^\s*else", str(self.string[self.count]), re.IGNORECASE):
                self.dedent_indent()
            elif re.search("^\s*elseif.*then", str(self.string[self.count]), re.IGNORECASE):
                self.dedent_indent()

            else:
                self.print_ln(self.string[self.count])
            self.count += 1

with open("scratch.html") as s:
    ind = Indenter(s.readlines())
    ind.main()

You should always write the main program protected within a if __name__ == '__main__': block, so that you can import the file in another program without automatically executing code.  Eg)

if __name__ == '__main__':
    ind = Indenter(s.readlines())
    ind.main()

Don't write classes where you create the class object just to call one instance method of the object.  See the video Stop Writing Classes.

Your code has:

ind = Indenter(s.readlines())

which reads every single line of the file into memory, and stores it in a list.  Then, you call the Indenter.main() function which executes ...

while self.count < len(self.string):
    # ... many lines of code omitted ...
    self.count += 1

... sequentially loops through every element of the list (line of the file) for processing one at a time.  Each member function, including this main() function, uses self.string[self.count] to reference the current line.  This must have been maddening to write, having to type (or copy/paste) the same code over and over.  It is frightening to read.

Let's clean this up.

print_ln(self, string) is structured correctly.  It takes the current line string as input.  Let's duplicate that in indent(), dedent(), and dedent_indent(), by adding the string argument to them, like:

def indent(self, string):
    self.print_ln(string)
    self.space += 4

In the main() function, we now need to pass the line to each function, just like it was passed to self.print_ln(self.string[self.count]).  But that is a lot of duplicate self.string[self.count] access, so let's make a temporary variable for it.  We can use that temporary variable in the re.search(....) calls too.

while self.count < len(self.string):
    string = self.string[self.count]
    if re.search("^\s*if.*then", string, re.IGNORECASE):
        self.indent(string)
    elif # ... many lines of code omitted ...
    else:
        self.print_ln(string)
    self.count += 1

Note that each string is a string, so the str(...) calls can just be omitted.

Now that self.count is only used in main(), we can make it a local variable, instead of an instance member.

count = 0
while count < len(self.string):
    string = self.string[count]
    # ... many lines of code omitted ...
    count += 1

A while loop which counts from 0 up to some limit an be replaced by a for ... in range(...) loop:

for count in range(len(self.string)):
    string = self.string[count]
    # ... many lines of code omitted ...

And since count is only ever used to index into the list that we are looping over the indices of, we can omit the indices altogether, and just loop over the list of strings itself.

for string in self.string:
    # ... many lines of code omitted ...

Now that we are looping directly over self.string, we don't need to explicitly know that it is a list of strings.  It can be any iterable object that returns, one-at-a-time, all the string we are interested in.  Like a file object.  And that file object can be passed to the main() function.

class Indenter:
    # ... omitted ...

    def main(self, lines):

        for line in lines:
            if re.search("^\s*if.*then", line), re.IGNORECASE):
                self.indent(line)
            elif # ... many lines of code omitted ...
            else:
                self.print_ln(line)

if __name__ == '__main__':
    with open('scratch.html') as file:
        ind = Indenter()
        ind.main(file)

Now we are no longer reading the entire file into memory.  Rather, we are opening the file, and reading it line-by-line, processing each line as it is read in, and then discarding it.

What does your code do if it encounters a file which is already indented?  It indents it some more!

That is probably undesired behaviour.  You probably want to strip off any existing indentation before adding the new indentation.

        for line in lines:
            line = line.strip()
            # ... if/elif/else statements omitted ...

Note that this removes the newline from the end of the lines as well, so you'll want to add that back in, perhaps by changing the sys.stdout.write(...) \ sys.stdout.flush() into a simple print(...) statement.

With each line stripped of any previous indentation, you can remove the \s* from the start of all of the reg-ex patterns.

There are 6 reg-ex patterns where you want to indent, 5 where you want to dedent, and 3 where you want to do both.  You always print.  You dedent before printing.  You indent after printing.

Instead of writing many lines of code which do the same thing with different patterns, it is much, much easier to write a loop, which loops through the patterns.  Let's put the patterns into lists:

indent_patterns = ['^if.*then', '^for', '^with', '^do until', '^do$', '^select case',
                   '^case', '^else', '^elseif.*then']
dedent_patterns = ['^end select', '^loop', '^end with', '^end if', '^next',
                   '^case', '^else', '^elseif.*then']

Now we can say if the current line matches any of the dedent_patterns, reduce space.  Then print the line.  Then, if the current line matches any of the indent_patterns, increase space.

if any(re.search(pattern, line, re.IGNORECASE) for pattern in dedent_patterns):
    self.space -= 4

self.print_ln(line)

if any(re.search(pattern, line, re.IGNORECASE) for pattern in indent_patterns):
    self.space += 4

... and the indent, dedent and dedent_indent functions just became unnecessary.

Since print_ln() is called in one spot, we can write than inline as well, which eliminates the need for self.space to be a member variable.

With no member variables left, the entire class can be replaced with one function.

import re

def indent(lines, spacing=4):

    indent_patterns = ['^if.*then', '^for', '^with', '^do until', '^do$', '^select case',
                       '^case', '^else', '^elseif.*then']
    dedent_patterns = ['^end select', '^loop', '^end with', '^end if', '^next',
                       '^case', '^else', '^elseif.*then']

    space = 0

    for line in lines:
        line = line.strip()

        if any(re.search(pattern, line, re.IGNORECASE) for pattern in dedent_patterns):
            space -= spacing

        print(" "*space + line)

        if any(re.search(pattern, line, re.IGNORECASE) for pattern in indent_patterns):
            space += spacing

if __name__ == '__main__':

    with open('scratch.html') as file:
        indent(file)

And like what was said above, stop writing classes.

This does not address the sporadic indentation caused by HTML and Javascript in the file, but

this is a Code Review, which only addresses the code you have actually written.  This is not a forum where you come to get help writing new code, but to get a review of code you have already written.
with a cleaner implementation of the indent code, you should have an easier time addressing that issue.  When you have finished, feel free to post a follow-up question.


Finding the item with the minimal value in a sequence This is an algorithm to find the item with the minimal value in a sequence. I've written a function called minimal to implement this algorithm.

Here is my script:

def minimal(*args):
    """Returns the item with the minimal value in a sequence. """
    if len(args) == 1:
        sequence = args[0]
    else:
        sequence = args

    index = 0
    successor = index + 1

    while successor < len(sequence):
        if sequence[index] > sequence[successor]:
            index = successor
        successor += 1

    return sequence[index]

Redundant use of indexing
Since you only need value, not index, you could store current minimal value instead of its index. This would allow for for loop, like
for item in sequence:
    if item < current_minimum:
        current_minimum = item

You both ditch overhead for array access, for tracking the index and - most important - code looks cleaner. (although that's subjective)
No support for generators
Generators provide items one at a time, instead of wasting memory on all of them at once. Your code would force generation of all items before first of them is processed. This is not really good.
Well, if you don't use indexing then you don't need len and generators would work just fine, your if len(args) == 1: clause is good in this regard.
Confusing behaviour with single argument
However, special treatment for a lone argument is not obvious from signature, that's not really good. If one does not know about this behaviour, one can easily use stuff like min_element = minimal(*some_list) which would generally work but will do an unexpected thing if some_list had only one element.
Yes, it seems extremely useful, but also confusing. Maybe limiting to one argument only and expecting users to do stuff like minimal((1,2))? I'm really not sure. It seems useful enough that built-in min works like that, but generally speaking decisions like this should not be taken lightly.
Also, take a closer look at what min does - there's also an option of using a key function and providing default value.


Functional programming approach to repeated function application Control like if..else and while

  If \$f\$ is a numerical function and \$n\$ is a positive integer, then we can form the \$n\$th repeated application of \$f\$, which is defined to be the function whose value at \$x\$ is \$f(f(...(f(x))...))\$. For example, if \$f\$ adds 1 to its argument, then the \$n\$th repeated application of \$f\$ adds \$n\$. Write a function that takes as inputs a function \$f\$ and a positive integer \$n\$ and returns the function that computes the \$n\$th repeated application of \$f\$:

def repeated(f, n):
    """Return the function that computes the nth application of f.

    f -- a function that takes one argument
    n -- a positive integer

    >>> repeated(square, 2)(5)
    625
    >>> repeated(square, 4)(5)
    152587890625
    """
    "*** YOUR CODE HERE ***"

The solution is implemented using functional paradigm style, as shown below:

from operator import mul
from operator import pow

def repeated(f, n):
    """Return the function that computes the nth application of f.

    f -- a function that takes one argument
    n -- a positve integer

    >>> repeated(square, 2)(5)
    625
    >>> repeated(cube, 2)(5)
    1953125
    """
    assert n > 0
    def apply_n_times(x):
        count = n
        next_acc = f(x)
        count = count - 1
        while count > 0:
            next_acc = f(next_acc)
            count = count - 1
        return next_acc
    return apply_n_times

def square(x):
    return mul(x, x)

def cube(x):
    return pow(x, 3)

print(repeated(square, 0)(2))

My questions:

If the code looks correct from a programming style perspective, can I further optimize this code written in function repeated?
Can the naming conventions and error handling be better?

In terms of the functional programming paradigm, I would suggest the following for repeated:

def repeated(f, n):
    """Return the function that computes the nth application of f.

    f -- a function that takes one argument
    n -- a positive integer

    >>> repeated(square, 2)(5)
    625
    >>> repeated(cube, 2)(5)
    1953125
    """
    if n == 1:
        return f
    return lambda x: f(repeated(f, n-1)(x))

Note that, per the rule of thumb you have previously discussed, each statement can be replaced with its result and the function still works correctly.

You could also simplify the imports:

from operator import mul, pow

In terms of naming conventions, pylint doesn't like single-character names, so perhaps func and num would be better than f and n/x, but in general you don't have a problem here.

In terms of error handling, you don't currently have any. I don't think that's a problem either, though - the docstrings explain what the inputs are supposed to be, and the user should expect errors/weird behaviour if they supply anything else!


Implementing Snap-To-Edge Functionality Recently I've written some code which implements snap-to-edge functionality for some windows.

There is some code duplication as you can see. I think this code can be simpler or possibly improved.

// If 'rtParent' and 'rtChild' are closed each other as 'ENABLE_STICK_VALUE', return true;
auto IsNearEachOther = []( const QRect& rtParent, const QRect& rtChild ) -> std::tuple< bool, QPoint>
{
    bool isNear = false;
    QPoint ptMoveTarget;

    // If 'rtChild's left position is closing to 'rtParent's right position.
    if ( rtChild.left() > rtParent.right() )
    {
        if ( ( ( rtChild.left() - rtParent.right() ) <= ENABLE_STICK_VALUE )
            && ( rtChild.top() >= ( rtParent.top() - ENABLE_STICK_VALUE ) )
            && ( rtChild.top() <= ( rtParent.bottom() + ENABLE_STICK_VALUE ) ) )
        {
            isNear = true;
            ptMoveTarget = QPoint( rtParent.right(), rtParent.top() );
        }
    }

    // If 'rtChild's right position is closing to 'rtParent's left position.
    if ( rtChild.right() < rtParent.left() )
    {
        if ( ( ( rtParent.left() - rtChild.right() ) <= ENABLE_STICK_VALUE )
            && ( rtChild.top() >= ( rtParent.top() - ENABLE_STICK_VALUE ) )
            && ( rtChild.top() <= ( rtParent.bottom() + ENABLE_STICK_VALUE ) ) )
        {
            isNear = true;
            ptMoveTarget = QPoint( rtParent.left() - rtChild.width() , rtParent.top() );
        }
    }

    return std::make_tuple( isNear, ptMoveTarget );
};

QRect rtParent = m_pParentChatRoom->geometry();
QRect rtCurrent = geometry();

bool result = false;
QPoint ptMoveTarget;
std::tie( result, ptMoveTarget ) = IsNearEachOther( rtParent, rtCurrent );
if ( result )
{
    move( ptMoveTarget );
}

Definitely extract method for the following code:

if ( rtChild.right() < rtParent.left() )
{
    if ( ( ( rtParent.left() - rtChild.right() ) <= ENABLE_STICK_VALUE )
        && ( rtChild.top() >= ( rtParent.top() - ENABLE_STICK_VALUE ) )
        && ( rtChild.top() <= ( rtParent.bottom() + ENABLE_STICK_VALUE ) ) )
    {
        isNear = true;
        ptMoveTarget = QPoint( rtParent.left() - rtChild.width() , rtParent.top() );
    }
}

Where you can pass through a left/right variable however you want and then change:

rtParent.left() - rtChild.width() to rely on the given parameter, falling back onto rtParent.right()

The same can be said for any other small changes in the code, such as

rtChild.right() < rtParent.left()
versus
rtChild.left() > rtParent.right()

I'm not going to give you the exact solution, but that if condition holds most of the repeated code and can be brought out into something like snapTo(right/left)


Stopping all Runnable tasks when one is terminated I want to run couple of Runnables. If one is failed, all others' execution should be terminated. Here is my approach. I got idea on this from this question   and this article.

In my code, I check the runnable which completes its task first among all and evaluate it. If that checking is failed (completionService.take().get()), all remaining executions are cancelled.

    java.util.List<String> tasks = Arrays.asList("1", "2", "3", "4", "5");
    List<Future> futureList = new ArrayList<Future>();
    final ExecutorService pool = Executors.newFixedThreadPool(5);
    final ExecutorCompletionService<String> completionService = new ExecutorCompletionService<>(
            pool);
    for (final String site : tasks) {

        futureList.add(completionService.submit(new Callable<String>() {
            @Override
            public String call() throws Exception {
                int i = Integer.parseInt(site);
                System.out.println("iterating i: " + i);
                if (i == 4) {
                    throw new RuntimeException("Forcd exception");
                }

                if (i % 2 == 0) {
                    try {
                        Thread.sleep(2000); // simply wanted to change the
                                            // execution time...this is for
                                            // 2s and others 5s.
                    } catch (InterruptedException e) {
                        System.err.println("Interrupted sleeping: "
                                + e.getCause());
                    }
                } else {
                    try {
                        Thread.sleep(5000);
                    } catch (InterruptedException e) {
                        System.err.println("Interrupted sleeping: "
                                + e.getCause());
                    }
                }
                return site;
            }
        }));
    }

    for (Future ft : futureList) {

        if (!futureList.isEmpty()) {
            try {
                System.out.println("future get.");
                String task = completionService.take().get();
            } catch (InterruptedException e) {
                System.err.println("Interuppted here: " + e.getMessage());
                // Thread.currentThread().interrupt();
                break;
            } catch (ExecutionException e) {

                System.out.println("Execution exception " + e.getMessage());
                for (Future future : futureList) {
                    if (!future.isDone()) {
                        System.out.println("Cancelling " + future);
                        future.cancel(true);
                    }
                }

                pool.shutdownNow();
                break;
            }
        }
    }
    pool.shutdownNow();

I want to know whether is this approach is good enough and any one see any problem in above code? I checked it and it does its job for me (i==4, it terminates) for few tests. But not sure whether it makes problems in long run.

This will run in a server, will this effect on server performance? I don't want to check the first completion runnable task, but if any runnable fails, all remainings should be removed. Sometime my approach will be long and another short solution will be there. Expect your ideas. Thanks.

Your general approach is sound

When your Callable throws, the throw will be propagated out of the Future, you catch it and cancel all remaining futures. I believe it is safe to cancel a completed job so you might not need the !isdone() check. But that's nitpicking.
Then you shutdown your thread pool to clean up.

Your implementation is a bit messy

The loop where you iterate over all the futures seems a bit messy to me.

For example:

for (Future ft : futureList) {
    if (!futureList.isEmpty()) {

this condition will always be true. And you are never removing from the list (if you were, you would get a ConcurrentModificationException. So the check is wholly redundant.

So you loop over all of the futures in futureList and call take() on your completionService. This doesn't make sense to me, if you're going to call take() use a loop that describes what you want to do better. This is further strengthened by the fact that you never use the ft loop variable.

Maybe something like this:

while(futureList.size() > 0) {
    Future ft = completionService.take();
    futureList.remove(ft);

    try{
        String task = ft.get();
    }catch(InterruptedException e){
        // Interrupted
    }catch(ExecutionException e){
        // Snap, something went wrong in the task! Abort! Abort! Abort!
        for(Future f : futureList){
            f.cancel(true);
        }
        futureList.clear();
    }
}
pool.shutdownNow();


Safely handling memory allocation across DLL boundaries in Windows I know that, in Windows, memory must be deallocated in the same module that allocated it.

I have 2 DLLs built with different C++ compilers (VS 6 and VS 2015). In the VS 2015 module, I have an exported function which returns a variable number of items (let's say of int type), so I need a variable-sized buffer.

I'm trying to automate as much as possible the use of the VS 2015 DLL from the VS 6 client, so I came up with this in the VS 2015 project:

/* dll.h */

#if EXPORTS  // EXPORTS is defined just in VC2015 project
#  define EXPORTED __declspec(dllexport)
#else
#  define EXPORTED __declspec(dllimport)
#endif

typedef int*(*Allocator_int__t)(size_t size);
int* allocator_int(size_t size);

#ifndef EXPORTS
inline int* allocator_int(size_t size){ return new int[size]; }
#endif

EXPORTED void cross_boundaries_int_buffer(int*& buffer, Allocator_int__t a = allocator_int);

and

/* dll.cpp */

EXPORTED void cross_boundaries_int_buffer(int*& buffer, Allocator_int__t a/* = allocator_int*/)
{
  buffer = a(10);
  buffer[0] = 0;
  /* ... */
  buffer[9] = 9;
}

This allows the VS 6 client DLL to do:

#include <dll.h>

void client_func()
{
  int *buffer = NULL;
  cross_boundaries_int_buffer(buffer); // default argument uses default allocator, **which is defined in client dll**
  /* use buffer elements as needed */
  delete[] buffer;
}

Is this a safe way to deal with the problem? Is there any better way?

The standard (and recommended) way to solve this is for each module to provide its own exported AllocMem and FreeMem functions (or whatever you want to call them—the names are not important).

This is what Windows DLLs do (for example, the Network API functions provide NetApiBufferAllocate and NetApiBufferFree to handle memory management), and you should strongly consider following the same model in your own project.

Regardless of their names, the implementation of these functions is downright trivial. AllocMem just needs to call new (or malloc if you're using C), and FreeMem just needs to call delete (or free if you're using C). They are just wrappers.

Header:

EXPORTED void* AllocMem(size_t cb);

EXPORTED void FreeMem(void* ptr);

Implementation:

EXPORTED void* AllocMem(size_t cb)
{
    return new char[cb];
}

EXPORTED void FreeMem(void* ptr)
{
    delete[] ptr;
}

You then follow the simple rule that every call to AllocMem is matched with a corresponding call to FreeMem from the same module. This ensures that the block of memory is always deallocated by the same module that allocated it.

For additional robustness, you might want to add error checking and even modify the function signatures to make extra information available to the callee for this purpose and/or return a status code to the caller.

Along similar lines lines, if you don't actually need general-purpose memory allocation, you might modify the function signatures to allocate only a certain type of memory and thus make them harder to use incorrectly. For example, AllocIntArray could always allocate an array of integers, taking as its only parameter the length of the array, and FreeIntArray would free a block of memory allocated by AllocIntArray.

These wrappers actually give you a lot of power. If diagnostics indicate that you have memory-fragmentation issues, you can add fixed-size allocation logic or switch over to the low fragmentation heap without introducing breaking behavior that would affect your clients.

As far as your goal of making it easy for the client to use, no competent programmer is going to have a hard time understanding how to use AllocMem and FreeMem functions. Personally, I make it a rule that all memory a caller needs to be freed must be explicitly allocated by the caller. This makes memory-management responsibilities significantly easier to reason about. But if you don't have such a rule (and I can't encourage you to adopt one, because you care more about simplicity than correctness), you could just have the DLL allocate the memory internally by calling the AllocMem function and return that block. Then, the client will just follow the rule that any memory it gets from the DLL, whether implicitly or explicitly, must be freed by calling FreeMem. There are Win32 functions that follow this model, too.

Alternatively, if you don't want to add additional exported functions to your libraries, you can just ensure that all of your code is standardized to use a single external memory allocator. For example, if you are targeting the Windows API, you can use either LocalAlloc and LocalFree, or CoTaskMemAlloc and CoTaskMemFree. This is safe because these allocators are universal and don't depend on the calling module.

I should also point out that if you are writing strictly C++ code and don't have any need for a lowest-common-denominator "C" API, then you can just return a shared smart pointer object from any function that needs to allocate memory. In addition to the obvious benefit of preventing the possibility of memory leaks and vastly simplifying the client code, smart pointer objects remember their associated deleter, thus guaranteeing that the memory will be correctly deallocated, even across module boundaries. If you're C++11, use std::shared_ptr; otherwise, you can use Boost's shared_ptr.