Easy to use Thread Pool Implementation awhile ago I asked about a thread pool implementation I had made. I took the advice down and modified it a lot over time to fit my needs. It has been serving me extremely well in all of my projects but I was wondering if there was anything else I could improve on?

Here is the class:

class ThreadPool{
public:
    /*  Constructor */
    ThreadPool(uint8_t numThreads) {
        assert(numThreads > 0);
        createThreads(numThreads);
    }

    //Destructor
    ~ThreadPool(){
        shutdown = true;
        notifier.notify_all();
        for(int i = 0; i < threads.size(); ++i){
            threads[i].join();
        }
    }

    //add any arg # function to queue
    template <typename Func, typename... Args >
    auto push(Func&& f, Args&&... args){

    //get return type of the function
    typedef decltype(f(args...)) retType;

    std::packaged_task<retType()> task(std::bind(f, args...));

    std::future<retType> future = task.get_future();

    {
        // lock jobQueue mutex, add job to the job queue
        std::unique_lock<std::mutex> lock(JobMutex);

        jobQueue.emplace(std::packaged_task<void()>(std::move(task)));
    }
    notifier.notify_one();

    return future;
    }

private:

    std::vector<std::thread> threads;
    std::queue<std::packaged_task<void()>> jobQueue;
    std::condition_variable notifier;
    std::mutex JobMutex;
    std::atomic<bool> shutdown = false;

    void createThreads(uint8_t numThreads) {
        auto threadFunc = [this]() {
            while (true) {
                std::packaged_task<void()> job;

                {
                    std::unique_lock<std::mutex> lock(JobMutex);
                    notifier.wait(lock, [this] {return !jobQueue.empty(); });

                    if(shutdown){
                        break;
                    }

                    //strange bug where it will continue even if the job queue is empty
                    if (jobQueue.size() < 1)
                        continue;

                    job = std::move(jobQueue.front());

                    jobQueue.pop();
                }
                job();
            }

        };
        threads.reserve(numThreads);
        for (int i = 0; i != numThreads; ++i) {
            threads.emplace_back(std::thread(threadFunc));
        }
    }

    ThreadPool(const ThreadPool& other) = delete;
    void operator=(const ThreadPool& other) = delete;

}; /* end ThreadPool Class */

You have a logical bug in the code. Imagine all jobs are finished and then shutdown is called. You'll never leave the notifier.wait(...).

Also, if a job exists but shutdown is called you quit instantly without doing it - I do not think that this is a good idea. I think you should finish all jobs prior to quitting. Make a special hard reset routine for the immediate exit. Though, it probably requires usage of exceptions.

Also, consider what will happen if an exception is called inside the function f. In the thread pool one of the threads will die and kill the whole program - most likely. It is not good. It should somehow forward the error to the caller. To this end I have thread pool functionality divided - made a supplementary class for user that wraps whatever additional functionality is needed.

Technical issues:

std::bind(f, args...) should be std::bind(f, std::forward<Args>(args)...) to ensure perfect forwarding as otherwise you might end up with unnecessary copying of data.
numThreads should be defaulted to std::thread::hardware_concurrency and I believe and uint8_t is getting too small for some of the latest processors. Additionally the ThreadPool should have the functionality to tell to its user how many threads it has.
I am not too fond of std::future - it is somewhat incomplete as Executors TS is still incomplete. Normally I'd expect that user would want to call the push task several times to complete a single operation. In which, case you generate multiple std::future<void>. It would be better to somehow to make it into a single wait instead of several. But I think I might be nitpicking - honestly I'd wait for Executor TS near completion and try to implement or get from somewhere something with the same interface as long as Executor TS is unavailable.
The declarations

ThreadPool(const ThreadPool& other) = delete;
void operator=(const ThreadPool& other) = delete;

are redundant as the class already has a std::mutex that makes the whole class non-copyable and non-movable. Yeah, it isn't important.


JavaScript function for to toggle multiple CSS-classes at once The method toggle of Element.classList expects a parameter which names a CSS-class.

You can't assign multiple classes at once by using for example an array.

So you have to write something like ...

div.classList.toggle("hidden-element");
div.classList.toggle("border-defs");
div.classList.toggle("green-theme");

Therefore I've made an function to which a list of classes can be passed.

var toggleButton = document.querySelector('.toggle-classes');

/**
 * Adds / removes CSS-class(es) from an HTML-element.

 * 1. String: CSS-class or a list of CSS-classes. Within a list
 *   the single values have to be separated by a space.
 * 2. String: A CSS-selector pointing to the HTML-element upon
 *   which the class(es) shall be added / removed.

 * Returns true to indicate success.
 * Throws Error in case of failure.
*/

function toggleClasses(cssSelector, listOfClasses) {
  var element;
  var classes;

  if (arguments.length !== toggleClasses.length)
    throw new Error('Lack of required parameter.');

  if (typeof listOfClasses !== 'string' || listOfClasses.length === 0)
    throw new Error('List of CSS-classes is not valid.');

  classes = listOfClasses.split(/\s/);

  if (typeof cssSelector !== 'string' || cssSelector.length === 0)
    throw new Error('Given CSS-selector is not valid.');

  element = document.querySelector(cssSelector);

  if (element === null)
    throw new Error('No HTML-element with selector ' + cssSelector + ' found.');

  classes.forEach(function(classItem, i) {
    element.className.indexOf(classItem) === -1
      ? element.classList.add(classItem)
      : element.classList.remove(classItem);
  });

  return true
}

// ------ USAGE EXAMPLE --------------------------------------------
var listPos = 'shadows font-special red-theme blue-theme misc-foo';

toggleButton.addEventListener('click', function() {
  try {
    toggleClasses('.demo', listPos);
  } catch (e) {
    console.error(e.message);
  }
});
.wrap {
  margin: 20px auto;
  width: 900px;
  background-color: rgba(245, 245, 245, 1.0);
  padding: 20px 30px;
}

.demo {
  width: 150px;
  height: 80px;
  line-height: 80px;
  text-align: center;
  margin: 10px 0;
}

.blue-theme {
  border: 3px solid pink;
  background-color: teal;
  color: white;
  font-weight: 800;
}

.red-theme {
  border: 3px solid teal;
  background-color: pink;
}

.shadows {
  text-shadow: 3px 3px 3px white;
  box-shadow: 3px 3px 3px grey;
}

.font-special {
  color: grey;
  font-weight: bold;
  font-family: georgia;
}

.misc-foo {
  border-radius: 12px;
  transform: skewY(5deg);
}

  <button class="toggle-classes">Toggle classes</button>

Demonstration

I'm still not sure if I shall stay with the way of assigning the classes. One could also use an array instead of a space-separated string.

Or make it optional?
If an array is assigned then that is used. If an string is assigned then it is separated.

Or would it be rather confusing to offer multiple options and better to stick with just one?

I also ask myself if my approach of throwing errors isn't a bit too drastically.

The CSS-classes are just for styling. So an application could go on with it's main purpose even if the function fails. It isn't really necessary to react so hard. Should I use something else instead? Returning, for example, a specified value?

Any hints and suggestions concerning flaws and possible improvements welcome.

This code is well-done, clear and rigourous, and it works fine.
But I would not work exactly the same.

Error processing.

I agree with your doubts, like this one:

  I also ask myself if my approach of throwing errors isn't a bit too drastically.

and this one:

  It isn't really necessary to react so hard.

As you already noticed this kind of function, only working on style, has nothing critical about failure.
So IMO it's preferable to silently not work when it's not possible, either because the target element is not found, or the list of classes is empty.

Obviously, at the opposite, it must take care of any "technical" failure which might fire an error message, but this should not lead to something blocking.
The only "not-silent" point I think good to keep is the true-or-undefined return: this allows to know how it turned when one is concerned, while it can be ignored otherwise.

Following this point of view, I suggest to merely ensure each involved data is clean, and work only if so.
As a supplemental benefit, it ends with a reduced code:

first regarding the presence of the two arguments, we can merely assign default values with classes = classes || ''; and selector = selector || '';
then use a unique if to ensure:

they are not of bad type
they are not empty
the selector really targets an element

List of classes argument.

Like you suggested:

  I'm still not sure if I shall stay with the way of assigning the classes. One could also use an array instead of a space-separated string.

I think it can be useful to offer a versatile way, even adding not only an array or a space-separated string but also a comma-separated string (with optional spaces).

This triple capability can be easily achieved by:

if (!Array.isArray(classes)) {
  classes = classes.replace(/(,\s*|\s+)/g, ' ').split(' ');
}

The toggle process itself.

Since it was your starting point, I'm surprised you didn't use the toggleClass() method in the final working part of your code:

element.className.indexOf(classItem) === -1
  ? element.classList.add(classItem)
  : element.classList.remove(classItem);

It can be replaced by:

element.classList.toggleClass(classItem);

Finally here is my proposed version, following all the above remarks:

/**
 * Adds / removes CSS-class(es) from an HTML-element.
 *
 * @param selector: a CSS selector for the involved element (string)
 *
 * @param classes: list of classes to be toggled, either:
 * - (string) comma- or space(s)-separated list of classes
 * - (array) of classes
 *
 * @return:
 * - true if could work
 * - undefined if any error occurred (bad, empty, or unsuccessfull
 *   selector; bad or empty classes)
 */

function toggleClasses(selector, classes) {
  selector = selector || '';
  classes = classes || '';
  if (
    typeof selector == 'string'
  &&
    (typeof classes == 'string' || Array.isArray(classes))
  &&
    classes.length // true if either string or array not empty
  &&
    (selector = document.querySelector(selector))
  ) {
    var classList = selector.classList;
    if (!Array.isArray(classes)) {
      classes = classes.replace(/(,\s*|\s+)/g, ' ').split(' ');
    }
    for (var className of classes) {
      classList.toggle(className);
    }
    return true;
  }
}

document.querySelector('.toggle-classes')
  .addEventListener('click', function() {
    console.log(toggleClasses(
    '.demo', 'shadows font-special red-theme blue-theme misc-foo'
    ));
});
.wrap {
  margin: 20px auto;
  width: 900px;
  background-color: rgba(245, 245, 245, 1.0);
  padding: 20px 30px;
}

.demo {
  width: 150px;
  height: 80px;
  line-height: 80px;
  text-align: center;
  margin: 10px 0;
}

