Coding style in Ruby format in an method and variable assign Case 1_a:

  zip_file = file.sub(directory, '').sub!(/^\//, '')
  zipfile.add( zip_file, file)

Case 1_b:

  zipfile.add(
    file.sub(directory, '').sub!(/^\//, ''),
    file)

Case 2 : If the String contains lots of parameters , I prefer write in an array.But I'm not sure to have a line break behind [ is better, or not

  def run_remote_focus()

    self.setup_remote_focus()
    created_at=(Time.now.to_f*1000000).to_i.to_s

    settings=[
      "-i #{params[:remote_focu][:ip]}",
      "-u #{params[:remote_focu][:username]}",
      "-p #{params[:remote_focu][:password]}",
      "-s #{params[:RemoteFocu].values.join(':')}",
      "-m #{params[:af_type]}",
      "-t #{params[:remote_focu][:tester]}",
      "-n #{@remote_focu.id}",
      "-c #{created_at}"
    ]

    cmd=["ruby",
         "-I #{@rf[:lib]}",
         "#{@rf[:bin]}",
         " #{settings.join(' ')}"
         ]
    cmd_str=" #{cmd.join(' ').strip()} & "

Case 3 : I can not tell what is the problem, but it just seems weird

@rf={
     folder: "#{Rails.root}#{ENV["module_remote_focus_folder_path"]}",
     bin: "#{Rails.root}#{ENV["module_remote_focus_bin_path"]}",
     lib: "#{Rails.root}#{ENV["module_remote_focus_lib_path"]}"
     }

@rf={folder: "#{Rails.root}#{ENV["module_remote_focus_folder_path"]}",
     bin: "#{Rails.root}#{ENV["module_remote_focus_bin_path"]}",
     lib: "#{Rails.root}#{ENV["module_remote_focus_lib_path"]}"}

Case 1_a. Don't mix pure functions (sub) with destructive functions (sub!). Whenever possible use pure methods:

zip_file = file.sub(directory, '').sub(/^\//, '')
zipfile.add(zip_file, file)

But can't you write simply?

zip_file = File.basename(file)

Case 1_b: I prefer to set variables. Giving names to things is good.

Case 2 : It's ok. I also prefer to write a comma at the last element so it can be freely reordered. I'd also change the structure of settings so it's easier to write:

settings = [
  ["-i", params[:remote_focu][:ip]],
  ["-u", params[:remote_focu][:username]],
  ...
].map { |opt, value| [opt, value].join(" ") }

Case 3 : Use the same indentation style that you used for array. Also, use the library facilities, don't manipulate paths as strings (extremely prone to bugs):

@rf = {
  folder: File.join(Rails.root, ENV["module_remote_focus_folder_path"]),
  ...
}


Moving from normal threads to ExecutorService thread pools in java I had my original threading code which worked well, but since my tasks were shortlived, I decided to use thread pools through ExecutorService.

This was my original code

public class MyRun implements Runnable
{
    private Socket socket = null;
    public MyRun(Socket s)
    {
        socket = s;
        thread = new Thread(this, "SocketThread");
        thread.start();
    }
    public void run()
    {
        // My actual thread code
    }
}

My main program

...
ss = new ServerSocket(port);
....
MyRun st = null;
while (!stop)
{
    st = new MyRun(ss.accept());
    st = null;
}

New code

public MyRun(Socket s)
{
    socket = s;
    thread = new Thread(this, "SocketThread");
}

run() left unchanged

Changed Main program

private static ExecutorService execService = Executors.newCachedThreadPool();
....
....
while (!stop)
{
    execService.execute(new MyRun(ss.accept()));
}

Changed code seems to be working fine, but I just want to make sure there is nothing I am missing. I want all threads to execute simultaneously.

A few simple remarks :

thread = new Thread(this, "SocketThread"); is no longer needed in MyRun, since the ExecutorService is the one creating and managing the Threads.
you will want to call execService.shutDown() to properly clean up the resources of the executorService.


Using Factory pattern for parsing system I am currently trying to write a Parser system for my project. There are different type of files to be parsed (file1, file2, file3):

file1 -> AData // stored in AData class using AParser class's parsing logic
file2 -> BData // stored in BData class using BParser class's parsing logic
file3 -> CData // stored in CData class using BParser class's parsing logic

Files could be binary or txt. Different files require different logic for parsing because of the way file are written.

I have used a factory pattern for this purpose. The Base class is Parser which is an abstract class.

#include <fstream>
#include <iostream>
#include <memory>
#include <string>

// Base class
template <class T>
class Parser {
   public:
    virtual void DoParsing(T&, std::ifstream& fs) = 0;
};

// Base class for data
struct Data {};
//////
struct AData : Data {
    int data;
};

class AParser final : public Parser<AData> {
   public:
    void DoParsing(AData& data, std::ifstream& fs) {
    // implementation goes here
    }
};
///
struct BData : Data {
    char* data;
};

class BParser final : public Parser<BData> {
   public:
    void DoParsing(BData& data, std::ifstream& fs) {
    // implementation goes here
    }
};

template <class T>
class IParsing {
   public:
    void action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs) {
        p->DoParsing(d, fs);
    }
};

class FParsing {
   public:
    FParsing() {}

    void HandleParsing(std::string type, Data& d, std::ifstream& fs) {
      if (type == "AParsing") {
        std::shared_ptr<IParsing<AData>> iparse =
        std::make_shared<IParsing<AData>>();
        iparse->action(std::make_shared<AParser>(), static_cast<AData&>(d),
               fs);
      } else if (type == "BParsing") {
        // iparse->action(std::make_shared<BParser>(), fs);
        std::shared_ptr<IParsing<BData>> iparse =
        std::make_shared<IParsing<BData>>();
        iparse->action(std::make_shared<BParser>(), static_cast<BData&>(d),
               fs);

      } else {
        std::cout << "Need shape\n";
      }
    }

   private:
};

int main() {
    std::ifstream iFile("data.txt");
    FParsing fparse;
    //AData is passed by ref because
    // it will be populated during parsing process
    AData ad;
    fparse.HandleParsing("AParsing", ad, iFile);

    // BData
    BData ab;
    fparse.HandleParsing("BParsing", ab, iFile);
}

My questions are:

Do you think this is a right approach for creating a parsing system?
Is the factory pattern implementation correct?
I just wanted to make sure I am not making things more complex than it should be.
Are there other design patterns better than factory pattern for this purpose?

Design issues

You are unnecessarily restricting the Parser interface by forcing fs to be a std::ifstream& when std::istream& would work as well.
AParser::DoParsing(AData&, std::ifstream&) should be marked override. Same for BParser::DoParsing(BData&, std::ifstream&).
I can't see any specific purpose for IParsing: Why not call AParser::DoParsing or BParser::DoParsing directly?
Besides, parameter d of IParsing::action(std::shared_ptr<Parser<T>> p, T d, std::ifstream& fs) should be of type T&.
FParsing::HandleParsing(std::string, Data&, std::ifstream&) doesn't use any class instance members. Maybe make it static?
Also, parameter type from that function should probably be an enum - no need to use strings for that.
Why allocate two std::shared_ptr in case a valid parser is found if they won't be shared and will be deallocated once the function returns?
What advantage do you hope to get from this extravagant implementation over overloading operator>>(std::istream&, AData&) and operator>>(std::istream&, BData&) instead?


Can my router code be improved? I have created a nice url structure for my site.

In my .htaccess I have:

FallBackResource /index.php

And in my router class I have:

class init {

function __construct($url)
{
    $URLElements = explode('/', $url) ; // Adjust if needed.

    if($url)
    {
        $class = $URLElements[0] ;

        if(substr_count($url, '/') == 1)
        {
            $method = $URLElements[1] ;
        }

    }

    if(($t = substr_count($url, '/')) > 1)
    {
        for($i=2;$i<$t+1;$i++) {
            echo $URLElements[$i].'<br />';
        }
    }

}

}

Is this ok and what are your thoughts (how can it be improved etc)?

There's not really much to review here, but a few quick items:

This is going to sound weird at first glance, but you have a route() method that takes a path instead of depending directly on the request uri.

A router's job is to route.  By hard coding the $_SERVER['REQUEST_URI'], you've made it limited to one data source.
route($path) would be much more flexible, and it would mean that one object could be used for routing more than 1 time.
Think about test driven design.  How would you test a hard coded $_SERVER['REQUEST_URI']?  (Other than putting a hacky assignment to it somewhere)

Never assume that array indexes exist

If someone goes to index.php or /foo, so on, then $URLElements might not have 2 elements

As it is, your class does nothing useful -- you probably know that though :)

A major improvement would be making it actually do something :)


Optimize Performance of Region Checking in List Comprehension I am modeling a linear process at a number of equally spaced time steps. I have a large list (~70k elements) which corresponds to a location at each time step, timerange = np.linspace(0, time, iterations, False) * speed. For each element in this list, I want to compare its value (location) to two other lists and see if that location falls within a valid region. These two other lists, rot.RegionStarts and rot.RegionEnds (~150 elements) contains the start and end locations, respectively, of each region.
I began tackling this problem with list comprehension, using the following code to achieve the desired result.

