Can this reactive code be improved? I have a view model that I'm migrating to RxUI. It exposes a collection of brokers along with a bool indicating whether there is a market (ie. at least one broker). In addition, it has a property for the currently selected broker (the user can choose a broker via a drop-down):

private IReactiveCollection<SourceViewModel> brokers;
public IReactiveCollection<SourceViewModel> Brokers
{
    get { return this.brokers; }
    private set { this.RaiseAndSetIfChanged(x => x.Brokers, ref this.brokers, value); }
}

private ObservableAsPropertyHelper<bool> isMarket;
public bool IsMarket
{
    get { return this.isMarket.Value; }
}

private SourceViewModel selectedBroker;
public SourceViewModel SelectedBroker
{
    get { return this.selectedBroker; }
    set { this.RaiseAndSetIfChanged(x => x.SelectedBroker, ref this.selectedBroker, value); }
}

I have these pieces hooked up and working, but I am not convinced I've done this in the most reactive-friendly fashion:

this.WhenAny(x => x.Volatilities, x => x.Value)
    .StartWith((ItemObservableCollection<VolatilityViewModel>)null)
    .Subscribe(x =>
        {
            if (x == null)
            {
                // no volatilities, so definitely no market
                this.Brokers = null;
                this.SelectedBroker = null;
                this.isMarket = Observable.Return(false).ToProperty(this, y => y.IsMarket);
            }
            else
            {
                // we have volatilities, so potentially we have a market

                // TODO: is there a better means of doing this?
                this.Brokers = x.CreateDerivedCollection(y => y.Source);

                // TODO: is there a cleaner way to do this?
                this.Brokers.Changed
                    .Select(y => true)
                    .StartWith(true)
                    .Subscribe(y =>
                        {
                            var volatility = x.FirstOrDefault();
                            this.SelectedBroker = volatility == null ? null : volatility.Source;
                        });

                // TODO: is there a cleaner way to do this?
                this.isMarket = this.Brokers.CollectionCountChanged.Select(y => y > 0).StartWith(x.Count > 0).ToProperty(this, y => y.IsMarket);
            }
        });

My Volatilities property is set when the user interacts with a specific set of vols. Everything hinges off of it. It's currently an ItemObservableCollection, but I may change it later to be a reactive collection. I don't believe that's relevant here, but I could be wrong.

The parts I'm concerned about have TODOs in the code, but to summarize:

when my Volatilities property changes, so too do the available brokers. I currently create a new derived collection each time Volatilities changes. I couldn't come up with any way to avoid this or implement it more cleanly.
when I recreate my brokers collection, I immediately set up a Changed observer so that any changes result in the first broker being selected. The reasoning is that the first broker will (once I do sorting) be the preferred broker, so any changes in the market should instigate the selection of the preferred broker. I couldn't think of a way to make this logic cleaner and simpler.
in addition, I also recreate my isMarket member, which is true whenever there is at least one broker in the brokers collection. Again, I'm not sure that this is the best approach to achieving this.

Can anyone weigh in with their thoughts on this?

I currently create a new derived collection each time Volatilities changes

This was a good idea, but in this case I'd just recreate the brokers list (since it's not a straight volatilities.Select(x => ...), it's a SelectMany. One thing that's important though, is whether you mutate (i.e. add and remove) items from Volatilities, or you only replace the list (i.e. via an API call, set this.Volatilities to a new collection).

Pick one or the other, don't mix them or else your code will be complicated. Let's assume you're using ReactiveCollection and you're adding/removing to Volatilities.

Volatilities.CollectionCountChanged.StartWith(Volatilities.Count)
    .Select(_ => Volatilities.SelectMany(x => x.BrokerList))
    .ToProperty(this, x => x.Brokers);

Then, we can watch when someone changes Brokers - we'll do this in the constructor (along with everything else). Generally, if you're not calling RxUI methods in the constructor, you're Doing Something Wrong. You're wiring up on startup what will happen when various parts of your ViewModel change, those wires typically are constant:

this.WhenAny(x => x.Brokers, x => x.Value)
    .Subscribe(_ => SelectedBroker = Brokers.FirstOrDefault());

Now let's set up the IsMarket:

this.WhenAny(x => x.Brokers, x => x.Value)
    .Select(x => x.Count > 0)
    .ToProperty(this, x => x.IsMarket);

What We Can Learn™

The key point here though, is that IsMarket is directly related to Brokers and nothing else - we should describe that relationship without mentioning Volatilities, then separately describe the relationship between Volatilities and Brokers. We don't want to mix the two.


Program to find the mean, median, and mode of numbers I was attempting to build a program using Ruby that asks the user to input at least three numbers, and it returns the mean, median, and mode of those numbers.

A developer friend of mine glanced over it and said that the program was wrong, but didn't state specifically what was wrong with it. I have looked over it and tested it repeatedly and can't figure out what's wrong with it.

puts "Please input three or more numbers with spaces inbetween them:"
numbers = gets.chomp
numbers = numbers.split(" ").map(&:to_i)
length = numbers.length
y = 0.000
numbers.each do |x|
    y = x + y
end
mean = y / length
print "Mean: #{mean}"
print "\n"
a_order = numbers.sort
length1 = (length - 1) / 2
if length%2 == 1
    median = a_order[length1]
else
    length2 = length1 + 1
    median = (a_order[length1] + a_order[length2]) / 2.000
end
print "Median: #{median}"
print "\n"
frequencies = Hash.new(0)
numbers.each do |number|
    frequencies[number] = frequencies[number] + 1
end
frequencies = frequencies.sort_by { |a, b| b }
frequencies.reverse!
frequencies = frequencies.to_a
if frequencies[0][1] == frequencies[1][1]
    print "Mode: invalid"
else
    print "Mode: #{frequencies[0][0]}"
end

It looks valid to me. Except you are not enforcing the '3 number minimum'.

You can use x.odd? to check odd or even.
You can total numbers with [1,2,3].inject(&:+).
The frequencies.to_a is not needed, as it's already an array after the sort_by.
You can sort_by -b so you don't have to reverse.

These are all just refactorings. No change in behaviour.


Count number of occurences of a digit in the range Left limit, right limit and the digit to be counted is entered. This code works only for whole numbers i. e. from 0 to n. This code returns the number of occurence of the digit n in the given range. Please review this code and suggest improvements.

#include <iostream>

int count_n(int l, int r, int n)
{
    if (r <= l)
    {
        std::cerr << "Right limit must greater then left limit\n";
        return 0;
    }

    if (l < 0)
    {
        std::cerr << "Only positive integers\n";
        return 0;
    }

    int count = 0;
    for (int i = l; i <= r; ++i)
    {
        int i_copy = i;
        while (i_copy != 0)
        {
            if (i_copy % 10  == n)
            {
                count++;
            }
            i_copy = i_copy / 10;
        }
    }
    return count;
}

int main()
{
    int left_limit, right_limit, num;
    std::cout << "Enter left limit, right limit and the number(Only positive numbers)\n";
    std::cin >> left_limit >> right_limit >> num;

    int result = count_n(left_limit, right_limit, num);
    std::cout << result << "\n";
}

Output:

Enter left limit, right limit and the number(Only for positive numbers)
1 20 0
2

In addition to this, you should specify validation for n ( digit ).
Also, it may be better to group validations into separate function.

void check_input(int min_left, int max_right, int digit) {
  if (max_right <= min_left)
    throw std::invalid_argument("right is <= left, expected right > left");
  if (min_left < 0)
    throw std::invalid_argument("left < 0, expected left > 0");
  if (digit <= 0)
    throw std::invalid_argument("digit <= 0");
    //throw std::invalid_argument("number <= 0");
  if (digit > 9)
    throw std::invalid_argument("digit > 9");

  ////number:
  //if (digit > max_right)
  //  throw std::invalid_argument("number > right limit");
}

int count_digit_entries(int min_left, int max_right, int digit) {
//...
}