.blue-theme {
  border: 3px solid pink;
  background-color: teal;
  color: white;
  font-weight: 800;
}

.red-theme {
  border: 3px solid teal;
  background-color: pink;
}

.shadows {
  text-shadow: 3px 3px 3px white;
  box-shadow: 3px 3px 3px grey;
}

.font-special {
  color: grey;
  font-weight: bold;
  font-family: georgia;
}

.misc-foo {
  border-radius: 12px;
  transform: skewY(5deg);
}

  <button class="toggle-classes">Toggle classes</button>

Demonstration


Simple code to check format of user inputted email address I am helping a friend with her Java homework and I adapted a solution I used in a similar project of my own for this. It is supposed to use loose/generous regex to make sure an email entered matches the forms abc123@xyz.net or abc.123@xyz.net.

import java.util.Scanner;

public class App {

    Scanner scanner;

    public App() {
        this.scanner = new Scanner(System.in);
    }

    public static void main(String[] args) {
        App app = new App();

        app.GetUsername(true);
    }

    public void GetUsername(Boolean firstRun) {

        if(!firstRun) {
            System.out.println("The username you have entered was in an incorrect format. Must match abc@xyz.net");
        } else {
            System.out.println("Please enter a username:");
        }

        String userInput = this.scanner.nextLine();

        UsernameCheck usernameCheck = new UsernameCheck(userInput);

        if(usernameCheck.isValid()) {
            System.out.println("Welcome, " + userInput + "!");
        } else {
            GetUsername(false);
        }
    }

}

UsernameCheck.java

import java.util.regex.*;

public class UsernameCheck {

    String username;

    public UsernameCheck(String username) {
        this.username = username;
    }

    public Boolean isValid() {
        return this.username.matches("[a-zA-Z0-9\\.]+@[a-zA-Z0-9\\-\\_\\.]+\\.[a-zA-Z0-9]{3}");
    }
}

I am most interested in hearing alternative solutions for re-prompting a user if the input they've given is invalid. This works in practice, but I am looking for the cleanest way to do it.

My preference for accepting user input in a blocking loop like this might look more like:

  public enum Returned { OK, ERROR_EMAIL };

…

  // Long loop here to get valid user registration info.
  /* In a transactional/event-driven (GUI, web) app this would not be
   * a loop, but with a blocking/modal (tty) program it works. */

  UserInfo u = new UserInfo ();
  // loop until you get valid one(s)
  while (!u.isReady()) {

    // for each field that must be validated, prompt and try to set it
    if (u.getName () == null) {
        System.out.print ("Enter an eMail address:");
        String entered = scanner.nextLine ();

        Returned settingName = u.setName (entered);

        // check each object's validity and report errors
        switch (settingName) {
            case Returned.OK:
                  System.out.println ("OK.");
                  break;
            case Returned.ERROR_EMAIL:
                  System.out.println ("That does not look like a valid eMail address.");
                  break;
             // No "default": if you add new error types later, you can handle them here.
             // The compiler will issue a warning about unhandled enum cases
        }
     }
  }

…and then in class UserInfo …

  final static Pattern rfc2822 = Pattern
                            .compile ("[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\\.[a-z0-9!#$%&'*+/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?");

  public Returned setName (final String address)
  {
       Returned valid = isValid(address);
       if (Returned.OK == valid) { this.name = address; }
       return valid;
  }

  public boolean isValid (final String address)
  {
         return ( (rfc2822.matcher(address).matches ())
                  ? Returned.OK : Returned.ERROR_EMAIL );
         // you could also check for a valid MX record for the domain part…
  }

  public boolean isReady () {
         return name != null; // and whatever else
  }


Index of letter removed to make string palindrome This code finds the index in the string and removes the one which will make the string a palindrome. If the string is already a palindrome, it should return -1.

This code works correct, but I am looking for a better implementation of it. Here is the problem description.

def palindrome(string):
    return string == string[::-1]

def palindrome_index(string, start, end):
    while start < end:
        if string[start] == string[end]:
            start, end = start+1, end-1
            continue
        if palindrome(string[start:end]): return end
        return start
    return -1
N = input()
strings = [str(raw_input()) for _ in range(N)]
for string in strings:
    print palindrome_index(string, 0, len(string)-1)

Can this be written shorter?  The size of input strings can be as large as 105, hence a recursive approach won't work.

Flow

The continue and returns inside the while loop make me feel the loop is doing too much.  I would say that the loop should be for moving start and end and then put the logic outside of it:

while ( start < end and string[start] == string[end] ):
    start, end = start + 1, end - 1

# now figure out the result

Result

The problem states that all strings that are not already palindromes are only off by one character.

Our loop moved start and end such that if start == end, then the string is a palindrome.  It has also moved these indices such that if the string is not a palindrome, one of them is 'pointing' to the character that needs to be removed.

To figure out which index is correct, I chose to compare the character pointed to by start to the character pointed to by end - 1.

if ( start == end ):
    return -1
elif ( string[ start ] == string[ end - 1 ] ):
    return end
else:
    return start

Palindrome Function

You don't need this function at all because we know that if the string is not a palindrome, it is only off by one character.  We were able to figure out the index of this character just by moving start and end

Anyway, since the function returns a boolean, it should be named isPalindrome


Constructing a 4×4 grid of words I have an ArrayList which contains Strings with a size of 1000. I am looping through that ArrayList and finding words and trying to form a square from them. For example as follows:

C A R D
A R E A
R E A R
D A R T

The square must match left to right and top to bottom. So first left to right word (card) must match first top to bottom (card) and so on. The list has [voes, rose, soon, odds] and so on, a 1000 of them all of length 4.

I am looping through them 1 by 1. So take 'voes' first, loop through remaining 999, find the first word starting with 'o', i store it as second word (example: odds) and move on to next loop.

This will give me for example:

voes
odds

loop 999 times again. first letter to start with 'e' and its 3rd letter is 'd' store that as 3rd name and move on to 4th loop.

This will give me for example:

voes
odds
eddo

loop 999 times again. Similar check for 4th word. And lets say I cant find any word that meets the condition. Break out of loop, go on top and replace the first word to the second word in the list and do it all over again.

I have managed to get it working but it involves a lot of nested looping. If I am reading this right, this is O(n^4). Is there a more efficient way to do this.

void makeWord(ArrayList<String> arr){

    String first, second, third, fourth;
    for (int i = 0; i < arr.size(); i++) {

        first = arr.get(i);

        for (int j = 0; j < arr.size(); j++) {
            if(first.substring(1,2).equals(arr.get(j).substring(0,1)) &&
                    (!first.equals(arr.get(j)))){
                second = arr.get(j);
            }
            else {
                second = "    ";
            }

            if (!second.trim().isEmpty()) {
                for (int k = 0; k < arr.size(); k++) {
                    if (first.substring(2, 3).equals(arr.get(k).substring(0, 1)) &&
                            second.substring(2, 3).equals(arr.get(k).substring(1, 2)) &&
                            (!first.equals(arr.get(k)) && (!second.equals(arr.get(k))))) {
                        third = arr.get(k);
                    } else {
                        third = "    ";
                    }

                    if (!third.trim().isEmpty()) {

                        for (int m = 0; m < arr.size(); m++) {
                            if(first.substring(3,4).equals(arr.get(m).substring(0,1)) &&
                                    second.substring(3,4).equals(arr.get(m).substring(1,2))&&
                                    third.substring(3,4).equals(arr.get(m).substring(2,3)) &&
                                    (!first.equals(arr.get(m)) && (!second.equals(arr.get(m))))){
                                fourth = arr.get(m);
                            }
                            else {
                                fourth = "    ";
                            }

                            if (!(fourth.trim().isEmpty())) {
                                System.out.println(first);
                                System.out.println(second);
                                System.out.println(third);
                                System.out.println(fourth);
                                System.out.println(" ");
                            }
                        }
                    }
                }
            }
        }
    }
}