valid = np.asarray([any((t * dt * speed >= rot.RegionStarts) & (t * dt * speed < rot.RegionEnds) for t in range(iterations)])

Upon using this, I realized that execution was rather slow, about 0.4s each time. As make several hundred calls to this line, this greatly slows down the process. I tried using numpy to achieve a similar effect, which required lots of reshaping and repeating.

valid = np.any(np.logical_and(
      np.greater_equal(np.repeat(np.reshape(timerange, (-1, 1)), rot.RegionStarts.shape[0], axis=1),
            np.repeat(np.reshape(rot.RegionStarts, (1, -1)), timerange.shape[0], axis=0)),
      np.less(np.repeat(np.reshape(timerange, (-1, 1)), rot.RegionEnds.shape[0], axis=1),
            np.repeat(np.reshape(rot.RegionEnds, (1, -1)), timerange.shape[0], axis=0)),
      axis = 1)

Unfortunately, this saw very little performance increase (~0.25s), probably due to the large size of arrays being repeated. Removing lines like np.repeat cause problems aligning shape; element-wise & could not be executed on 2-d arrays.
What optimization techiniques can be used speed up this code?

Loops over large arrays are not really a good idea in Python. This is why your original list comprehension is not terribly fast.

Your numpy version is loop free, but as far as I know, np.repeat actually makes copies of your data, which again, is really inefficient. An alternative would be to use np.tile, which maybe does not need to copy the data. But we don't really need to bother since numpy has a great feature called broadcasting, which often makes np.repeat/np.tile completely unneccessary. Broadcasting basically does np.repeat/tile automatically.

To evaluate the performance, I created a more abstract version of your list comprehension:

def get_valid_op(arr, lowers, uppers):
    return np.asarray([any((val >= lowers) & (val < uppers)) for val in arr])

and also a broadcasting version

def get_valid_arr(arr, lowers, uppers):
    valid = np.logical_and(arr.reshape(1, -1) >= lowers.reshape(-1, 1), arr.reshape(1, -1) < uppers.reshape(-1, 1))
    return valid.any(axis=0)

The second one is virtually the exact same algorithm as your repeat/reshape code.

With some test data modeled after your description above

arr = np.linspace(0, 1000, 70000)
starts = np.linspace(0, 150, 151) * 400
ends = starts + np.random.randint(0, 200, region_starts.shape)  # I assumed non-overlapping regions here

we can first assert all(get_valid_op(arr, starts, ends) == get_valid_arr(arr, starts, ends)) and then time:

%timeit -n 10 get_valid_op(arr, starts, ends)
511 ms ± 5.42 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

%timeit -n 10 get_valid_arr(arr, starts, ends)
37.8 ms ± 3.1 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

An order of magnitude faster. Not bad to begin with ;-)

Since working with large arrays (valid has a shape of (150, 70000) before reduction) also has a cost, I then took a step back and returned to loopy-land (just a little bit).

def get_valid_loop(arr, lowers, uppers):
    valid = np.zeros(arr.shape, dtype=bool)
    for start, end in zip(lowers, uppers):
        valid = np.logical_or(valid, np.logical_and(start <= arr, arr < end))
    return valid

In contrast to your list comprehension, this version now only iterates over the shorter region limit vectors, which means about two orders of magnitude fewer iterations.

We can then again assert all(get_valid_op(arr, starts, ends) == get_valid_loop(arr, starts, ends)) and time it:

%timeit -n 10 get_valid_loop(arr, starts, ends)
18.1 ms ± 865 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

As the results show, this version is even faster on my "synthetic" benchmark inputs.

In the end you will have to check the versions in your application and see which one performs best.


Word frequency in a large text file I've am trying to read a large text file and output the distinct words in it along with it's count. I've tried a couple of attempts so far, and this is by far the fastest solution I have come up with.

private static readonly char[] separators = { ' ' };

public IDictionary<string, int> Parse(string path)
{
    var wordCount = new Dictionary<string, int>();

    using (var fileStream = File.Open(path, FileMode.Open, FileAccess.Read))
    using (var streamReader = new StreamReader(fileStream))
    {
        string line;
        while ((line = streamReader.ReadLine()) != null)
        {
            var words = line.Split(separators, StringSplitOptions.RemoveEmptyEntries);

            foreach (var word in words)
            {
                if (wordCount.ContainsKey(word))
                {
                    wordCount[word] = wordCount[word] + 1;
                }
                else
                {
                    wordCount.Add(word, 1);
                }
            }
        }
    }

    return wordCount;
}

How I am measuring my solution

I have a 200MB text, which I know the total word count for (via a text editor). I'm using the Stopwatch class and counting the words to ensure accuracy and measuring the time taken. So far, it is taking around 9 seconds.

Other attempts

I have tried to utilise multithreading to split out the work via the TPL library. This involved batching multiple lines, sending out the processing of the batch of lines to a separate task and locking the read/write operations in the dictionary. This however, seems to not provide me any performance improvements.

It took around 30 seconds. I suspect the locking to read/write to the dictionary is too costly to gain any performance.I am sure there is a faster way to achieve this! Any suggestions/criticisms to my solution are welcome.

Here is the link to the test file I'm using.

Let's set up code to benchmark different approaches. Every word counter will implement this interface:

interface IWordCounter
{
    IDictionary<string, int> CountWords(string path);
}

And here's our benchmark runner:

var wordCounters = new IWordCounter[]
{
    // ...
};

foreach (var wordCounter in wordCounters)
{
    GC.Collect();
    GC.WaitForPendingFinalizers();

    var sw = Stopwatch.StartNew();
    var wordCount = wordCounter.CountWords(path);
    sw.Stop();

    Console.WriteLine("{0}, {1} entries, {2}", wordCounter.GetType().Name, wordCount.Count, sw.Elapsed);
}

Timings were taken with a release build, on the test file provided, no debugger attached, on .NET 4.5.2.

Here's the original code:

class OriginalWordCounter : IWordCounter
{
    private static readonly char[] separators = { ' ' };

    public IDictionary<string, int> CountWords(string path)
    {
        var wordCount = new Dictionary<string, int>();

        using (var fileStream = File.Open(path, FileMode.Open, FileAccess.Read))
        using (var streamReader = new StreamReader(fileStream))
        {
            string line;
            while ((line = streamReader.ReadLine()) != null)
            {
                var words = line.Split(separators, StringSplitOptions.RemoveEmptyEntries);

                foreach (var word in words)
                {
                    if (wordCount.ContainsKey(word))
                    {
                        wordCount[word] = wordCount[word] + 1;
                    }
                    else
                    {
                        wordCount.Add(word, 1);
                    }
                }
            }
        }

        return wordCount;
    }
}

On my machine, this takes about 8.2s.

We see an improvement using Heslacher's suggestion to use TryGet:

class OriginalTryGetWordCounter : IWordCounter
{
    private static readonly char[] separators = { ' ' };

    public IDictionary<string, int> CountWords(string path)
    {
        var wordCount = new Dictionary<string, int>();

        foreach (var line in File.ReadLines(path, Encoding.UTF8))
        {
            var words = line.Split(separators, StringSplitOptions.RemoveEmptyEntries);
            foreach (var word in words)
            {
                int count;
                wordCount.TryGetValue(word, out count);
                wordCount[word] = count + 1;
            }
        }

        return wordCount;
    }
}

This takes about 6.7s. (The use of File.ReadLines here doesn't seem to effect the timing, it's just a bit cleaner.)

We get another improvement with Parallel.ForEach together with a ConcurrentDictionary:

class ParallelWordCounter : IWordCounter
{
    public IDictionary<string, int> CountWords(string path)
    {
        var result = new ConcurrentDictionary<string, int>();
        Parallel.ForEach(File.ReadLines(path, Encoding.UTF8), line =>
        {
            var words = line.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
            foreach (var word in words)
            {
                result.AddOrUpdate(word, 1, (_, x) => x + 1);
            }
        });

        return result;
    }
}

This takes about 5.2s.

You might want to try some of the Parallel.Foreach overloads to see if you can get any further improvements, and remember to take these results with a grain of salt.


Converting string array to string (and vice-versa) for storing via XML I'm using XML to store data and I need to store a string array in it. The best way (that I can think of) to store a string array is by converting it to a single string. In order to do that, I'm using string.Join to join the strings with \0 and then use Base64 so that the \0 (and any other special character) can be retrieved easier. Because it's possible that a string in a array already has \0 in it (causing another string to be the array when they're converted back to an array), I'm escaping the existing \0 by first adding another backslash to existing backslashes and changing \0 to \\0. Is there any better way to do this? Should I be using \r\n instead (but wouldn't that also be the same as using \0)? I should also note that its so I can use IXmlSerializable and it is also storing byte[], uint, ulong and string in the XML as well.

Retrieving String Array (Via XML)

this.Name = reader.GetAttribute("Name");

string strType = reader.GetAttribute("Type");

byte[] buffer = new byte[1000];
int readBytes = 0;
MemoryStream ms = new MemoryStream();
BinaryWriter bw = new BinaryWriter(ms);
while ((readBytes = reader.ReadElementContentAsBase64(buffer, 0, 50)) > 0)
{
    bw.Write(buffer, 0, readBytes);
}

bw.Close();

List<string> unescapedStrs = new List<string>();
string[] val = strValue.Split('\0');
string newStr;

foreach (string s in val)
{
    newStr = Regex.Replace(s, @"([^\\])(\\0)", "$1\0").Replace("\\\\", "\\");

    unescapedStrs.Add(newStr);
}

if (val.Length > 0)
    this.Value = unescapedStrs.ToArray();

Storing String Array (Via XML)

writer.WriteAttributeString("Name", this.Name);

writer.WriteAttributeString("Type", this.Type);

string strValue = string.Empty;

string[] val = (string[])this.Value;

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

foreach (string s in val)
{
    string newStr = s;

    // Escape backslashes so existing \\0 don't get mixed up
    if (newStr.IndexOf('\\') > 0)
        newStr = newStr.Replace("\\", "\\\\");

    // If theres NULL chars in strings already, escape them
    if (newStr.IndexOf('\0') > 0)
        newStr = newStr.Replace("\0", "\\0");

    escapedStrs.Add(newStr);
}

strValue = string.Join("\0", escapedStrs.ToArray());

int byteLen = Encoding.Unicode.GetByteCount(strValue);
byte[] bytes = Encoding.Unicode.GetBytes(strValue);
writer.WriteBase64(bytes, 0, byteLen);

My first reaction is: don't do this.

By creating your own encoding for an array of strings, your XML is not going to be (easily) consumable by other programs. By Base64 encoding your strings, your XML is not going to be human-readable.

These are two major benefits of XML, and if you're willing to sacrifice them, maybe XML is not the right format. If the serialized data is only going to be consumed by the same program, binary serialization would be a better option.

That said, if you want to serialize a list of strings to XML, this is what I would recommend:

public class Email
{
    private static readonly XmlSerializer Serializer = new XmlSerializer(typeof(Email));

    [XmlArrayItem("Attachment")]
    public List<string> Attachments { get; set; }

    public XDocument ToXDocument()
    {
        var document = new XDocument();
        using (var writer = document.CreateWriter())
        {
            Serializer.Serialize(writer, this);
        }

        return document;
    }
}

And some sample client code:

var email = new Email
{
    Attachments = new List<string> { "<hello></hello>", "asdf" }
};
var xml = email.ToXDocument().ToString();
Console.WriteLine(xml);

This will output

<Email xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
  <Attachments>
    <Attachment>&lt;hello&gt;&lt;/hello&gt;</Attachment>
    <Attachment>asdf</Attachment>
  </Attachments>
</Email>

Now, this chokes if the strings contain a null-byte, which you mentioned was a possibility. It's hard to say what to do about this without knowing more about your problem domain. Hopefully, stripping null-bytes is an option for you.

Serializing a byte[] as a Base64-encoded string can be done using a surrogate property:

[XmlIgnore]
public byte[] Bytes { get; set; }

public string BytesBase64
{
    get { return Convert.ToBase64String(this.Bytes); }
    set { this.Bytes = Convert.FromBase64String(value); }
}


Introduce bindings for macro user I'm doing the Racket track on exercism.io and solved the grains exercise:

  Write a program that calculates the number of grains of wheat on a chessboard given that the number on each square doubles.

(test cases etc on exercism)

So I wrote a straightforward recursive solution:

#lang racket

(provide square total)

(define (square n)
  (cond
    [(n . = . 1)    1]
    [(n . > . 1)    (* 2 (square (- n 1)))]
    [else           (error "invalid number:" n)]))

(define (sum n)
  (cond
    [(n . = . 1)    1]
    [(n . > . 1)    (+ (square n) (sum (- n 1)))]
    [else           (error "invalid number:" n)]))

(define (total)
  (sum 64))

Then I went to clean it up by factoring out the induction:

#lang racket

(provide square total)

(define-syntax (induction stx)
  (syntax-case stx ()
    ((induction base rule)
     (with-syntax ((n (datum->syntax stx 'n)))
       #'(lambda (n)
           (cond
             ((n . = . 1)   base)
             ((n . > . 1)   rule)
             (else          (error "invalid number:" n))))))))

(define square  (induction 1 (* 2 (square (- n 1)))))
(define sum     (induction 1 (+ (square n) (sum (- n 1)))))

(define (total)
  (sum 64))

I got a couple of questions:

1) Is that the most straightforward way to introduce a new binding for the macro user?

Like, this is quite wordy. I guess I could wrap the "with-syntax" part into another macro, 'cause right now this feels very "built out of raw plumbing" to me.

It's also annoying that "define-syntax-rule" doesn't give me "stx", so I don't see a way to introduce new bindings. I'd like to write something like:

(define-syntax-rule (induction base rule)
  (syntax-let ((n))
    (lambda (n)
      (cond
        ((n . = . 1)    base)
        ((n . > . 1)    rule)
        (else           (error "invalid number:"))))))

Is there anything built-in for that?

2) For argument clarity, I'd like to write it more like this:

(define (square n)
  (induction 1 (* 2 (square (- n 1)))))

How'd I do that? Like, I'd... have to capture the "n" argument correctly (how?), and return, uh... the body of the lambda? I'm confused.

When writing macros, it is best to avoid using functions like datum->syntax to conjure up identifiers. Instead, your macro should take the identifiers to bind from the macro use site.

For your example, I would write it like this:

#lang racket

(provide square total)

;; note how `n` is passed in as an argument
(define-syntax-rule (induction n base rule)
  (lambda (n)
    (cond [(n . = . 1) base]
          [(n . > . 1) rule]
          [else        (error "invalid number:" n)])))

(define square (induction n 1 (* 2 (square (- n 1)))))
(define sum    (induction n 1 (+ (square n) (sum (- n 1)))))

(define (total)
  (sum 64))

Macros written in this style are more composable and cooperate with macro hygiene. Racket's macro system lets you break this hygiene in a controlled way, but that is usually a bad idea.

Also, for a simple program like this I would usually not bother writing a macro. In this case, you could even have abstracted this using only first-class functions instead of macros.

You may also wish to read Macros and Languages in Racket and Fear of Macros if you haven't already.


Graph representation using adjacency list for BFS, DFS, and Karger's algorithm I want to implement some graph algorithms for learning purposes (e.g BFS, DFS, Karger's min-cut algo) and before diving into them I want to make sure I have got the graph representation right (using an adjacency list). I want to keep things minimum to exactly what I need to implement these algorithms on undirected graphs for the time being.

Here's my Vertex class:

public final class Vertex {
    private final int label;
    private VertexColor color = VertexColor.WHITE;
    private int distance;
    private Vertex parent;

    public Vertex(int label, int distance, Vertex parent) {
        this(label);
        this.distance = distance;
        this.parent = parent;
    }

    public Vertex(int label) {
        this.label = label;
    }

    public int getLabel() {
        return label;
    }

    public VertexColor getColor() {
        return color;
    }

    public void setColor(VertexColor color) {
        this.color = color;
    }

    public Vertex getParent() {
        return parent;
    }

    public void setParent(Vertex parent) {
        this.parent = parent;
    }

    public int getDistance() {
        return distance;
    }

    public void setDistance(int distance) {
        this.distance = distance;
    }

    @Override
    public String toString() {
        return "Vertex{" +
                "label=" + label +
                ", color=" + color +
                ", distance=" + distance +
                ", parent=" + parent +
                '}';
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;

        Vertex vertex = (Vertex) o;

        if (label != vertex.label) return false;
        if (distance != vertex.distance) return false;
        if (color != vertex.color) return false;
        return !(parent != null ? !parent.equals(vertex.parent) : vertex.parent != null);

    }

    @Override
    public int hashCode() {
        int result = label;
        result = 31 * result + (color != null ? color.hashCode() : 0);
        result = 31 * result + distance;
        result = 31 * result + (parent != null ? parent.hashCode() : 0);
        return result;
    }
}

My VertexColor enum:

public enum VertexColor {
    WHITE, GREY, BLACK
}

My adjacency list implementation:

public class SimpleAdjacencyList {
    private Map<Integer, List<Vertex>> adjacencyList = new HashMap<>();

    /**
     * Initializes a new graph from a file.
     * @param file format should be:
     * 1    2   4
     * 2    3   1
     * 3    2   4
     * 4    3   1
     */
    public SimpleAdjacencyList(String file) {
        String line;
        try {
            BufferedReader br = new BufferedReader(new FileReader(file));
            while ((line = br.readLine()) != null) {
                String[] numbers = line.trim().split("(\\s)+");
                int key = Integer.parseInt(numbers[0]);
                List<Vertex> vertices = new ArrayList<>(numbers.length);
                Vertex parent = new Vertex(key);
                vertices.add(parent);
                for(int i = 1; i < numbers.length; i++){
                    int label = Integer.parseInt(numbers[i]);
                    vertices.add(new Vertex(label, 0, null));
                }
                adjacencyList.put(key, vertices);
            }
        } catch (IOException ex) {
            ex.printStackTrace();
        }
    }

    @Override
    public String toString() {
        return "SimpleAdjacencyList{" +
                "adjacencyList=" + adjacencyList +
                '}';
    }

    public Map<Integer, List<Vertex>> getAdjacencyList() {
        return adjacencyList;
    }

    // TODO: remove this
    public static void main(String[] args) {
        SimpleAdjacencyList list = new SimpleAdjacencyList("/Users/orestis/Documents/test.txt");
        System.out.println(list);
    }
}

In general your code is good. There are some small issues I dislike, and there's an issue with the Adjacency list that's small too. Basically, if this was a real review, I would probably say: fine - but small things to work on next time.

Constructors

I have no reference for this, but I much prefer convencience constructors to call canonical ones. In your code, you have... hmmm, you don't have an actual canonical constructor - you can't construct with a VertextColor:

public Vertex(int label, int distance, Vertex parent) {
    this(label);
    this.distance = distance;
    this.parent = parent;
}

public Vertex(int label) {
    this.label = label;
}

Here, you have a canonical constructor (int, int, Vertex) calling a convenience one (Vertex). I would prefer the opposite, and have code like:

public Vertex(int label, int distance, Vertex parent, VertexColor color) {
    this.label = label;
    this.distance = distance;
    this.parent = parent;
    this.color = color;
}

public Vertex(int label, int distance, Vertex parent) {
    this(label, distance, parent, VertexColor.WHITE);
}

public Vertex(int label) {
    this(label, 0, null, VertexColor.WHITE);
}

This way it makes it obvious what is what, and also, the actual fields are only set in a single constructor too.... Additionally, it opens up an easier way to make the fields final, if you wanted to.

Finally, I don't like code that relies on using the not-supplied default values of declared fields, and the Canonical constructor solves that (what I mean here, is that your current constructor Vertex(int label) does not set any other fields, but relies on the default values for private int distance; and private Vertex parent;.

toStrings

Your toStrings are using string concatenations. This is not horrible, but I would prefer to see string formatting.

This code:

public String toString() {
    return "Vertex{" +
            "label=" + label +
            ", color=" + color +
            ", distance=" + distance +
            ", parent=" + parent +
            '}';
}

would be better as:

public String toString() {
    return String.format("Vertex{label=%d, color=%s, distance=%d, parent=%s}",
        label, color, distance, parent);
}

equals & hashCode

Your two methods appear to follow the equals/hashCode contract, which is great. I would suggest two changes though in each.... in the equals, you have:

public boolean equals(Object o) {
    if (this == o) return true;
    if (o == null || getClass() != o.getClass()) return false;

    Vertex vertex = (Vertex) o;

    if (label != vertex.label) return false;
    if (distance != vertex.distance) return false;
    if (color != vertex.color) return false;
    return !(parent != null ? !parent.equals(vertex.parent) : vertex.parent != null);

}

You should use instanceof instead of o == null || getClass() != o.getClass(). instanceof will return false for a null value, and, it is more clear what its intent is. Additionally, you should use Objects.equals(a,b). Your code could be:

public boolean equals(Object o) {
    if (this == o) {
        return true;
    }
    if (!(o instanceof Vertex)) {
        return false;
    }

    Vertex vertex = (Vertex)o;

    return label == vertex.label
        && distance == vertex.distance
        && color == vertex.color
        && Objects.equals(parent, vertex.parent);
}

In the hashCode method, I suspect this is auto-generated code. It's OK, but I thought there would be a way to integrate Objects.deepHashCode(...) but there may not be.