int main() {
  int left_limit, right_limit, num;
  std::cout << "Enter left limit, right limit and the number(Only positive numbers)\n";
  std::cin >> left_limit >> right_limit >> num;

  check_input(left_limit, right_limit, num);

  int result = count_digit_entries(left_limit, right_limit, num);

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

You may want to find not only digits but a whole numbers.

//...

#define DEBUG
#ifdef  DEBUG
#define debug(x) x
#else
#define debug(x)
#endif

int count_digit_entries(int min_left, int max_right, int search_number) {
  int count = 0;

  int mod = 10;
  while(search_number / mod > 0) mod = mod * 10; // 279 -> 1000

  for (int current_number = min_left, right = max_right; current_number < right; ++current_number) {
    int copied_current_number = current_number; // 542793

    while (copied_current_number >= search_number) {
        // 542793 - 279 % 1000    >0
        // 54279 - 279 % 1000     =0

      if ( ( (copied_current_number - search_number) % mod ) == 0 ) {
        debug(std::cout << current_number << " ");
        ++count;
      };
      copied_current_number = copied_current_number / 10;
      // 542793 -> 54279

    };
  };

  return count;
};

Output:

Enter left limit, right limit and the number(Only positive numbers)
0 1000 25
25 125 225 250 251 252 253 254 255 256 257 258 259 325 425 525 625 725 825 925
Total: 20


Dim Hero & Add Background to Nav on scroll I have followed a tutorial that initially was for adding a 'sticky' nav. I adapted this slightly so it adds a background to the Nav when the page scroll reaches the top-third point of the hero area.

I then added a similar function that dims the hero area once the scroll gets to the midway point of the hero.

Both functions work in terms of displaying the expected results, but I am questioning the need for two almost identical functions and event listeners.

Could these be combined into one, complete function?

const hero = document.querySelector("#heroArea");
// Finds the top third of the element by adding the top of the element to the height of the element then divide by 3
const bottomOfNav = hero.offsetHeight / 3;
const middleHero = hero.offsetHeight / 2;

function fixNav() {
  // If the window position is greater or equal to the bottom of the nav
  if (window.scrollY >= bottomOfNav) {
    // Adds a class to the body tag
    document.body.classList.add("fixed-nav");
  } else {
    document.body.classList.remove("fixed-nav");
  }
}

window.addEventListener("scroll", fixNav);

function dimHero() {
  if (window.scrollY >= middleHero) {
    document.body.classList.add("dim-hero");
  } else {
    document.body.classList.remove("dim-hero");
  }
}

window.addEventListener("scroll", dimHero);
* {
  padding: 0;
  margin: 0;
  font-family: sans-serif;
}

nav {
  position: fixed;
  width: 100%;
  left: 0;
  right: 0;
  color: #fff;
  padding: 15px;
  text-align: center;
  text-transform: uppercase;
  font-family: sans-serif;
  transition: .3s;
}

.fixed-nav nav {
  background: #333;
}

.hero {
  height: 100vh;
  background: black;
  opacity: 1;
  transition: .3s;
}

.dim-hero .hero {
  opacity: 0;
}

.text-block {
  height: 50vh;
  background: #555;
  color: #fff;
  font-size: 30px;
  text-align: center;
  padding: 40px;
}

.text-block-two {
  background: #333;
}

  Navigation

Lorem ipsum, dolor sit amet consectetur adipisicing elit. Eos ipsum et, omnis sit vero ab doloremque quia dolores mollitia. Doloremque maxime dolores quo eius ea. Ad, reiciendis minus. Dolorum, hic.

Lorem ipsum, dolor sit amet consectetur adipisicing elit. Eos ipsum et, omnis sit vero ab doloremque quia dolores mollitia. Doloremque maxime dolores quo eius ea. Ad, reiciendis minus. Dolorum, hic.

Bearing in mind that you already were able to abstract the functionality of toggling the class name into a separate function, I see other changes that can be made to clean up the code:

Toggle Method: Use the toggle() method of classList to shorten the function changeClass()

function changeClass(offset, clsName) {
    document.body.classList.toggle(clsName, window.scrollY >= offset);
}

DOM access method: Use getElementById() to select the element with Id heroArea instead of querySelector(). While it may likely never be noticeable on a sample page this small, it generally works faster. Check out this Sitepoint forum
and this SO question and its answers (and related posts).
Combined event handlers: The scroll event handlers can be combined to a single function that calls changeClass() twice.

window.addEventListener("scroll", function() {
    changeClass(bottomOfNav, 'fixed-nav');
    changeClass(middleHero, 'dim-hero');
});

Arrow functions: Because ecmascript-6 features like const are used, others like arrow functions could be used for the functions- e.g.

const changeClass = (offset, clsName) =>  document.body.classList.toggle(clsName, window.scrollY >= offset);

Though some would argue that is too long for a single line, so brackets can be used:

const changeClass = (offset, clsName) =>  {
    document.body.classList.toggle(clsName, window.scrollY >= offset);
};

CSS: combine selectors for common styles: I noticed there are two selectors that both have the same background style - those can be combined to a single ruleset:

.fixed-nav nav,
.text-block-two {
  background: #333;
}

The changes have been applied to the sample code below.

const hero = document.getElementById("heroArea");
// Finds the top third of the element by adding the top of the element to the height of the element then divide by 3
const bottomOfNav = hero.offsetHeight / 3;
const middleHero = hero.offsetHeight / 2;
const changeClass = (offset, clsName) =>  document.body.classList.toggle(clsName, window.scrollY >= offset);

window.addEventListener("scroll", function() {
  changeClass(bottomOfNav, 'fixed-nav');
  changeClass(middleHero, 'dim-hero');
});
* {
  padding: 0;
  margin: 0;
  font-family: sans-serif;
}

nav {
  position: fixed;
  width: 100%;
  left: 0;
  right: 0;
  color: #fff;
  padding: 15px;
  text-align: center;
  text-transform: uppercase;
  font-family: sans-serif;
  transition: .3s;
}

.fixed-nav nav,
.text-block-two {
  background: #333;
}

.hero {
  height: 100vh;
  background: black;
  opacity: 1;
  transition: .3s;
}

.dim-hero .hero {
  opacity: 0;
}

.text-block {
  height: 50vh;
  background: #555;
  color: #fff;
  font-size: 30px;
  text-align: center;
  padding: 40px;
}

  Navigation

Lorem ipsum, dolor sit amet consectetur adipisicing elit. Eos ipsum et, omnis sit vero ab doloremque quia dolores mollitia. Doloremque maxime dolores quo eius ea. Ad, reiciendis minus. Dolorum, hic.

Lorem ipsum, dolor sit amet consectetur adipisicing elit. Eos ipsum et, omnis sit vero ab doloremque quia dolores mollitia. Doloremque maxime dolores quo eius ea. Ad, reiciendis minus. Dolorum, hic.


Simplified LogService (from concurrency in practice) with shutdown feature Brian Goetz provided following code of LogService with shutdown feature:

public class LogService {
    private final BlockingQueue<String> queue;
    private final LoggerThread loggerThread;
    private final PrintWriter writer;
    @GuardedBy("this") private boolean isShutdown;
    @GuardedBy("this") private int reservations;

    public LogService(Writer writer) {
        this.queue = new LinkedBlockingQueue<String>();
        this.loggerThread = new LoggerThread();
        this.writer = new PrintWriter(writer);
    }

    public void start() {
        loggerThread.start();
    }

    public void stop() {
        synchronized (this) {
            isShutdown = true;
        }
        loggerThread.interrupt();
    }

    public void log(String msg) throws InterruptedException {
        synchronized (this) {
            if (isShutdown)
                throw new IllegalStateException(/*...*/);
            ++reservations;
        }
        queue.put(msg);
    }

    private class LoggerThread extends Thread {
        public void run() {
            try {
                while (true) {
                    try {
                        synchronized (LogService.this) {
                            if (isShutdown && reservations == 0)
                                break;
                        }
                        String msg = queue.take();
                        synchronized (LogService.this) {
                            --reservations;
                        }
                        writer.println(msg);
                    } catch (InterruptedException e) { /* retry */
                    }
                }
            } finally {
                writer.close();
            }
        }
    }
}

I think that I have simpler variant(I don't understand why we need reservations counter), please review its safety:

public class LogService {
    private final BlockingQueue<String> queue;
    private final LoggerThread loggerThread;
    private final PrintWriter writer;
    @GuardedBy("this")
    private boolean isShutdown;

    public LogService(Writer writer) {
        this.queue = new LinkedBlockingQueue<String>();
        this.loggerThread = new LoggerThread();
        this.writer = new PrintWriter(writer);
    }

    public void start() {
        loggerThread.start();
    }

    public void stop() {
        synchronized (this) {
            isShutdown = true;
        }
        loggerThread.interrupt();
    }

    public void log(String msg) throws InterruptedException {
        synchronized (this) {
            if (isShutdown)
                throw new IllegalStateException(/*...*/);
            queue.put(msg); // inside synchronized now
        }

    }

    private class LoggerThread extends Thread {
        public void run() {
            try {
                while (true) {
                    try {
                        synchronized (LogService.this) {
                            if (isShutdown && queue.isEmpty())
                                break;
                        }
                        String msg = queue.take();
                        writer.println(msg);
                    } catch (InterruptedException e) { /* retry */
                    }
                }
            } finally {
                writer.close();
            }
        }
    }
}

Concurrency issue

I think one critical part is found here:

synchronized (LogService.this) {
    if (isShutdown && queue.isEmpty()) break;
}
--> ... <--
String msg = queue.take();
--> ... <--
writer.println(msg);

You query isShutdown and queue.isEmpty() but you cannot ensure this state anymore if you leave the monitor and access the queue afterwards.

After that you also cannot ensure the output order (println) that is specified by the queue.

Both should be done in the monitor.

GuardedBy obsolete

Furthermore I do not think that the GuardedBy annoptation is neccessary as isShutdown is already only requested and modified within the monitor (synchronized(this)).

@GuardedBy("this")
private boolean isShutdown;

Writer

I do not know if it is intended to close the PrinterWriter after the queue is empty. This will cause a problem when restarting the LogThread. Closed PrintWriters may not work anymore which depends on the underlying Writer that interpretes the "close()"-operation:

PrintWriter pw = new PrintWriter(new FileOutputStream("hello.txt"));
pw.close();
pw.println("Hallo"); // will produce no output in file

try with resources

If you want to close resources properly you should use following construct:

try (PrintWriter writer = new PrintWriter(...)) {
    ...
} catch (...) {
    ...
}

Another point is that you should have a symmetry in creating and closing a resource. Your LogService is now responsible to close the Writer but it doesn't create it. Either you provide a factory for the resource so the LogService is responsible for creating and closing it OR the user of the LogService that passes in the Writer should be aware of creation and closing. Nevertheless the maximum proximity for those operations is the goal.

State handling

Currently you micro manage the state handling (running/stopped). You should consider implementing a real state pattern as a great concept to make clear when which method is allowed to be called.

You have some semantic things I find confusing. Logging is allowed only when the service is not stopped but you may always start and stop the service.

Thread interrupt

Your thread should not be stopped through the interrupt method. Try to reformulate your communication with the thread to stop without using such harsh mechanism that throws an exception.

Restarting

You do allow starting the loggerThread although this will cause an exception. I would expect either NOOP or a domain specific exception as the caller is not interested in a thread specific exception. Should the caller even know that a thead is working? Another possibility is to start over with a new thread. You already handle state so the obvious functionality for calling the start()-method on a stopped LogService is to start it again.

Personal opinion

As this is I would trust neither one nor the other implementation as they are riddled with semantic ambiguities, concurrency issues and micro management of state. I suggest to rethink the concept with real state handling and a proper monitor definition.

Code

State

The state pattern will cover three methods:

private interface State {
    void start();
    void log(String message);
    void stop();
}

LogService

The LogService will delegate all public calls to the state pattern. It provides a queue that will buffer log messages.

Initially the LogService is in the state "StateStopped".

You see that we changed from a blocking queue to a normal queue that is not thread-safe. But that is ok as the LogService serves as a monitor around it beside the other two variables loggerThread and state.

LogTarget may be access concurrently but this is not our responsibility AND we can only ensure the correct implementation by convention.

public class LogService implements LogTarget, LogSource {

    // states
    ...

    private State state;

    private final Queue<String> messageQueue;
    private LogProcessorThread loggerThread;
    private final LogTarget logTarget;

    public LogService(LogTarget logTarget) {
        this.messageQueue = new ArrayDeque<String>();
        this.logTarget = logTarget;
        this.state = new StateStopped();
    }

    public void start() {
        this.state.start();
    }

    public void stop() {
        this.state.stop();
    }

    public void log(String message) {
        this.state.log(message);
    }

    public String getNextQueuedMessage() {
        synchronized (this) {
            return this.messageQueue.poll();
        }
    }

}

StateStopped

As the state pattern has a context object (here LogService) this context object will be used as the monitor (synchronized(LogService.this)).

My implementation accepts log messages put it will not notify any consumer.

Calling stop() will be a NOOP. If your flavour is to throw an exception do that.

Calling start() will instantiate a new thread and start it. After that the internal state is changed to be "StateRunning".

private class StateStopped implements State {

    @Override
    public void start() {
        synchronized (LogService.this) {
            loggerThread = new LogProcessorThread(LogService.this, logTarget);
            loggerThread.start();
            state = new StateRunning();
        }
    }

    @Override
    public void log(String message) {
        synchronized (this) {
            messageQueue.add(message);
        }
    }

    @Override
    public void stop() {
        // ignore or throw IllegalStateException... your choice
    }

}

StateRunning

As the state pattern has a context object (here LogService) this context object will be used as the monitor (synchronized(LogService.this)).

Calling start() will lead to a NOOP. Here you have the choice to throw an exception as well.

Calling log() will accept new log messages but as the service knows that a thread is available (StateRunning) that may wait for new messages it is notified.

Calling stop() will safely shutdown the thread by communicating a stop request.

We will change the internal state of the LogService within the monitor and waiting until the thread terminates outside the monitor.

private class StateRunning implements State {

    @Override
    public void start() {
        // ignore or throw IllegalStateException... your choice
    }

    @Override
    public void log(String message) {
        synchronized (this) {
            messageQueue.add(message);
            loggerThread.awakeOnWait(); // this is more a callback or an observer pattern
        }
    }

    @Override
    public void stop() {
        Thread lastBreath = null;
        synchronized (LogService.this) {
            lastBreath = loggerThread;
            loggerThread.requestStop();
            loggerThread = null;
            state = new StateStopped();
        }
        try {
            lastBreath.join();
        } catch (InterruptedException e) {
        }
    }

}

LogProcessorThread

This thread while refactoring seems to become more and more an artefact that will ask for log messages from somewhere and passes these log message to somewhere.

Here again: The thread itself is the monitor over "shouldStop" and the critical operations source.getNextQueuedMessage() and target.log(message) that should not be intercepted.

public class LogProcessorThread extends Thread {

    private LogTarget target;
    private LogSource source;
    private boolean shouldStop = false;

    public LogProcessorThread(LogSource source, LogTarget target) {
        this.target = target;
        this.source = source;
    }

    public void run() {
        boolean expectedMoreMessages = true;
        while (expectedMoreMessages) {
            expectedMoreMessages = logNextMessage();
        }
    }

    public synchronized void requestStop() {
        this.shouldStop = true;
        this.notify();
    }

    public synchronized void awakeOnWait() {
        this.notify();
    }

    public synchronized boolean logNextMessage() {

        boolean shouldStop = this.shouldStop; // keep the state stable in this method invocation

        String message = source.getNextQueuedMessage();
        if (message != null) {
            target.log(message);
        }
        if (message == null && !shouldStop) {
            try {
                this.wait();
            } catch (InterruptedException e) {
                throw new RuntimeException("unexpected", e);
            }
        }
        boolean expectMoreMessages = message != null || !shouldStop;
        return expectMoreMessages;
    }

}

LogSource, LogTarget

These are not neccessary but they show the inherent abstraction the thread makes use of. So A LogService is of course a LogSource but it can also be a LogTarget.

public interface LogSource {
    String getNextQueuedMessage();
}

public interface LogTarget {
    public void log(String message);
}


Slowly working word counter I have a problem where, given N terms, the task is to find the k most frequent terms from given N terms. My code is correct, but I have a time limit 10 seconds and my code doesn't pass.

Constraints:

\$0 < N < 300000\$
\$0 < \textrm{term length} < 25\$

N = gets.to_i

if N < 300000 and N
  a=[]

  (0...N).each do |i|
    el = gets
    el_size = el.length
    if el_size < 25 and el_size
      a << el
    end
    i+=1
  end

  k = gets.to_i

  new_array = {}
  size = a.length
  count = 0

  (0..size).each do |i|
    (0..size).each do |j| count += 1 if a[i] == a[j] end
    new_array[a[i]] = count
    count = 0
  end

  s_l = new_array.to_a.sort{|a, b| (b.last > a.last) ? 1 : (b.last < a.last ? -1 : (a.first <=> b.first)) }

  i = 0
  for key, val in s_l
    puts key if i < k
    i += 1
  end
end

And how can I print only key of hash? Currently I use for key, val in hash.

The solution is slow because you aren't using data structures effectively.  In particular, the nested (0..size).each loops will cause it to take \$O(N^2)\$ time, which will be very problematic for large \$N\$.

You created new_array (which is actually a hash, not an array as the name suggests), but populate it the hard way.  The easy way is to loop once through all of the words; for each word you encounter, increment its count by 1.  There is no need to construct the intermediate array a.

# Read N, then count occurrences of the next N lines of text
word_counter = Hash.new { |count, word| count[word] = 0 }
n_lines = gets.to_i
n_lines.times { word_counter[gets] += 1 }

# Read k; limit it to the number of distinct words
k = gets.to_i
k = [k, word_counter.keys.length].min

# Sort words by descending frequency, breaking ties by alphebetical order
words_desc_freq = word_counter.sort do |a, b|
  cmp = a.last - b.last                    # .last is the count
  cmp != 0 ? -cmp : (a.first <=> b.first)  # .first is the word
end

# Output k most frequent words
(0...k).each do |i|
  puts words_desc_freq[i].first
end

The solution above has complexity \$O(N \log N)\$, dominated by the time it takes to sort.

Usually, when constraints are given for problems like this, it means that your program is not expected to handle input exceeding the stated limits gracefully.  It does not mean that your program needs to actively reject input that exceeds those limits.  In any case, your "validation" isn't quite right: in Ruby, 0 is treated as a true value.

Your code is formatted reasonably well.  I would just note that the line that performs the sort is long and should be broken up.  One-line blocks are typically written using braces, like (0..size).each { |j| count += 1 if a[i] == a[j] }


Redundant email checking in PHP This essentially checks if any of these if statements are true, then echos a statement.

Unfortunately, wrapping the error message with a div tag is impossible since I set error to 1 after I would have the div tag show up. I assume I could use a function for this but I don't understand how they work.

Is there a better way to check all these parameters then return a string that I can check if something exists in that string or not to post?

if ($error == '1'){ echo '
';}
if(strlen($display)<4)
{
echo "Display name is too short.
";
$error = '1';
}
if ($email == '')
{
echo "You did not put a valid email adress. We require this to verify your account.
";
$error = '1';
}
if(mysql_num_rows($check_display) != 0)
{
echo "Display name is already in use.
";
$error = '1';
}

if(mysql_num_rows($check_user) != 0)
{
echo "Username is already in use.
";
$error = '1';
}

if(mysql_num_rows($check_mail) != 0)
{
echo "Email is already in use.
";
$error = '1';
}

if ($pass != $pass2)
{
echo 'Passwords do not match.
' ;
$error = '1';
}
if ($error == '1'){ echo '
';}

Like so:
Here you can see that is easier to have errors as an array, use alternative control statments to separate php from html:

 <?php
    $messages = array();
    if(strlen($display)<4)
    {
        $messages[] = 'Display name is too short';
    }
    if (empty($email))
    {
        $messages[] ='You did not put a valid email address. We require this to verify your account.';
    }
    if(mysql_num_rows($check_display) != 0)
    {
        $messages[] = 'Display name is already in use';
    }

    if(mysql_num_rows($check_user) != 0)
    {
        $messages[] = 'Username is already in use.';
    }

    if(mysql_num_rows($check_mail) != 0)
    {
        $messages[] = 'Email is already in use.';
    }

    if ($pass != $pass2)
    {
        echo 'Passwords do not match.';
    }
    ?>

        <?php if(!empty($messages):?>

            <?php foreach($messages as $message):?>

<?php echo $message;?>

            <?php endforeach;?>

        <?php endif;?>


Brainfuck compiler with tcc backend I wrote simple compiler for Brainfuck with tcc backend (dont ask for assembler/assembly code generator!). It's output runs incredibly slow. Even Hello World program takes a LOOONG while to execute. I have added some optimizations, but it didnt help. This is code i actually made; It actually doesn't hang, but output work VERY slow (i need to wait ~4 minutes to Hello World display...):

#include <string.h>
#include <stdlib.h>
#include <stdio.h>
#include "libtcc.h"

int minus(FILE* f){
    int c;
    int amount = 0;
    while ((c = getc(f)) == '-')
        amount++;
    ungetc(c, f);
    return amount;
}

int plus(FILE* f){
    int c;
    int amount = 0;
    while ((c = getc(f)) == '+')
        amount++;
    ungetc(c, f);
    return amount;
}

int main(int argc, char** argv){
    int pos = 0;
    int brackets = 0;
    if(argc!=3){
        printf("Brainfuck Compiler by Joel.\n");
        printf("Cannot open input file.");
        exit(0);
    }
    int amount = 1;
    FILE* f = fopen(argv[1], "r");
    printf("Brainfuck Compiler by Joel. All rights reserved.\n");
    if(f!=NULL){
        char* outbuffer = (char*)malloc(1024*1024);
        strcpy(outbuffer, "int getch(void);\nvoid putch(int);\nint mem[65537];\nint main(){\n");
        while(!feof(f)){
            char c = fgetc(f);
            if(c == '<'){
                if(pos==0)pos=65536;
                else pos--;
            }
            if(c == '>'){
                if(pos==65536)pos=0;
                else pos++;
            }
            if(c == '['){
                brackets++;
                sprintf(outbuffer, "%swhile(mem[%d]){",outbuffer,pos);
            }
            if(c == ']'){
                brackets--;
                sprintf(outbuffer, "%s}", outbuffer);
            }
            if(c == '+'){
                amount = plus(f)+1;
                sprintf(outbuffer, "%smem[%d]+=%d;",outbuffer, pos, amount);
            }
            if(c == '-'){
                amount = minus(f)+1;
                sprintf(outbuffer, "%smem[%d]-=%d;",outbuffer, pos, amount);
            }
            if(c == '.'){
                sprintf(outbuffer, "%sputch(mem[%d]);",outbuffer, pos);
            }
            if(c == '.'){
                sprintf(outbuffer, "%smem[%d]=getch();",outbuffer, pos);
            }
        }
        if(brackets == 0){
            sprintf(outbuffer, "%sreturn 0;\n}\n",outbuffer);
            printf("Translation Succesful. ");
            compile(argv[2], outbuffer);
            free(outbuffer);
        }
        else{
            printf("Comilation fault. Unbalanced brackets.");
            free(outbuffer);
        }
    }
    else{
        printf("Cannot open one of specified files.");
    }
}

I don't think it's optimal code, and i need to "optimize" output somewhat. I have generated assembly listing here (shown with output c code): link. I cant find solution, why its so SLOW. Can anyone help me?

First off, your code does not compile, because (at least my just installed) libtcc does not contain a compile function.

As far as I can see it, your code is that slow because you emit a getch() and a putch() when you run across a . (as a side note: according to MSDN, the correct signature for the latter is int putch(int) and both getch() and putch() are deprecated. For portability, you should use int getchar(void) and int putchar(int) anyway).

You must always check the return value of malloc calls, because an allocation request can fail.

You should use a switch instead of multiple if statements for handling the characters, like in the following:

switch (c) {
case '<':
    /* Do work */
    break;
case '>':
    /* Do work */
    break;
case '[':
    /* Do work */
    break;
case ']':
    /* Do work */
    break;
case '+':
    /* Do work */
    break;
case '-':
    /* Do work */
    break;
case '.':
    /* Do work */
    break;
case ',':
    /* Do work */
    break;
default:
    /* Comment */
    break;
}

The advantages are that the intention is more obvious, the compiler may be able to generate better code and it prints an error if you try to handle a character twice.

The main function should always return a value and that value should allow the user to distinguish between a successful compiler run and an error. So you should add a return 0; to the end of the function and add a return 1; (or whatever nonzero value you like) at the end of every error path.

You should always close files you have opened.

Error messages should be printed to stderr, not stdout.

For protection against buffer overflows, you can use snprintf (which is available since C99) instead of sprintf. It returns either a negative value (if an error occurred) or the number of written bytes (excluding the \0 terminator), but it does not write more than size bytes (including the \0 terminator) to the output. Thus you can use

int ret = snprintf(outbuffer, 1024*1024, format, ...);
if (ret < 0 || ret >= 1024*1024) {
    /* Error or overflow */
    free(outbuffer);
    return 1;
}

Another thing you may want to consider is to reduce the memory size by one byte (thus every allowed pos value uses only 16 bits) and then replace

if (pos == 0) pos = 65536;
else pos--;

by

pos = (pos - 1) & 0xFFFF;

and

if (pos == 65536) pos = 0;
else pos++;

by

pos = (pos + 1) & 0xFFFF;

and thus make those two operations branchless. The effect of those two statements is that the operation is carried out on the pos variable, but only the lower 16 bits of the result are used (that is, 65535+1 becomes zero and 0-1 becomes 65535 (*)).

Putting all this together, you would have (including a compile function that can indicate an error and ):

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

static int compile(char const *out, char const *buf) {
    TCCState *tcc = tcc_new();
    if (tcc_set_output_type(tcc, TCC_OUTPUT_EXE) < 0) {
        fprintf(stderr, "Internal compiler error.\n");
        tcc_delete(tcc);
        return 1;
    }
    if (tcc_compile_string(tcc, buf) < 0) {
        fprintf(stderr, "Failed compiling generated code.\n");
        tcc_delete(tcc);
        return 1;
    }
    if (tcc_add_library(tcc, "c") < 0) {
        fprintf(stderr, "Failed linking in C library.\n");
        tcc_delete(tcc);
        return 1;
    }
    if (tcc_output_file(tcc, out) < 0) {
        fprintf(stderr, "Failed writing program to file.\n");
        tcc_delete(tcc);
        return 1;
    }
    tcc_delete(tcc);
    return 0;
}

int minus(FILE* f){
    int c;
    int amount = 0;
    while ((c = getc(f)) == '-')
        amount++;
    ungetc(c, f);
    return amount;
}

int plus(FILE* f){
    int c;
    int amount = 0;
    while ((c = getc(f)) == '+')
        amount++;
    ungetc(c, f);
    return amount;
}

int main(int argc, char** argv){
    static int const buflen = 1024*1024;
    int pos = 0;
    int brackets = 0;
    fprintf(stderr, "Brainfuck Compiler by Joel. All rights reserved.\n");
    if(argc!=3){
        fprintf(stderr, "Usage: %s <input> <output>", argv[0]);
        return 1;
    }
    int amount = 0;
    int ret;
    FILE* f = fopen(argv[1], "r");
    if(f!=NULL){
        char* outbuffer = (char*)malloc(buflen);
        if (outbuffer == NULL) {
            fprintf(stderr, "Internal compiler error.\n");
            fclose(f);
            return 1;
        }
        ret = snprintf(outbuffer, buflen, "int getchar(void);\nint putchar(int);\nint mem[65536] = {0};\nint main(){\n");
        if (ret < 0 || ret >= buflen) {
            fprintf(stderr, "Internal compiler error.\n");
            free(outbuffer);
            fclose(f);
            return 1;
        }
        while(!feof(f)){
            switch (fgetc(f)) {
            case '<':
                pos = (pos - 1) & 0xFFFF;
                break;
            case '>':
                pos = (pos + 1) & 0xFFFF;
                break;
            case '[':
                ++brackets;
                ret = snprintf(outbuffer, buflen, "%swhile(mem[%d]){", outbuffer, pos);
                if (ret < 0 || ret >= buflen) {
                    fprintf(stderr, "Internal compiler error.\n");
                    free(outbuffer);
                    fclose(f);
                    return 1;
                }
                break;
            case ']':
                --brackets;
                ret = snprintf(outbuffer, buflen, "%s}", outbuffer);
                if (ret < 0 || ret >= buflen) {
                    fprintf(stderr, "Internal compiler error.\n");
                    free(outbuffer);
                    fclose(f);
                    return 1;
                }
                break;
            case '+':
                amount = plus(f)+1;
                ret = snprintf(outbuffer, buflen, "%smem[%d]+=%d;", outbuffer, pos, amount);
                if (ret < 0 || ret >= buflen) {
                    fprintf(stderr, "Internal compiler error.\n");
                    free(outbuffer);
                    fclose(f);
                    return 1;
                }
                break;
            case '-':
                amount = minus(f)+1;
                ret = snprintf(outbuffer, buflen, "%smem[%d]-=%d;", outbuffer, pos, amount);
                if (ret < 0 || ret >= buflen) {
                    fprintf(stderr, "Internal compiler error.\n");
                    free(outbuffer);
                    fclose(f);
                    return 1;
                }
                break;
            case '.':
                ret = snprintf(outbuffer, buflen, "%sputchar(mem[%d]);", outbuffer, pos);
                if (ret < 0 || ret >= buflen) {
                    fprintf(stderr, "Internal compiler error.\n");
                    free(outbuffer);
                    fclose(f);
                    return 1;
                }
                break;
            case ',':
                ret = snprintf(outbuffer, buflen, "%smem[%d]=getchar();", outbuffer, pos);
                if (ret < 0 || ret >= buflen) {
                    fprintf(stderr, "Internal compiler error.\n");
                    free(outbuffer);
                    fclose(f);
                    return 1;
                }
                break;
            default:
                break;
            }
        }
        fclose(f);
        if(brackets == 0){
            ret = snprintf(outbuffer, buflen, "%sreturn 0;\n}\n",outbuffer);
            if (ret < 0 || ret >= buflen) {
                fprintf(stderr, "Internal compiler error.\n");
                free(outbuffer);
                fclose(f);
                return 1;
            }
            if (compile(argv[2], outbuffer)) {
                free(outbuffer);
                return 1;
            }
            free(outbuffer);
        }
        else{
            fprintf(stderr, "Compilation fault. Unbalanced brackets.\n");
            free(outbuffer);
            return 1;
        }
    }
    else{
        fprintf(stderr, "Cannot open one of specified files.\n");
        return 1;
    }
    return 0;
}

(*) This is only true for systems representing signed integers using two's complement, where (using signed 16 bit integers) -1 equals 0xFFFF. When using one's complement representation, -1 equals 0xFFFE (with 0xFFFF being -0), but practically every machine commonly available today uses two's complement for representing signed integers.


Is this implementation of binary search correct? Is this implementation of binary search correct? Any special or edge cases that I missed out? Maybe it should be optimized for elements that are less than the first element of the array or greater than the last element?

bool binarySearch (int* array, int arraySize, int element)
{
    if ( arraySize == 0 )
    {
        return false;
    }

    int startPos = 0;
    int endPos = arraySize;
    while ( true )
    {
        int spanSize = endPos - startPos;
        if ( spanSize == 1 )
        {
            break;
        }
        int pivotPos = startPos + (spanSize >> 1);
        if ( element < array[pivotPos] )
        {
            // go left
            endPos = pivotPos;
        }
        else
        {
            // go right
            startPos = pivotPos;
        }
    }
    return element == array[startPos];
}

http://ideone.com/bcGsWG

I believe it works correctly, but I'm not enthused about the style.

A few points in no particular order:

Array indices should be size_t rather than int.

I really dislike loops of the form:

while (true) {
    if (something) break;
    do_real_loop_body();
}

I'd rather see the condition for exiting the loop written into the loop condition itself.

In this case, I'd also rather see a for loop than a while loop. We need to do some initialization, a test on every iteration, and update some variables ever iteration. When we have all three elements of the for loop, we might as well use it as simulate it on our own.

Although I know some misguided people agree, it's also generally best to avoid using braces when (for example) each leg of your if statement is only controlling a single statement.

Incorporating all these, we end up with a function that looks more like this:

bool binarySearch(int* array, size_t endPos, int element) {
    if (endPos == 0)
        return false;

    size_t startPos = 0;
    for (size_t pivotPos = endPos/2;
        endPos-startPos != 1;
        pivotPos = startPos + (endPos - startPos) / 2)
    {
        if (element < array[pivotPos])
            endPos = pivotPos;
        else
            startPos = pivotPos;
    }
    return element == array[startPos];
}

The next step would be to make it generic, working with iterators so it can deal with different containers and different types in those containers. For that we could end up with something closer to this:

template <class Iter, class T>
bool bfind(Iter left, Iter right, T val) {
    Iter o = right;
    while (left < right) {
        Iter middle = left + (right - left) / 2;
        if (*middle < val)
            left = middle + 1;
        else
            right = middle;
    }
    return left != o && *left == val;
}

Interestingly, the generic version is not only more versatile, but also somewhat simpler.


Make a Deal program - using sets in Python Mostly because I didn't believe the outcome of the classic game play Make-a-Deal (see https://math.stackexchange.com/questions/608957/monty-hall-problem-extended) I made this little program... and indeed if there are 3 doors and the quiz-master opens a door and you then switch from your initial choice of door your chances go up from 33% to 67%! This program is the generalized version whereby you can choose the number of doors and how many doors the quiz-master will open, it calculates the chance for a price for the initial choice (basically 1 over the number of doors) and the chance if you change your selected door after the doors have been opened.

I was pleasantly taken by the elegant solution using Python sets, but I wonder if this is the most efficient way. Comparing with other methods it looks like it becomes relatively more efficient with more doors and doors to open.

Thank you for your input...

#!/usr/bin/env python
'''  application of Make a deal statistics
     application is using sets {}
     for reference:
 https://math.stackexchange.com/questions/608957/monty-hall-problem-extended
'''
import random

def Make_a_Deal(doors, doors_to_open):
    '''  Generalised function of Make_a_Deal. Logic should be self explanatory
         Returns win_1 for the option when no change is made in the choice of
         door and win_2 when the option to change is taken.'''

    win_1, win_2 = False, False

    doors_set = set(range(1, doors+1))
    price_set = set(random.sample(doors_set, 1))
    choice1_set = set(random.sample(doors_set, 1))
    open_set = set(random.sample(doors_set.difference(price_set).
                   difference(choice1_set), doors_to_open))
    choice2_set = set(random.sample(doors_set.difference(open_set).
                      difference(choice1_set), 1))
    win_1 = choice1_set.issubset(price_set)
    win_2 = choice2_set.issubset(price_set)

    return win_1, win_2

def main():
    '''  input:
         - throws: number of times to Make_a_Deal (must be > 0)
         - doors: number of doors to choose from (must be > 2)
         - doors_to_open: number of doors to be opened before giving the option
           to change the initial choice (must be > 0 and <= doors-2)'''

    try:
        throws = int(input('how many throws: '))
        doors = int(input('how many doors: '))
        doors_to_open = int(input('how many doors to open: '))
        if (throws < 1) or (doors < 3) or \
                (doors_to_open > doors-2) or (doors_to_open < 1):
            print('invalid input')
            return

    except Exception as e:
        print('invalid input: ', e)
        return

    number_of_wins_1, number_of_wins_2, counter = 0, 0, 0

    while counter < throws:
        win_1, win_2 = Make_a_Deal(doors, doors_to_open)

        if win_1:
            number_of_wins_1 += 1
        if win_2:
            number_of_wins_2 += 1

        counter += 1
        print('completion is {:.2f}%'.
              format(100*counter/throws), end='\r')

    print('number of wins option 1 is {:.2f}%: '.
          format(100*number_of_wins_1/counter))
    print('number of wins option 2 is {:.2f}%: '.
          format(100*number_of_wins_2/counter))

if __name__ == '__main__':
    main()

You can do this in \$O(1)\$ space and time, where you're doing it in \$O(n)\$ space and time. I cleaned your code and got roughly the same as Graipher's code:

def make_a_deal(doors, doors_to_open):
    doors = range(1, doors+1)
    price = set(random.choice(doors))
    choice_1 = set(random.choice(doors))
    doors = set(doors)
    open_doors = set(random.sample(doors - price - choice_1, doors_to_open))
    choice_2 = set(random.sample(doors - open_doors - choice_1, 1))
    return choice_1 <= price, choice_2 <= price

But there are some changes you can make to make it faster.

After generating price and choice_1 you can check if the user has won, and return True, False.
You don't need to randomly select price, this is as you can rotate the doors until price is the first, always. And so you can make price = 0.
Expanding on the above we now know that 0 is the only door with the price, and so \$1 \to n\$ are all loser doors. From this we know that one of the doors we picked was a loser door, and so we can remove that door, and shift all the other doors down. Which is the same as removing the last door.
Expanding on the above, you can remove \$k\$ more loser doors, as they are randomly selected.
And so the second choice has a range of \$0 \to n - k - 1\$.

And so you can get \$O(1)\$ space and time:

def make_a_deal(doors, doors_to_open):
    price = 0
    choice_1 = random.randrange(doors)
    if choice_1 == price:
        return True, False

    choice_2 = random.randrange(doors - 1 - doors_to_open)
    return False, choice_2 == price


Formatting "Bond, JAMES (Mr)" I need to call this function over a big dataset. I need it to be as efficient as possible but still be readable. Can I make it anymore efficient by using a different function for capitalize or blank check?

public static String nameFormat(String title, String firstname, String lastName) {

    if (StringUtils.isBlank(firstname) && StringUtils.isBlank(lastName)) {
        return "Unknown";
    }

    StringBuilder fullNameSB = new StringBuilder(lastName.trim().toUpperCase());
    fullNameSB.append(", ");
    fullNameSB.append(WordUtils.capitalizeFully(firstname.trim()));
    if (title != null) {
        fullNameSB.append(" ");
        // replace (mr.) with mr
        title = title.trim().replaceAll("\\.|\\)|\\(", "");
        fullNameSB.append("(");
        fullNameSB.append(WordUtils.capitalize(title));
        fullNameSB.append(")");
    }
    //Remove any double spaces that might be in a multipart first or surname
    return fullNameSB.toString().replaceAll("  ", "");

}

I assume WordUtils and StringUtils are Apache Commons?  If so, it would be helpful for us for you to link to their online documentation/source.

The first thing to do when optimizing is to make sure you're optimizing the right thing.  You might find ways to shave off a bit of running time in this method, but if this method is only responsible for 1% of the total running time then that's not very good use of your effort.

The thing that strikes me is all of the safety for weird conditions.  I can pretty much guarantee you that your replaceAll calls are the slowest things.  Maybe upstream code should be ensuring that the title is of the correct form to begin with, the strings are trimmed, and there are no double spaces.


Adding polynomials represented as maps I'm using maps to represent a polynomial.
The key is the exponent and the value is the coefficient : Map(exp -> coef)

e.g. 2.5*x^5 + 3.0 is represented by : Map(5 -> 2.5, 0 -> 3.0)

I wrote 3 functions (in Scala) to add such polynomials. Which one is the most elegant ? Do you have another version that is preferable/better ? (Implementations in another language such as Haskell are welcome)

Here is a sample testcase :

val polynom1 = Map(0 -> 1.0, 1 -> 2.0, 2 -> 1.0)
val polynom2 = Map(0 -> 1.0, 1 -> (-2.0), 2 -> 1.0)
val expectedSum = Map(0 -> 2.0, 1 -> 0.0, 2 -> 2.0)

Here are the implementations :

def addMap1(lhs: Map[Int, Double], rhs: Map[Int, Double]) =
  ((lhs.keys ++ rhs.keys) map { k =>
    (k, lhs.getOrElse(k, 0.0) + rhs.getOrElse(k, 0.0))
  }).toMap

def addMap2(lhs: Map[Int, Double], rhs: Map[Int, Double]) = {
    def adjust(kv: (Int, Double)) = {
      val (exp, coef) = kv
      (exp, coef + lhs.getOrElse(exp, 0.0))
    }
    lhs ++ (rhs map adjust)
  }

def addMap3(lhs: Map[Int, Double], rhs: Map[Int, Double]) = {
    def addTerms(acc: Map[Int, Double], x: (Int, Double)) = {
      val (exp, coef) = x
      acc + (exp -> (coef + rhs.getOrElse(exp, 0.0)))
    }
    (lhs foldLeft rhs)(addTerms)
  }

I think that the second implementation of the function is the most readable and concise. There is just the name of def adjust that should be changed. adjust is too vague, sumCoeffs would be more concrete.

It's probably a matter of taste, but the first implementation seems to contain too much nesting (parenthesis and braces). The third implementation looks also a bit more complex than the second, mostly due to the mechanism of foldLeft.

By the way, I've got two suggestions that may slightly improve this code.

Omitting dots for function calls and parenthesis for arguments are not among the recommendations of the official Scala style guide (sections Arity-0 and Arity-1). This practice can reduce the readability. For example, I find that rhs.map(adjust) and lhs.foldLeft(rhs)(addTerms) are easier to perceive for a human-shaped reader than the respective expressions in the original code.
There are several occurrences of getOrElse(exp, 0.0) that can be extracted in a small helper function like private def coefFor(exp : Int, polynoms : Map[Int, Double]) = polynoms.getOrElse(exp, 0.0). This will allow to transform the respective instructions into more elegant, for example: (exp, coef + coefFor(exp, lhs)). But since among the three functions only one will be chosen/used, this will eliminate the duplications and the relevance of this remark.


Random positioning and movement of a rabbit on a table I was inspired by this:

  If you would put a rabbit randomly on a circular table with radius r=
  1 meter and it moves 1 meter in a random direction, what is the
  chance it won't fall off?

I decided to develop a program to manually test this scenario. My code will run as many tests as you want with (hopefully) more tests resulting in a correct answer. I know a bit of C++, but have decided to learn Python recently.

My code does the following:

Initialises a random starting within a circle with radius 1
Moves 1 unit in a random direction
Updates the pass/fail rate depending on if the movement resulted in a position within the original circle

import math
import random

class RandPoint:
    x = 0
    y = 0

    def __init__(self):
        self.x = random.uniform(-1,1)
        self.y = random.uniform(-1,1)

    def move(self):
        angle = random.uniform(0, 2 * math.pi)
        self.x+=math.cos(angle)
        self.y+=math.sin(angle)

    def check(self):
        return math.sqrt(math.pow(self.x,2)+math.pow(self.y,2)) < 1

passed = 0.0
num = int(raw_input("How many trials would you like? \n"))

for value in range (1,num):
    startingpoint = RandPoint()

    while not(startingpoint.check()):
        startingpoint = RandPoint()

    startingpoint.move()
    passed += int(startingpoint.check())

print (str(num) + " trys, " + str(passed) + " passes, " +      str((passed/num)*100) + "%")

Sampling

This is your approach to placing the rabbit randomly on a circular table:

startingpoint = RandPoint()

while not(startingpoint.check()):
    startingpoint = RandPoint()

How many tries does it take to place a rabbit?  Each attempt succeeds with a probability \$p = \dfrac{\pi}{4}\$, so the expected number of tries is

$$\frac{\pi}{4}\left(1 + 2\left(1 - \frac{\pi}{4}\right) + 3\left(1 - \frac{\pi}{4}\right)^2 + 4\left(1 - \frac{\pi}{4}\right)^3 + \ldots\right) \approx 1.273$$

Can it be done without retrying?  Yes.  You can use

$$\begin{array}{rl}
x =& \sqrt{r}\ \cos \theta \\
y =& \sqrt{r}\ \sin \theta
\end{array}$$

… where \$r\$ is sampled uniformly from the interval [0, 1] and \$\theta\$ is sampled uniformly on [0, 2π].

Which is better for performance: taking a chance with looping (and consuming 27% more random numbers), or performing a square root and trigonometric calculations?  It's hard to say in advance, but it turns out that the performance of my solution below is about the same.  I think that the \$\langle \sqrt{r}\ \cos \theta, \sqrt{r}\ \sin \theta\rangle\$ method is more elegant.

Critique

In Python 2, classes should explicitly derive from object.

In RandPoint, x = 0 and y = 0 are unnecessary, and in fact misleading.  Those variables are not the same as self.x and self.y.

In this case, math.pow() should be written more succinctly using the exponentiation operator, since you don't need the validation that math.pow() performs.  You also don't need math.sqrt() in check().

move() and check() are specific to the exercise at hand and feel out of place in a RandPoint class.  Furthermore, based on just the method name, it's not obvious what check() would check.  Perhaps is_in_unit_disc() would be clearer, but I would choose to reorganize that code altogether.

passed is a count, so it should be an int rather than a float.  A better way to count a bunch of boolean values is to use the built-in sum() function with a generator expression.

The idiom to repeat something num times is for _ in xrange(num): ….  By starting a 1, you skipped one iteration.

To print the result, don't do so many string concatenations.  You can use str.format() instead.

Suggested solution

import math
import random

class Point(object):
    @staticmethod
    def random_polar(r=None):
        """Pick a point in a random direction with some magnitude.
           If r=1, this samples a unit circle uniformly.
           If r=None, this samples a unit disc uniformly."""
        if r is None:
            r = random.uniform(0, 1)**0.5
        theta = random.uniform(0, 2 * math.pi)
        return Point(r * math.cos(theta), r * math.sin(theta))

    def __init__(self, x, y):
        self.x = x
        self.y = y

    def add(self, other):
        self.x += other.x
        self.y += other.y
        return self

def rabbit_trial():
    """Rabbit at a random point in the unit disc moves one unit in
       a random direction.  Return True if the rabbit is still in
       the unit disc, or False if the rabbit is outside the disc."""
    p = Point.random_polar().add(Point.random_polar(r=1))
    return p.x**2 + p.y**2 < 1**2

trials = int(raw_input("How many trials would you like? "))
inside = sum(rabbit_trial() for _ in xrange(trials))
print '{} tries, {} passes, {}%'.format(trials, inside, 100.0 * inside / trials)


Emulating super() in Python 3.x using Python 2.7 Depending on the name of the first argument, self.__sup or cls.__sup behaves like super() in Python 3, while this code is written in Python 2.7. It works for both ordinary methods and class methods(static methods don't use super()). See my code for detailed examples:

class mySuper(object):
    def __init__(self, cls):
        self.cls = cls
    def __get__(self, obj, cls):
        if obj is None:
            return super(self.cls, cls)
        else:
            return super(self.cls, obj)

class Meta(type):
    def __init__(cls, name, bases, clsdict):
        setattr(cls, '_' + name + '__sup', mySuper(cls))
        super(Meta, cls).__init__(name, bases, clsdict)

# Usage examples:

class A(object):
    __metaclass__ = Meta
    def meth(self):
        print 'A.meth(%s)' % self
    @classmethod
    def clsmeth(cls):
        print 'A.clsmeth(%s)' % cls

class B(A):
    def meth(self):
        self.__sup.meth()                # super().meth()
        print 'B.meth(%s)' % self
    @classmethod
    def clsmeth(cls):
        cls.__sup.clsmeth()              # super().clsmeth()
        print 'B.clsmeth(%s)' % cls

class C(A):
    def meth(self):
        self.__sup.meth()
        print 'C.meth(%s)' % self
    @classmethod
    def clsmeth(cls):
        cls.__sup.clsmeth()
        print 'C.clsmeth(%s)' % cls

class D(B, C):
    def meth(self):
        self.__sup.meth()
        print 'D.meth(%s)' % self
    @classmethod
    def clsmeth(cls):
        cls.__sup.clsmeth()
        print 'D.clsmeth(%s)' % cls

d = D()
d.meth()
d.clsmeth()

Here is the output:

A.meth(<__main__.D object at 0x103c8aed0>)
C.meth(<__main__.D object at 0x103c8aed0>)
B.meth(<__main__.D object at 0x103c8aed0>)
D.meth(<__main__.D object at 0x103c8aed0>)
A.clsmeth(<class '__main__.D'>)
C.clsmeth(<class '__main__.D'>)
B.clsmeth(<class '__main__.D'>)
D.clsmeth(<class '__main__.D'>)

I'm sorry if the examples are not very meaningful, but I'm just a beginner. (Maybe emulating super() itself is pointless.)

Apart from the "Usage examples", it's just a little snippet, so please tell me any problem you can spot in this piece of code.

First of all, +1 on the idea.  Maybe you could submit this to the Python developers for Python 2.7.13 :)

Your super class is called mySuper.  Oh, the sound of my in a variable name!  Such are the names that are used when describing code to a non-programmer.  We shouldn't allow that in real code.  It is a Super class; that's all there is to it.  If you were to keep my in there, however, it should be PascalCase, not mixedCase.  (That is, MySuper, not mySuper).  It should be thus for compliance with PEP 8, the Python style guide.

Why Meta?  Well, obviously it is a meta class, but for what?  The name should describe it more aptly.  This happens to be an incredible meta class, so call it SuperMeta ;)

Instead of a couple concatenations, use string formatting:

setattr(cls, '_{}__sup'.format(name), Super(cls))


Building a simple tree structure I'm trying to build this simple tree:

Multiverse
|
|--- Universe
    |
    |--- Supercluster
        |
        |--- Galaxy
            |
            |--- Andromeda
            |
            |--- Milky way

I came up with this simple method which recursively walk a tree:

/**
 * recursively walk a tree structure
 */
public static void recursive(TreeViewItem treeNode){

        System.out.println(treeNode.getText());

        Iterator<TreeViewItem> itr = treeNode.getItems().iterator();

        while (itr.hasNext()) {
            TreeViewItem item = (TreeViewItem) itr.next();
            recursive(item);
        }
}

Is there a better way or more elegant way to do this?

You're visiting and printing the tree elements in a depth-first manner, so depthFirstPrint would be a much better name than "recursive" (which is always a terrible name for any recursive function).

You didn't need this cast:

TreeViewItem item = (TreeViewItem) itr.next();

because you correctly declared the iterator as Iterator<TreeViewItem> itr

As of Java 1.5, you can use a for-each loop, which is simple and elegant:

for (TreeViewItem item : treeNode.getItems()) {
    depthFirstPrint(item);
}


Statistical sampling from a vector I have written a function/script to simulate data for sample size estimation. The following code samples from a vector of generated values with varying numbers of sample sizes and then concatenates means and standard deviations for a number of simulations.

I am using nested loops but would like to vectorize the code for efficiency and am wondering if anyone could suggest some optimizations.

library(MCMCglmm)
library(tidyverse)

Est <- function(n, mean, sd, lower, upper, samp_min, samp_max, samp_int, nsim){

        Data <- round(rtnorm(n, mean, sd, lower, upper), digits = 0) # Create a vector to sample from

        Samp_size <- seq(samp_min, samp_max, samp_int) # Create vector of sample sizes

        # Set up enpty results data frames
        Results_samp <- data.frame()
        Results <- data.frame()

        for(i in 1:nsim){ ## Loop through number of simulations

                for (j in seq_along(Samp_size)) { # Loop through sample sizes
                     SUS_Score <- sample(Data, Samp_size[j], replace = TRUE)
                     Nsubj <- Samp_size[j]
                     Mean <- mean(SUS_Score, na.rm = TRUE)
                     SD <- sd(SUS_Score, na.rm = TRUE)
                     Results_samp <- rbind(Results_samp,
                                      data.frame(
                                              Nsubj,
                                              Mean,
                                              SD))
                }
             Results <- rbind(Results, Results_samp)
             Results_samp <- data.frame()
        }

       Results %>%
               arrange(Nsubj)
}

Sims <- Est(n = 1000, mean = 55, sd = 37, lower = 0, upper = 100, samp_min = 5, samp_max = 35, samp_int = 5, nsim = 1000)

Any suggestions would be greatly appreciated!

Your code is quite good. Simulations is hard to vectorize. The largest slowdown here is the repeated calling of rbind in loop. It is faster to crate list of vectors and concatenate the results at the end. So  I edited the necessary parts:

Est3 <- function(n, mean, sd, lower, upper, samp_min, samp_max, samp_int, nsim) {
  Data <- round(rtnorm(n, mean, sd, lower, upper), digits = 0)
  Samp_size <- seq(samp_min, samp_max, samp_int)
  Results <- list() # crate emty list
  for (i in 1:nsim) {
    Results_samp <- list() # crate emty list
    for (j in seq_along(Samp_size)) {
      Nsubj <- Samp_size[j]
      SUS_Score <- sample(Data, Nsubj, replace = TRUE)
      Mean <- mean(SUS_Score, na.rm = TRUE)
      SD <- sd(SUS_Score, na.rm = TRUE)
      Results_samp[[j]] <- c(Nsubj, Mean, SD) # add values to list
    }
    Results[[i]] <- Reduce(rbind, Results_samp) # convert list to matrix and add to main list
  }
  Results <- Reduce(rbind, Results) # 'rbind' list of matrices
  Results <- as.data.frame(Results, row.names = F) # convert to data.frame
  colnames(Results) <- c('Nsubj', 'Mean', 'SD') # add names
  Results %>% arrange(Nsubj)
}


Finding the maximum GCD of all pairs The code below is for Hackerrank competition. Seems that it works correctly but performs very slowly and because of it can't pass some tests.

The task is to find the maximum GCD of all possible pairs of two positive integers \$x, (1 \leq x \leq 10^6)\$ and \$y, (1 \leq y \leq 10^6)\$, where \$x\$ belongs to array \$A\$ and \$y\$ belongs to array \$B\$. Both arrays have the same size, \$(1 \leq \text{size} \leq 10^5)\$. If there is more than one pair with the same GCD, then use the GCD with the maximum sum of its numbers i.e. \$(x + y)\$. Print the sum of elements of this maximum-sum pair.

Input is given in the following way

first line - size of the arrays
second line - elements for the first array
third line - elements for the second array

e.g

5
3 1 4 2 8
5 2 12 8 3

#include <cmath>
#include <cstdio>
#include <vector>
#include <iostream>
#include <algorithm>
using namespace std;

inline size_t countGcd( size_t n1, size_t n2 ) {
   return ( n2 != 0 ) ? countGcd( n2, n1 % n2 ) : n1;
}

int main() {

size_t num = 0;
std::cin >> num;
size_t elem = 0;
std::vector< size_t > arr;
for ( size_t i = 0; i < num; ++i ) {
    std::cin >> elem;
    arr.push_back( elem );
}
size_t max_sum = 0;
size_t max_gcd = 0;
for ( size_t i = 0; i < num; ++i ) {
    std::cin >> elem;

    for ( size_t j = 0; j < arr.size(); ++j ) {
        size_t gcd = ( ( elem >= arr.at( j ) ) ? ( elem % arr.at( j ) == 0 ? arr.at( j ) : countGcd( elem, arr.at( j ) ) ) : ( arr.at( j ) % elem == 0 ? elem : countGcd( elem, arr.at( j ) ) ) );
        if ( gcd > max_gcd ) {

            max_sum = elem + arr.at( j );
            max_gcd = gcd;
        } else if ( gcd == max_gcd ) {
            size_t sum = elem + arr.at( j );
            if ( sum > max_sum )
                max_sum = sum;
        }
    }
}
std::cout << max_sum << std::endl;

return 0;
}

I tried different GCD implementations ( Euclidean algorithm ( both recursive and iterative ), Binary GCD algorithm ( both recursive and iterative ) ) but without success. Seems that I have to change something inside main function, maybe rid off the if-else inside the for-loops but actually have no idea how to do this.

My explorations on GCD and performance

Aside: you might find My blog post interesting, where I found bottlenecks in GCD including branch prediction, lack of instruction parallelism, and the DIV instruction.  Even things like Modulo is significantly slower for signed types compared to unsigned types of the same size!  Since the values being used are all positive, that is another speedup.

For some operations, the opcode is faster for 32-bit than 64-bit; so switching to a smaller version once the numbers get small enough will speed it up.

I found that an algorithm written for computers in 1961 that does no division runs slower than the modern expression when coded naively, but once the bottlenecks in that are found and eliminated, it runs much faster.  The CPU can burn through lots of primitive instructions so the increased length is not significant; it’s the traffic jams inside the CPU that kill the performance.

Your code review

Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc.  (See SF.7.)

You should break up the monolithic main into smaller functions.  It is clear that first you read the input, then you do the actual work.

Write the function that does the real work isolated from any real I/O.  It takes parameters and returns values.  You can call that from test cases that means you don’t have to type the input yourself every time!  And you can have a function to read the input in the manner needed, for the final deliverable.

Meanwhile, the functions you write to load arrays from a file like that can be reused for the next problem.  You can be sure it has no code tangled up with the old program — it is just for getting input.

Your code seems complex since GCD itself is just a few lines.  Is that what countGcd is?  The name is odd if it returns GCD, not the count of something.  No, that’s just one step as there is no while loop.  Point is, I have no idea from the names and complete lack of comments just what this code does or how it works.

    for ( size_t j = 0; j < arr.size(); ++j ) {

It appears that this could be a range-based for loop.  I see you refer to arr.at( j ) eight times!  And you are wondering about performance?

for (auto current : array) {

and then I think the long line is the actual GCD calculation because of its name:

    auto GCD= compute_GCD (current, elem);

put that in a separate (readable) function.  It will also make it easy to change the implementation of the GCD algorithm itself without messing with the rest of the program!

        if ( gcd > max_gcd ) {

            max_sum = elem + arr.at( j );
            max_gcd = gcd;
        } else if ( gcd == max_gcd ) {
            size_t sum = elem + arr.at( j );
            if ( sum > max_sum )
                max_sum = sum;
        }

You want to accumulate the maximum incrementally.  It’s probably not worth using an stl algorithm for that since you are giving it the input incrementally rather than having a prepared list.  It can, however, but split into a separate function or accumulator class.

The complex part here is the tie-breaking.  Understand that the description is simply that of a compound key!  Think of sorting names:  if the last name matches, then sort by the first.

Now std::pair already does this for comparison operators.  So make a pair{GCD,sum} of the current trial, and have the max variable of the same type.  Now you just need a straightforward remember the largest accumulator.

using respair = std::pair<size_t, size_t>;

static respair best { };

void check_for_best (const respair& x)
{
    best = std::max (best,x);
}

basically, it reduces to one line.

max.second will be the max sum.


Calculation of duration between two timestamps in different rows In my sample data table below, all datatypes are varchar(255).

|      event      |            timestamp           |       traceid       |
| accepted        | 2016-01-16T09:35:48.653418931Z | AIP1452936948-44467 |
| published_event | 2016-01-16T09:35:48.659767348Z | AIP1452936948-44467 |

Here is the expected output:

|       traceid       | duration |
| AIP1452936948-44467 |   108    |

And the code I would like reviewed...

select traceid, bUnixMS - aUnixMS as duration
from (
select
a.traceid,
CONVERT(UNIX_TIMESTAMP(STR_TO_DATE(a.mydatetime,'%Y-%m-%d %H:%i:%s')), SIGNED) as aUnix,
CONVERT(UNIX_TIMESTAMP(STR_TO_DATE(b.mydatetime,'%Y-%m-%d %H:%i:%s')), SIGNED) as bUnix,
(CONVERT(UNIX_TIMESTAMP(STR_TO_DATE(a.mydatetime,'%Y-%m-%d %H:%i:%s')), SIGNED) + (CONVERT(LEFT(a.ms, 3), SIGNED) / 1000)) * 1000 as aUnixMS,
(CONVERT(UNIX_TIMESTAMP(STR_TO_DATE(b.mydatetime,'%Y-%m-%d %H:%i:%s')), SIGNED) + (CONVERT(LEFT(b.ms, 3), SIGNED) / 1000)) * 1000 as bUnixMS,
CONVERT(LEFT(a.ms, 3), SIGNED) as aMS,
CONVERT(LEFT(b.ms, 3), SIGNED) as bMS

from (
   select
      event,
      traceid,
      CONCAT(SUBSTRING_INDEX(timestamp, 'T', 1), ' ', SUBSTRING_INDEX(SUBSTRING_INDEX(timestamp, 'T', -1), '.', 1)) as mydatetime,
      SUBSTRING_INDEX(SUBSTRING_INDEX(timestamp, 'T', -1), '.', -1) as ms
   from applogs
   where event = "accepted"
) a

join (
   select
      event,
      traceid,
      CONCAT(SUBSTRING_INDEX(timestamp, 'T', 1), ' ', SUBSTRING_INDEX(SUBSTRING_INDEX(timestamp, 'T', -1), '.', 1)) as mydatetime,
      SUBSTRING_INDEX(SUBSTRING_INDEX(timestamp, 'T', -1), '.', -1) as ms
   from applogs
   where event = "published_event"
) b on a.traceid = b.traceid
) as x

You are not giving enough details (such as specific table structure, index, volume, types of queries, etc.) nor your constraints.

Nor do you explain how you come with value 108.

So my answer supposes you have only two timestamps per traceid and that the events are ordered logically in the same order as the timestamps.

SELECT
TIMESTAMPDIFF(MICROSECOND,
    STR_TO_DATE(MIN(`timestamp`), '%Y-%m-%dT%H:%i:%S.%fZ'),
    STR_TO_DATE(MAX(`timestamp`), '%Y-%m-%dT%H:%i:%S.%fZ'))
FROM applogs
GROUP BY traceid

This gives you 6319 which is a number of microseconds between the two. No idea on how you come to 108 from there. Note that it works only if you change your values to have only 6 digits after the dot, as the parser searches for microseconds. See here and the documentation.

If you really need your sub microseconds format you will need to split the string in two, first part being what is used above and last part being the last 3 digits as fractions of microseconds. And then add all of it.

This begs to ask why you are storing such a timestamp in a text field, this is not a good design. That would be the main point of the review: change your model and do not store timestamps in text fields, use the appropriate types of your RDBMS.

PS: since timestamp is the name of a type I do not recommend naming your column with it. Also, on the modeling part, your structure seems to reflect a key-value pattern, which is not always a good idea in RDBMS.


Creating a 2D array of map tiles I am a C++ programmer new to Ruby and someone suggested that I post my code here since it doesn't quite match Ruby standards. Is there a more Ruby way of doing this?

I have created a 2D array full of MapTile class objects and then I draw the tiles to the screen by iterating through the 2D Array using a for loop. Here is some code.

class MapTile
attr_accessor :tileSprite, :attribute

    def initialize(sprite, attr)
    @tileSprite = sprite
    @attribute = attr
    end

    def tileSprite
        @tileSprite
    end

    def attribute
        @attribute
    end

end

def array2D(width,height)
  a = Array.new(width, MapTile.new(123,0))
  a.map! { Array.new(height, MapTile.new(123,0)) }
  return a
end

@mapData = array2D(@mapSize,@mapSize)

for i in 0..@mapSize - 1
   for j in 0..@mapSize - 1
      mapData[i][j].tileSprite = tileNum
      @tile.draw(mapData[i][j].tileSprite)
   end
end

First of all your indentation is a bit wacky. Sometimes you indent by 4 spaces, sometimes by 2 (which is the default in the ruby community) and sometimes not at all (for example you didn't indent the call to attr_accessor or the contents of the initialize method). You should fix that.

attr_accessor :tileSprite, :attribute
# ...
def tileSprite
    @tileSprite
end

def attribute
    @attribute
end

No need to call attr_accessor and define the getter methods yourself. Do one or the other.

def array2D(width,height)
  a = Array.new(width, MapTile.new(123,0))
  a.map! { Array.new(height, MapTile.new(123,0)) }
  return a
end

Creating an array containing a default value and then mapping over that array to replace the default value makes very little sense. Why assign a default value at all if you throw it away right after?

Also it's a very bad idea to make the inner arrays all contain a reference to the same map tile. This just screams for you to introduce hard to find bugs. As a guide line you should never use the 2-argument version of Array.new with a mutable value as its argument. Use the block-version instead.

Also it's customary in ruby to not use explicit return statements in the last line of a method.

Here's how I'd write the array2d method:

def array2D(width,height)
  Array.new(width) do
    Array.new(height) do
      MapTile.new(123,0)
    end
  end
end


int32 to char[4] conversion functions I need to pass some int32_t values into and later retrieve them from a local storage API that uses void* and size_t parameters. Nothing else interprets the void* values.

I hand-made some simple functions to translate back and forth between the types, which work, but I wonder if anyone would care to let me know if they see any problems, weaknesses or improvements I should make.

int32_t Char4ToInt(char* pChar4)
{
    return (pChar4[3] << 24) | (pChar4[2] << 16) | (pChar4[1] << 8) | (pChar4[0]);
}

void StuffIntIntoChar4(char* pIntoChar4, int32_t val)
{
    pIntoChar4[3] = val>>24;
    pIntoChar4[2] = val>>16;
    pIntoChar4[1] = val>>8;
    pIntoChar4[0] = val;
}

If they were for more general use, I'd probably check the char* parameters for errors, but I only use them where they are passed

char valBuf[4];

as valBuf, so I know it always points to 4 allocated chars.

The API I am doing this for is s3eSecureStoragePut and s3eSecureStorageGet from the Marmalade API - the documentation doesn't go into great detail about guarantees, but it seems to work to store and retrieve a blob of bytes of the supplied size. This is a smartphone App and I just save my blob before exit and load it on entry, copying the data over if there is no error, otherwise using default data. The data just goes into UI controls (in this case, slider position values). However if there are weaknesses in using these functions more broadly, I'd be interested in any suggestions, as I might end up using them (or whatever gets pointed out as better) for some later project if they seem reasonable.

This is almost right, but it has two flaws:

First, you can improve type safety by passing char(&)[4] instead of char*:

int32_t Char4ToInt(char (&pChar4)[4]);

void StuffIntIntoChar4(char (&pIntoChar4)[4], int32_t val);

Second, you are running into undefined behavior. In the C++11 standard (section [expr.shift]), it says

  The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated
  bits are zero-filled. If E1 has an unsigned type, the value of the
  result is \$E1 × 2^{E2}\$, reduced modulo one more than the maximum value
  representable in the result type. Otherwise, if E1 has a signed type
  and non-negative value, and \$E1 × 2^{E2}\$ is representable in the result
  type, then that is the resulting value; otherwise, the behavior is
  undefined.

The last part is what you're running into -- you're left-shifting a negative value in some cases, which is undefined behavior.

You have a couple of choices, depending on your portability requirements.

If you will always run this on the same architecture (or in any case only on architectures with the same byte ordering), you can store like this:

void SerializeInt32(char (&buf)[4], int32_t val)
{
  std::memcpy(buf, &val, 4);
}

int32_t ParseInt32(const char (&buf)[4])
{
  int32_t val;
  std::memcpy(&val, buf, 4);
  return val;
}

If you would like instead to be able to store with a program built on one platform and load on a program built on a different platform (as long as its implementation-defined unsigned int -> int conversion uses two's complement, which is almost all modern platforms), you can write it like this:

void SerializeInt32(char (&buf)[4], int32_t val)
{
    uint32_t uval = val;
    buf[0] = uval;
    buf[1] = uval >> 8;
    buf[2] = uval >> 16;
    buf[3] = uval >> 24;
}

int32_t ParseInt32(const char (&buf)[4])
{
    // This prevents buf[i] from being promoted to a signed int.
    uint32_t u0 = buf[0], u1 = buf[1], u2 = buf[2], u3 = buf[3];
    uint32_t uval = u0 | (u1 << 8) | (u2 << 16) | (u3 << 24);
    return uval;
}


the fox is black - a simple image scraper I wrote a simple image scraper for the Desktop Wallpaper project by The Fox is Black. The site is a fantastic (yet, apparently dead) blog full of fabulous images. However, there's no way to download an entire bundle of images, hence this simple scraper.

I'm putting the code up for scrutiny. Any areas for code style and/or implementation improvements? I'd appreciate your feedback.

#!/usr/bin/python

import os
import requests

from bs4 import BeautifulSoup as bs

MAX_PAGES = 1  # Max. number of pages is 41
SAVE_DIRECTORY = 'fox_backgrounds'
BASE_URL = "http://www.thefoxisblack.com/category/the-desktop-wallpaper-project/page"
RESOLUTIONS = [
            "1280x800", "1440x900", "1680x1050", "1920x1200", "2560x1440",
            "iphone", "iphone-5", "iphone6", "iphone-6-plus", "iphone6plus",
            "ipad",
            ]

def fetch_url(url):
    return requests.get(url).text

def clip_url(href):
    return href[href.rfind('/'):]

def save_image(href):
    print(f"Downloading: {clip_url(href)}")
    request = requests.get(href)
    with open(f"fox_backgrounds{clip_url(href)}", 'wb') as output:
        output.write(request.content)

def get_images_from_page(url):
    html = fetch_url(url)
    soup = bs(html, "html.parser")
    for link in soup.find_all("a", class_="btn_download"):
        href = link["href"]
        for resolution in RESOLUTIONS:
            if resolution in href:
                save_image(href)

def make_dir():
    os.makedirs(SAVE_DIRECTORY, exist_ok=True)

def get_backgrounds():
    make_dir()
    for page in range(0, MAX_PAGES):
        get_images_from_page(BASE_URL + str(page + 1))

def main():
    get_backgrounds()

if __name__ == '__main__':
    main()

Suggested:

#!/usr/bin/python

import os
import requests

from bs4 import BeautifulSoup as bs
from pathlib import Path
from shutil import copyfileobj

MAX_PAGES = 1  # Max. number of pages is 41
SAVE_DIRECTORY = Path('fox_backgrounds')
BASE_URL = 'http://www.thefoxisblack.com/category/the-desktop-wallpaper-project/page'
RESOLUTIONS = {
    '1280x800', '1440x900', '1680x1050', '1920x1200', '2560x1440',
    'iphone', 'iphone-5', 'iphone6', 'iphone-6-plus', 'iphone6plus',
    'ipad'
}

def fetch_url(url):
    return requests.get(url).text

def clip_part(href):
    return href.rpartition('/')[-1]

def save_image(href):
    part = clip_part(href)
    print(f'   Downloading: {part}')
    fn = SAVE_DIRECTORY / part
    with requests.get(href, stream=True) as response, \
         open(fn, 'wb') as output:
        copyfileobj(response.raw, output)

def get_images_from_page(url):
    html = fetch_url(url)
    soup = bs(html, 'html.parser')
    for link in soup.find_all('a', class_='btn_download'):
        href = link['href']
        if any(href.endswith(f'-{res}.jpg') for res in RESOLUTIONS):
            save_image(href)
        else:
            print(f'Unknown resolution {href}')

def make_dir():
    os.makedirs(SAVE_DIRECTORY, exist_ok=True)

def get_backgrounds():
    make_dir()
    for page in range(1, MAX_PAGES+1):
        print(f'Fetching page {page}...')
        get_images_from_page(f'{BASE_URL}{page}')

def main():
    get_backgrounds()

if __name__ == '__main__':
    main()

Comments:

You initialized SAVE_DIRECTORY but then use it for creation and not file write
RESOLUTIONS should be a set, or maybe a tuple
clip_url is slightly misleading; it returns a URL part and not a whole URL
It's best if you have a prompt before downloading the index page; otherwise it hangs without the user knowing what's happening
Stream your content so that you don't use up memory for big files
Your RESOLUTIONS check is a little puzzling. Maybe it's a validation step? But if it's a validation step, you silently fail instead of printing a warning. Also, you keep iterating even after you've found the correct resolution. I rewrote this to just check the current resolution, and also be a little bit more careful about where it's seen in the filename.
range(0, ...) is redundant, but for your use case you're better off with range(1 anyway.
rpartition does basically the same thing as what you wrote, but doesn't require any fancy array slicing
Don't call clip_path twice

Edit

The following version makes sane use of generators so that the iteration functions only need to know about their iteration, and not the inner business logic.

Also, your resolution check needs to be case-insensitive for many of the files on the site; and the site has gif and png images as well as jpg. You were missing some resolutions and some alternate iPhone spellings. I don't think that it's worth doing a resolution check at all, especially given these edge cases, but I left it in.

#!/usr/bin/python

import os
import requests

from bs4 import BeautifulSoup as bs
from pathlib import Path
from shutil import copyfileobj

MAX_PAGES = 1  # Max. number of pages is 41
SAVE_DIRECTORY = Path('fox_backgrounds')
BASE_URL = 'http://www.thefoxisblack.com/category/the-desktop-wallpaper-project/page'
RESOLUTIONS = {
    '1280x800', '1440x900', '1680x1050', '1920x1200', '2560x1440', '3840x2400',
    'iphone', 'iphone5', 'iphone-5', 'iphone6', 'iphone-6-plus', 'iphone6plus', 'iphone6-plus',
    'ipad'
}

def clip_part(href):
    return href.rpartition('/')[-1]

def save_image(href):
    part = clip_part(href)
    print(f'   Downloading: {part}')
    fn = SAVE_DIRECTORY / part
    with requests.get(href, stream=True) as response, \
         open(fn, 'wb') as output:
        copyfileobj(response.raw, output)

def urls_from_page(url):
    soup = bs(requests.get(url).text, 'html.parser')
    for link in soup.find_all('a', class_='btn_download'):
        href = link['href']
        if any(href.lower().contains(f'-{res}.') for res in RESOLUTIONS):
            yield href
        else:
            print(f'Unknown resolution {href}')

def make_dir():
    os.makedirs(SAVE_DIRECTORY, exist_ok=True)

def all_urls():
    for page in range(1, MAX_PAGES+1):
        print(f'Fetching page {page}...')
        yield from urls_from_page(f'{BASE_URL}{page}')

def main():
    make_dir()
    for url in all_urls():
        save_image(url)

if __name__ == '__main__':
    main()


Finding the largest not-necessarily contiguous alternating binary string after inverting any sub-string from a given string I'm having problems solving this problem within the time limit.

  Alternative Thinking
   http://codeforces.com/contest/603/problem/A
  Kevin has just recevied his disappointing results on the USA Identification of Cows Olympiad (USAICO) in the form of a binary string of length n. Each character of Kevin's string represents Kevin's score on one of the n questions of the olympiad—'1' for a correctly identified cow and '0' otherwise.
  However, all is not lost. Kevin is a big proponent of alternative thinking and believes that his score, instead of being the sum of his points, should be the length of the longest alternating subsequence of his string. Here, we define an alternating subsequence of a string as a not-necessarily contiguous subsequence where no two consecutive elements are equal. For example, {0, 1, 0, 1}, {1, 0, 1}, and {1, 0, 1, 0} are alternating sequences, while {1, 0, 0} and {0, 1, 0, 1, 1} are not.
  Kevin, being the sneaky little puffball that he is, is willing to hack into the USAICO databases to improve his score. In order to be subtle, he decides that he will flip exactly one substring—that is, take a contiguous non-empty substring of his score and change all '0's in that substring to '1's and vice versa. After such an operation, Kevin wants to know the length of the longest possible alternating subsequence that his string could have.
  Input
  The first line contains the number of questions on the olympiad n (1 ≤ n ≤ 100 000).
  The following line contains a binary string of length n representing Kevin's results on the USAICO.
  Output
  Output a single integer, the length of the longest possible alternating subsequence that Kevin can create in his string after flipping a single substring.

My Code
A little explanation: I try every substring and invert that, then I find the longest alternating substring by taking every first occuring 0 and 1. 'f' is the value 0 or 1 which I'm finding, which alternates. I try f=0 and 1 both for index=0 for the string for which we are calculating the score. e.g. the score for 10011101001100001 will be calculated by 10011101001100001, i.e. 9.

import java.util.Scanner;

public class Problem_A {
    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        final int n = sc.nextInt();
        sc.nextLine();
        final String str = sc.nextLine();
        sc.close();
        int maxScore = score(str);
        for (int i = 0; i < n-1; i++) {
            for (int j = i + 1; j < n; j++) {
                String str2 = invert(str, i, j);
                int score = score(str2);
                if (score > maxScore) {
                    maxScore = score;
                }
            }
        }
        System.out.println(maxScore);
    }

    private static int score(String str) {
        char[] chArr = str.toCharArray();
        int finalScore = 0;
        for (int f = 0; f < 2; f++) {
            int fval = f;
            int score = 0;
            for (int i = 0; i < chArr.length; i++) {
                if (chArr[i] - '0' == f) {
                    score++;
                    f = 1 - f;
                }
            }
            if (score > finalScore) {
                finalScore = score;
            }
            f = fval;
        }
        return finalScore;
    }

    private static String invert(String str, int i, int j) {
        char[] chArr = str.toCharArray();
        for (int k = i; k <= j; k++) {
            chArr[k] = (char) (1 - chArr[k] + 2 * '0');
        }
        return new String(chArr);
    }
}

The score is equal to the number of edges, that is transitions from 0 to 1 and from 1 to 0. After flipping number of edges inside the flipped range remains unchanged. Flipping may add two extra edges on the range boundaries; for that you need to find two occurrences of 11 or 00 and make the range to flip just one digit in each.

For example,

    100101001
      ^^^^^
    101010101

Of course there are some corner cases to be considered.


Unix 'sleep' clone - unsure of safety I decided to try making some simple clones of the various user-space Unix utilities in an effort to learn the system-specific headers and improve my C skills. The code that follows is a clone of sleep and I'm curious as to whether there are any pitfalls I'm overlooking (including security vulnerabilities). It compiles and seems to behave fine so there's no issue there.

The program attempts to comply with the Open Group spec for sleep.

#include <ctype.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#define MAX_TIME_DIGITS (11)

int main(int argc, char *argv[]) {
    if(argc != 2) {
        fprintf(stderr, "sleep: too %s operands\n",
                argc < 2 ? "few" : "many");
        return 1;
    }

    if(strnlen(argv[1], MAX_TIME_DIGITS + 1) > MAX_TIME_DIGITS) {
        fprintf(stderr, "sleep: operand too long\n");
        return 1;
    }

    int i = 0;
    while(argv[1][i] && i < MAX_TIME_DIGITS) {
        if(!isdigit((int)argv[1][i])) {
            fprintf(stderr, "sleep: invalid operand\n");
            return 1;
        }
        ++i;
    }

    unsigned long int time = strtoul(argv[1], NULL, 10);
    sleep(time);
    return 0;
}

An error message ("too few"/"too many") is not helpful. Typically when an Unix utility cannot deal with the command line it prints what it expects:

For example,

fprintf(stderr, " Usage: %s sleep_duration", argv[0])

It is also customary to provide a usage() function to print such message.

In any case, do not hardcode a sleep string. Use argv[0].
strtoul has very good error handling capabilities. It is pointless to try to intercept problems it can detect.

Just let it do its job:

char * stop;
unsigned long int time = strtoul(argv[1], &stop, 10);
if (*stop != 0) {
    // strtoul failed to parse the argument. Now printing an error message is appropriate.
    usage("...");
}

It would be nice to add an option to define a SIGALRM behaviour. OpenGroup does not specify which action shall be taken, meaning it is up to you to define it.


C++ wrapper around uniform mt19937 SequenceContainer With the following interface in mind

EasyRandom<unsigned int> prng(a, b);
auto x = prng();   // scalar
auto v = prng(10); // vector

I wrote the following class:

// https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution
template <typename T = unsigned int>
class EasyRandom
{
private:
  std::random_device rd;
  std::unique_ptr<std::mt19937> gen;
  std::unique_ptr<std::uniform_int_distribution<T>> dist;

public:
  EasyRandom(T a, T b)
  {
    gen = std::make_unique<std::mt19937>(rd());
    dist = std::make_unique<std::uniform_int_distribution<T>>(a, b);
  }

  T operator()() { return (*dist)(*gen); }

  std::vector<T> operator()(size_t n)
  {
    std::vector<T> v;
    for (; n > 0; v.push_back(operator()()), --n);
    return v;
  }
};

I also have a few specific questions:

Is there a way to instantiate EasyRandom without the use of pointers?
Is it possible to change operator()(size_t n) to return any user-specified SequenceContainer (e.g. vector, list, deque) instead of hard-coding it to a particular implementation (e.g. std::vector)?

gen and dist don't need to be pointers.  Just declare them as members, and use the member initializer list in the constructor to initialize them.

std::random_device rd;
std::mt19937 gen;
std::uniform_int_distribution<T> dist;

EasyRandom(T a, T b): gen(rd()), dist(a, b) {
}

In the operator() that returns a vector, you can reserve space for the vector to avoid memory reallocations during the insertions (v.reserve(n)).  While not an issue with ints, if you use emplace_back rather than push_back that can avoid potential extra copies of a value for non-simple types.

The second question is off topic (code not implemented).