I'm not sure if there's a faster way of doing it. However, your current code can be improved.
Enhanced for loop
Your loop are written using the traditional index:
String first;
for (int i = 0; i < arr.size(); i++) {
    first = arr.get(i);
    // ...

When you don't need the index, it is better to use the enhanced for loop. This has the advantage that it makes the code more compact and easier to read:
for (String first : arr) {
     // ...

You are also calling multiple times arr.get(j) with the index instead of storing the result in a local variable. Using the enhanced for loop solves that directly.
String comparisons
Right now, you are comparing each String together with
if(first.substring(1,2).equals(arr.get(j).substring(0,1)) &&
    (!first.equals(arr.get(j)))){

There are a couple of comments here:

This is very hard to read: it is unclear what the code is doing ;
This is split in two lines and has a lot of parentheses which doesn't help
Each line is "compact" so it's hard to see what is actually happening.

To rewrite that, we should remember that what we're trying to do here: test whether the 2nd character of the 1st string is the same as the 1st character of the 2nd string. So, instead of using substring, let's use charAt which directly returns the character at the given index. The 2nd character of the first String is first.charAt(1); the 1st charcter of the second String is second.charAt(0). As such, we could have:
if (!first.equals(second) && first.charAt(1) == second.charAt(0)) {
    // ...
}

Note the added spaces before the curly braces, between the operators and after the if statement, it also contributes to clarity.
Bizarre default condition
When the word currently considered is not a property fit, you are dping
else {
    second = "    ";
}

if (!second.trim().isEmpty()) {
    // ...
}

You are explicitely setting the String to whitespaces, only to trim it afterwards. This is a bit awkward.
What you want to do here is to continue when you reach an unwanted word. So you could have instead:
for (String first : arr) {
    for (String second : arr) {
        if (second.equals(first) || second.charAt(0) != first.charAt(1)) {
            continue;
        }
        for (String third : arr) {
            if (third.equals(first) || third.equals(second) ||
                first.charAt(2) != third.charAt(0) || second.charAt(2) != third.charAt(1)) {
                continue;
            }
            for (String fourth : arr) {
                if (fourth.equals(first) || fourth.equals(second) || fourth.equals(third) ||
                    first.charAt(3) != fourth.charAt(0) || second.charAt(3) != fourth.charAt(1) || third.charAt(3) != fourth.charAt(2)) {
                    continue;
                }

but now we're back to a mess: this is hard to read. However, it gives us insight into how can we do it better.
What we want is the following: we have a list of already found words (first and second for example); with that, we want to continue when the third is incorrect. What makes it incorrect is:

either it is equal to one of the previous found word
its characters do not match with the previously found words

So we can make a method of that!
Factoring common logic
private static boolean isIncorrect(String word, String... others) {
    boolean alreadyFound = Arrays.stream(others).anyMatch(word::equals);
    boolean notMatchedChars =
        IntStream.range(0, others.length)
                 .anyMatch(i -> word.charAt(i) != others[i].charAt(others.length));
    return alreadyFound || notMatchedChars;
}

This method is split in two (and uses Java 8 idioms but the logic would be the same without it). It takes as parameter a word to test and a vararg of the currently found words.

The first test checks whether any of the currently found words others are equal to the tested word.
The second test checks whether the characters are correct. If we have 3 currently found words then

the 1st character of the word must be equal to the 3rd character of the first word;
the 2nd character of the word must be equal to the 3rd character of the second word;
the 3rd character of the word must be equal to the 3rd character of the third word;

With that method, we can refactor our code to
void makeWord(List<String> arr) {
    for (String first : arr) {
        for (String second : arr) {
            if (isIncorrect(second, first)) {
                continue;
            }
            for (String third : arr) {
                if (isIncorrect(third, first, second)) {
                    continue;
                }
                for (String fourth : arr) {
                    if (isIncorrect(fourth, first, second, third)) {
                        continue;
                    }
                    System.out.println(first);
                    System.out.println(second);
                    System.out.println(third);
                    System.out.println(fourth);
                    System.out.println(" ");
                }
            }
        }
    }
}

which is very clean, although surely O(n4).


Anatomy of a string-reversal program with less coupling I'm currently trying to improve my C understanding. Would you mind telling me if there are good/bad things that I did here? (I implemented a function to reverse a string in place).

For example things I feel uncomfortable with:

I forwarded pointers to a buffer so that I could loosen the coupling of different parts of the program, is it a bad thing to forward many times a pointer?
I would like to set the functions I extracted to a header, somehow but I don't know if this is stupid or not

Here is what I came up with so far with my main.c file (the actual implementation of reverse_str is not what I want to focus on so I did not include it):

#include "reverse_str.h"

const long unsigned MAX_BUFFER_SIZE = 100;

void remove_new_line(char *buffer, const unsigned length)
{
        long unsigned buffer_last_index = length - 1;

        if (buffer[buffer_last_index] == '\n')
                buffer[buffer_last_index] = '\0';
}

char * get_buffer()
{
        char *buffer = malloc(MAX_BUFFER_SIZE * sizeof (*buffer));
        return buffer;
}

unsigned read_input(char *buffer, const long unsigned size)
{
        fgets(buffer, size, stdin);
        return strlen(buffer);
}

char * get_input()
{
        char *buffer = get_buffer();
        const unsigned length = read_input(buffer, MAX_BUFFER_SIZE);

        remove_new_line(buffer, length);

        return buffer;
}

char * ask_for_input()
{
        printf("Enter a string, whatever\n");
        char *buffer = get_input();

        return buffer;
}

void manipulate(char * buffer)
{
        reverse_str(buffer);
}

void show_result(char *buffer)
{
        printf("After: %s\n", buffer);
}

// I know it is a bit redundant, but it keeps symetry with ask_for_input
void clear(char *buffer)
{
        free(buffer);
}

int main()
{
        char *buffer = ask_for_input();
        manipulate(buffer);
        show_result(buffer);
        clear(buffer);

        return 0;
}

When you're passing a buffer around, you should always pass the size of the buffer (or the desired size of the buffer) as well. Your get_buffer() function that creates a buffer should return both the buffer and the buffer's size (either in a struct or as separate parameters), so that the caller of the function knows how much data is safe to put in it. (Yes, you've got that global constant, but what if you move to a paradigm where the allocator can allocate different-sized buffers?)
When you're using a buffer to hold a variable-length string,, you should pass the string length around as well, so you don't have to repeatedly call strlen(). Again you'll need to create a new struct or use separate parameters. fgets() returns the size of the string. Since fgets() doesn't return the length of the string, you do have to do a single strlen() (and you should, to make sure that you got the whole string ending with the newline).
I don't understand why you have a function that creates a buffer, but no function that destroy a buffer. Symmetric structure is good. The current setup locks you in to using malloc() and free(). If you wanted to switch to another memory allocation method, there's no simple way to do it.


Algorithm to calculate semi perfect integers, lack of efficiency I'm working on an algorithm to calculate weird numbers, and to do so there are several properties that needs to be calculated, one of them being, if it is NOT a semi-perfect/pseudoperfect number.

My code can surely be done a lot more effectively because semi perfect numbers have very interesting properties. The first one being, every multiple of a semiperfect number is semiperfect. So for every semi perfect number calculated, the multiple of that number can be stored so it will not have to be recalculated.

What I don't know is if there is a way to calculate if a number is NOT a semi perfect number without having to calculate if it is. It really is ineffective to calculate if a number is semi perfect, just to know if it is NOT.

Imagine infinity, which has infinite valid divisors, to prove if it is a semi perfect number would take infinite amount of calculations, but would proving the opposite also take infinite amount of calculations?

Anyways here is my code (written in Java) which will return a boolean indicating whether or not a number is semi perfect:

/**
 *
 * @param x
 *          a number to test if it is semiperfect or not.
 * @param list
 *          this is a array of the factors of x (e.g factors of 6 are 3, 2, 1 excluding itself).
 * @return true if x is semiperfect, false otherwise.
 */
public static boolean isSemiPerfect(int x, List<Integer> list) {
    if (x == 0)
        return true;
    for (int i = 0; i < list.size(); i++) {
        int temp = list.remove(i);
        if (isSemiPerfect(x - temp, list)) // using recursion
            return true;
        list.add(i, temp);
    }

    return false;
}

It seems like it is not possible to create a more effective algorithm to check if a number is NOT semi perfect, on the other hand, my current algorithm to check if a number is semi perfect or not can be improved.

I believe semi perfect numbers have some properties that would prevent the need to calculate every number, for instance. So far NO weird number has been discovered which is a odd number.

So unless the number in question (x) is above 1.8 * 1019, every odd number doesn't have to be checked if it is semi perfect or not, because every weird number below that is even.

Taking a look at this, a list of properties can be found, which can reduce the amount of numbers that needs to be checked.

I've realized that it will be hard to eliminate the calculation aspect completely, but can be reduced a lot. So I would like to have help with my algorithm to improve it, and perhaps implement a few of these properties to reduce the need to check every single number.

Funny enough I was working on the same problem.  My performance issue is not the Semi-Perfect part, but the check of abundancy.

You can call this (C#) function to check for a number being SemiPerfect.

static bool IsSemiPerfect(BigInteger delta, BigInteger[] d, BigInteger[] dSum, int index, BigInteger sum)
{
    if (sum == delta)
        return true;

    if (index >= d.Length || sum > delta || delta > (dSum[index] + sum))
        return false;

    for (int i = index; i < d.Length; i++)
    {
        if (IsSemiPerfect(delta, d, dSum, i + 1, sum + d[i]))
            return true;
    }

    return false;
}

The parameters are the following:

delta: sum of the divisors - n
d: divisors, descending sorted!
dSum: a cached calculation of the remaining sum

BigInteger[] dSum = new BigInteger[d.Length];
for (int i = d.Length - 1; i > -1; i--)
{
    BigInteger sum = 0;
    for (int j = divsIndex - 1; j >= i; j--)
        sum += d[j];
    dSum[i] = sum;
}

index: 0, used for recursion
sum: 0, used for recursion

The code runs much faster compared to the other answer, because of 3 major cuts in the search tree and a slightly different approach of the problem.  It is also optimised to find an answer in a progressive way.

A check of a big N = 190000006875 with 119 divisors runs well below 1 second.  I didn't do real profiling but my performance bottleneck is really somewhere else.  It might be NP, but very well doable this way.

It seems that posting a reply in C# code on a Java topic is a bit of bad behaviour.  To make it up I'll give some slightly off-topic but cool code to find really big record breaking weird numbers in Java, using Sidney Kravitz's algorithm.

public static BigInteger getPossibleWeirdNumber(int bits)
{
    BigInteger q = BigInteger.probablePrime(bits, new Random());
    int k = q.bitLength() - 1;
    BigInteger power2k = BigInteger.valueOf(2).pow(k);
    BigInteger r = ((power2k.multiply(q)).subtract(q.add(BigInteger.ONE))).divide(q.add(BigInteger.ONE).subtract(power2k));
    if(r.compareTo(power2k) > 0)
    {
        if(r.isProbablePrime(1000000) && q.isProbablePrime(1000000)) //Some final checking on primes
        {
            BigInteger weirdNumber = (BigInteger.valueOf(2).pow(k-1)).multiply(q).multiply(r);
            return weirdNumber;
        }
    }
    return null;
}

public static BigInteger getWeirdNumber(int bits)
{
    BigInteger r = getPossibleWeirdNumber(bits);
    while(r == null)
        r = getPossibleWeirdNumber(bits);

    return r;
}

The only problem is that this algorithm does not find odd weird numbers. It's easy to see why: The \$2^{k-1}\$ always will be an even number. Multiplying with an even number always results in an even number, so Sidney Kravitz's trick will help you find large weird numbers, but no odd ones. That's why I manually started looking.

By the way, the reason I used C# is the more clean look of the code when using BigInteger. The operator overloading in .Net is lovely.


speeding up matrix corruption I need to randomly erase 33% of the non-zero elements in each line of a matrix.
Side note: The end goal is to train a denoising autoencoder to remove this noise.
My solution:
import numpy as np

matrix = np.random.rand(1000,3000)

for i in range(matrix.shape[0]):
    clean = matrix[i, :] # original matrix line
    # find non zero elements
    msk = np.nonzero(clean)
    assert sum(msk[0]) != 0

    # keep 66% of them
    idx = np.random.randint(0, len(msk[0]), size=max(1, len(msk[0])//3)) #erase at least 1
    msk = np.delete(msk, idx)

    dirty = clean
    dirty = [j if i in msk else 0 for i,j in enumerate(dirty)]
    assert sum(clean-dirty) != 0

    #save clean and dirty
    #...

My guess is that the bottleneck is drawing random numbers at each iteration.
Is there an alternative way to do this?

My guess is that the bottleneck is drawing random numbers at each iteration.

Why guess when you can measure? I changed the height to 100 so the whole thing takes me about 8 seconds, reasonable to play around with. Removing the assert line on the bottom makes it a bit faster. Also removing the dirty = [j if...] line makes it a lot faster. As you'd expect from that inefficient line.
Not sure why you're making it so complicated. I think clean[idx] = 0 does the deletions you want, and it's fast.


Function in a board game with chests and bandits This function is part of a python board game program. The game is a board game with chests and bandits hidden throughout the board. The function is dedicated to the "easy" section of the game (where it is a 8x8 grid).

def easy_level(Coins):
#This function is for the movement of the game in easy difficulty
    while  True:
        oldcurrent=current
        boardeasy[oldcurrent[0]][oldcurrent[1]]='*'
        table_game_easy()
        boardeasy[oldcurrent[0]][oldcurrent[1]]=' '

        n = input('Enter the direction followed by the number Ex:Up 5 , Number should be < 8 \n')
        n=n.split()
        if n[0].lower() not in ['up','left','down','right']:#Validates input
            print('Wrong command, please input again')
            continue
        elif n[0].lower()=='up':
            up(int(n[1].lower()),8)#Boundary is set to 8 as the 'easy' grid is a 8^8
        elif n[0].lower()=='down':
            down(int(n[1].lower()),8)
        elif n[0].lower()=='left':
            left(int(n[1].lower()),8)
        elif n[0].lower()=='right':
            right(int(n[1].lower()),8)

        print("5 chests left")
        print("8 bandits left")
        print("Coins:",Coins)#Acts as a counter, displays the number of coins that the player has
        if current[0] == Treasure1_Row and current[1] == Treasure1_Col\
           or current[0] == Treasure2_Row and current[1] == Treasure2_Col\
           or current[0] == Treasure3_Row and current[1] == Treasure3_Col\
           or current[0] == Treasure4_Row and current[1] == Treasure4_Col\
           or current[0] == Treasure5_Row and current[1] == Treasure5_Col\
           or current[0] == Treasure6_Row and current[1] == Treasure6_Col\
           or current[0] == Treasure7_Row and current[1] == Treasure7_Col\
           or current[0] == Treasure8_Row and current[1] == Treasure8_Col\
           or current[0] == Treasure9_Row and current[1] == Treasure9_Col\
           or current[0] == Treasure10_Row and current[1] == Treasure10_Col:
            print("Hooray! You have found booty! +10 gold")
            Coins = Coins+10 #Adds an additional 10 points
            print("Coins:",Coins)

        if current[0] == Bandit1_Row and current[1] == Bandit1_Col\
             or current[0] == Bandit2_Row and current[1] == Bandit2_Col\
             or current[0] == Bandit3_Row and current[1] == Bandit3_Col\
             or current[0] == Bandit4_Row and current[1] == Bandit4_Col\
             or current[0] == Bandit5_Row and current[1] == Bandit5_Col:
            print("Oh no! You have landed on a bandit...they steal all your coins!")
            Coins = Coins-Coins #Removes all coins
            print("Coins:",Coins)

        boardeasy[current[0]][current[1]]='*'#sets value to players position

We're definitely missing some context here to run your code or understand what it is supposed to do, but there is enough to detect a few things that could be easily improved.

        Coins = Coins-Coins #Removes all coins

This should be:

        Coins = 0  #Removes all coins

Also

        Coins = Coins+10 #Adds an additional 10 points

can be written

        Coins += 10  #Adds an additional 10 points

Don't perform the same operations more than needed. In particular when handling the user input, you can limit the number of index accesses, to call to lower function, to call to int function:

    user_input = input('Enter the direction followed by the number Ex:Up 5 , Number should be < 8 \n').split()
    if len(user_input) != 2:
        print('Wrong command, please input again')
        continue
    direct, number = user_input
    direct = direct.lower()
    number = int(number.lower())
    if direct not in ['up','left','down','right']:#Validates input
        print('Wrong command, please input again')
        continue
    elif direct == 'up':
        up(number, 8)  #Boundary is set to 8 as the 'easy' grid is a 8^8
    elif direct == 'down':
        down(number, 8)
    elif direct == 'left':
        left(number, 8)
    elif direct == 'right':
        right(number, 8)

Then, you can actually change the condition order so that you don't need to list twice the valid directions:

    if direct == 'up':
        up(number, 8)  #Boundary is set to 8 as the 'easy' grid is a 8^8
    elif direct == 'down':
        down(number, 8)
    elif direct == 'left':
        left(number, 8)
    elif direct == 'right':
        right(number, 8)
    else:
        print('Wrong command, please input again')
        continue

You could probably rewrite the comparisons to have something like:

    if current == Treasure1_Pos\
       or current == Treasure2_Pos\
       or current == Treasure3_Pos\
       or current == Treasure4_Pos\
       or current == Treasure5_Pos\
       or current == Treasure6_Pos\
       or current == Treasure7_Pos\
       or current == Treasure8_Pos\
       or current == Treasure9_Pos\
       or current == Treasure10_Pos:
        print("Hooray! You have found booty! +10 gold")
        Coins += 10  #Adds an additional 10 points
        print("Coins:",Coins)

    if current == Bandit1_Pos\
         or current == Bandit2_Pos\
         or current == Bandit3_Pos\
         or current == Bandit4_Pos\
         or current == Bandit5_Pos:
        print("Oh no! You have landed on a bandit...they steal all your coins!")
        Coins = 0  #Removes all coins
        print("Coins:",Coins)

And you could even define a data structure (list, set) to hold all the relevant positions and write something like:

    if current in Treasure_Positions:
        print("Hooray! You have found booty! +10 gold")
        Coins += 10  #Adds an additional 10 points
        print("Coins:",Coins)

    if current in Bandit_Positions:
        print("Oh no! You have landed on a bandit...they steal all your coins!")
        Coins = 0  #Removes all coins
        print("Coins:",Coins)

Then more things look wrong/improvable about boardeasy but we'd need to see what it does.


LINQ query using StartsWith and Contains to limit the results for an autocomplete field As a source for a jquery-ui autocomplete field, I've written this action:

public ActionResult Autocomplete(string term)
{
    List<Relation> relations =
        db.Relation
            .Where(l => l.AdministrationId == Helpers.Session.AdministrationId &&
                        l.IsSupplier == true)
            .ToList();

    term = term.ToLowerInvariant();

    List<string> results = new List<string>();

    foreach (Relation r in relations) {
        string name = r.Name.ToLowerInvariant();

        if (name.StartsWith(term) || name.Contains(term)) {
            results.Add(name);
        }
    }

    results = results.OrderBy(l => l).Take(5).ToList();

    return Json(results, JsonRequestBehavior.AllowGet);
}

Since this is very verbose and probably far from optimized, I'd like to know what I can improve.

I've tried to put it all in a single LINQ query, like the code below. However, LINQ will try to interpret everything as extension methods and throw an error (ToLowerInvariant() and StartsWith()).

term = term.ToLowerInvariant();

IEnumerable<string> suppliers =
    db.Relations
        .Where(l => l.AdministrationId == Helpers.Session.AdministrationId &&
                    l.IsSupplier== true &&
                    (l.Name.ToLowerInvariant().StartsWith(term) ||
                     l.Name.ToLowerInvariant().Contains(term)))
        .Select(l => l.Name)
        .OrderBy(l => l)
        .Take(5);

What I'm looking for is a way to improve the first code snippet. It looks very verbose now and I'm pretty sure it's far from optimized when handling a lot of requests and when I have lots of rows in my database.

Naming
Instead of term maybe searchTerm would be better. As in your second example you name the IEnumerable<> variable suppliers, your method would be better named as AutocompleteSuppliers.

Review

A Boolean variable can be evaluated just by using the variable, so && l.IsSupplier == true can be replaced by && l.IsSupplier. As an evaluation of an boolean is faster as the comparing of an int (assuming AdministrationId  is an int) we change the order of the where condition like

List<Relation> relations =
    db.Relation
        .Where(l => l.IsSupplier &&
               l.AdministrationId == Helpers.Session.AdministrationId)

Inside the foreach loop you are first checking if the Relation.Name is starting with term and if not, you are checking if term is contained inside Relation.Name. This will be faster if term is often the starting of the name, but will be slower if it is contained as of the double check.
So let us just remove the StartsWith().

Your combination of the Linq statement and the loop does look ok, but will produce different results than in the Autocomplete() method, as you are selecting Relation.Name instead of Relation.Name.ToLowerInvariant().

Refactoring

As this refactoring is based on the original method we use the Select() with ToLowerInvariant() first and are checking afterwards if it contains the searchTerm

public ActionResult AutocompleteSuppliers(string searchTerm)
{
    searchTerm = searchTerm.ToLowerInvariant();

    IEnumerable<string> suppliers = db.Relations
        .Where(l => l.IsSupplier && l.AdministrationId == Helpers.Session.AdministrationId)
        .Select(l => l.Name.ToLowerInvariant())
        .Where(s => s.Contains(searchTerm))
        .OrderBy(s => s)
        .Take(5);

    return Json(suppliers, JsonRequestBehavior.AllowGet);

}


Determining optimal weight lifting bar configuration This program is meant to take two weights and return the best way to load a barbell for two people using different weights, so you don't have to keep changing out super heavy weights. I also made functionality for one person to use it to calculate the most efficient way to load the bar (least # of plates used).

If you notice, user_one() and user_two() are mostly the same code, with minor adjustments. perfect_combo() also shares some of the same.  I feel like there's a way to cut down on the code in this program.

weights=[100, 45, 35, 25, 10, 5, 2.5]
bar_weight = 45
prog_select = int(input('How many people on this bar?'
'(enter 1 or 2) '))
if prog_select == 1:
    target_weight = int(input('Enter weight: '))
elif prog_select == 2:
    target_weight = int(input('Enter first weight: '))
    target_weight1 = int(input('Enter second weight: '))

def user_one():
    nweights = []
    remaining = target_weight - bar_weight
    for  i, weight in enumerate(weights):
        if int(remaining / weight) % 2 == 0:
            nweights.append(int(remaining/ weight))
        else:
            nweights.append(int(remaining/ weight) - 1)
        remaining = remaining - nweights[i]*weights[i]
        if remaining == 0:
            break

    listweights=zip(nweights, weights)
    print(f'{bar_weight}lb bar')
    print(f'{target_weight}lbs')

    for weight in listweights:
        if weight[0] >= 2:
            print(f"{weight[0]} | {weight[1]}'s")

    if remaining !=0:
        print(f'the correct weight combination cannot be found,'
              f'the remaining weight is: {remaining} pound')

def user_two():
    nweights = []
    remaining = target_weight1 - bar_weight
    for  i, weight in enumerate(weights):
        if int(remaining / weight) % 2 == 0:
            nweights.append(int(remaining/ weight))
        else:
            nweights.append(int(remaining/ weight) - 1)
        remaining = remaining - nweights[i]*weights[i]
        if remaining == 0:
            break

    listweights=zip(nweights, weights)
    if prog_select == 1:
        print(f'{target_weight1}lbs')
    else:
        print(f'{bar_weight}lb bar')
        print(f'{target_weight1}lbs')
    for weight in listweights:
        if weight[0] >= 2:
            print(f"{weight[0]} | {weight[1]}'s")

    if remaining !=0:
        print(f'the correct weight combination cannot be found,'
              f'the remaining weight is: {remaining} pound')

def perfect_combo():
    nweights = []
    remaining = (target_weight - target_weight1)

    print("Add " + str(remaining) + "lbs for first weight")

    for  i, weight in enumerate(weights):
        if int(remaining / weight) % 2 == 0:
            nweights.append(int(remaining/ weight))
        else:
            nweights.append(int(remaining/ weight) - 1)
        remaining = remaining - nweights[i]*weights[i]
        if remaining == 0:
            break

    listweights=zip(nweights, weights)

    for weight in listweights:
        if weight[0] >= 2:
            print(f"{weight[0]} | {weight[1]}'s")
    if remaining !=0:
        print(f'the correct weight combination cannot be found,'
              f'the remaining weight is: {remaining} pound')

# Run the program

if prog_select == 1:
    user_one()
elif prog_select == 2:
    remaining = (target_weight - target_weight1)
    if remaining > 99:
        print("You might want to use seperate bars...")
    else:
        user_two()
        perfect_combo()
else:
    print("That's too many peeps on one bar!")

The layout of your program is non-standard.  You have:

main program part 1
function definitions
main program part 2

Keep your main program together, don't split it up by adding functions in the middle, at least, not without a compelling reason.

There is minimal error checking in the program.  If the target weight is less than the bar weight, this is not reported.

    for  i, weight in enumerate(weights):
        if int(remaining / weight) % 2 == 0:
            nweights.append(int(remaining/ weight))
        else:
            nweights.append(int(remaining/ weight) - 1)
        remaining = remaining - nweights[i]*weights[i]
        if remaining == 0:
            break

    listweights=zip(nweights, weights)

This code is duplicated in user_one(), user_two() and perfect_combo().  You should move this into a function.

The code computes the number of a specific weight, and then adds the number to a list if it is even, or 1 less if it is odd.  Presumably, this is because the weights have to be added in pairs, one on each side of the bar, but this is never spelt out.

Finally, the list is zipped with the original list of weight options, to create pairs of counts and weights.

Let's create the function which does this computation, but let's make it more general.  You might have different bar types, say a light weight bar and a heavy weight bar.  The light weight bar would be lighter, and perhaps uses a different set of weights, but the algorithm would remain the same.  So the available weights should be passed in:

def weights_in_pairs(added_weight, available_weights):

    """Compute and return list of numbers and sizes of weights to
    form the desired additional weight for bar-bell.  Also returns any
    excess weight that could not be added."""

    weights = []
    remaining = added_weight
    for weight in available_weights:
        pairs = int(remaining / (2*weight))
        if pairs  > 0:
            weights.append((pairs * 2, weight))
            remaining -= 2 * pairs * weight

    return weights, remaining

Instead of dealing with even/odd numbers, we double the weight and compute the number of pairs that can be added add.  Instead of zipping at the end, we add the (count, weight) tuple to the weights list; this allows us to omit the zero counts, to simplify printing later

    for weight in listweights:
        if weight[0] >= 2:
            print(f"{weight[0]} | {weight[1]}'s")

    if remaining !=0:
        print(f'the correct weight combination cannot be found,'
              f'the remaining weight is: {remaining} pound')

This code is also duplicated between user_one(), user_two() and perfect_combo().  Again, we'll move this into a function.

def print_weights(weights, remaining):

    """Print out the weights to be added to the bar-bell,
    and the amount of un-added weight, if necessary."""

    for n, weight in weights:
        print(f"{n} | {weight}'s")

    if remaining != 0:
        print(f'the correct weight combination cannot be found,'
              f'the remaining weight is: {remaining} pound')

Note that the for loop immediate extracts the tuple into two variables, with the names n and weight.  This allows us to use these named quantities in the body of the loop, instead of indexes like weight[0] and weight[1] where the reader will have to look elsewhere to determine what was stored in the different indexes.

Also, since we skipped added the zero-count weights, we didn't need the if weight[0] >= 2: statement.

We can combined the above two functions, to compute and print out the information displayed by user_one() and user_two().  Again, we'll pass in arguments for the desired weight, the bar weight, and the available weights:

def print_bar_configuration(target_weight, bar_weight, weights):

    """Compute and print bar-bell configuration for given weights."""

    listweights, remaining = weights_in_pairs(target_weight - bar_weight, weights)

    print(f'{bar_weight}lb bar')
    print(f'{target_weight}lbs')

    print_weights(listweights, remaining)

With the above functions, perfect_combo() can be simplified:

def perfect_combo(heavy_weight, light_weight):

    """Compute and print optimal additional weight configuration."""

    weight_difference = heavy_weight - light_weight

    print(f"Add {weight_difference}lbs for heavier weight")

    listweights, remaining = weights_in_pairs(weight_difference, weights)
    print_weights(listweights, remaining)

Note: This raise the question of "what happens if the first weight was lighter than the second?"  Perhaps your program should mention that requirement, and test for it.  Or adapt the reversed data input.

Using the above functions, we can simplify the main program, moving and combining the code at the top with the code at the bottom.  Also, renaming odd variables like target_weight1 to something more meaningful:

if prog_select == 1:
    target_weight = int(input('Enter weight: '))
    print_bar_configuration(target_weight, bar_weight, weights)
elif prog_select == 2:
    heavy_weight = int(input('Enter heavier weight: '))
    light_weight = int(input('Enter lighter weight: '))

    if heavy_weight - light_weight > 99:
        print("You might want to use separate bars...")
    else:
        print_bar_configuration(light_weight, bar_weight, weights)
        perfect_combo(heavy_weight, light_weight)
else:
    print("That's too many peeps on one bar!")


A generic IEnumerator to enumerate COM collections As I wrapped the VBIDE API, I encountered a number of "collection types" (Windows, CodePanes, VBComponents, VBProjects, References, etc.) - these types implement the non-generic IEnumerable interface, so if I wanted to be able to iterate my wrappers like this for example:

using (var projects = _vbe.VBProjects)
{
    foreach (var project in projects.Where(p => p.Protection == ProjectProtection.Unprotected))
    {
        yield return project.Name;
    }
}

...then I was going to be needing some custom IEnumerator implementation, because even if I could explicitly .Cast<Rubberduck.VBEditor.DisposableWrappers.Project>() every time, at runtime the cast would fail because the enumerator yields a COM object, not a wrapper type.

So I made a generic ComWrapperEnumerator<TComCollection, TWrapperItem>, where TComCollection is the collection type (must implement IEnumerable) and TWrapperItem is the type of the SafeComWrapper<T> to be created and returned - am I doing this right? I don't like the way I'm assuming the constructor only has a single T parameter (where T is the COM type being wrapped by TWrapperItem), is there a better way?

using System;
using System.Collections;
using System.Collections.Generic;

namespace Rubberduck.VBEditor.DisposableWrappers
{
    public class ComWrapperEnumerator<TComCollection, TWrapperItem> : IEnumerator<TWrapperItem>
        where TComCollection : IEnumerable
    {
        private readonly IEnumerator _internal;

        public ComWrapperEnumerator(TComCollection items)
        {
            _internal = items.GetEnumerator();
        }

        public void Dispose()
        {
            var disposable = _internal as IDisposable;
            if (disposable == null)
            {
                return;
            }
            disposable.Dispose();
        }

        public bool MoveNext()
        {
            return _internal.MoveNext();
        }

        public void Reset()
        {
            _internal.Reset();
        }

        public TWrapperItem Current
        {
            get { return (TWrapperItem)Activator.CreateInstance(typeof(TWrapperItem), _internal.Current); }
        }

        object IEnumerator.Current
        {
            get { return Current; }
        }
    }
}

Anything else?

For context, here's the SafeComWrapper<T> abstract base class' constructor:

protected SafeComWrapper(T comObject)
{
    _comObject = comObject;
}

Here's how the enumerator is used, here in the CodePanes wrapper:

IEnumerator<CodePane> IEnumerable<CodePane>.GetEnumerator()
{
    return new ComWrapperEnumerator<Microsoft.Vbe.Interop.CodePanes, CodePane>(ComObject);
}

IEnumerator IEnumerable.GetEnumerator()
{
    return new ComWrapperEnumerator<Microsoft.Vbe.Interop.CodePanes, CodePane>(ComObject);
}

I can't think of a clean way to get generic type parameters inferred by the compiler, so every new ComWrapperEnumerator needs these ugly generic type parameters.

Your ComWrapperEnumerator is generic in two types without needing to.
You don't really care about TComCollection aside that it must be an IEnumerable,
thus that generic type can disappear from your class and you will end up with a constructor method that receives an IEnumerable.

public ComWrapperEnumerator(IEnumerable items)
{
    _internal = items.GetEnumerator();
}

Normally the non generic version calls the generic version of the method, removing your code repetition:

IEnumerator IEnumerable.GetEnumerator()
{
    //call the generic GetEnumerator
    return GetEnumerator();
}

In this particular case I think there isn't any other way to improve the instantion of ComWrapperEnumerator.
However one way that normally is used to avoid specifying types on constructors is to specify a static method to instantiate the type.
If you look for example to Tuple.Create this is exactly what they do.

The hack could work with code like this:

public static ComWrapperEnumerator<T> Create<T>(T t, IEnumerable value){
    return new ComWrapperEnumerator<T>(value);
}

//usage
ComWrapperEnumerator.Create(default(CodePane), ComObject);

You can always provide methods for all types that you care about:

public static ComWrapperEnumerator<CodePane> CreateCodePane(IEnumerable value){
    return new ComWrapperEnumerator<CodePane>(value);
}

But anyway, now that you have one less generic type maybe you are no longer concerned about that.

  I don't like the way I'm assuming the constructor only has a single T
  parameter (where T is the COM type being wrapped by TWrapperItem), is
  there a better way?

Yes there is. Not only there is but would also remove the need of that pesky Activator.CreateInstace. The most straightforward approach that I can think of is to pass a Func that would receive the COM object and return an instance of the type you want. This would be a parameter of the constructor.

private readonly Func<object, TWrapperItem> _createItem;
public ComWrapperEnumerator(IEnumerable items, Func<object, TWrapperItem> createItem)
{
    _internal = items.GetEnumerator();
    _createItem = createItem;
}

public TWrapperItem Current
{
    get { return _createItem(_internal.Current); }
}

object IEnumerator.Current
{
    get { return Current; }
}

Obviously you can always fallback to Activator:

private TWrapperItem CreateItem(object value){
   return (TWrapperItem )Activator.CreateInstance(typeof(TWrapperItem), value);
}

public ComWrapperEnumerator(IEnumerable items, Func<object, TWrapperItem> createItem = null)
{
    _internal = items.GetEnumerator();
    _createItem = createItem ?? CreateItem;
}

Note that if you go with this approach you may consider to come back to having two generic types on your class, so you could have a typesafe _createItem.


C++ Not-So-Simple Entire HTML Generating Code v0.3 As a follow-up to my code's v0.1 revision, we now introduce V0.3, with capability for storage of attributes and a struct for each HTML tag. Just start the struct, call its functions, change its variables then use <fstream> to make the complete HTML file using generateHTML()!

#include <iostream>
#include <fstream>
#include <vector>

struct HtmlTag {
    std::string tagname;
    std::string tagcontent;
    std::vector<std::string> tagattributes;
    std::vector<std::string> tagattrcontent;
    void addAttribute(std::string attrName, std::string attrCont) {
        tagattributes.push_back(attrName);
        tagattrcontent.push_back(attrCont);
    }
    int removeAttribute(int pos) {
        tagattributes.erase(tagattributes.begin() + pos);
        tagattrcontent.erase(tagattrcontent.begin() + pos);
        return tagattributes.size();
    }
    std::string returnTag() {
        std::string attributes = "";
        if (tagattributes.size() != tagattrcontent.size()) {
            return "ERR:UNMATCHINGLENGTH";
        }
        unsigned short i;
        if(tagattributes.size() > 0 && tagattrcontent.size() > 0) {
            for(i = 0; i < tagattributes.size(); i++) {
                attributes = attributes + " " + tagattributes[i] + "=\"" + tagattrcontent[i] + "\"";
            }
        }
        else {
            return "<" + tagname + ">" + tagcontent + "</" + tagname + ">";
        }
        return "<" + tagname + attributes + ">" + tagcontent + "</" + tagname + ">";
    }
};

std::string generateHTML(std::string doctype, std::string head, std::string body) {
    return "<!DOCTYPE " + doctype + " />\n\n<html>\n    <head>\n        " + head + "\n    </head>\n    <body>\n        " + body + "\n    </body>\n</html>";
}

int main()
{
    HtmlTag myHTML;
    myHTML.addAttribute("spam", "eggs");
    myHTML.tagcontent = "bar";
    myHTML.tagname = "foo";
    unsigned int i2;
    for(i2 = 0; i2 < myHTML.tagattrcontent.size(); i2++) {
        std::cout << myHTML.tagattrcontent[i2] << ":" << myHTML.tagattributes[i2] << std::endl;
    }
    std::cout << myHTML.returnTag() << std::endl;
    std::cout << generateHTML("html", "<title>Hello World!</title>", myHTML.returnTag()) << std::endl;
}

I see some things that may help you improve your code.

Prefer class to struct

The way it's currently defined, any code can modify the contents of the structure.  Better would be to make the data members private and provide necessary accessors to assure that the class is always complete and coherent.

Don't use parallel structures

Right now, the tag attributes and tag contents are stored in parallel vectors. The only association between an attribute and its value is that they have the same position within both vectors.  Instead, use a std::pair and create a vector of those.  That way, each attribute is a single entity.  We can replace the two vectors with this one:

std::vector<std::pair<std::string,std::string>> attributes;

Use "range-for" to simplify code

C++11 and newer allow the use of "range-for" which can really simplify code.  For instance, the current code has this loop:

unsigned int i2;
for(i2 = 0; i2 < myHTML.tagattrcontent.size(); i2++) {
    std::cout << myHTML.tagattrcontent[i2] << ":" << myHTML.attributes[i2] << std::endl;
}

If we follow the previous point and have a single attributes vector instead, we can rewrite this as:

for (const auto &attr : myHTML.attributes) {
    std::cout << attr.first << ":" << attr.second << std::endl;
}

Use const where practical

In the returnTag routine, the underlying HtmlTag is not altered.  Make this explicit by declaring that method const:

std::string returnTag() const {

Think of the user

As a user of this code, I think I'd prefer to remove an attribute by name rather than by position.  It's also not clear that returning the new count of attributes would be useful if the caller already has to keep track of the indexing.

Provide a constructor

It seems to me that it would be nice to be able to write code like this:

HtmlTag myHTML{"foo","bar",{{"spam","eggs"}}};

We can do that by providing the appropriate constructor.  In this case, it's actually quite simple:

HtmlTag(std::string name, std::string content, std::vector<std::pair<std::string,std::string>> attr) :
    tagname{name},
    tagcontent{content},
    attributes{attr}
{}

Note that I'm assuming the attributes are now pairs as previously suggested.

Use your own code

Why doesn't the generateHTML code use any HtmlTags?  Here's a way it could be rewritten to do so:

std::string generateHTML(std::string doctype, std::string headtext, std::string bodytext) {
    HtmlTag body{"body",bodytext};
    HtmlTag head{"head",headtext};
    HtmlTag html{"html",head.returnTag()+body.returnTag()};
    return "<!DOCTYPE " + doctype + " />\n\n" + html.returnTag();
}

If that's not the way you'd like to write the code, it may suggest some improvements that might be made to the HtmlTag class.


Caching wrapper for digest computation I need to review this code:

import java.util.HashMap;
import java.util.Map;

// REVIEW
public abstract class Digest {
    private Map<String, byte[]> cache = new HashMap<>();

    public byte[] digest(String input) {
        byte[] result = cache.get(input);
        if (result == null) {
            synchronized (cache) {
                result = cache.get(input);
                if (result == null) {
                    result = doDigest(input);
                    cache.put(input, result);
                }
            }
        }
        return result;
    }

    protected abstract byte[] doDigest(String input);
}

Should I mark cache as volatile ?
I have doubts because we use cashe.get in method body instead of direct cashe
Would it make sense to replace the map with ConcurrentHashMap?
Is there way to fix code without replacing map implementation?

Volatile won't do anything here, as this basically only affects the reference to the map, not the contents of the map.

As for ConcurrentHashMap (as opposed to explicit synchronizing I suppose): this depends. If you want to make absolutely sure that no digest is ever computed twice, stick to your approach. If you just want general concurrency, but can accept that eventually in race-conditions, digest is called twice for the same input (sidenote: I suppose digest is deterministic, so that two calls with the same input yield the same result), you may simply repace the map with a ConcurrentHashMap and the whole method with:

return cache.computeIfAbsent(input, this::doDigest);


Permutations with a sum constraint I have made the following function that gives the combinations permuted of n elements with a constrain/rule. This consist on creating a dataframe with all combination of numbers from 0 to n that sums n.

Function

import pandas as pd
import itertools
import numpy as np

def combinations_permuted(n_elm):
    items = list(range(0,n_elm+1,1))
    combinations = pd.DataFrame(list(filter(lambda x: np.sum(x)==n_elm,list(itertools.combinations_with_replacement(items, n_elm)))))
    comb_permuted = pd.DataFrame()
    for index, combination in combinations.iterrows():
        comb_permuted=comb_permuted.append(list(filter(lambda x: np.sum(x)==n_elm,list(itertools.permutations(combination.tolist(), n_elm)))))

    return(comb_permuted.drop_duplicates().as_matrix())

Example

array([[0, 0, 3],
       [0, 3, 0],
       [3, 0, 0],
       [0, 1, 2],
       [0, 2, 1],
       [1, 0, 2],
       [1, 2, 0],
       [2, 0, 1],
       [2, 1, 0],
       [1, 1, 1]], dtype=int64)

The problem is that is taking a long time when n_elm is "big" for example 9. I think this code can be improving in terms of time performance.

Maybe by replacing the for loop with a map function.

Any help to get it?

def combinations_permuted(n_elm):

IMO n (as used in the description of the code in the question) is more readable than n_elm.

    items = list(range(0,n_elm+1,1))

1 is the default step, and range isn't so obscure a function that the maintenance programmer would wonder what range(0, n+1) does.

    combinations = pd.DataFrame(list(filter(lambda x: np.sum(x)==n_elm,list(itertools.combinations_with_replacement(items, n_elm)))))
    comb_permuted = pd.DataFrame()
    for index, combination in combinations.iterrows():
        comb_permuted=comb_permuted.append(list(filter(lambda x: np.sum(x)==n_elm,list(itertools.permutations(combination.tolist(), n_elm)))))

    return(comb_permuted.drop_duplicates().as_matrix())

There are two things here which sound warning bells:

comb_permuted=comb_permuted.append. The documentation for DataFrame.append says

  Iteratively appending rows to a DataFrame can be more computationally intensive than a single concatenate. A better solution is to append those rows to a list and then concatenate the list with the original DataFrame all at once.

drop_duplicates(). That says that the generation process is doing more work that it needs to.

A KISS approach would be to replace the combinations_with_replacement, permutations, drop_duplicates chain with itertools.product. This is much faster at n = 3, but already slower at n = 5 (because it's still doing more work that it needs to, and filtering).

The efficient approach is to do only the work that's necessary. A quick and dirty implementation (i.e. please tidy it up before using) calculates for n = 6 in less time than it takes the original code to calculate n = 3:

def combinations_recursive_inner(n, buf, gaps, sum, accum):
  if gaps == 0:
    accum.append(list(buf))
  else:
    for x in range(0, n+1):
      if sum + x + (gaps - 1) * n < n: continue
      if sum + x > n: break
      combinations_recursive_inner(n, buf + [x], gaps - 1, sum + x, accum)

def combinations_recursive(n):
  accum = []
  combinations_recursive_inner(n, [], n, 0, accum)
  return pd.DataFrame(accum).as_matrix()

Online test and benchmark


Good conduct of including numbers in css classes I'm creating a couple of dynamic css classes, and the idea is that i get a string from an episerver context and add that to the class list of an element.

I know that numbers can be included in css classes, but what is good css code conduct, is it a good idea to include numbers in class names, or should i change it from body1 --> body-one.

body1 {
    font-size: 1.4rem;
}
body2 {
    font-size: 1.6rem;
}

body-one {
    font-size: 1.4rem;
}
body-two {
    font-size: 1.6rem;
}

Perfect naming in any language allowing freedom for your names are:

Descriptive. (Makes it clear what it means at a glance)
Short. (less chance for typos, types faster and therefore easier to work with.)

In your case, the names only mean to affect the text size. So appropriate names would be something like:

text-medium {
    font-size: 1.4rem;
}
text-big {
    font-size: 1.6rem;
}

Since this makes it clear what they do.


How long a PID has been running (taking multiple inputs) Tonight I tried to write a POSIX shell script which would allow me to check how long a process is running (via it's ID) and be able to handle multiple inputs:

#!/bin/sh

is_this_a_number()
{
    if [ "$1" -eq "$1" ] 2> /dev/null
    then
        return 0
    else
        return 1
    fi
}

does_pid_exist()
{
    if ps --pid "$1" > /dev/null 2>&1
    then
        return 0
    else
        return 1
    fi
}

how_long_pid_has_been_running()
{
    ps --pid "$1" --format etime=
}

trim_leading_and_trailing_spaces()
{
    awk '{$1=$1;print}'
}

arg_pos=0

while [ "$#" -gt 0 ]
do

    arg_val="$1"
    arg_pos=$((arg_pos + 1))

    if ! is_this_a_number "$arg_val"
    then

        echo "The argument at position $arg_pos is not a number."

    else

        echo "Process ID $arg_val"

        if ! does_pid_exist "$arg_val"
        then

            echo "This process does not exist."

        else

            how_long_pid_has_been_running "$arg_val" | trim_leading_and_trailing_spaces

        fi

    fi

    printf "\\n"

    shift

done

Usage:

./how-long-pid-is-running 1 10078 c 40545454

Where 10078 is an instance of Chrome.

The output is as follows:

Process ID 1
09:04:31

Process ID 10078
07:32:32

The argument at position 3 is not a number.

Process ID 40545454
This process does not exist.

GNU long options are not POSIX, so instead of ps --pid "$1" --format etime=, write ps -p "$1" -o etime=.
kill -s 0 "$1" >/dev/null 2>&1 is much faster than ps --pid "$1" > /dev/null 2>&1, partly because kill is a builtin in most shells. Signal 0 is used to test for the existence of a process without sending it a signal.
Consider redirecting the echo messages to the appropriate output streams, i.e. stdout and stderr.
is_this_a_number, does_pid_exist: these are used directly in if clauses, so it'd read more naturally if they are renamed to is_a_number and pid_exists.
A minor point here, but I'd reverse the if-else statements to remove the ! negations.
Another minor point: during the short time span between testing the existence of a PID and querying the elapsed time since the process with that PID was started, the process with that PID could have finished running or even been substituted with a newly spawned one. You could avoid that by running the second command first, saving the output in a variable and then check for nullness, like thus:

echo "Process ID $arg_val"
running_time=$(how_long_pid_has_been_running "$arg_val" 2>/dev/null |
    trim_leading_and_trailing_spaces)
if [ -z "${running_time}" ]
then
    echo "This process does not exist."
else
    echo "${running_time}"
fi

I've seen you write printf "\\n" in another post also, so I'm curious: is it any different from printf "\n"? And in any case, an echo would do the job just as well.


Euler problem 3: largest prime factor of the number I have been trying to learn Python and just solved Euler Problem 3:

  The prime factors of 13195 are 5, 7, 13 and 29.

  What is the largest prime factor of the number 600851475143 ?

I am looking for any feedback on improving efficiency and the code itself.

My code:

from itertools import count, islice

def is_prime(number):
    for n in islice(count(2), int(number**0.5 -1)):
        if number % n == 0:
            return False
    return True

def find_factors(number):
    lst = []
    for n in islice(count(2), int(number/2-1)):
        if number % n == 0:
            if is_prime(n):
                lst.append(n)
    return lst

print(find_factors(13195).pop())

I used islice instead of xrange because of int limits, which I learned about from another question at StackOverflow.com

In code above, I have created two separate functions, one to find a prime and the other to find factors.

The program works this way,

It takes an input number
Find all the factors of that number from 2->number/2
If it finds a factor, it checks if its a prime, and append it to a list.
Eventually I print the last element of list since list in python are ordered.

When I look at your solution, the first impression I get is that it's too complicated. To solve the problem you don't need any fancy algorithms for prime numbers, or islice and count. It's a simple task of trying to divide by possible factors.

One important thing to note is that we only need to check for prime factors that are smaller than the square root of the number. Once we have reached the square root, there can be only one remaining factor, which can be proved by contradiction.

Another important thing to note is that once we have found a prime factor, we can divide the original number by that factor and reduce our problem size.

Doing both of these things, I end up with something like this:

def get_max_prime_factor(n):
    d = 2
    factors = []
    while d*d <= n:
        while n%d == 0:
            n //= d
            factors.append(d)
        d += 1
    if n != 1:
        factors.append(n)
    return max(factors)

Now, I'll say that even this implementation might be too complicated, since I'm saving all prime factors in a list instead of only keeping track of the largest one. If you want you could change that implementation and make it even more efficient, but to solve the problem above it's not necessary.


Regex version of strip() - Ch. 7 Automate the Boring Stuff Here is a practice exercise — Regex version of strip() \$-\$

  Write a function that takes a string and does the same thing as the
  strip() string method. If no other arguments are passed other than the
  string to strip, then whitespace characters will be removed from the
  beginning and end of the string. Otherwise, the characters specified
  in the second argument to the function will be removed from the
  string.

I have written the following code. Is there any better way to write it? Any feedback is highly appreciated.

import re

def regex_strip(s, chars = None):

    if chars == None:
        strip_left = re.compile(r'^\s*')
        strip_right = re.compile(r'\s*$')

        s = re.sub(strip_left, "", s)
        s = re.sub(strip_right, "", s)
    else:
        strip_left = re.compile(r'^[' + re.escape(chars) + r']*')
        strip_right = re.compile(r'[' + re.escape(chars) + r']*$')
        s = re.sub(strip_left, "", s)
        s = re.sub(strip_right, "", s)
    return s

Here is an example output -

s = '.*    alphabetatheta   *4453   +-'
print(regex_strip(s, '.+-*'))

>>>    alphabetatheta      *4453

If you call regex_strip(s, ""), you will get:

  re.error: unterminated character set at position 0

because neither ^[] nor []$ is a valid regular expression.  You could avoid this by using if not chars: instead of if chars == None:.

There is no need to re.compile() your regular expressions; you aren't saving the compiled patterns anywhere for re-use.

You can simplify your logic by using the reg-ex to capture the middle, non-stripped portion of the string, instead of doing two replacements for the start and end trim operations:

import re

def regex_strip(s, chars = None):

    if chars:
        trim = '[' + re.escape(chars) + ']*'
    else:
        trim = r'\s*'

    return re.fullmatch(f"{trim}(.*?){trim}", s).group(1)

I'm not sure the point of asking you to write your own strip() function is to delegate the task to the reg-ex engine.  It seems like going out and buying a sledge hammer when the problem is to build a nut cracker.


Triangle numbers finder in Scala I'm new to Scala and learn it by solving some Project Euler problems. Here is the solution to problem 42 from Project Euler.

The task is to read words from file, then sum ordinal numbers of each character (e.g. sky =19+11+25=55). One need to count all words for which this sum is a triangular number (satisfying \$\frac{n(n+1)}{2}\$ for some \$n\$).

import scala.io.Source
import java.nio.file.Files
import java.nio.file.Paths

def makeTriangleNumber(word:String): Int = {
    word.map(ordinals(_)).reduceLeft(_ + _)
}

val inputFile = Paths.get(args(0));
assert(Files.exists(inputFile))

val ordinals = ((('A' to 'Z').toList) zip Stream.from(1)).toMap
assume(ordinals('Z') == 26)

var triangleNumbers = scala.collection.SortedMap(1 -> 1)
val words = Source.fromFile(inputFile.toUri()).getLines.mkString.replace("\"", "").split(",").toList
var counter = 0

assert(makeTriangleNumber("SKY") == 55)

for (w <- words) {
    var candidate = makeTriangleNumber(w)
    if (!triangleNumbers.contains(candidate) && candidate > triangleNumbers.max._1) {

        var tmp = Stream.from(triangleNumbers.max._2 + 1)
          .map((x) => (x * (x + 1) / 2, x))
          .takeWhile(_._1 <= candidate)

        triangleNumbers = triangleNumbers ++ tmp.toMap
    }
    if (triangleNumbers.contains(candidate)) {
        counter += 1
    }
}
println("The answer is: " + counter)

The organization of the program is rather haphazard:

It starts with a word value calculation function
Then it checks for the existence of the input file
Then defines the ordinals map (which the first function depends on!)
Then it creates a triangleNumbers map
Then it reads the words from the input file
Then it counts the words that pass the triangularity test

Interspersed among that are three unit tests.

Surely these definitions could be grouped together more logically.

makeTriangleNumber is altogether wrongly named.  It actually has nothing to do wth triangular numbers.  Rather, it sums the values of the letters of a word.  I would call it wordValue instead.

Within the makeTriangleNumber function, .reduceLeft(_ + _) could just be written as .sum.

Personally, I would simplify the definition of ordinals by taking advantage of Seq.zipWithIndex, then compensate for the off-by-one difference by adding the length of the word.  (That may be a controversial move, since ordinals would no longer match the problem description.)

For extracting the words from the file, I suggest using Regex.findAllMatchIn(source:CharSequence), which gives you an Iterator of regular expression matches.

The heart of your program is the for loop.  It's not very good functional programming, though.  Ideally, the entire exercise should revolve around one line of code that looks like this:

words.count(word => isTriangularNumber(wordValue(word)))

How should isTriangularNumber be defined?  In your loop, you're discovering triangular numbers like this:

var tmp = Stream.from(triangleNumbers.max._2 + 1)
  .map((x) => (x * (x + 1) / 2, x))
  .takeWhile(_._1 <= candidate)

… using the triangleNumbers map for memoization.  I propose a more elegant definition — a lazy stream:

val triangularNumbers = Stream.from(1).scan(0)(_ + _)

Rather than the given formula, this definition relies on the recurrence principle that makes the numbers "triangular" in the first place.

Suggested solution

val wordValue = {
  val ordinals = ('A' to 'Z').zipWithIndex.toMap
  (word:String) => word.length + word.map(ordinals).sum
}

val isTriangularNumber = {
  val triangularNumbers = Stream.from(1).scan(0)(_ + _)
  (n:Int) => triangularNumbers.find(_ >= n).get == n
}

val inputFile = Source fromFile Paths.get(args(0)).toUri
val words = "\"([^\"]*)\"".r findAllMatchIn(inputFile.mkString) map(_.group(1))

val count = words.count(word => isTriangularNumber(wordValue(word)))
println("The answer is: " + count)


^P\d{5}-13-TP\d{2}-\d{3}(-(L|R|\d{4})(-\d{3,4})*)?$

Your current regex is WAY too forgiving.  First of all, every one of your example starts with a P followed by some numbers, but you accept ANY COMBINATION of letters at the beginning. I'm assuming that ALEX01867-13-TP02-019-L-1137 isn't a valid key, so you should take steps to reject it by using hungry quantifiers as little as possible (*, +). Using \d{3,4} matches a digit between 3 and 4 times, so that will
let you limit the sort of input you accept.

The same goes for the 5th group -  according to your examples, it's either L, R, or 4 digits. In Regex, that looks like this: (L|R|\d{4})

Next, you are using alternation (option1|option2) to capture the different "forms" your string comes in as, but you are repeating a bunch of stuff (for example, the \w*\d* at the beginning). You can limit the scope of the alternation by surrounding it in brackets (()). You can see this in action with the (L|R|\d{4}) example - that whole bracket group becomes a single token that matches somewhere in a string (or doesn't).

Sometimes the string ends after the 4th group (Before the L/R group), and sometimes it doesn't. Instead of using alternation to solve this, which makes the regex VERY long, you can just surround the entire regex AFTER that point in brackets with a question mark (an-(example)?). This makes the entire second part optional.

Finally, your problem asks if there is a simple method to improve the regex. By ending it in (-\d{3,4})* you can match ANY length of additions to the end, assuming the all come in the form -015 or -1992 or whatever. If you knew that there was always a max of 15 numbers added to the end, you could change that star (*) to a max quantifier ({,15}). If sometimes the number only has two digits, change the {3,4} to {2,4}, etc.

See it in action here


Project Euler #14 in Swift - Longest Collatz sequence I just finished Project Euler #14 in Swift, and since there is not any version yet on Code Review, I would like to have some comments on what I did to try to improve it.

  The following iterative sequence is defined for the set of positive
  integers:

  n → n/2 (n is even)

  n → 3n + 1 (n is odd)

  Using the rule above and starting with 13, we generate the following
  sequence:

  13 → 40 → 20 → 10 → 5 → 16 → 8 → 4 → 2 → 1

  It can be seen that this
  sequence (starting at 13 and finishing at 1) contains 10 terms.
  Although it has not been proved yet (Collatz Problem), it is thought
  that all starting numbers finish at 1.

  Which starting number, under one million, produces the longest chain?

  NOTE: Once the chain starts the terms are allowed to go above one
  million.

import Foundation

func collatzSequenceForNumber(var number:Int, inout steps:[Int]) -> Int {

    var numberOfSteps = 0
    var found = false
    let initialNumber = number

    while (found == false) {

        if number % 2 == 0 {
            number /= 2
        } else {
            number = (number * 3 + 1) / 2
            numberOfSteps++
        }

        found = number < steps.count ? steps[number] > 0 : false
        numberOfSteps++
    }

    numberOfSteps += steps[number]
    steps[initialNumber] = numberOfSteps

    return numberOfSteps
}

func longestCollatzSequence(maxNumber:Int) -> (Int, Int) {
    var collatzSequence = (number:0, steps:0)
    var steps:[Int] = [Int](count: maxNumber, repeatedValue: 0)
    steps[1] = 1

    for var number = 2; number < maxNumber; number++ {

        let steps = collatzSequenceForNumber(number, &steps)
        if steps > collatzSequence.steps {
            collatzSequence = (number, steps)
        }
    }

    return collatzSequence
}

func euler14() {

    let result:(number:Int, steps:Int) = longestCollatzSequence(999_999)

    println(result.number)
}

func printTimeElapsedWhenRunningCode(operation:() -> ()) {
    let startTime = CFAbsoluteTimeGetCurrent()
    operation()
    let timeElapsed = CFAbsoluteTimeGetCurrent() - startTime
    println("Time elapsed : \(timeElapsed) s")
}

printTimeElapsedWhenRunningCode(euler14)

The code executes in 0.07s.

collatzSequenceForNumber() returns the length of the Collatz sequence and not a sequence itself, so a better name might be collatzLength():

func collatzLength(var number:Int, inout steps:[Int]) -> Int {
    var numberOfSteps = 0
    let initialNumber = number
    // ...
}

The steps
parameter is an array used as cache. But in longestCollatzSequence(), the same name steps is used for both the caching array and the computed number of steps.
For more consistence, I would suggest to use steps for the number of steps
only and a different name cache for the caching array:

func collatzLength(var number:Int, inout cache:[Int]) -> Int {
    var steps = 0
    let initialNumber = number
    // ...
    return steps
}

The number parameter is declared as a variable
and modified in the function. That is fine generally, but here you need the original value of the parameter at the end of the function. It might be
clearer to treat the  parameter as constant and modify a local copy:

func collatzLength(number:Int, inout cache:[Int]) -> Int {
    var steps = 0
    var n = number
    // ...
    cache[number] = steps
    return steps
}

The expression

found = number < steps.count ? steps[number] > 0 : false

can simpler be written as

found = number < steps.count && steps[number] > 0

and I prefer if (!found) to if (found == false). But you don't need
the found variable at all, as this condition can be put directly into
the while() expression.

So now we have

func collatzLength(number:Int, inout cache:[Int]) -> Int {
    var steps = 0
    var n = number

    while n >= cache.count || cache[n] == 0 {
        if n % 2 == 0 {
            n /= 2
        } else {
            n = (n * 3 + 1) / 2
            steps++
        }
        steps++
    }

    steps += cache[n]
    cache[number] = steps
    return steps
}

Interestingly, this is a tiny bit faster than your original version
(0.027 s instead of 0.032).

You can improve the performance a bit by taking advantage of the fact that
the caching array is filled sequentially. So (in the context of this program)

    while n >= cache.count || cache[n] == 0 {

can be replaced by

   while n >= number {

and this reduces the time to 0.016 s.

In longestCollatzSequence() you are using a tuple with named components
(number:0, steps:0) internally, but return an unnamed tuple (Int, Int).
In the calling function euler14() the return value is assigned to
a named tuple again. I would recommend to stick to one variant, e.g.
the named tuple:

func longestCollatzSequence(maxNumber:Int) -> (number: Int, steps: Int) {
    var collatzSequence = (number:0, steps:0)
    var cache:[Int] = [Int](count: maxNumber, repeatedValue: 0)
    cache[1] = 1

    for var number = 2; number < maxNumber; number++ {
        let steps = collatzLength(number, &cache)
        if steps > collatzSequence.steps {
            collatzSequence = (number, steps)
        }
    }

    return collatzSequence
}

The for loop can equivalently  be written using a Swift range:

for number in 2 ..< maxNumber { ... }

The main function can now be simplified to

func euler14() {
    let result = longestCollatzSequence(999_999)
    println(result.number)
}

or alternatively

func euler14() {
    let (number, steps) = longestCollatzSequence(999_999)
    println(number)
}

Finally note that your longestCollatzSequence() function considers
all number below the given parameter, so you should either call

longestCollatzSequence(1_000_000)

or change the function to include the given upper bound.


Computing the average of several numbers I am learning C++. This is a simple program I had to write for class. Is there anything I should fix?

#include <iostream>
using namespace std;

int main()
{
    const int ListSize = 5;
    int ValuesProcessed = 0;
    float ValueSum = 0;

    cout << "Please enter " << ListSize << " numbers" << endl;

    while(ValuesProcessed < ListSize){
        float Value;
        cin >> Value;
        ValueSum += Value;
        ++ValuesProcessed;
    }
    float Average = ValueSum / ValuesProcessed;
    cout << "Average: " << Average << endl;

    return 0;
}

This is quite good for a homework exercise.

I would recommend a for loop instead of the while:

for (int ValuesProcessed = 0; ValuesProcessed < ListSize; ++ValuesProcessed) {

This puts all the logic of iteration together where you are less likely to overlook
some detail, unlike the while loop where the initial value of ValuesProcessed,
the test for termination of the loop, and setting ValuesProcessed for the next
iteration are done in three different places.

I tend to follow the philosophy of declaring things as close to where they are used
as possible, so I would move the declaration and initialization of ValueSum after
the "Please enter" printout. But I don't think there's a strong argument for doing this.
(It would merely swap the order of two lines of code, not a big deal.)

Contrary to another answer, I like your decision to declare ListSize in your main()
function. (It's another example of "declaring things as close to where they are used
as possible".) Although ListSize is a "constant" in the sense that you have declared it
const (which I think is a good idea), it is really just the end condition of a
single loop in the program, or to put it another way, the length of a list of inputs
that the program is about to read.