AdjacencyList

This code looks fine too. The one comment here is that I would prefer to see a more batch-oriented integer conversion of each line.

I would have a method like:

private static int[] lineToInts(String line) {
    String[] parts = line.split("\\s+");
    int[] numbers = new int[parts.length];
    for (int i = 0; i < parts.length; i++) {
        numbers[i] = Integer.parseInt(parts[i]);
    }
    return numbers;
}

If you have Java 8, I would make that:

private static int[] lineToInts(String line) {
    return Stream.of(line.split("\\s+"))
           .mapToInt(Integer::parseInt)
           .toArray();
}

Then you can simplify the other code...

Also, in this class, use printf for the toString...


Compose valid subtractions using all digits from a set I have created a brainteaser, which I called the "impossible problem" (see this video I made).  The aim is to create a valid 3-digit subtraction which uses every digit from 1 to 9 once. Here is an example of one of the solutions: 873-254=619.

After finding it hard to come up with answers I created this program. What it basically does is iterate through every possible 3-digit subtraction and if it finds one which fits the criteria, it prints it.

from math import trunc

def solver(number,number2, numberofdigits):
    seq = str(number),str(number2), str(number - number2)
    digits = "".join(seq)
    goodChecks = 0
    count= numberofdigits/3
    for i in range(1,10):
        if digits.count(str(i)) == count:
            goodChecks += 1
    if goodChecks == 9:
        return digits
    else:
        return False

middlenumber =[]
successes = 0
num_of_digits = int(input("please enter a number of digits, which is a multiple of 3"))
if num_of_digits == 3:
    minY = 381
    maxY = 987
if num_of_digits == 6:
    minY =246912
    maxY = 998877

if num_of_digits == 3:
    minX = 123
if num_of_digits == 6:
    minX =123123

for y in range(minY, maxY+1):
    numberlist = []
    if y%100 == 0:
        print(y)
    for x in range(minX,trunc(y/2)):
        digits = solver(y,x,num_of_digits)
        if digits is not False:
            successes += 2
            print(digits)
            numberlist.extend([x,y-x])
            middlenumber.extend([x, y-x])

print("")
print("I found: ", successes, " successful solution to your brainteaser")
if successes < 20:
    print("there were almost no solutions")
elif successes < 100:
    print("there were not many solutions")
elif successes < 1000:
    print("there were more than a hundred solutions it is definitely not impossible :)")
else:
    print("that's a lot of successes")

print("There were ", len(middlenumber) - len(set(middlenumber)) , " duplicates, by the way :)")

My program worked quite well but then I decided could you do the same for 6 digits? For the 3 digit problem the program had 96 possiblities to iterate over. Which is a measly 531,441 iterations. However for the 6 digit there is 912 possiblities, which is a colossal 282,429,536,481 iterations. This is going to take my computer days to solve.

I have already tried optimizing my program but I can't figure out how to do it any faster.

first of all try use middlenumber as set. Than adding numbers no by extends but something as middlenumber.add(x < y - x and (x, y-x) or (y-x, x)). You have strange duplicates number
naive brute force algorithm use 9^6 checking as you mentioned
something better when you change to permutations (9!/3! checking). There example for simpler 3-digit version

from itertools import permutations

def permutations_solver():
    digits = set('987654321')
    numbers = (''.join(x) for x in permutations(digits, 6))
    for x in numbers:
        a = int(x[0 : 3])
        b = int(x[3 : 6])
        c = a - b
        if b > c and c > 0 and set(str(c) + x) == digits:
            yield a, b, c

print(len(list(permutations_solver())))

checking b > c and c > 0 and set(str(c) + x) == digits above still is very ineffective
you can try write iterator on digits, something as

"""           set('abcdefgh') == set('123456789') and set('zxyr') == set('01')
zxyr          abc < def (enough a < d)
 abc          z == 0
+def          x + a + d == g + 10 * z
----          y + b + e == h + 10 * x
 ghi          r + c + f == i + 10 * y
 zxy          r == 0 // it looks like logic programming problem
"""
from itertools import product

def digits_solver():
    column = [(x, a, d, (x + a + d) % 10, (x + a + d) // 10)
              for x, a, d in product(range(2), range(1, 10), range(1, 10))
              if len(set([a, d, (x + a + d) % 10])) == 3 and (x + a + d) % 10 != 0]
    first_column = [(x, a, d, g) for x, a, d, g, z in column if z == 0 and a < d]
    for x, a, d, g in first_column:
        second_column = [(y, b, e, h) for y, b, e, h, xx in column if xx == x and not set([a,d,g]) & set([b,e,h])]
        for y, b, e, h in second_column:
            third_column = [(r, c, f, i) for r, c, f, i, yy in column
                            if yy == y and not set([a,d,g,b,e,h]) & set([c,f,i]) and r == 0 ]
            for _, c, f, i  in third_column:
                yield (100 * g + 10 * h + i, 100 * d + 10 * e + f, 100 * a + 10 * b + c)

in above example for by for by for is bad idea, you can remove it by recursion. The code will be much less messy
if you want try 6-digits number, you can change check unique digits (using set at examples) to multiset (at python you can use Counter class from collections library to emulate it)


Counting repetitions in a file I've written this code to get input from a file and count the repetition of number of that file and give output to another file. How can I make this code better?

#include <iostream>
#include <cmath>
#include <fstream>

using namespace std;

int main()
{
    ofstream outputFile("outPutData.txt");
    ifstream theFile ("inputData.txt");
    int MaxRange= 50;

    int myint[100]={0};

    int mycompare[90]={0};

    int mycount[90] = {0};

    int i = 0, j = 0, k = 0, sum = 0;

    for(j=0;j <= MaxRange;j++){
        mycompare[j] = j;
    }

    do
    {
        theFile>>myint[i];

        for(j=0;j<=MaxRange;j++)
            for(k=0;k<=MaxRange;k++)
            {

                j = k;
                if(myint[i] == mycompare[j])
                    mycount[k] = mycount[k]+1;
            }
        i++;

    }
    while((myint[i-1] >=0) && (myint[i-1] <= MaxRange));

    cout<< "The Number\t\t Frequency"<<endl;
    cout<<"------\t\t------"<<endl;

    for(k=0; k <=MaxRange ;k++)
    {

        if(mycount[k] != 0)
        {
            outputFile<<k<<"  "<<mycount[k]<<endl;
            cout<< k<<"\t\t"<<mycount[k]<<endl;

            sum = sum + mycount[k];
        }
    }

    cout<<"Total input from file  = "<<sum<<endl;

    return 0;
}

If I have the series of number in my input file like this:

9
9
9
6
6
5
3
7
9
1
2

It gives the output:

The Number  Frequency
---------   --------
1             1
2             1
3             1
5             1
6             2
7             1
9             4

Please do not do this:

using namespace std;

I know every stupid shitty book about C++ does this. But don't. It works fine if your code is 10 lines long but anything larger it becomes a problem. So it is a really bad habit to start and breaking habits is hard so don't start it. Do yourself a favour and get into the habit of not using it. See Why is “using namespace std;” considered bad practice? for details.

This looks like it should be a constant:

int MaxRange= 50;

You should make it const. Then you can also use it as the size of the arrays below.

Declare one variable per line:

int i = 0, j = 0, k = 0, sum = 0;

The is one corner case where it does not do what you expect. But that's not the real reason for not doing this. Readability. And declare the variable as close to the point of use as possible. No point in declaring a variable that is not used until the bottom line of a program. Also loop variables should be declared as part of the loop so there affect does not bleed beyond the loop.

for(int loop = 0; loop < 5; ++loop)
{
}
// loop not valid here.

Also use better names. If you have a loop that covers multiple lines. Try and find all uses of the variable i. Your editor is going to hit a bunch of words in comments and other things. Which is a real pain when you are trying to fix code.

Why do you only initialize half of the mycompare. It may be valid but a comment on why would definately be in order.

int MaxRange= 50;
int mycompare[90]={0};
for(j=0;j <= MaxRange;j++){
    mycompare[j] = j;
}

After reading the code it seems that mycompare is not really needed.
Even in your loop to print out the values you don't use it.

This is an anti pattern for reading from a stream.
What happens if the read (operator>>) fails?

do
{
    theFile>>myint[i];
    // STUFF
}
while((myint[i-1] >=0) && (myint[i-1] <= MaxRange));

The correct patter for reading from a file is:
Note: You will find that nearly all programming languages work this way. It is a VERY common pattern. Put the read as part of the loop conditional to validate the read worked.

while(theFile>>myint[i])
{
    // Read worked so do work.
}

You can combine your other conditions:

while((theFile>>myint[i]) && (myint[i] >=0) && (myint[i] <= MaxRange))
{
    // Read worked and value in the correct range.
}

Try to always use {} around block statements. It will save your butt one day.

    for(j=0;j<=MaxRange;j++)
        for(k=0;k<=MaxRange;k++)

Prefer

    for(j=0;j<=MaxRange;j++)
    {
        for(k=0;k<=MaxRange;k++)
        {
        }
    }

Not sure what the outer loop is doing here.

    for(j=0;j<=MaxRange;j++)              // This line seems redundant.
        for(k=0;k<=MaxRange;k++)
        {
            j = k;                        // because of this line.
            if(myint[i] == mycompare[j])
                mycount[k] = mycount[k]+1;
        }

You may as well remove the outer loop.

Prefer not to use std::endl to mark end of line. Perfer '\n'. The difference is that std::endl puts the '\n' character on the stream then flushes the stream. If you don't need the flush do not use it. It makes the stream much more efficient not to use it.

cout<< "The Number\t\t Frequency"<<endl;
cout<<"------\t\t------"<<endl;

In C++ the return in main() is optional.
If you only return 0 then I would encourage you not put the return in the code. It is an indication that the program can not logically fail.

return 0;

Conclusion:

You need to learn how to use the built in types. This problem becomes trivial to do by using the std::map.

int main()
{
    std::map<int,int>   data;
    int  tmp;

    // Read the data
    ifstream theFile ("inputData.txt");
    while(theFile >> tmp)
    {
        ++data[tmp];
    }

    // Write the data
    for(auto loop = data.begin(); loop != data.end();++loop)
    {
         std::cout << loop->first << "\t\t" << loop->second << "\n";
    }
}


A closable footer bar with a floating info box I wrote this page with the goal in mind of having a footer that runs along the bottom of the page that can be closed, and has a floating info box that appears when the footer is hovered over. I'm just learning JS, HTML and CSS now, so I'd appreciate any critiques on style.

Am I doing anything the long way? Is any of the JavaScript un-idiomatic? Please let me know!

What I'm not looking for advice on:

I know that external CSS and JS would likely be preferable. I'm using internal JS and CSS to ease learning.

This is a very broad review request. I'd like opinions on any aspect of any of the languages used.

FooterTest.html:

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Footer Test</title>
    <style>
        body {
            background-color: lightseagreen;
        }

        #floatingBox {
            display: none;
            position: fixed;

            background-color: black;
            color: white;

            left: 0px;
            top: 0px;

            width: 50px;
            height: 50px;

            z-index: 1;
        }

        #fixedFooter {
            display: block;
            position: fixed;
            background-color: bisque;
            color: black;

            left: 0px;

            border-top: 10px double black;
            bottom: 0px;

            height: 20%;
            width: 100%;

            text-align: center;

            z-index: 0;
        }

        #closeButton {
            display: inline-block;
            position: absolute;

            right: 0px;
            top: 0px;

            color: white;
            background-color: red;

            width: 5%;
            height: auto;

            z-index: 1;
        }
    </style>
</head>
<body>
    <div id="floatingBox">
        Test Block
    </div>

    <footer id="fixedFooter">
        <button id="closeButton">X</button>
    </footer>
    <script>
        var fixedFooter = document.getElementById("fixedFooter");
        var positionText = document.createElement('p');
        var closeButton = document.getElementById("closeButton");

        fixedFooter.appendChild(positionText);

        var floatingBox = document.getElementById("floatingBox");

        var defaultDisplay = "inline-block";

        //To prevent the mouse from entering the floating box
        var posBuffer = 10;

        fixedFooter.onmousemove = function(e) {
            var x = e.pageX;
            var y = e.pageY;

            positionText.innerHTML = formatPosition(x, y) + " - "
                    + formatPosition(floatingBox.style.left, floatingBox.style.top);

            setElementPosition(floatingBox, x + posBuffer, y + posBuffer);
        };

        fixedFooter.onmouseover = function(e) {
            floatingBox.style.display = defaultDisplay;
            console.log("\tMouse In");
        };

        fixedFooter.onmouseout = function() {
            floatingBox.style.display = "none";
            //setElementPosition(floatingBox)
            console.log("\tMouse Out");
        };

        closeButton.onclick = function() {
            fixedFooter.style.display = "none";
        }

        var setElementPosition = function(element, xPos, yPos) {
            element.style.left = xPos + "px";
            element.style.top = yPos + "px";

            //console.log("Setting " + element + "'s position to " + formatPosition(xPos, yPos));
        };

        var formatPosition = function(x, y) {
            return "(" + x + ", " + y + ")";
        };

    </script>
</body>
</html>

OK here's a few pointers on the above code:

IDs in css are not a good idea. The problem is that an ID can only be over-ridden by an ID, inline style or !important declaration. It's pretty well established best practice to use classes rather than IDs as a way of hooking in your css. It's great you aren't using the HTML tags themselves as selectors.
It's very interesting how you've grouped your css properties. I think you've gone for logical groupings rather than the arbitrary alphabetical order that is preferred by many. This might be contentious, but I think it's better to make logical groups.
JS variables are not name-spaced. This is fine in tiny examples, but will be very problematic in larger sites. For example, the very generic closeButton variable in the JS could easily be over-written somewhere else, by someone else and cause a tricky and unnecessary bug.

The same applies with css and while we're here, separating out JS hooks from class attributes used for styling will help edits to styles effecting JS functionality on the page.

Consider this as an alternative:

var FooterPopup = {
  closeButton: document.getElementById('#js-footer-popup__close-button');
}

.footer-popup__close-button {
  /* styles */
}

Where FooterPopup.closeButton will access the close button within the context of the object and the page. You can now port the FooterPopup anywhere else and re-use it.

Note, the js- prefix makes it very clear that the hook is used by javascript. The double underscore is nicked from BEM notation and I've used it here to define a block and it's element in a way that is clear to read and shouldn't be hit anywhere else in the site and over-ridden.

Addition of hard-coded styles in the JS. You use the .style.display JS declaration to hide the element and close it. This is problematic because everywhere you want to add this functionality you'll have to directly define it. There are literally 100s of ways of closing something when you consider all the interface nuances designers might want to introduce so isolating this and making it re-usable is a good idea.

A way to do this is to use a utility class then add and remove that using JS. Then you define the closed style in css - recommended at the bottom of the file so the styles over-ride any that might be added to the element generally. Note that this has other benefits - if you wanted something to load on the page hidden you could just add this class to the element rather having to add all the properties copied from JS to the css for that element.

Here's a simple example:

.element {
  opacity: 1;
  top: 0;
  transition: all 0.2s;
}

.is-hidden {
  opacity: 0;
  top: -50px;
}

$('.element').addClass('is-hidden'); (I've used JQuery here - it's a robust, common library that takes some of the pain out of DOM manipulation, you may have considered it however and it's very informative and ultimately better, if a little harder to start with, to tackle the raw JS).

Here is the more complicated way to add and remove classes with just JS.

Finally, check out JSFiddle or CodePen. Here's a link to your code in JSFiddle: https://jsfiddle.net/ujLjr0kp/1/ - it's really useful front-end playground and very good for sharing code on Stack Exchange :-)


Does this code actually give a valid representation of how quick a system is? So I wrote this code a while ago as a way of seeing how many times it could compute a simple sum in a second (one tick). I was wondering if this gives me valid results or is in fact giving me bogus ones.

from datetime import datetime
from datetime import timedelta

start_time = datetime.now()

def millis():
   dt = datetime.now() - start_time
   ms = (dt.days * 24 * 60 * 60 + dt.seconds) * 1000 + dt.microseconds / 1000.0
   return ms
def tickscheck(start):
    x = 0
    count = 0
    while millis() - start < 1000:
        x = 4+5
        #counting up
        count = count + 1

    print("It Took " + str(count) + " Counts\nOver " + str(millis()- start) + "ticks")

running = True
while(running == True):
    tickscheck(millis())
    restart = input("Do You Want Restart?")
    restart = restart.lower()
    if restart in ("yes", "y", "ok", "sure", ""):
        print("Restarting")
    else:
        print("closing Down")
        running = False

No, your benchmark is completely bogus. Let's look at the main loop:

while millis() - start < 1000:
    x = 4+5
    #counting up
    count = count + 1

The addition 4+5 can be constant-folded when the code is compiled. If you are lucky, the assignment remains although a constant assignment might be lifted out of the loop.

But aren't we then still benchmarking the counter increment? Likely, no. The function call millis() in the loop condition is probably at least an order of magnitude more expensive. So essentially, you're benchmarking how fast you can check whether you're done with benchmarking.

The next problem is which time you are measuring. You are currently measuring the elapsed wall clock time (sometimes: “real” time), i.e. the time that you observed passing. However, this is not the same as the time your loop spent executing, because the operating system is free to interrupt execution to schedule other processes in between. The correct time to measure is the “user” time to see how much your program itself worked, and the “system” time to see how much the OS kernel worked on your program's behalf via system calls. The used CPU time is the sum of user and system time. This paragraph uses Unix terminology, but the problem is the same on Windows.

All of this is complicated, so don't do it yourself. Python has a timeit module where you can measure how long a function took to execute a specific number of times. The number of iterations per second is then the inverse of the time spent executing. To prevent the overhead of the function call from affecting the result, the timed function should include a loop itself, e.g. with a million iterations. To execute the body of the loop 10M times, you would then time the function ten times etc.

Edit: code example

Assuming Python 3.3 or later, we can use the time.process_time function which measures the used CPU time of our process.

import time
import timeit

n = 10000000 # 10M
timings = timeit.repeat(
    setup='x, y = 4, 5',
    stmt='z = x + y',
    timer=time.process_time,  # comment this line for pre-3.3 pythons
    number=n
)
best_time = max(timings)
repetitions_per_second = n/best_time
print("repetitions per second: %.2E" % repetitions_per_second)

Unfortunately I can't test it right now because I don't have Python 3.3 or later installed. So we might have to use a timer that uses wallclock time instead – this isn't so catastrophic because repeat will take three measurements, and we'll use the best. This does not eliminate the effect of scheduling, but it does minimize it.

Example ballpark results:

           3.2     2.7
Machine 1  <= 6E6  = 1E7
Machine 2  <= 2E7  > 2E7

So with a back of the envelope-calculation, we can say that machine 2 is roughly 2–3 times as fast as machine 1. This is consistent with their specs (desktop vs. low-end server).


A prime number sieve using recursion Out of curiosity, I built a recursive Sieve of Eratosthenes in Python and I was wondering how it could be improved for efficiency and how it can improved to be aligned with python best practices.

Essentially, there is a recursive function (coded horrendously IMO), filter_multiples, which removes the multiples of some number. Then there is a for loop which cycles through the list which needs to be filtered and uses each number in that list for the recursive function. After the numbers in the list runs out, the code stops and outputs the list which remains i.e. the list of primes. Below is the code:

# Generate odd numbers to filter through, add number 2 after filtering
odd_nums = range(3,100,2)

def filter_multiples(lst, n, counter):
    ''' A recursive method used to remove multiples of n in a list(lst)
        and using a counter to iterate through that list
    '''
    if len(lst) == counter:
        return lst
    else:
        # See if a number in a list is divisble by n and if so, remove it
        if lst[counter] % n == 0 and lst[counter] != n:
            lst.remove(lst[counter])
        if len(lst) == counter:
            return lst
        return filter_multiples(lst, n, counter+1)

# Cycle through the numbers and use filter_multiples to remove the factors and keep the primes
for i in odd_nums:
    filter_multiples(odd_nums, i, 0)

odd_nums.insert(0,2)
print odd_nums

I would like to know how to improve upon the recursive function and furthermore, how to improve my program so that is in line with python best practices.

This is not a Sieve of Eratosthenes for a few reasons.

Instead of just throwing a real one at you, lets see how we can deduce a Sieve of Eratosthenes just by improving the current code.

The first thing to note is that we can clean up the recursive function be removing the return; you're only returning the list they already have.

Secondly, you can remove the else and remove the last if by making the first check stricter. That gives:

def filter_multiples(lst, n, counter):
    ''' A recursive method used to remove multiples of n in a list(lst)
        and using a counter to iterate through that list
    '''
    if len(lst) <= counter:
        return

    # See if a number in a list is divisble by n and if so, remove it
    if lst[counter] % n == 0 and lst[counter] != n:
        lst.remove(lst[counter])

    filter_multiples(lst, n, counter+1)

You then find that the recursive function is easier as a loop:

def filter_multiples(lst, n, counter):
    while counter < len(lst):
        # See if a number in a list is divisble by n and if so, remove it
        if lst[counter] % n == 0 and lst[counter] != n:
            lst.remove(lst[counter])

        counter += 1

If this insistence to use an imperative style makes you confused about what the point of recursion is, let me assure you there are uses. Traversing tree data structures is traditionally done recursively. Here's some example of that. It's not, however, good to use recursion for the sake of it unless using a language specifically designed to do so.

This uses remove repeatedly to filter elements. remove is O(n) so the overall time for this is O(n²). You know this can't be a Sieve of Eratosthenes because the whole sieve is meant to be O(n log n)!

One way to improve this is to use a list comprehension:

def filter_multiples(lst, n):
    lst[:] = [i for i in lst if i == n or i % n]

which, you might think, is then better just using return:

def filter_multiples(lst, n):
    return [i for i in lst if i == n or i % n]

However, the outer loop

for i in odd_nums:
    filter_multiples(odd_nums, i)

would be changed to

for i in odd_nums:
    odd_nums = filter_multiples(odd_nums, i)

and this means the for i in odd_nums would be iterating over the old odd_nums. Thus, this needs to be in place with how you've made it. However, modifying the number of elements in lists while you're iterating over them is heavily discouraged. This means that maybe we should think of a different technique for filter_multiples.

One such technique is marking all of the elements to remove and then doing a list comprehension:

for i, val in enumerate(lst):
    if not (val == n or val % n):
        lst[i] = None

lst[:] = [val for val in lst if val is not None]

The advantage of this is that we can hoist the final comprehension outside of the two loops and just skip None in the outer loop:

def filter_multiples(lst, n):
    for i, val in enumerate(lst):
        if val is None:
            continue

        if not (val == n or val % n):
            lst[i] = None

# Cycle through the numbers and use filter_multiples to remove the factors and keep the primes
for val in odd_nums:
    if val is not None:
        filter_multiples(odd_nums, val)

odd_nums = [val for val in odd_nums if val is not None]

This seems more complicated, true, and it's probably also slower than the comprehension because it's looping over more numbers. However, think about this check:

if not (val == n or val % n):
    lst[i] = None

Since we're not compressing lst, val will directly map to a given i (or None). As such, if we work out that mapping we can avoid the check by just going over all the i that map to {2n, 3n, 4n, ...}.

One way of doing that is just making val map to i:

sieve = range(100)
sieve[0] = sieve[1] = None

def filter_multiples(lst, n):
    for i in range(2*n, len(lst), n):
        if lst[i] is None:
            continue

        lst[i] = None

# Cycle through the numbers and use filter_multiples to remove the factors and keep the primes
for val in sieve:
    if val is not None:
        filter_multiples(sieve, val)

sieve = [val for val in sieve if val is not None]

print sieve

In fact, the if is no longer needed inside this loop and we can do:

def filter_multiples(lst, n):
    for i in range(2*n, len(lst), n):
        lst[i] = None

Then you'd probably want to tidy up the code by putting it in a function:

def mask_indicies(lst, start, step):
    for i in range(start, len(lst), step):
        lst[i] = None

def primes_below(maximum):
    sieve = list(range(maximum))
    sieve[0] = sieve[1] = None

    for val in sieve:
        if val is not None:
            mask_indicies(sieve, 2*val, val)

    return [val for val in sieve if val is not None]

if __name__ == "__main__":
    print(primes_below(100))

That is a Sieve of Eratosthenes. I've added Python 3 compatibility.

Note that I have previously talked about doing similar things really fast, first with a Sieve of Sundaram and then comparing it to a Sieve of Eratosthenes (Eratosthenes won).


Removing the largest values encountered so far in a list The problem is, in summary:

  A set of values will be given. Whenever the input is -1, the largest number from the values inputted until then needs to be printed, and is then deleted.

Here is the code that I wrote for it:

#include<iostream>
#include<vector>
#include<algorithm>

using namespace std;
int main()
{
  int val;
  int N, M, x=0, i=0;
  vector<int> visits;
  cin>>N>>M;
  for(x=0; x<N+M; x++)
  {
    cin>>val;
    visits.push_back(val);
     if(val==-1)
      {

         cout<<*max_element(visits.begin(), visits.end())<<endl;
         i++;
         visits.erase(max_element(visits.begin(), visits.end()));
      }
  }

    return 0;
}

The program gives correct answers, but it exceeds the time limit for some of the test cases. The test data isn't given, so I don't know for what size of test data it fails. Is there some other way by which I can optimize my code?

You are using a C++ compiler!

So you should also use its features:

don't declare all variables up front but as late as possible
don't using namespace std; that only gives you trouble later on
you don't need to return 0; at the end of main

Formatting

Your formatting can be improved. You should use more spaces especially around (binary) operators, this makes it easier to parse (or rather tokenize) the code with your eye.

Naming

You have nearly only single letter names which is bad. The fact that two of them look like macro names does not make it better. Consider:

N -> populationHeadCount
M -> executionCount
val -> currentVisitorWealth
visits -> stillLivingWealthiest

Unused variable

The variable i is initialized and incremented but never read.

Inserting "the king"

Your code unconditionally inserts the wealth val even if it is the king (-1), thus introducing even more entries in visits.

Speed up

Reading rules more closely than you have given them here yields:

  You may assume that no two citizens have the same wealth.

Which makes this problem a perfect fit for an std::set (as mentioned by Loki):

#include <iostream>
#include <set>
#include <algorithm>

int main() {
  int populationHeadCount, expectedExecutionCount;
  std::cin >> populationHeadCount >> expectedExecutionCount;
  std::set<int> stillLivingWealthiest;
  int executionNumber = 0;
  while (executionNumber < expectedExecutionCount) {
    int currentVisitorWealth;
    std::cin >> currentVisitorWealth;
    if (currentVisitorWealth == -1) {
      std::cout << *--stillLivingWealthiest.end() << std::endl;
      stillLivingWealthiest.erase(--stillLivingWealthiest.end());
      ++executionNumber;
    } else {
      stillLivingWealthiest.insert(currentVisitorWealth);
    }
  }
}

Explanation: A std::set stores the elements sorted from minimum to maximum, so we only need to look at the end to get and delete the maximal element.


Login system in PHP using AJAX I am beginning the cycle of creating my role based access control into my framework. I now want to log the user into my application and my _user table looks like this:
CREATE TABLE `_users` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `username` varchar(255) NOT NULL,
  `hash` varchar(255) NOT NULL,
  `session` varchar(255) DEFAULT NULL,
  PRIMARY KEY (`id`)
)

I create my user like so in my model, where bin2hex() is my IV / salt.
Database::getInstance()->Prepare( 'INSERT INTO _users (username, hash, session) VALUES (?, ?, ?)' )
                       ->execute( array(
                           'Test',
                           password_hash( 'Test', PASSWORD_BCRYPT ),
                           md5( time() . bin2hex( random_bytes( 32 ) ) )
                       ) );

To then log into the user, I use a method. The method I use in the request is POST because my application has built in CRSF tokens. If the CRSF token is passed, then a xauth_protected aliased class is instanced which ensures that X-Auth header exists and is of md5(session_id()) to ensure that no cross-site-origin requests can be made.
public function login( Request $request )
{
    $this->middleware( 'json_response', $request );
    $this->middleware( 'xauth_protected', $request );

    if( !$request->has( 'username' ) || !$request->has( 'password') )
    {
        echo json_encode( array(
            'status' => FALSE,
            'reason' => 'Please fill in the required fields...',
        ) );
        return;
    }

    $stmt = Database::getInstance()->Prepare( 'SELECT id, hash, session FROM _users WHERE username = ? LIMIT 1' );
    $stmt->execute( array( $request->username ) );
    $row  = (object) $stmt->fetch();

    if( isset( $row->id ) )
    {
        if( password_verify( $request->password, $row->hash ) )
        {
            // $_SESSION['oath'] = $row->session
            // This is used to find the user that is logged in, if it is set
            $request->setSession( 'oauth', $row->session );

            echo json_encode( array(
                'status' => TRUE,
            ) );

            return;
        }

        echo json_encode( array(
            'status' => FALSE,
            'reason' => 'Invalid credentials...',
        ) );

        return;
    }

    echo json_encode( array(
            'status' => FALSE,
            'reason' => 'Sorry, that username was not found in our records...',
        ) );
}

To then test this method, I can use:
const formData = new FormData();

formData.append( 'crsf_token', sessionStorage.getItem( 'token' ) ); // CRSF token is set elsewhere
formData.append( 'username', document.getElementById( 'username' ).value ); // Example input field
formData.append( 'password', document.getElementById( 'password' ).value ); // Example input field

fetch( App.__viewFactory.homepage.login, { // This is a prestored route that looks like /oauth/login
    method: 'POST',
    headers: {
        'X-Auth': sessionStorage.getItem( 'session' ) // md5( session_id() ) which is set elsewhere
    },
    body: formData
} )
.then( response => response.json() )
.then( data     => {
    if( data.result ) {
        window.location.href = App.__viewFactory.dashboard.view; // prestored route to the dashboard
        return;
    }

    document.getElementById( 'login-error' ).innerHTML = data.reason;
} )
.catch(error => {
    document.getElementById( 'login-error' ).innerHTML = 'Oh no! Something went wrong.';
});

I have incorporated CRSF protected, custom headers using the session_id() to prevent cross-origin requests (since my framework rewrites all requests to my index page, this then turns on CORS). Is everything I am doing on the database side as secure? Is there something I could be doing better?

Your querying practices look secure to me.  Your are using a prepared statement with bound parameters and using password_hash() and password_verify().  It really can be as simple as doing those basic/essential things.
As for other refinements, I recommend writing the failure branches before successful ones, doing early exit()s, and only passing back an empty or populated reason to reduce the data structure to its vital value.

I will assume/hope that the username is a UNIQUE table column, so LIMIT 1 provides no value.
If you want the result set / row to be an object, just tell pdo that that is what you want -- fetch(PDO::FETCH_OBJ).
The result set will either be an object or false, so just check for a falsey result set.  For this reason, you can remove id from the SELECT.
I don't like to give too much specificity when giving failed login responses.  I would tell the user that the credentials generally failed without spelling out which field was the problem.

Recommendation:
public function login(Request $request)
{
    $this->middleware('json_response', $request);
    $this->middleware('xauth_protected', $request);

    if(!$request->has('username') || !$request->has('password')) {
        exit(json_encode(['reason' => 'Please fill in the required fields']);
    }

    $stmt = Database::getInstance()->Prepare(
        "SELECT hash, session FROM _users WHERE username = ?"
    );
    $stmt->execute([$request->username]);
    $rowObject = $stmt->fetch(PDO::FETCH_OBJ);

    if (!$rowObject || !password_verify($request->password, $rowObject->hash)) {
        exit(json_encode(['reason' => 'Invalid credentials']));
    }

    $request->setSession('oauth', $rowObject->session);
    exit(json_encode(['reason' => null]));
}

Then in your js, you can use this:
.then(data => {
    if (data.reason) {
        document.getElementById('login-error').innerHTML = data.reason;
    } else {
        window.location.href = App.__viewFactory.dashboard.view;
    }
})


An attempt at inheritance (Geometric Shapes) I'm reading through the chapter on inheritance from C++ Primer 5th Edition. It asks

Organize the following type into an inheritance hierarchy:
(b) Geometric primitives (such as box, circle, sphere, cone)
Identify some of the likely virtual functions as well as public and protected
members.

I was hoping this is a suitable arrangement for the organisation of the hierarchy. My line of thinking was:

The most general property of any shape is its dimension. Thus the code could be factored in to having a base class at the top named Shape which would contain this piece of data.

The necessary member functions will change depending on whether I have a 1D, 2D or 3D shape. 2D shapes will require an area and perimeter function, and 3D shapes a surfaceArea and volume function. The operations of these functions will vary depending on the shape. So define abstract base classes corresponding to each dimension with pure-virtual member functions.

Derive classes from the dimension classes corresponding to the final geometric shapes.

My main concern is that maybe the Shape class isn't all that necessary if its only purpose is to hold dimension and the static member pi. A second concern is how to prevent creation of Shape objects, since it only represents a concept and nothing tangible. I'm guessing to do this I would need to define the destructor as pure virtual - although I have not reached this stage of reading.
Shape.h:
#ifndef SHAPE
#define SHAPE

#include <cmath> //sqrt needed

class Shape{
public:
    Shape() = default;
    Shape(unsigned d): dimensions(d) { }
    virtual ~Shape() = default;
private:
    unsigned dimensions = 0;
protected:
    static const double pi;

};

class DimensionOne : public Shape {
public:
    DimensionOne(): Shape(1) { }

    virtual double length() const = 0;
};

class DimensionTwo : public Shape {
public:
    DimensionTwo(): Shape(2) { }

    virtual double perimeter() const = 0;
    virtual double area() const = 0;
};

class DimensionThree : public Shape {
public:
    DimensionThree(): Shape(3) { }

    virtual double surfaceArea() const = 0;
    virtual double volume() const = 0;
};

//2D SHAPES

class Rectangle : public DimensionTwo {
public:
    Rectangle() = default;
    Rectangle(double a): DimensionTwo(), length(a), width(a) { }
    Rectangle(double x, double y): DimensionTwo(), length(x), width(y) { }

    double perimeter() const override { return 2*(length+width); }
    double area() const override { return length*width;}
private:
    double length = 0, width = 0;
};

class Circle : public DimensionTwo {
public:
    Circle() = default;
    Circle(double r): DimensionTwo(), radius(r) { }

    double perimeter() const override { return 2*pi*radius; }
    double area() const override { return pi*radius*radius;}
private:
    double radius = 0;
};

//3D SHAPES

class Box : public DimensionThree {
public:
    Box() = default;
    Box(double a): DimensionThree(), length(a), width(a), height(a) { }
    Box(double x, double y, double z): DimensionThree(), length(x), width(y), height(z) { }

    double surfaceArea() const override { return (2*length*height) + (4*width*height); }
    double volume() const override { return length*width*height;}
private:
    double length = 0, width = 0, height = 0;
};

class Sphere : public DimensionThree {
public:
    Sphere() = default;
    Sphere(double r): DimensionThree(), radius(r) { }

    double surfaceArea() const override { return (4*pi*radius*radius); }
    double volume() const override { return (4*pi*radius*radius*radius)/3;}
private:
    double radius = 0;
};

class Cone: public DimensionThree {
public:
    Cone() = default;
    Cone(double b, double h): DimensionThree(), base(b), height(h) { }

    double surfaceArea() const override { return pi*base*(base + sqrt(base*base + height*height)); }
    double volume() const override { return (pi*base*base*height)/3;}
private:
    double base = 0, height = 0;
};

#endif // SHAPE

Shape.cpp:
#include "Shape.h"

const double Shape::pi = 3.14159;

A second concern is how to prevent creation of Shape objects, since it only represents a concept and nothing tangible. I'm guessing to do this I would need to define the destructor as pure virtual - although I have not reached this stage of reading.

Yep:

class Shape{
public:
    virtual ~Shape() = 0;
};
Shape::~Shape() {}

Now you can't create objects of Shape.


Changing the view based on these if/else statements If the user has not responded = edit mode.
If there are responses present but the overall status is 'Completed' = lock everything into display mode.
If the stage is less than or equal to 'editable in stage', it is considered editable and if also the response given has a code of 0 or 2 (standardized responses via "coding" them).
Otherwise set it to display.

This code works as desired but it's hard to read/maintain.

    if (typeof response !== 'string') {
        mode = edit;
    } else {
        if (status.indexOf('Completed') !== -1) {
            mode = display;
        } else {
            if ((stage <= isEditable) && (code == 0 || code == 2)) {
                mode = edit;
            } else {
                mode = display;
            }
        }
    }

First step, remove that first else

if (typeof response !== 'string') {
    mode = edit;
} else if (status.indexOf('Completed') !== -1) {
    mode = display;
} else if ((stage <= isEditable) && (code == 0 || code == 2)) {
    mode = edit;
} else {
    mode = display;
}

After this, your text and your code dont look the same to me... you need better variables or having methods:

var noResponse = typeof response !== 'string';
var statusIsNotCompleted = status.indexOf('Completed') !== -1;
var stageIsEditable = (stage <= isEditable) && (code == 0 || code == 2);
if (noResponse) {
    mode = edit;
} else if (statusIsNotCompleted) {
    mode = display;
} else if (stageIsEditable) {
    mode = edit;
} else {
    mode = display;
}

After this, is there an option in real life where putting else if (stageIsEditable) before else if (statusIsNotCompleted) make something not work? Otherwise, you could join them in a variable:

var yourThingIsEditable = noResponse ||  stageIsEditable;


Is it good practice in Java to pass a StringBuffer to a method to get error messages I have a class that has a method which takes a user as an argument and performs several validation checks on the user and returns a boolean to indicate if theyare valid.

I want to be able to get error messages back from the validation method along with the boolean return value.

I was thinking of doing it by passing a StringBuffer as an argument along with the user and in the validation code appending error messages to the buffer so that the caller gets the boolean returned and then can look at the StringBuffer for error messages.

// validation class
public boolean validateUser(User user, StringBuffer errors) {
    ...
}

// caller
StringBuffer errors = new StringBuffer();
boolean validUser = validateUser(user, errors);
if (!validUser) {
    log(errors.toString();
}

Is this good practice in Java?  Or would it be better to log the errors to a buffer in the validation class and have a public method to get the errors?  Or should the validateUser() method return an object that contains the boolean and errors?

Thank you!

Since you are dealing with a validation method, I believe you will want to read my question Is it better practice to have void method throw an exception or to have the method return a boolean? and its answers. My point there is that you don't really need to return a boolean, you could also have a void method throw an exception if the validation fails. The exception could then, in your case, contain the errors as its payload. But I think that passing a logger as a parameter isn't a bad practice, just basically a matter of personal preference and to be decided on a case by case basis, much like the question about whether to return a boolean or to throw an exception.

In any case, however, I would never use a StringBuffer as an error logger like in your case. For one, I might be building a web application where the output is rendered in HTML. In that case the StringBuffer would basically need to contain the HTML markup, which would then become a problem once you one day figure out that you need to run your process from the command line as well, and then you get all the HTML markup to mess up the output there. Also, with the StringBuffer you cannot easily get the error count, just whether there were any errors or not. So I would probably create an implementation of some IErrorHandler kind of interface wrapping a List<String> rather than a StringBuffer, as in @dreza's answer.


Graph library - graph interface I am trying to write simple graph lib in C++, BFS, DFS etc..
I want to create class GraphI which will represent any graph and will include necessary methods so my library could work.
Could I add/remove/improve something from my code?

#pragma once
#include <algorithm>
#include "colors.h"
struct Vertex;
template < typename T >
class GraphI {

public:
    struct Vertex {
        Vertex( const T& k, color col = color::w, int c = -1 )
            : key( k )
            , color(col)
            , cost( c ) {}

        int getCost() const { return cost; }
        const T& getKey() const { return key; }
        T& getKey() { return key; }
        void setColor( color c ) { color = c; }
        color getColor() { return color; }

        int cost;
        color color;
        T key;
        bool operator<( const Vertex& v1 ) const {
            return key < v1.key;
        }
    };
    using Graph = std::map<Vertex, std::list<Vertex> >;
    GraphI() = default;
    virtual ~GraphI() = default;

    virtual void add_vertex( const T& vertex ) {
        // if exists then add else do nothing
        _graph[ vertex ] = {};
        ++_size;
    }
    virtual void add_edge( const T& from,
                   const T& to,
                  int cost = -1 ) {
        // if vertices do NOT exist return
        auto it = _graph.find( from );
        if ( it == _graph.end() ) return;

        auto it2 = _graph.find( to );
        if ( it2 == _graph.end() ) return;

        auto& neighbours = it->second;
        auto it_exists = std::find_if( neighbours.begin(),
                        neighbours.end(),
                        [&to]( const Vertex& vertex ) { return vertex.getKey() == to; } );
        // if edge already exists
        if ( it_exists != neighbours.end() ) return;
        neighbours.emplace_back( Vertex(to, color::w, cost) );
    }
    virtual void remove_vertex( const T& vertex ) {
        _graph.erase( vertex );
        --_size;
        // remove all edges going TO vertex
        for ( auto& v : _graph ) {
            auto it = std::find_if( v.second.begin(),
                        v.second.end(),
                        [&vertex]( const auto& v )
                        { return vertex == v.getKey(); } );
            // if vertex does NOT exist
            if ( it == v.second.end() ) return;
            v.second.erase( it );
        }

    }
    virtual void remove_edge( const T& from, const T& to ) {
        auto it = _graph.find( from );
        if ( it == _graph.end() ) return;

        auto &edges = it->second;
        auto it2 = std::find_if( edges.begin(), edges.end(),
                    [&]( const auto& vertex ) { return vertex.key == to; } );

        if ( it2 == edges.end() ) return;
        edges.erase( it2 );
    }

    const Graph& graph() const { return _graph; }
    unsigned size() const { return _size; }
private:
    Graph _graph;
    unsigned _size;
};

The comment in add_vertex is confusing and at contradicts what the code does.  If vertex already exists in _graph, the existing vertex will be replaced with an empty one, then the size increased (causing the internal _size to be incorrect).  Depending on what behavior you want when adding a vertex that already exists, you can use _graph.try_emplace, _graph.insert_or_assign, or just _graph[vertex] (to add a new entry if it does not exist, and do nothing if it does).

There is no benefit to maintaining a distinct _size variable.  As shown above, it can get out of sync with the actual size.  It is better to just call _graph.size() whenever the user calls Graph::size.

In remove_vertex, you should check the return value from _graph.erase to see if something was actually removed.  If nothing was removed there's no benefit to running any of the other code in the function.  Also, the if (it == v.second.end()) check should not return, as you avoid scanning the rest of the vertexes in _graph to remove edges that connect to the vertex that was just removed.  Alternatively, you can just replace that whole loop body with a call to either v.second.remove or v.second.remove_if.  (The same change can be made in remove_edge.)


Access resolve function outside of a JavaScript Promise I have an object that needs to have a single point of initialization, and other callers need to wait until the initialization is complete before continuing.

I have done this with jQuery, and it feels like I'm working with the design, but the same approach with a native JavaScript Promise seems like it is going against the grain.

I need to replace use of jQuery promise with a native JS Promise.

Is this initialization pattern OK, or is there a better approach?

On the surface it seems that an event pub/sub pattern would be a better fit, but I can't guarantee that the init() will be called before the ready() is called, which would result in missed events.

jQuery

function Foo() {

    var dfd= $.Deferred();
    this.ready = function () {
        return dfd.promise();
    }
    this.init = function(){
        setTimeout(function(){
            dfd.resolve();
        }, 2000);
    }
};

var foo = new Foo();

//foo.ready() may be called from multiple places.
foo.ready().then(function () {
    alert('foo.ready');
});

//foo.init would only be called once.
foo.init();

JavaScript Promise

function Foo() {

    var resolver,
        rejector,
        readyPromise = new Promise(function (resolve, reject) {
            resolver = resolve;
            rejector = reject;
        });

    this.ready = function () {
        return readyPromise;
    }
    this.init = function () {
        setTimeout(function () {
            resolver();
        }, 2000);
    }
};

var foo = new Foo();

//foo.ready() may be called from multiple places.
foo.ready().then(function () {
    alert('foo.ready');
});

//foo.init would only be called once.
foo.init();

If I understand your question correctly, your question runs on these assumptions:

You want to hook up multiple listeners to a single Promise.
The promise is triggered arbitrarily, not necessarily on promise creation.
You want to use native promises.
You run on an engine that has Promises.

The second bullet point, where the operation is triggered arbitrarily, is the tricky part because native promises are read-only. You'll need to catch resolve and reject from the constructor every time.

So while your implementation is correct, you'll find yourself repeating the pattern. jQuery Deferreds are better in this case, but we don't want to lug around jQuery, or any library for that matter.

What you could do is create your own little Deferred constructor. It just wraps around the existing native Promise API, essentially just forwarding arguments. The only difference is that it's exposing resolve and reject.

function Deferred(callback){
  var instance = this;

  // Catch the resolve and reject
  this._resolver = null;
  this._rejector = null;
  this._promise = new Promise(function(resolve, reject){
    instance._resolver = resolve;
    instance._rejector = reject;
  });

  // Deferred has { resolve, reject }. But personally, I like the Promise
  // version of resolve and reject as separate args.
  if(typeof callback === 'function')
    callback.call(this, this._resolver, this._rejector);
}

Deferred.prototype.then = function(resolve, reject){
  return this._promise.then(resolve, reject);
};

// resolve, reject etc.

// For other APIs, refer to jQuery for Deferred and MDN for Promises:
// https://api.jquery.com/category/deferred-object/
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise

Now with Deferred out of the way, let's go back to your jQuery version and improve it. Since we created Deferred as a constructor, we can subclass from it. This means we can have Foo become a Deferred and have the luxury of the Deferred API.

// Setup Foo as a Deferred in as little as 2 lines.
function Foo(){ Deferred.apply(this, arguments); }
Foo.prototype = Object.create(Deferred.prototype);

// Your Foo API
Foo.prototype.init = function(){
  var instance = this;
  setTimeout(function(){
    instance.resolve();
  }, 2000);
};

In the end, foo will have Deferred powers, care of an extended Promise API.

var foo = new Foo();

// We can now use `then`.
foo.then(function(){
  alert('foo.ready!');
});

//
foo.init();

Now if I wrote the code properly, this should work:

function Deferred(callback) {
  var instance = this;
  this._resolver = null;
  this._rejector = null;
  this._promise = new Promise(function(resolve, reject) {
    instance._resolver = resolve;
    instance._rejector = reject;
  });
  if (typeof callback === 'function')
    callback.call(this, this._resolver, this._rejector);
}

Deferred.prototype.then = function(resolve, reject) { return this._promise.then(resolve, reject); };
Deferred.prototype.resolve = function(resolution) { this._resolver.call(null, resolution); return this; };
Deferred.prototype.reject = function(rejection) { this._rejector.call(null, rejection); return this; };

// The setup code:

function Foo() { Deferred.apply(this, arguments); }
Foo.prototype = Object.create(Deferred.prototype);

Foo.prototype.init = function() {
  var instance = this;
  setTimeout(function() {
    instance.resolve('YAY!');
  }, 2000);
};

// Phew! Now your Foo is a "native" Deferred.

var foo = new Foo();

foo.then(function(value) {
  alert('foo.ready! ' + value);
});

foo.init();


Calculate range of ArrayList Write a method range that accepts an ArrayList of integers as a parameter and that returns the range of values contained in the list, which is defined as 1 more than the difference between the largest and smallest elements.

I feel like I'm beginning to understand conditionals, but for some reason this just makes me cringe. Is it good practice to have these conditionals as catch-alls in the beginning? Should I be using else ifs instead?

public static int range(ArrayList<Integer> list){
    if(list.size() == 0){
        return 0;
    }
    if(list.size() == 1){
        return 1;
    }

    int firstElement = list.get(1);
    int max = firstElement;
    int min = firstElement;

    for(int i = 0; i < list.size(); i++){
        int elementValue = list.get(i);
        if(max < elementValue){
            max = elementValue;
        }
        if(elementValue < min){
        min = elementValue;
        }
    }
return (max - min) + 1;
}

Use this:

if (list.size() <= 1) {
    return list.size();
}

list.get(1) refers to the second element in the list, not the first. Either rename the variable or use list.get(0). With that change, you don't need to use a list size of 1 as a special case.

Only being able to do the job on ArrayList is quite useless. Your method can take an argument of type List instead.

No, it is no need to use else if in this case.

Rewriting with Iterable

You currently check the first item in the list twice. If your code would accept an Iterable<Integer>, you would support even more data types (You still support ArrayList also).

The only thing you need is the Iterator from the iterable. When you have that, you can first see if it contains at least one item. If it doesn't, return 0. Otherwise, you do similar to what you are already doing, but with different code.

public static int range(Iterable<Integer> numbers) {
    Iterator<Integer> iterator = numbers.iterator();
    if (!iterator.hasNext()) {
        return 0;
    }

    int firstElement = iterator.next();
    int max = firstElement;
    int min = firstElement;
    while (iterator.hasNext()) {
        int elementValue = iterator.next(i);
        if (max < elementValue) {
            max = elementValue;
        }
        if (elementValue < min) {
            min = elementValue;
        }
    }
    return (max - min) + 1;
}

Happy coding!


Routes for reminders I have a controller with just a few actions, not the whole 7 RESTful actions situation. Here's what I have so far for routing:

Snip::Application.routes.draw do
  get "appointment_reminder_responses/confirmation_thanks", to: "appointment_reminder_responses#confirmation_thanks"
  get "appointment_reminder_responses/declination_thanks", to: "appointment_reminder_responses#declination_thanks"
  get "appointment_reminder_responses/:hash", to: "appointment_reminder_responses#create", as: :create_appointment_reminder_response
end

I think there certainly has to be a way to express this in a more DRY way, but but I'm not sure how that might go.

Not a lot to review,

obviously the most repeated part of your code is appointment_reminder_responses, if you create a variable called root and then built your routes with concatenations like
root + "/confirmation_thanks" then that would be a good start.

Furthermore you create a function that takes a string and modifies so that
"xxx/zzzz/yyyy" becomes "xxx/zzz#yyyy" and use that function for the first 2 routes.