Forced and easy hton/ntoh conversion Context:

When sending message across the network, the sender has to convert the message from host byte order to network byte order and the receiver has to do the the reverse -  network order to host order.

There are multiple ways of going about it. One neat way to do is to convert the message to host byte
order as soon as it is received.

Eg:

struct Msg {
   uint32_t a;
   uint16_t b;
};

run_ntoh (Msg *m)
{
    m->a = ntohl(m->a);
    m->b = ntohs(m->b);
}

process (Msg *m)
{
   run_ntoh(m);
   further_process(m);
}

But there are cases where this will not work. Say, the message buffer is shared by multiple receivers.
We can always copy the message to local buffer and do it. But that is inefficient if the message is large.

The third option is to always access the fields of 'Msg' using ntohx function. But this is error prone.

a. The user might forget to use ntoh always  - it is not enforced.

b. Also user has to be very careful - use htonl or htons? What if the size of the varible is changed -
    nothing enforces the change of htonl instead of htons if the variable size changes from 16 to 32.

Here my idea is to enforce the use of ntoh, while making it easy for the user.

Code:

#include <cstdint>
#include <cstddef>
#include <type_traits>

//
// class to extract N number of bytes and form a number
// uint32_t value = ExtractValue<uint32_t>::get(buffer);
//
template <typename T, size_t val = sizeof(T)>
struct ExtractValue {
    static T get (uint8_t *p)
    {
        return (ExtractValue<T, val - 1>::get(p) << 8) |
                static_cast<T>(p[val - 1]);
    }
};

template <typename T>
struct ExtractValue<T, 0> {
    static T get (uint8_t *p)
    {
        return 0;
    }
};

// Get a field of the class T, in host byte order.
// The class T is wrapped in NetworkData class. The get() function
// takes the offset to the member as an argument, gets the address
// of field and extract the field
template <typename T>
struct NetworkData {

    using type = T;

    NetworkData (uint8_t *p) : _p(p) {}

    template<typename R, typename U = std::decay_t<R>,
             typename std::enable_if_t<!std::is_pointer<U>::value>* =  nullptr>
    U get (uintptr_t offset)
    {
        return ExtractValue<U>::get(_p + offset);
    }

    template<typename R, typename U = std::decay_t<R>,
             typename std::enable_if_t<std::is_pointer<U>::value>* = nullptr>
    U get (uintptr_t offset)
    {
        return (U)(_p + offset);
    }

private:
    uint8_t *_p;
};

// Macro to easily call the NetworkData::get()
#define GetNtoh(h, f)  \
    h.get<decltype(decltype(h)::type::f)>(offsetof(decltype(h)::type, f))

Usage:

struct Msg {
   uint32_t a;
   uint16_t b;
};

void process (uint8_t *p)
{
    NetworkData<Msg> h(p);

    auto a = GetNtoh(h, a);
    auto b = GetNtoh(h, b);
}

It is always better to use something like goolge protobuf to serialize/deserialize. But that is not possible when the sender is legacy code.

Unqualified names from C headers

It appears that your <cstdint> happens to declare its identifiers in the global namespace as well as in std; this means that you weren't notified of the consistent misspelling of std::uint8_t, std::uint16_t and the like.

Interface

It's not clear why ExtractValue is visible at file scope (rather than being a private static method within NetworkData).

The get() methods ought to be declared const (and if you never implement set() methods, then perhaps _p should be a pointer-to-const).

Pass pointer to member rather than offsetof

The use of offsetof() and pointer arithmetic with std::uint8_t* (which should be char*, given that offsetof() works in units of char) is deeply disturbing.  One possibility is for NetworkData<T> to hold a private reference to the T, and accept pointer-to-member to indicate which value is desired:

#include <type_traits>
#include <utility>

// Get a field of the class T, in host byte order.
// The class T is wrapped in NetworkData class. The get() function
// takes the offset to the member as an argument, gets the address
// of field and extract the field
template <typename T>
class NetworkData
{
    const T& data;

public:
    NetworkData (void *data)
        : data(*reinterpret_cast<T*>(data))
    {
    }

    template<typename V>
    V get(V T::*field) const
    {
#if BIG_ENDIAN
        return data.*field;
#else
        return byte_swap(data.*field);
#endif
    }

private:
    template<typename V>
    static
    std::enable_if_t<std::is_integral_v<V> && std::is_signed_v<V>, V>
    byte_swap(V value)
    {
        // We manipulate using unsigned bitwise arithmetic.
        return reinterpret_cast<V>(byte_swap(reinterpret_cast<std::make_unsigned_t<V>>(value)));
    }

    template<typename V>
    static
    std::enable_if_t<std::is_integral_v<V> && std::is_unsigned_v<V>, V>
    byte_swap(V value)
    {
        // We assume CHAR_BIT == 8 (so that sizeof gives us octets)
        static const V mask = 0xff;
        V result = 0;
        for (auto shift = 0u;  shift < 8 * sizeof value;  shift += 8)
            result = (result << 8) + ((value >> shift) & mask);
        return result;
    }

    // We could add overloads here to take advantage of processor-native byte swap instructions, as available.
};

We can then use it like this:

#include <cstdint>
struct Msg {
   std::uint32_t a;
   std::uint16_t b;
};

void process (std::uint8_t *p)
{
    NetworkData<Msg> h(p);

    auto a = h.get(&Msg::a);
    auto b = h.get(&Msg::b);
}

This eliminates the use of preprocessor macros, and makes a slightly more natural object-access syntax.

Consider modifying the structure members

If we're allowed to re-write Msg, we could make its members be network-endian values:

#include <cstdint>
struct Msg {
   NetworkEndian<std::uint32_t> a;
   NetworkEndian<std::uint16_t> b;
};

We can declare NetworkEndian like this:

template<typename Integer>
class NetworkEndian
{
    Integer value;

public:
    NetworkEndian(Integer value)
        : value(byte_swap(value))
    {}
    // default copy, assign, destructor

    operator Integer() const
    {
        return byte_swap(value);
    }

private:
    static Integer byte_swap(Integer v)
    {
        using Unsigned = std::make_unsigned_t<Integer>;
        Unsigned u = reinterpret_cast<Unsigned>(v);
        // Assume CHAR_BIT == 8 (so that sizeof gives us octets)
        static const Unsigned mask = 0xff;
        Unsigned result = 0;
        for (auto shift = 0u;  shift < 8 * sizeof u;  shift += 8)
            result = (result << 8) + ((u >> shift) & mask);
        return reinterpret_cast<Integer>(result);
    }
};

The conversion operators mean that we can now read the members in a completely natural manner:

void process(const Msg& message)
{
    auto a = message.a;
    auto b = message.b;
}

Not only that, but by providing a converting constructor, we also get to assign values equally naturally.


Find next number whose sum of digits is N This program takes two numbers M(<10k) and N (<100) and finds the smallest number greater than M whose sum of digits = N. Additionally, as per the question's requirements, it prints the number of digits in the printed number.

Example: M = 919
,         N = 34

Required number = 7999 as 7+9+9+9 = 34

   /**
     *Takes M and N , finds the number greater than M whose sum of digits = N
     */
    import java.util.*;
    class MtoN
    {
        static int sumOfdigits(int n)
        {
            int sum =0 ;
            while(n!=0)
            {
                sum = sum + n%10;
                n = n/10;
            }
            return sum;
        }

        static int numOfdigits(int n)
        {
            String str = Integer.toString(n);
            return str.length();
        }

        public static void main(String[] args)
        {
            Scanner sc = new Scanner(System.in);
            System.out.println("Enter M (100 to 10000)");
            int M = sc.nextInt();
            System.out.println("Enter N (less than 100)");
            int N = sc.nextInt();
            if(N> 100 || M>10000 || M<100)
            {
                System.out.println("INVALID INPUT");
            }
            else
            {
                for(int i = M+1 ; ; i++)
                {
                    if(sumOfdigits(i) == N)
                    {
                        System.out.println("Required Number is: "+ i);
                        System.out.println("Number of digits in it "+ numOfdigits(i));
                        break;
                    }
                }
            }
        }
    }

It works fine for smaller number but takes TOO much time for numbers like: M = 9181 and N = 99. (In fact, it doesn't even print the result on my screen...)

How do I reduce this execution time?

This is not a review, but an extended comment.

Thou shalt not brute force. Instead of inspecting every number larger than M (it indeed takes too much time), consider partitioning N into a sum of single digit numbers, and use these partitions to form the answer.

Notice that you don't have to inspect all the partitions. The digit composition of M gives enough leads to just construct the answer.


Calculating rectangular area This program is working correctly, but I need to check if the structure is good or can be improved. Plus, if I called ra.area() immediately after declaring ra, I get 687194768. Why is that?

#include "stdafx.h"
#include "iostream"
using namespace std;
class rectangleclass
{
 int w,h;
public:
void setnum(int x, int y)
{
w=x;
h=y;
}
int area()
{
    return w*h;
}
};
int main()
{
rectangleclass ra;
int l;
int L;
cout<<"please write the height and width in m\n" ;
cin>>L;
cin>>l;
ra.setnum(L,l);
cout<<ra.area()<<"m^2";
 system("pause");
 }

Readability

First things first. Formatting. Indent after each brace, consistently. It is more or less impossible to read your code.

Secondly, variable naming. 1 (el) is a terrible name for a variable, since it looks exactly like a l (one), so much so that I just flipped them and you probably didn't notice. Not to mention that one of your Ls is really width. So you should name them length and width.

Rectangle

Rather than having setnum(), which is not a meaningful name, instead require your Rectangle to be constructed directly with the values:

class Rectangle {
public:
    Rectangle(int w, int h)
    : width_(w), height_(h)
    { }

private:
    int width_, height_;
};

Note the improved naming of the member variables and the class. Adding class to the type name is unnecessary and redundant. This constructor will additionally solve your other problem: accidentally misusing Rectangle by accessing its member functions before you set any of the values (uninitialized ints have indeterminate value). This way, you're guaranteeing that width_ and height_ are set.

Usage here would be like:

cin >> width;
cin >> height;
Rectangle ra(width, height);
cout << ra.area();

const-ness

The area() member function doesn't (and shouldn't) modify any of its members. So it'd be better to mark it const:

int area() const { ... }

This lets you get the area of a const Rectangle.


Finding the k-th element in a BST Given a Binary Search Tree, determine its k-th  element in inorder traversal.

Here's my node structure:

struct node
{
    int elem;
    node *left, *right;

    static node* create(int elem)
    {
        node *newnode = new node;
        newnode->elem = elem;
        newnode->left = newnode->right = NULL;
        return newnode;
    }

        // Forget freeing up memory

};

Here's my BST:

class tree
{
  private:
  node *root;

  public:
  tree(int elem)    { root = node::create(elem); }

  bool insert(int elem)
  {
    node *newnode = node::create(elem);
    node *travnode = root;

    while(1)
    {
      if(elem &lt travnode->elem)
      {
        if(travnode-> left == NULL)
    {
      travnode->left = node::create(elem);
      return true;
    }
    else travnode = travnode->left;
      } // elem < travnode->elem
      else
      if(elem > travnode->elem)
      {

        if(travnode->right == NULL)
    {
      travnode->right = node::create(elem);
      return true;
    }
    else
          travnode = travnode->right;

      }
      else
         return false;
 }

  /*  findKthInorder
  @param mynode [in]     -- the root of tree whose kth largest is to be found
  @param k [in]          -- value k
  @param count [in]      -- a counter to keep track of which node we're in
  @param result [in,out] -- returns mynode->elem once we're in kth node
  */
  void findKthInorder(const node *mynode, const int k, int &count,  int &result) const
  {
    if(mynode != NULL)
    {
      findKthInorder(mynode->left,k,count,result);
      if(!--count)
      {
        result = mynode->elem;
    return;
      } // if (!--count)

      findKthInorder(mynode->right,k,count,result);
    } // if (mynode != NULL)
  } // findKthInorder

  /* findKthInorder
     abstracts away previous function and is exposed to outside world
  */
  int findKthInorder(const int k) const
  {
    int count = k,result = 0;
    findKthInorder(root,k,count,result);
    return result;

   }

}; // class tree

Here's some test code that I wrote:

int main()
{
   tree T = tree(5);
   T.insert(1); T.insert(7);   T.insert(-1);T.insert(6);

   for(int i = 1;i != 5; ++i)
       printf("%d, " T.findKthInorder(i)); // -1, 1,5,6,7
   return 0;
}

I'll be happy to listen to any suggestions for a more  elegant findKthInorder() function.

If you add a total count field to each node, you can find the k-th element efficiently (in logarithmic time) by writing a method like this (untested):

node *kth(int k)
{
    assert(k >= 0 && k < total);

    if (left != NULL) {
        if (k < left->total)
            return left->kth(k);
        k -= left->total;
    }

    if (k == 0)
        return this;

    assert(right != NULL);
    return right->kth(k - 1);
}

Otherwise, the recursive algorithm you used for findKthInorder is the most elegant way I can think of to do this.  I would clean it up a bit, though:

static const Node *kth_(const Node *node, int &k)
{
    if (node == NULL)
        return NULL;

    const Node *tmp = kth_(node->left, k);
    if (tmp != NULL)
        return tmp;

    if (k-- == 0)
        return node;

    return kth_(node->right, k);
}

int kth(int k) const
{
    assert(k >= 0);

    const Node *node = kth_(this, k);
    if (node == NULL) {
        std::cerr << "kth: k is too large\n";
        exit(1);
    }

    return node->elem;
}

Returning a node pointer instead of an element from the helper function has two advantages:

We can use NULL to indicate failure.
We get to drop an argument from the helper function.
In the future, it will be easier to write a function to update the kth element.

In your findKthInOrder helper function, the k argument is never actually used, and can be dropped as well.

A couple cleanups on the side:

I renamed the class node to Node to avoid having to say mynode all over the place.  I suppose this is just a matter of taste, seeing how the STL uses lowercase type names.
I switched to zero-based indexing.  Again, this is a matter of taste, but zero-based indexing is far more common in C++, and is easier to work with in many cases.


Drawing a word, with a box around one letter I'm working on a draw function which needs to draw the word 'crux' in the center of the current view and then draw a red box around the 'x' character like this:

My code to do this looks like this

override func draw(_ rect: CGRect) {

    guard let context = UIGraphicsGetCurrentContext() else { return }

    // Flip the coordinate system
    context.textMatrix = .identity
    context.translateBy(x: 0, y: bounds.size.height)
    context.scaleBy(x: 1.0, y: -1.0)

    // Create the logo as an attributed string
    let font = UIFont(name: "EntangledPlainBRK", size:self.bounds.size.width * 0.4)!
    let attrs:[String:Any] = [
        kCTForegroundColorAttributeName as String: UIColor.black.cgColor,
        kCTFontAttributeName as String: font
    ]
    let logo = NSMutableAttributedString(string: "crux", attributes: attrs)

    // Work out the position of the logo in the view.
    let path = CGMutablePath()
    let textSize = logo.size()
    let logoOrigin = CGPoint(x: (rect.width - textSize.width) / 2.0, y: (rect.height - textSize.height) / 2.0)
    let logoRect = CGRect(origin: logoOrigin, size: logo.size())
    path.addRect(logoRect)

    // get the frame setters.
    let framesetter = CTFramesetterCreateWithAttributedString(logo as CFAttributedString)
    let frame = CTFramesetterCreateFrame(framesetter, CFRangeMake(0, logo.length), path, nil)

    CTFrameDraw(frame, context)

    // Set the text position of the context to the location of the run on screen.
    var lineOrigins = [CGPoint] (repeating: .zero, count: 1)
    CTFrameGetLineOrigins(frame, CFRange(), &lineOrigins)
    let lineOrigin = lineOrigins[0]
    context.textPosition = CGPoint(x:logoOrigin.x + lineOrigin.x, y:logoOrigin.y + lineOrigin.y)

    // Get the position of the last character.
    let lines = CTFrameGetLines(frame)
    let line = unsafeBitCast(CFArrayGetValueAtIndex(lines, 0), to:CTLine.self)
    let runs = CTLineGetGlyphRuns(line)
    let run = unsafeBitCast(CFArrayGetValueAtIndex(runs, 0), to:CTRun.self)
    let glyphPosition = CTRunGetImageBounds(run, context, CFRange(location:3, length:1))

    // Draw a box around it.
    context.setStrokeColor(UIColor.red.cgColor)
    context.setLineWidth(2.0)
    context.stroke(glyphPosition)

}

It took some time to work out and feels a bit messy. I was wondering if there are any Core Text experts out there who know some tricks to shorten this code.

You can get rid of the "unsafe bit casts", an CFArrayRef can
be bridged to [CTLine] (resp. [CTRun]) directly (tested with Swift 3 and Swift 4).

If you know for
sure that there is at least one line with at least one run then
it would be:

let lines = CTFrameGetLines(frame) as! [CTLine]
let line = lines.first!
let runs = CTLineGetGlyphRuns(line) as! [CTRun]
let run = runs.first!

Otherwise use optional binding with if let or guard let.
A most defensive version would be

guard let lines = CTFrameGetLines(frame) as? [CTLine],
    let line = lines.first,
    let runs = CTLineGetGlyphRuns(line) as? [CTRun],
    let run = runs.first else { return }


Convert XML files to JSON and move them What the code does (tested basic, no errors, usecase):

Reads all XML files (ending in .yin) from a directory.
converts each to JSON
store it in another directory, with the same basename as source file
... with .yin replaced by .json

Overt issues:

The code has too many nested functions
walk module's next() method in multiple places
lack of robust error checks and ensuring no uncaught errors left behind

Q. How to refactor the code using Node.js best practices?
(things like):

flatten out the nesting.
avoiding having to remember to call next(), on all if(err) scenarios.
does any other (than those included) module improve readability/efficiency?

Two Node.js files, a module and a main file invoking that module, are attached.

File 1: Main:

// main.js
var path = require ('path');
var yin2json = require ('./yin2json');

console.log ("__dirname: " + __dirname);
var templateDir = path.resolve (__dirname, "resources", "mgmtDataTemplates");
var yinDir = path.resolve (templateDir, "yins");
var jsonDir = path.resolve (templateDir, "jsons");

yin2json.convertYins (yinDir, jsonDir);

File 2: Module:

// yin2json.js

    var fs = require('fs');
    var path = require('path');
    var walk = require('walk');
    var xml2js = require('xml2js');

    var jsonDir ;
    var convertYins = function (yin_dir, json_dir) {
        jsonDir =  json_dir;

        var walker = walk.walk(yin_dir, { followLinks: true });
        walker.on("errors", fDirWalkError);
        walker.on("end", fDirWalkEnd);
        walker.on("file", fDirWalkFile);

    }

    function fDirWalkError (err) {
        console.log ("fDirWalkError: " + err);
        next (err);
    }

    function fDirWalkEnd  () {
        console.log ("======= End of directory walk");
    }

    function fDirWalkFile (root, fileStat, next) {
        if (fileStat.name.indexOf(".yin") < 0) {
            console.log ("skipping file " + fileStat.name + " (does not end in .yin)");
            return;
        } else {
            var yin_file = path.resolve(root, fileStat.name);
            console.log ("yin file: " + yin_file);

            fs.readFile(yin_file, function (err, data) {
                if (err) {
                    console.log ("error reading file:" + yin_file);
                    next (err);
                }
                xml2js.parseString (data, function (err, json_obj) {
                    if (err) {
                        console.log (err);
                        next (err);
                    }
                    var json_string = JSON.stringify(json_obj, null, 2);
                    var json_file = path.resolve (jsonDir, path.basename(yin_file).replace(/\.yin$/, ".json"));
                    console.log ("json file: ", json_file);

                    fs.writeFile(json_file, json_string, "utf8", function (err) {
                        if (err) {
                            console.log ("error converting yin (%s) to json(%s)", yin_file, json_file);
                            next (new Error ("error converting yin(" + yin_file + ") to json(" + json_file + ")"));
                        }
                        else {
                            console.log ("Converted yin (%s) to json(%s)", yin_file, json_file);
                        }
                    });
                });
            });
        }
        next ();
    }

module.exports.convertYins = convertYins;

Try using a Promise library like bluebird. it will save you from callback hell and allow you to write your code synchronously.

let Promise = require('bluebird'),
    fs = Promise.promisifyAll(require('fs'))

function readAllFilesInDir(pathArr) {
    // assume i get all files path as array here
    let allPromises = pathArr.forEach(function (path) {
        return fs.readFileAsync(path, 'utf8')
    })

    return allPromises
}

function doYourThing() {
    // body...
}
readAllFilesInDir(pathArr)
.then(doYourThing)


Save token id into a database in Swift I want to save my users tokens id into my database using POST request in Swift (here PHP code here). I am a bit confused about synchronous and asynchronous request.

Is this code the right way to do it?
would the app crash if there is a connection problem while it is sending the synchronous request
shall I use an api key (not sure is the correct word) to "identify" the user

func application(application: UIApplication, didRegisterForRemoteNotificationsWithDeviceToken deviceToken: NSData) {

    var userId = "001" //I just use it as USER number 1

    // prepare the tolen to be saved from <xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx xxxxxxxx> to xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
    var token = NSString(format: "%@", deviceToken)
    token = token.stringByReplacingOccurrencesOfString(
        "<",
        withString: "")
    token = token.stringByReplacingOccurrencesOfString(
        ">",
        withString: "")
    token = token.stringByReplacingOccurrencesOfString(" ", withString: "")

    dispatch_async(dispatch_get_main_queue(), { () -> Void in

        var postBody = NSString(format: "user=%@&token=%@", userId, token)
        var endBody = NSURL(string: "http://www.myServer.com/api/v1.0/saveToken.php")
        var request = NSMutableURLRequest(URL: endBody!, cachePolicy: NSURLRequestCachePolicy.ReloadIgnoringLocalCacheData, timeoutInterval: 30.0)

        request.HTTPMethod = "POST";
        request.HTTPBody = postBody.dataUsingEncoding(NSUTF8StringEncoding)
        request.setValue("application/x-www-form-urlencoded", forHTTPHeaderField: "Content-Type")

        var response: NSURLResponse?
        var error: NSError?

        NSURLConnection.sendSynchronousRequest(request, returningResponse: &response, error: &error)

        println(response)
        println(error)

    })

    }

This:

var token = NSString(format: "%@", deviceToken)
token = token.stringByReplacingOccurrencesOfString("<", withString: "")
token = token.stringByReplacingOccurrencesOfString(">", withString: "")
token = token.stringByReplacingOccurrencesOfString(" ", withString: "")

is actually a bad method to convert an NSData object to an NSString
(containing the bytes in hexadecimal). It relies on description
having the format

<01020304 05060708 090a0b0c 0d0e0f10>

which is not officially documented. In most cases, the description of
an object is only suitable for debugging purposes, but not for further
processing.

I would convert all data bytes explicitly to create the string. Here is a possible implementation as an NSData extension method:

extension NSData {
    func hexString() -> String {
        // "Array" of all bytes:
        let bytes = UnsafeBufferPointer<UInt8>(start: UnsafePointer(self.bytes), count:self.length)
        // Array of hex strings, one for each byte:
        let hexBytes = map(bytes) { String(format: "%02hhx", $0) }
        // Concatenate all hex strings:
        return "".join(hexBytes)
    }
}

which can be used as

let token = deviceToken.hexString()

With

dispatch_async(dispatch_get_main_queue(), { ... }

you dispatch the URL request to the main queue where it blocks the
UI. You can use dispatch_get_global_queue() instead to dispatch
it to a background thread, or simply use sendAsynchronousRequest():

NSURLConnection.sendAsynchronousRequest(request, queue: NSOperationQueue.mainQueue()) {
    (response, data, error) -> Void in

    if data != nil {
        println("data: \(data)")
    } else {
        println("failed: \(error.localizedDescription)")
    }
}

Now the request is done in the background and then the completion
handler called on the main thread.

In the case of an connection problem, the completion handler is called
with data == nil and error != nil containing information about the
problem.

  shall I use an api key (not sure is the correct word) to "identify" the user

This question is a bit broad and depends on what you are trying to
achieve. Here are some comparisons of the available methods:

CFUUID Vs. advertisingIdentifier Vs. identifierForVendor
The Developer’s Guide to Unique Identifiers

which might serve as a starting point for your further research.


How do I simplify the following slice of a Makefile? Considering the following Makefile:

%.o: %.cpp
    $(C) $(CF) -c $< -o $@

Audio/%.o: Audio/%.cpp
    $(C) $(CF) -c $< -o $@

Game/%.o: Game/%.cpp
    $(C) $(CF) -c $< -o $@

Generation/%.o: Generation/%.cpp
    $(C) $(CF) -c $< -o $@

Input/%.o: Input/%.cpp
    $(C) $(CF) -c $< -o $@

Particle/%.o: Particle/%.cpp
    $(C) $(CF) -c $< -o $@

Screen/%.o: Screen/%.cpp
    $(C) $(CF) -c $< -o $@

Utility/%.o: Utility/%.cpp
    $(C) $(CF) -c $< -o $@

How can I simplify it so that it only depends on a list of directories, such as:

SUBD = Audio/\
 Game/\
 Generation/\
 Input/\
 Particle/\
 Screen/\
 Utility/

All of these rules

Audio/%.o: Audio/%.cpp
        $(C) $(CF) -c $< -o $@

# etc.

are redundant!  Your first pattern rule

%.o: %.cpp
        $(C) $(CF) -c $< -o $@

already covers them all.


Spread values to an object (Computed property names) I would like to get a review of something,

I have a function that creates an array of objects for a Drop-Down component (react-select).

The function gets data, valueKey, labelKey and otherInfo from an object,
and creates the array of objects based on the valueKey and the labelKey.

const createOptions = ({data, valueKey, labelKey, ...otherInfo}) => {
  return data.map(item =>
    ({
      value: item[valueKey],
      label: item[labelKey],
      ...appendOtherInfo(item, otherInfo)
    })
  );
}

most of the time, valueKey and labelKey are enough, but there are some cases where there is additional info to add to each object, which is otherInfo, which is an object as well.

appendOtherInfo() function creates an object which is then appended to each object that map iteration returned.

const appendOtherInfo = (item, otherInfo) => {
  return Object.entries(otherInfo).reduce((total, [key, value]) => {
    total[key] = item[value];

    return total;
  }, {});
}

for example, after calling createOptions() with the array:

const people = [
  {id: '123', name: 'Dani', type: 'a', city: 'Rome'},
  {id: '222', name: 'John', type: 'a', city: 'London'},
  {id: '333', name: 'David', type: 'b', city: 'Madrid'}
]

and the rest of the info to create the new array,

createOptions({
  data: people,
  valueKey: 'id',
  labelKey: 'name',
  type: 'type',
  capitalCity: 'city'
});

The result is:

[
  {
    "value": "123",
    "label": "Dani",
    "type": "a",
    "capitalCity": "Rome"
  },
  {
    "value": "222",
    "label": "John",
    "type": "a",
    "capitalCity": "London"
  },
  {
    "value": "333",
    "label": "David",
    "type": "b",
    "capitalCity": "Madrid"
  }
]

In the appendOtherInfo() function, I used Object.entries and reduce to create the additional info for each object.
It's working as expected, but I'd like to know if there is a better/easier way to achieve the same result.

Your solution is okay; although I'd make appendOtherInfo() a local function of createOptions(). But the problem is, in terms of abstraction - you build a function that you want to re-use and that gives you the feeling it is generic enough for all future use cases for your drop down. However, there are two problems - your "generic" function is just generic enough for flat objects and cannot easily handle nested objects.

Furthermore because it's only argument is an object, different properties in it have different meanings - this is not good for code readability.

So my advice is, why bother writing something that is seemingly generic, but actually not generic enough that it will ever see proper reuse.

Instead simply use direct mappers; they are more readable and they give you the full flexibility, so instead of:

<react-select options = { createOptions({
  data: people,
  valueKey: 'id',
  labelKey: 'name',
  type: 'type',
  capitalCity: 'city'
})}>/

Simply do:

<react-select options = { people.map(peopl => {
  value: peopl.id,
  label: peopl.name,
  type: peopl.type
  capitalCity: peopl.city
}
}>/

(I did not look up <react-select>'s API):
Do not hide simple extracting and remapping behind functions if there is no need to - make code explicit and readable.

You can happily use destructuring with renaming and ..., if you only need to map label and value as well:

<react-select options = { people.map(peopl => ({
  const {id: value, name: label, ...other} = peopl;
  return {value, name, ...other}})
)}>/

Or to be explicit and combining above:

<react-select options = { people.map(peopl => ({
  const {id: value, name: label, type, city: capitalCity} = peopl;
  return {value, name, type, capitalCity}})
)}>/


Fixing logs that overlap Here we trying to fix logs that have overlaps with each other. Am I doing it right? Is there any way to improve/refactor it?

 DROP TYPE IF EXISTS logs_with_error;
  CREATE TYPE logs_with_error AS (
      model_id INT,
      model_type TEXT,
      initiator_id INT,
      initiator_type TEXT,
      error_type_id INT,
      state INT
  );

  CREATE OR REPLACE FUNCTION normalize_moderation_log_overflow() RETURNS INTEGER AS
      $BODY$
  DECLARE
      logs logs_with_error%rowtype;
      model moderation_logs%rowtype;
      initiator moderation_logs%rowtype;
  BEGIN
      FOR logs IN SELECT model_id, model_type, initiator_id, initiator_type, error_type_id, state
    FROM errors
    WHERE state = #{ Error.states[:initial] }
    AND error_type_id = (SELECT id FROM error_types WHERE normalizer_type = 'moderation_log_overflow')
      LOOP
        SELECT * INTO model FROM moderation_logs WHERE id = logs.model_id;
        SELECT * INTO initiator FROM moderation_logs WHERE id = logs.initiator_id;

        IF model.curr_state = initiator.curr_state AND model.next_state = initiator.next_state THEN
          IF model.ended_at < initiator.ended_at THEN
            UPDATE moderation_logs SET ended_at = initiator.ended_at WHERE id = model.id;
          END IF;
          DELETE FROM moderation_logs WHERE id = initiator.id;
        ELSE
          UPDATE moderation_logs SET started_at = model.ended_at WHERE id = initiator.id;
        END IF;

        UPDATE errors
        SET state = #{ Error.states[:resolved] };

        RAISE NOTICE '%', logs;
      END LOOP;
      RETURN 1;
  END
  $BODY$
  LANGUAGE 'plpgsql' ;

There's really only rather small things I'd change / adapt

CREATE OR REPLACE FUNCTION normalize_moderation_log_overflow() RETURNS INTEGER AS

Since you always return 1, this function actually should be a void.

CREATE OR REPLACE FUNCTION normalize_moderation_log_oveflow () RETURNS VOID AS

Your Indentation seems off a little:

BEGIN
    FOR logs ...
  FROM ...

I'd prefer to indent the SELECT statement in reference to the FOR .. LOOP like this:

BEGIN
  FOR logs IN SELECT ...
      FROM errors

AND error_type_id = (SELECT id FROM error_types WHERE normalizer_type = 'moderation_log_overflow')

This can (and probably should be) extracted into a constant

log_overflow CONSTANT INT := SELECT id FROM error_types WHERE normalizer_type = 'moderation_log_overflow' LIMIT 1;

Which makes the condition look like this:

AND error_type_id = log_overflow

    UPDATE errors
    SET state = #{ Error.states[:resolved] };

All other DML statements you have are in a single line, why not this one, too?

UPDATE errors SET state = #{ Error.states[:resolved] };


Rendering the AuthorName using the DOMParser to read XML I need to concatenate the LastName and Initials and all of that authors affiliations. Is there a better way or faster way to do this?  I'm very new to working XML in JavaScript.

This is a portion of an XML string that I get back from an API that I parse in JavaScript:

   <AuthorList CompleteYN="Y">
                <Author ValidYN="Y">
                    <LastName>Basree</LastName>
                    <ForeName>Mustafa M</ForeName>
                    <Initials>MM</Initials>
                    <AffiliationInfo>
                        <Affiliation>The Cancer Center...</Affiliation>
                    </AffiliationInfo>
                </Author>
                <Author ValidYN="Y">
                    <LastName>Shinde</LastName>
                    <ForeName>Neelam</ForeName>
                    <Initials>N</Initials>
                    <AffiliationInfo>
                        <Affiliation>The Comprehensive..</Affiliation>
                    </AffiliationInfo>
                </Author>
                <Author ValidYN="Y">
                    <LastName>Koivisto</LastName>
                    <ForeName>Christopher</ForeName>
                    <Initials>C</Initials>
                    <AffiliationInfo>
                        <Affiliation>Hollings ...</Affiliation>
                    </AffiliationInfo>
                    <AffiliationInfo>
                        <Affiliation>Department ...</Affiliation>
                    </AffiliationInfo>
                </Author>
                <Author ValidYN="Y">
                    <LastName>Cuitino</LastName>
                    <ForeName>Maria</ForeName>
                    <Initials>M</Initials>
                    <AffiliationInfo>
                        <Affiliation>Hollings...</Affiliation>
                    </AffiliationInfo>
                    <AffiliationInfo>
                        <Affiliation>Department ..</Affiliation>
                    </AffiliationInfo>
                </Author>
</AuthorList>

my Code:

      let HTMLContent = ""
      parser = new DOMParser();
      xmlDoc = parser.parseFromString(response.data, "text/xml");

      const AuthorsInfo = xmlDoc.querySelectorAll("Author");

      AuthorsInfo.forEach(a => {
        let affiliations = "";
        let AuthorName = "";

        for (var i = 0; i < a.children.length; i++) {
          let tt = a.children[i].nodeName;
          let t = a.children[i].textContent;
          if (tt === "AffiliationInfo") {
            affiliations += a.children[i].textContent;
            affiliations += "

";
          } else if (tt === "LastName") {
            AuthorName = a.children[i].textContent;
          } else if (tt === "Initials") {
            AuthorName += " " + a.children[i].textContent;
            AuthorName += "

";
          }
        }
        HTMLContent  += AuthorName + " " + affiliations;
      });

Style

Inconsistent use of line ending semi colon

Undeclared variables parser and xmlDoc

Unused code. let t = a.children[i].textContent; t is never used the line should not be there

Poor naming

Capitalisation of variable names. Only if objects to be instanciated via new token, if accronim (eg HTML, XML) or if a constant (optional).
Examples; AuthorsInfo should be authorsInfo,  AuthorName as authorName, xmlDoc as XMLDoc.
Non descripttive names, a, tt, t
Semanticly inacurate names, AuthorsInfo either authorNodes or just authors

Use for of loops rather than for if loops if you do not need the index counter

Use for of loops rather than Array iterators

Function scoped variables should be declared as var and hoisted to the top of the function

Code design
As you have several potential errors in the code (undeclared variables) you should ALWAYS use the directive "use strict" in your code to ensure these types of syntax problems are caught early in the development process.
Always present your code as a function/s. Presenting code as flat global scoped source string is very poor design practice.
Flat code often ends up performing many tasks that should be separated. Writing functions helps you define task roles under clearly named functions.
The parsing of the XML to HTML is more than one task. I suggest you have two functions parseAuthors to extract the author details, and authorsToHTML to create the markup.
Example rewrite
From your code it is unclear as to the nature of the XML. Can authors have no last name, initials, or affiliations. Do authors always have one last name, does the last name always come before the initials. And many more unanswered questions regarding the XML.
The example assumes that the XML given defines the XML.
The example breaks the code into many smaller functions and puts them all together inside a IIF (immediately invoked function). In the real world you would put all the code inside a module.

"use strict";
// As IFF or better as module (No need for use strict if module)
const authors = (() => {
    const AUTHORS = "Author";
    const AFFILIATION = "AffiliationInfo";
    const LAST_NAME = "LastName";
    const INITIALS = "Initials";
    const SEP = "

";
    const nodeTxt = (node, name) => (name ? node.querySelector(name) : node).textContent;

    function Author(authorNode) {
        const author = {
            lastName: nodeTxt(authorNode, LAST_NAME),
            initials: nodeTxt(authorNode, INITIALS),
            affiliations: []
        }
        for (const affNode of authorNode.querySelectorAll(AFFILIATION)) {
            author.affiliations.push(nodeTxt(affNode));
        }
        return author;
    }

    return {
        parse(XMLString) {
            const authors = [];
            const authorNodes = new DOMParser()
                .parseFromString(XMLString, "text/xml")
                .querySelectorAll("Author");
            for (const authorNode of authorNodes) { authors.push(Author(authorNode)) }
            return authors;
        },
        toHTML(authors) {
            var markup = "";
            for (const author of authors) {
                markup += author.lastName + " " + author.initials + SEP;
                markup += " " + author.affiliations.join(SEP) + SEP;
            }
            return markup;
        },
    };
})();
document.body.innerHTML = authors.toHTML(authors.parse(getXML()));

function getXML() { return `<AuthorList CompleteYN="Y">
<Author ValidYN="Y">
    <LastName>Basree</LastName>
    <ForeName>Mustafa M</ForeName>
    <Initials>MM</Initials>
    <AffiliationInfo>
        <Affiliation>The Cancer Center...</Affiliation>
    </AffiliationInfo>
</Author>
<Author ValidYN="Y">
    <LastName>Shinde</LastName>
    <ForeName>Neelam</ForeName>
    <Initials>N</Initials>
    <AffiliationInfo>
        <Affiliation>The Comprehensive..</Affiliation>
    </AffiliationInfo>
</Author>
<Author ValidYN="Y">
    <LastName>Koivisto</LastName>
    <ForeName>Christopher</ForeName>
    <Initials>C</Initials>
    <AffiliationInfo>
        <Affiliation>Hollings ...</Affiliation>
    </AffiliationInfo>
    <AffiliationInfo>
        <Affiliation>Department ...</Affiliation>
    </AffiliationInfo>
</Author>
<Author ValidYN="Y">
    <LastName>Cuitino</LastName>
    <ForeName>Maria</ForeName>
    <Initials>M</Initials>
    <AffiliationInfo>
        <Affiliation>Hollings...</Affiliation>
    </AffiliationInfo>
    <AffiliationInfo>
        <Affiliation>Department ..</Affiliation>
    </AffiliationInfo>
</Author>
</AuthorList>`
};


XSL template for converting XML to JSON I used saxon v9 to profile an XSL transformation which converts XML to JSON. The profiler tells me that the function which escapes certain characters takes about 70% of the total processing time. The conversion is important because otherwise the created JSON file will be invalid because of characters that break the strings.

java -jar saxon9he.jar -xsl:jsontransform.xslt -s:input.xml -o:output.json -TP

The "method" used to escape the values looks like this:

<xsl:template name="escapejson">
        <xsl:param name="string"/>
        <xsl:sequence select="replace(
                              replace(
                              replace(
                              replace(
                              replace(
                              replace(
                              replace(
                              replace(
                              replace($string, '\\','\\\\'),
                              '/', '\\/'),
                              '&quot;', '\\&quot;'),
                              '&#xA;','\\n'),
                              '&#xD;','\\r'),
                              '&#x9;','\\t'),
                              '\n','\\n'),
                              '\r','\\r'),
                              '\t','\\t')"/>
    </xsl:template>

Unfortunately, my XSL knowledge is too limited to create an optimum solution on my own. I assume the repeated call of replace is quite inefficient but don't know how to do it better.

I can believe it when the replace is the problem... it's creating a mess of nested code that's creating a lot of intermediate string values.

There's not much I can see to improve, though. The nesting can be reduced by combining a few of replaces...:

replace(.... , '\n|&#xA', '\\n')

Doing the same for \t and \r will reduce six calls to just three.

Additionally, I would consider wrapping the entire replace function in a matches call, that matches any of the patterns you are searching for, so you only need to do the full replacement call stack on values that actually require it. A large matches for all replaced values will, depending on how often values are actually required to be replaced, save time.


ASP.net service that sends email whenever an entity is updated I have an ASP.net MVC5 project which involve different domain services, and one of it are EmailService which will send email if entity is updated.

public class Project {
    string ProjectType {get; set;}
    string Name {get; set;}
}

public interface IProjectService {
    event void EventHandler<ProjectEventArgs> Updated;
}

public interface IEmailLogRepository {
    void Insert(EmailLog obj);
}

public class EmailService {

    private readonly IEmailLogRepository _repos;

    public string SmtpServer {get;set;}
    public string SmtpPort {get;set;}
    public string SmtpSender {get;set;}
    public string SmtpUsername {get;set;}
    public string SmtpPassword {get;set;}
    ......

    public string TemplateASubjectFormat {get;set;}
    public string TemplateBSubjectFormat {get;set;}
    public string TemplateAContentFormat {get;set;}
    public string TemplateBContentFormat {get;set;}

    public EmailService(IProjectService service, IEmailLogRepository repos) {
        service.Updated += OnProjectUpdated;
        _repos = repos;
    }

    private void OnProjectUpdated(object sender, ProjectEventArgs args) {
        if (args.Entity.ProjectType == "A")  SendEmailWithTemplateA(args.Entity);
        if (args.Entity.ProjectType == "B")  SendEmailWithTemplateB(args.Entity);
    }

    private void SendEmailWithTemplateA(Project obj){
        MailMessage mail = new MailMessage();
        mail.Suject = string.Format(TemplateASubjectFormat, obj.Name);
        mail.Content = string.Format(TemplateAContentFormat, obj.Name);
        ......
        SendEmail(mail);
    }

    private void SendEmailWithTemplateB(Project obj){
        MailMessage mail = new MailMessage();
        mail.Suject = string.Format(TemplateBSubjectFormat, obj.Name);
        mail.Content = string.Format(TemplateBContentFormat, obj.Name);
        ......
        SendEmail(mail);
    }

    private void SendEmail(MailMessage mail){

        EmailLog log = new EmailLog();
        log.Email = mail.ToAdress;

        try{
            SmtpClient smtp = new SmtpClient();
            ........
            smtp.Send(mail);
            log.Status = "Success";
            _repos.Insert(log);
        }catch(Exception ex){
            log.Status = string.Format("Error: {0}", ex.Message);
            _repos.Insert(log);
        }
    }
}

public class ProjectController : Controller
{
    private readonly IProjectService _service;
    private readonly EmailService _emailService;

    public ProjectController(IDbConnection connection, IProjectService service, IEmailLogRepository emailRepos){
        _service = service;
        _emailService = new EmailService(_service, emailRepos);
        Dictionary<string, string> settings = Settings.Load(connection);
        _emailService.SmtpServer = settings["SmtpServer"];
        ........
        _emailService.TemplateASubjectFormat = settings["TemplateASubjectFormat"];
        _emailService.TemplateAContentFormat = settings["TemplateAContentFormat"];
        _emailService.TemplateBSubjectFormat = settings["TemplateBSubjectFormat"];
        _emailService.TemplateBContentFormat = settings["TemplateBContentFormat"];
        ........
    }
}

In the above code, the EmailService contains lots of settings like Smtp and template info for different project types, those settings are located in database currently (one settings table but it stored all settings for different services), and the service properties are assigned in ASP.net controller constructor.

This kind of setting properties assignment caused the constructor contains a lots of lines, some controller may require more than one service to work, each services in my projects are designed like this, and I feel it's not a good design, so may I check what's the best practice for assign settings value for setting assignment?

The very first problem is to move some responsibilities out of the controller. IMHO controllers should do as little as possible because their responsibility should be to glue the HTTP request to the domain model.

In your case the controller should not create and configure the service also because it makes impossible (OK...just hard) to test both in isolation. Please pick a better name but the very first thing to do is to receive an IEmailService service exactly the same way you receive the others.

This MIGHT not be always possible without some major refactoring then I'd start introducing a factory:

public ProjectController(IProjectService service, IEmailServiceFactory factory) {
   _service = service;
   _emailService = factory.GetService();
}

That hypothetical factory method is simply:

public GetService() {
    var emailService = new EmailService(_service, emailRepos);
    var settings = Settings.Load(connection);
    emailService.SmtpServer = settings["SmtpServer"];

    // ...
    emailService.TemplateASubjectFormat = settings["TemplateASubjectFormat"];
    // ...

    return emailService;
}

This is easy because you're just moving your code from one class to another and you introduced another service.

Now controllers came back to a reasonable level of responsibilities (and complexity) and they're also testable because you can mock the IEmailServiceFactory service (no need to send tons of e-mails when running your tests on the controller...)

As soon as you're ready (or simply as a second step on your refactoring) you should receive the IEmaiLService service directly - instead of going trough a factory.

There are also few things to say about the other code. Those initialization lines are tedious, if you're OK to tie your EmailService to its DB representation then you can decorate its properties:

[ConfigurationColumName("SmptServer")]
public string SmtpServer {get;set;}

You can then use a touch of Reflection to enumerate its properties:

foreach (var property in typeof(EMailService).GetProperties()) {
    var attribute = property.GetCustomAttribute<ConfigurationColumNameAttribute>();
    if (attribute == null) {
        continue;
    }

    property.SetValue(obj, settings[attribute.Name]);
}

Of course you need to define a ConfigurationColumNameAttribute class and add proper error handling.

We then arrive to the second error-prone (and not scalable) part of your code. You hard-coded the number of templates. What if you will ever need 3? Or 4? Or just one? You may simply use a dictionary (if not an array):

public Dictionary<string, TemplateDefinition> Templates { get; } = new ...;

Where TemplateDefinition has Subject and Content properties. Code should now be simple enough that you may even avoid Reflection.

SendEmailWithTemplateA() and SendEmailWithTemplateB() are almost exact duplicates, if you use a dictionary then there should not be any repeated code.

Mechanism you're using for templating is fragile. It's hard to write long strings using {0} and {1} as placeholders. Do yourself a BIG favor and switch to a small, fast and well-tested templating engine like Mustache. You'll be able to write your template as Hello {{username}}, we write to you to inform you about the new {{product}}.... It'll be much easier to configure and edit your templates and you'll minimize the chances to mix-up things when changing something in your code. Do not forget that this has to be tested too.

Do not catch Exception, is OutOfMemoryException or AccessViolationException something you want to handle that way?


Simulation study in R with two variables and 10000 iterations The generic version of what I am trying to do is to conduct a simulation study where I manipulate a few variables to see how that impacts a result. I'm having some speed issues with R. The latest simulation worked with a few iterations (10 per experiment). However, when I moved to a large-scale version (10k per experiment), the simulation takes too long (it has been running for 14 hours and is still going).

Below is the code (with comments) that I am running. Being a rookie with R, I am struggling to optimize the simulation. My hope is to learn from the comments and suggestions provided here to optimize this code and use the code future simulation studies.

Let me say a few things about what this code is supposed to do. I am manipulating two variables: effect size and sample size. Each combination is run 10k times (i.e., 10k experiments per condition). I initialize a data frame to store my results (called Results). I loop over three variables: Effect size, sample size, and iterations (10k).

Within the loops, I initialize four NULL components: p.test, p.rep, d.test, and d.rep. The former two capture the p-value of the initial t-test and the p-value of the replication (i.e., replicated under similar conditions). The latter two calculate the effect size (Cohen's d).

I generate my random data from a standard normal for the control condition (DVcontrol), and I use my effect size as the mean for the experimental condition (DVexperiment). I take the difference between the values and throw the result into the t-test function in R (paired-samples t-test). I store the results in a list called Trials and I rbind this to the Results data frame. This process is repeated 10k times until completion.

# Set Simulation Parameters
## Effect Sizes (ES is equal to mean difference when SD equals Variance equals 1)
effect_size_range <- seq(0, 2, .1) ## ES
## Sample Sizes
sample_size_range <- seq(10, 1000, 10) ## SS
## Iterations for each ES-SS Combination
iter <- 10000

# Initialize the Vector of Results
Results <- data.frame()

# Set Random Seed
set.seed(12)

# Loop over the Different ESs
for(ES in effect_size_range) {

  # Loop over the Different Sample Sizes
  for(SS in sample_size_range) {

    # Create p-value Vectors
    p.test <- NULL
    p.rep <- NULL
    d.test <- NULL
    d.rep <- NULL

    # Loop over the iterations
    for(i in 1:iter) {

      # Generate Test Data
      DVcontrol <- rnorm(SS, mean=0, sd=1)
      DVexperiment <- rnorm(SS, mean=ES, sd=1)
      DVdiff <- DVexperiment - DVcontrol
      p.test[i] <- t.test(DVdiff, alternative="greater")$p.value
      d.test[i] <- mean(DVdiff) / sd(DVdiff)

      # Generate Replication Data
      DVcontrol <- rnorm(iter, mean=0, sd=1)
      DVexperiment <- rnorm(iter, mean=ES, sd=1)
      DVdiff <- DVexperiment - DVcontrol
      p.rep[i] <- t.test(DVdiff, alternative="greater")$p.value
      d.rep[i] <- mean(DVdiff) / sd(DVdiff)
    }

    # Results
    Trial <- list(ES=ES, SS=SS,
                  d.test=mean(d.test), d.rep=mean(d.rep),
                  p.test=mean(p.test), p.rep=mean(p.rep),
                  r=cor(p.test, p.rep, method="kendall"),
                  r.log=cor(log2(p.test)*(-1), log2(p.rep)*(-1), method= "kendall"))
    Results <- rbind(Results, Trial)
  }
}

Possible Bug

If I understand what you are trying to do correctly, the lines...

DVcontrol <- rnorm(iter, mean=0, sd=1)
DVexperiment <- rnorm(iter, mean=ES, sd=1)

...should probably be

DVcontrol <- rnorm(SS, mean=0, sd=1)
DVexperiment <- rnorm(SS, mean=ES, sd=1)

This was noted in the comments by @flodel.

Identifying slow steps

The best way to figure out what lines of the code are slowest is to use a line profiling tool.  I like profvis.  In general, the slow steps in R are element-wise assignment to vectors/matrices/data.frames, and unvectorized functions.  In your case,

p.rep[i] <- t.test(DVdiff, alternative="greater")$p.value
d.rep[i] <- mean(DVdiff) / sd(DVdiff)

are likely to be slow. And the t.test() function in R is unvectorized as also a bottleneck.

Possible solutions

Vectorization of assignments

I'm a big fan of the tidyverse system of packages.  For this case, the tidyr, purrr, and dplyr system of packages allows a very nice functional implementation of your problem.  However, they won't automatically vectorize functions like t.test().

Vectorization of t.test()

Googling around, it appears there are a few vectorized implementations of t.test().  One I found easy to use is from the genefilter package, colttest().  And although it isn't as much of a bottleneck, I also used a vectorized computation of d by using the colSds() and colMeans() functions from matrixStats.

Putting it together

Below I implement some of what your original code does.  I didn't worry about the particular flavor of t-test that you were using (i.e. alternative = "greater"), and I didn't implement computing the correlation between p.values.

require(tidyverse)
require(purrr)
require(genefilter)
require(matrixStats)

N_ITER <- 10000
effect_size_range <- seq(0, 2, 0.5)
sample_size_range <- seq(10, 1000, 100)
test_or_repeat <- c('test', 'rep')

input <-
    expand.grid(effect_size_range, sample_size_range, test_or_repeat)
names(input) <- c('ES', 'SS', 'test_or_rep')

output <-
    input %>%
    group_by(ES, SS) %>%
    mutate(ctrl = map(SS, ~rnorm(.x * N_ITER)),
           expt = map2(SS, ES, ~rnorm(.x * N_ITER, mean=.y)),
           diff = map2(expt, ctrl, ~ .x - .y),
           diff.matrix = map(diff, ~matrix(., nrow=N_ITER)),
           p = map(diff.matrix,  ~colttests(., gl(1, nrow(.)))$p.value),
           d = map(diff.matrix, ~colMeans(.)/colSds(.)),
           mean.p = map(p, ~mean(.)),
           mean.d = map(d, ~mean(.))
          ) %>%
    select(ES, SS, test_or_rep, mean.p, mean.d) %>%
    unite(p_and_d, mean.p, mean.d, sep='___') %>%
    spread(test_or_rep, p_and_d) %>%
    separate(test, c('p.test', 'd.test'), sep='___') %>%
    separate(rep, c('p.rep', 'd.rep'), sep='___')

This run, which covers about 1/5th of the effect sizes and 1/10th of the sample sizes you are interested in, takes about 5 minutes to run on my laptop.  Since it uses N_ITER = 10000, I think scaling up to your full system size would take about 50 times this number, or about 4 hours.  The bottlenecks here are still the t-tests, but also simply calling rnorm, a sign that we got rid of the assignment-by-element bottlenecks and reduced the t-test bottlneck at least somewhat.


Ticket management system I am building a ticket management system for a state server we use at work.  Back end is node.js with multiple front ends.  I am rewriting my back end to be more maintainable as the scope grows.

For this particular function I start with this:

[
    {
        "id": 4993,
        "ticket": "A142032570",
        "sort": "07/22/14"
    },
    {
        "id": 5060,
        "ticket": "A142043078",
        "sort": "07/23/14"
    }
]

with many more entries than shown here.  My function takes this data and creates this:

[
    {
        "date": "07/22/14",
        "tickets": [
            {
                "id": 4993,
                "ticket": "A142032570"
            }
        ]
    },
    {
        "date": "07/23/14",
        "tickets": [
            {
                "id": 5060,
                "ticket": "A142043078"
            }
        ]
    }
]

The function used is:

function sortMyTicketsByDate(data){
    var arr = [];
    for(var o in data){
        arr.push(data[o].sort);
    }
    arr = _.uniq(arr);
    for(var i in arr){
        arr[i] = {date: arr[i]};
    }
    for(var d in arr){
        console.log(arr[d]);
        arr[d].tickets = [];
        var elem = arr[d]['date'];
        for(var o in data){
            //data[o].sort, ticket, id
            if (elem === data[o].sort) {
                arr[d]['tickets'].push({id: data[o].id, ticket: data[o].ticket});
            };
        }
    }
    return arr;
}

What improvements can be made to sortMyTicketsByDate to be more efficient/understandable?

As mentioned in the comments by @elclanrs, you should not use for...in to loop through arrays.
As mentioned by @megawac, lexical comparison will not produce the results you expect. So you'll need to parse those strings.
Use more descriptive names. data should probably be tickets, arr could be dates. Nothing should be just o or d. It makes the code much, much harder to read.
Remember to remove all console.log calls in production code (or do a check to make sure that console exists, and has a function called log before you try calling it - otherwise things break)

Since you seem to be using underscore.js, you should look at the groupBy function

var grouped = _.groupBy(tickets, function (ticket) { return ticket.date });

The above will create an object with the date strings as keys, and arrays of tickets as values. Pretty much the same as the first half of your current function.

But we can do more with underscore. All of it actually:

function sortMyTicketsByDate(tickets) {
  return _.chain(tickets)
    .groupBy(function (ticket) { return ticket.sort })
    .sortBy(function (group, date) {
      var parts = date.split(/\/0?/);
      return new Date(parts[3], parts[1] - 1, parts[2]);
    })
    .map(function (group, date) { return { date: date, tickets: group }; })
    .value();
}

Caveat emptor: The major difference between the above function and your original one, this one returns the same objects it was given, whereas yours return copies. So any change made to the tickets afterward are being made to the originals.

That also means that the sort property remains (whereas you just don't copy it)

To avoid these points (if you really need to, which I very much doubt you do), you can do this:

function sortMyTicketsByDate(tickets) {
  return _.chain(tickets)
    .map(_.clone) // copy objects
    .groupBy(function (ticket) { return ticket.sort })
    .sortBy(function (group, date) {
      var parts = date.split(/\/0?/);
      return new Date(parts[3], parts[1] - 1, parts[2]);
    })
    .map(function (group, date) { return { date: date, tickets: group }; })
    .each(function (group) { // remove sort property
      _.each(group.tickets, function (ticket) { delete ticket["sort"] });
    })
    .value();
}


Numeronym generation Problem statement
I need to generate numeronyms from a string, let's say qwerty as follows:
qwerty
q1erty
q2rty
q3ty
q4y

Code
Can this solution can be improved or if there's any better approach?

const str = 'qwerty';
const lastCharPos = str.length - 1;

str
  .split('')
  .map(
    (_, i) =>
      i !== lastCharPos &&
      console.log(`${str[0]}${i ? i : ''}${str.slice(i + 1, lastCharPos)}${str[lastCharPos]}`)
  );

Know the language
Part of being a programmer is being familiar with the language/s you are using. These days languages are changing yearly thus keeping up to date is very important. MDN JavaScript provides a good JavaScript reference. Every now and then its good to peruse the site to keep your knowledge up to date.
Strings
String.slice will slice to the end of the string if you don't include the second argument. Thus there is no need for the variable lastCharPos
If the arguments for String.slice are negative the position of the slice if from the end of the string. eg "abcd".slice(-2) will return "cd"
You can String.split a string using ... (Spread syntax) spread operator. eg[...str].map
Arrays
Array.map creates an array. You do not use the array and so you should have used Array.forEach
Often when using arrays and their iteration functions like map, it forces you to adapt the code to fit the array function's needs, increasing the complexity of the source and at runtime.  You do this twice...

The need to soak the first argument of map map((_, i) => which is a hack in my view.
As you have no control over the number of iterations you are forced to add two statements i !== lastCharPos and i ?  in the ternary expression i ? i : ''

Improving the code.
Always make your code portable by wrapping it in a function
Code should be focused on one task (role) at a time. Because you created the code inline (not a function) you have combined various tasks into one.

Creating the set of strings
Outputting to the console.

As a function you can separate the output from the creation, returning an array of strings that you can then use as needed. eg Output to console.
The Array iteration functions are too cumbersome for what your code does. You can use a while loop and push the built string to an array which you return at the end.
Rewrite
Your function does not output anything if the string length is <= 1. It is unclear if this is just an output requirement. The rewrite will return the input string (as first item in the array) no matter what its size
The string is sliced using a negative index to slice from the end of the string.
I used a while loop but you can also use a for loop
Using while loop
function numeronyms(str) {
    const result = [str], len = str.length;
    var i = 1;
    while (i < len - 1) { result.push(str[0] + (i++) + str.slice(i - len)) }
    return result;
}

Using for loop
function numeronyms(str) {
    const result = [str], len = str.length;
    for (let i = 2; i < len; i ++) {
        result.push(str[0] + (i - 1) + str.slice(i - len));
    }
    return result;
}

Usage
To use the function so that it behaves the same as your code it needs to check the string length.
const str = "qwerty";
if (str.length > 1) { console.log(numeronyms(str)) }

or
const str = "qwerty";
str.length > 1 && console.log(numeronyms(str));

Or just assign the array for use later
const numers = numeronyms("qwerty");


Credit card validity check I want to go more in depth, but this is the 'boilerplate'.

Is there any way I can optimize this or use more idiomatic Python code?

#Luhn algorithm - distinguish valid numbers from mistyped or incorrect numbers.
def checkNumber(cc_number=''):
    sum_ = 0
    parity = len(cc_number) % 2
    for i, digit in enumerate([int(x) for x in cc_number]):
        if i % 2 == parity:
            digit *= 2
            if digit > 9:
                digit -= 9
        sum_ += digit
    return sum_ % 10 == 0

def main():
    number = input('Input credit card number: ')

    if 12 < len(number) < 17:
        first_two = number[0:2]
        first_four = number[0:4]

        vendor = None
        if number[0] == 4:
            vendor = 'Visa'
        elif number[0] == '5' and '0' < number[1] < '6':
            vendor = 'Mastercard'
        elif number[0] == '6' or first_four == '6011':
            vendor = 'Discover'
        elif first_two in ('36', '38'):
            vendor = "Diners Club"
        elif first_two in ('34', '37'):
            vendor = 'American Express'

        if vendor is not None:
            if checkNumber(number):
                print(f'This is a valid {vendor} credit card!')
            else:
                print(f'This is not a valid {vendor} credit card.')
        else:
            print('Unknown vendor.')
    else:
        print('Sorry, this is not a valid credit card number!')

print(checkNumber('12345'))

Welcome to Code Review. A few basic pointers about programming in python:

Go through the code style guide as documented in PEP-8.
Instead of putting comments, make use of docstrings.
Use test framework for writing a few tests, and use asserts instead of print() for checks and validations.

From the PEP-8 rules:

Convert the comment about Luhn's algortihm into a docstring.
Variables and functions are named using snake_case convention.
No unnecessary blank lines are needed. Keep the code clean and concise.

Your main function can be broken further, into get_provider, valid_card_number and the one your already have check_number. It is completely your choice if you want to raise an exception in case of wrong inputs or whether the functions should return falsy values.

Instead of having an approximately 10 line of code inside if-statement, deal with the else clause first, and return early.

While you didn't actually write the check_number code, I'll just put my views about that as well here.

Using a list comprehension, with the help of % 9 (\$ \mod 9 \$), you can make it quite pythonic. You can then have a sum(<list comprehension>) % 10 == 0 as a one-liner.

sum(digit * 2 % 9 if i % 2 else digit for i, digit in enumerate(map(int, cc_number[::-1]))) % 10 == 0


String manipulation over 2 tables in database to application I have the following function in the database to do string manipulation:

create or replace FUNCTION convert_string
(
    input_string IN VARCHAR2
) RETURN VARCHAR2 AS
output_string VARCHAR(8);
BEGIN
    IF (input_string IS NULL) THEN
        output_string := '';
    ELSIF (NOT (SUBSTR(input_string, 6, 1) >= '0' AND SUBSTR(input_string, 6, 1) <= '9')) THEN
        output_string := '000' || SUBSTR(input_string, 1, 5);
    ELSE
        output_string := '00' || input_string;
    END IF;

    RETURN output_string;
END convert_string;

I'm using the function to perform checking on 2 tables, so my SQL query will be:

SELECT     t1.name
FROM       table1 t1
JOIN       table2 t2 on t1.id = convert_string(t2.id)

But let's say I do not want to create a convert_string function in my database, and would like to do everything on the application side. What is the best way to do it?

The function above can be converted to C# code:

public string ConvertString(input)
{
    if (String.IsNullOrEmpty(input))
        return "";
    else if (!(input[5] >= '0' && input[5] <= '9'))
        return "000" + input.Substring(0,5);
    else
        return "00" + input;
}

Then the SQL query is to be split into 2 database calls:

SELECT     *
FROM       table1

and

SELECT     *
FROM       table2

Those will be returned to their own data tables, and then do a loop or Linq to join these 2 tables. But I wonder whether there is a more efficient / better way to do this.

Even if I don't know the actual content, joining tables on client-side is basically a bad idea because you have to load the whole content to the client.

As BCdotWEB mentioned in the comment, the prefered way is to consolidate your data model to avoid such hacks. If that is not possible, your solution with the convert_string should work. If you need a more efficient solution, you could use function-based-indexes or virtual columns with indices, which should significantly increase query performance.

One comment to the posted code: The oracle function as well as the c# method do not check the length of the string before accessing the 6'th element which may result in an index out of range exception.


Check that an image is available to be served I have a couple of images in my database and I need to check if the images are still good or if they're deleted or corrupted. I came up with a solution but it's very slow. I was wondering if there was a way to make this faster.

img = 'http://some_image_cdn.com/images/image.jpg'
begin
  Nokogiri::HTML(open(img))
rescue
  # do something
end

To make this more efficient, you need to break down this problem into smaller pieces. From your description you are:

Testing the image to see if it exists
Testing the image to see if it is corrupted

The first test is pretty easy, and can be optimized by issuing a HEAD request to the server. The second test is where things get inefficient because you need to download the image and parse it.

Something like a remote image verifier that has methods to test for the image existence and corruption would be beneficial:

class RemoteImageVerifier
  def image_exists?(uri)
    Net::HTTP.start(uri.host, uri.port) do |http|
      begin
        response = http.request_head uri.path
        # check response["header-name"] if need be
        true
      rescue Net::HTTPNotFound
        # Image URL is no longer valid
        false
      rescue Exception
        # Another unknown error happened
        false
      end
    end
  end

  def image_corrupted?(uri)
    Net::HTTP.start(uri.host, uri.port) do |http|
      begin
        response = http.request_get uri.path
        # parse response.body as a JPEG
        false
      rescue Exception
        # Another unknown error happened
        true
      end
    end
  end
end

And to use it:

verifier = RemoteImageVerifier.new

verifier.image_exists? 'http://some_image_cdn.com/images/image.jpg'
verifier.image_corrupted? 'http://some_image_cdn.com/images/image.jpg'


Merge two already sorted linked list This is code to merge two sorted linked lists. l1 and l2 are sorted. While calling the merge function via l3, I am passing l3 as an object by reference in that function. The code is running fine, but can someone suggest how I can code better, with regards to dealing with class objects and passing them in a function?

#include<iostream>
using namespace std;

class linklist{
    private:
        struct node{
            int data;
            node* link;
        }*start;

    public:
        linklist(){
            start=NULL;
        }
        void append(int);
        void display();
        void merge(linklist, linklist,linklist &);
        ~linklist()
        {
            node *q;
            while(start!=NULL)
            {
                q=start->link;
                delete start;
                start=q;
            }
        }
};
void linklist::append(int num){

    node *temp, *r;
    temp=start;
    if(start==NULL)
    {temp=new node;
    temp->data=num;
    temp->link=NULL;
    start=temp;
    }
    else{

        while(temp->link!=NULL)
        {
            temp=temp->link;
        }
        r= new node;
        r->data=num;
        r->link=NULL;
        temp->link=r;
    }
}
void linklist::display() {
    node *temp;
    temp=start;
    cout<<"Linked List is"<<endl;
    while(temp!=NULL)
    {
        cout<<temp->data<<"  ";
        temp=temp->link;
    }
    cout<<endl;
}

void linklist::merge(linklist l1, linklist l2,linklist& l3) // Here is my doubt
{
    node *p, *q;
    p=l1.start;
    q=l2.start;

    int dat;
    while(p!=NULL && q!=NULL)
    {
        if(p->data > q->data)
        {
            dat=q->data;
            l3.append(dat);
            q=q->link;
        }

        else{
            dat=p->data;
            l3.append(dat);
            p=p->link;
        }
    }
    if(p==NULL)
    {
        while(q!=NULL)
        {
            dat=q->data;
            l3.append(dat);
            q=q->link;
        }
    }
    else{
            while(p!=NULL)
        {
            dat=p->data;
            l3.append(dat);
            p=p->link;
        }
    }
    cout<<"lists merged"<<endl;

}

int main()
{
    linklist l1,l2,l3;

    l1.append(2);
    l1.append(5);
    l1.append(23);
    l1.append(34);
    l1.append(45);

    l2.append(6);
    l2.append(9);
    l2.append(35);
    l2.append(98);

    l3.merge(l1, l2,l3);
    l3.display();

    return 0;
}

Here are some things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Use nullptr rather than NULL

Modern C++ uses nullptr rather than NULL.  See this answer for why and how it's useful.

Use more whitespace

Lines like this one:

cout<<"Linked List is"<<endl;

become much easier to read and understand with a little more whitespace:

cout << "Linked List is" << endl;

Use consistent formatting

The code seems to have inconsistent indentation and inconsistent placement of brackets {}.  It doesn't matter as much which convention you use as much as it matters that you follow some convention.  Doing so makes your code easier to read, understand and maintain.

Don't use std::endl if '\n' will do

Using std::endl emits a \n and flushes the stream.  Unless you really need the stream flushed, you can improve the performance of the code by simply emitting '\n' instead of using the potentially more computationally costly std::endl.  For instance, the line quoted above could be written like this:

std::cout << "Linked List is\n";

Prefer modern initializers for constructors

The constructor use the more modern initializer style rather than the old style you're currently using.  Instead of this:

    linklist(){
        start=NULL;
    }

you could use this:

    linklist() : start{nullptr} {}

This uses the more consistent {} form for initializing start (and so requires a C++11 compiler) but this can be done using older C++99 if needed.  There is not a significant difference in this code, but it's a good habit to get into using.

Use const where practical

The display member functions of linklist does not alter the underlying object and therefore should be declared const:

void display() const;

Make your class destructor virtual

If there's any chance that your class will be derived from, the class destructor should be virtual to avoid problems with collections of objects. See this question for more details on why.  If it can't be derived from, enforce that by declaring the class final.

With that said, as @isanae has correctly pointed out, you shouldn't be left with the impression that those are the only options.  For a plain old concrete class such as this one, you can reasonably just leave it as it is right now.

Return something useful from functions

The merge function takes two sorted linked lists and returns a new one; or it should.  Right now it's declared like this

void linklist::merge(linklist l1, linklist l2,linklist& l3)

but it would make much more sense like this:

linklist linklist::merge(linklist l1, linklist l2)

Better still would be for the first list to be implicit so that one could write this:

l3 = l1.merge(l2);

To do that, I'd write the function like this:

linklist linklist::merge(const linklist &l2) const {
    linklist l3;
    node *nodes[2]{start, l2.start};

    while (nodes[0] && nodes[1]) {
        size_t index = nodes[0]->data < nodes[1]->data ? 0 : 1;
        l3.append(nodes[index]->data);
        nodes[index] = nodes[index]->link;
    }
    l3 += nodes[0];   // one of the lists is empty at this point
    l3 += nodes[1];
    return l3;
}

Note that this assumes the existence of an operator+= which might be implemented like this:

linklist &operator +=(const node *p) {
    while (p) {
        append(p->data);
        p = p->link;
    }
    return *this;
}

Use operator<< instead of display

It would be convenient to be able to redirect the output to something other than std::cout, so the usual idiom is to use something like this:

friend std::ostream& operator<<(std::ostream& out, const linklist &list) {
    for (auto temp=list.start; temp; temp=temp->link) {
        out << temp->data << "  ";
    }
    return out << '\n';
}

Now we can use it within main like this:

std::cout << "finished merging\n" << l1 << l2 << l3;

Consider using a template

As the code is currently written, it can only store a single int as data, but with only a very minor change in the code, it could become a template.  The first few lines could look like this:

template <class T>
class linklist{
private:
    struct node{
        T data;
        node* link;

Thereafter, anywhere you specifically refer to an int for the data, replace it with the templated type T and you now have a generic container.  For your int version, you can write this:

linklist<int> l1;

Consider providing a std::initializer_list constructor

linklist(std::initializer_list<T> list) : start{nullptr} {
    for (auto item : list) {
        append(item);
    }
}

Implementing all of the suggestions above give a main like this:

int main()
{
    linklist<int> l1{2, 5, 23, 34, 45};
    linklist<int> l2{6, 9, 35, 98};

    std::cout << "Before merging\n" << l1 << l2;
    auto l3 = l1.merge(l2);
    std::cout << "After merging\n" << l1 << l2 << l3;
}

Don't destroy passed objects

Right now the code destroys the passed objects in creating the new list.  That's not a good design, especially because the lists are not even passed by reference, so it would be very surprising to someone looking at the code. That can be fixed with the following suggestion.

Create a copy constructor if you need one

The compiler will create a default copy constructor which does a shallow copy, but this won't work for data structures, like yours, which use pointers.  Instead, if you really want to create a new linklist from an existing one, you'll need to create your own copy constructor:

linklist(const linklist &other) : start{nullptr} {
    for (auto temp = other.start; temp; temp = temp->link) {
        append(temp->data);
    }
}

Consider using smart pointers

Using something like a std::unique_ptr would free you (pun intended) from having to manage the mechanics of new and delete explicitly.

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.


Vectorizing matrix operation instead of for loop in circular matrices I have a vector \$X_k\$ and a matrix \$Y_{k,j}\$, where \$k \in \{1,\dots,K\}\$ and \$j \in \{1, \dots, J\}\$. They are circular, meaning that $$\eqalign{X_{k+K} &= X_k \\ Y_{k+K,j} &= Y_{k,j} \\ Y_{k,j+J} &= Y_{k,j}}$$

I want to compute a \$ZX\$ vector and \$ZY\$ matrix according to the equations $$\eqalign{ZX_k &= -X_{k-1}(X_{K-2}-X_{K+1})-X_k \\ ZY_{k,j} &= -Y_{k,j+1}(Y_{k,j+2}-Y_{k,j-1})-Y_{k,j}+X_k}$$

Currently, I'm doing this through a loop, where first I compute the edge cases (for \$ZX\$, \$k = 1, 2, K\$; for \$ZY\$, \$j = 1, J, J-1\$). And for the others, I use the equations above.

I'm wondering if this calculation can be vectorized. Here is the example code.

import numpy as np

np.random.seed(10)
K = 20
J = 10

# initial state (equilibrium)
x = np.random.uniform(size=K)
y = np.random.uniform(size=(K*J))
y = y.reshape(K,J)

# zy
zy = np.zeros((K*J))
zy = zy.reshape(K,J)

# Edge case of Y
for k in range(K):
    zy[k,0] = -y[k,1]*(y[k,2]-y[k,J-1])-y[k,0]+ x[k]
    zy[k,J-1] = -y[k,0]*(y[k,1]-y[k,J-2])-y[k,J-1]+ x[k]
    zy[k,J-2] = -y[k,J-1]*(y[k,0]-y[k,J-3])-y[k,J-2]+ x[k]

    # General Case of Y
    for j in range(1,J-2):
        zy[k,j] = -y[k,j+1]*(y[k,j+2]-y[k,j-1])-y[k,j]+ x[k]

# zx
zx = np.zeros(K)

# first the 3 edge cases: k = 1, 2, K
zx[0] = -x[K-1]*(-x[1] + x[K-2])  - x[0]
zx[1] = - x[0]*(-x[2] + x[K-1])- x[1]
zx[K-1] = -x[K-2]*(-x[0] + x[K-3]) - x[K-1]

# then the general case for X
for k in range(2, K-1)
    zx[k] = -x[k-1]*(-x[k+1] + x[k-2]) - x[k]

print(zx)
print(zy)

I suspect that is possible to optimize with matrix operations but not sure if it is possible without the loops (at least for the edge cases).

Any suggestion of how to improve the performance?

This is easy using numpy.roll, for example:

zx = np.roll(x, 1) * (np.roll(x, 2) + np.roll(x, -1)) - x


Searching through a huge set with threads I wrote a class that finds values in a huge search volume. I have a pretty strong feeling that my code is not good and would like to get your opinion on it. My experience with C++11 threading is very limited and I think I use the wrong features for my task.

#include <iostream>
#include <vector>
#include <utility>
#include <future>

class Searcher
{
public:
    std::future<bool> Find(int x) {
        m_searches.emplace_back();
        m_searches.rbegin()->first = x;
        return m_searches.rbegin()->second.get_future();
    }

    void Search()
    {
        // very large search data
        static const std::vector<int> haystack = { 4, 5, 1, 3, 9, 8, 7, 6, 11, 42, 13 };

        std::vector<int> foundIndices;

        // find values
        for (auto h : haystack) {
            for (size_t i = 0; i < m_searches.size(); i++) {
                if (m_searches[i].first == h) {
                    foundIndices.push_back(i);
                    m_searches[i].second.set_value(true);
                }
            }
        }

        // resolve promises, where no value was found
        for (size_t i = 0; i < m_searches.size(); i++) {
            bool found = false;
            for (auto j : foundIndices)
                if (j == i)
                    found = true;

            if (!found)
                m_searches[i].second.set_value(false);
        }
    }

private:
    std::vector<std::pair<int, std::promise<bool>>> m_searches;
};

int main()
{
    Searcher s;
    auto s1 = s.Find(1);
    auto s2 = s.Find(2);
    auto s3 = s.Find(3);

    s.Search();

    std::cout << s1.get() << std::endl;
    std::cout << s2.get() << std::endl;
    std::cout << s3.get() << std::endl;

    return 0;
}

There should be one search running for many Finds, because the set that is searched is very large. It could however run concurrently to split the search up to multiple threads. For example with two threads: The first thread searches the first half, the second thread searches the second half.

The futures are not really guaranteed to be valid, because the user has to call the Search function. Would it be possible to start the search, once the first .get was called?

My "algorithm" to fulfil the promises looks very ugly and complicated, but I can't think of a better way.

There are definitely some problems if you want to make this threaded. Firstly, access to m_searches is not protected in any way. Since this is a shared bit of memory, it would need to be protected by something (say a mutex) before you modify it.

Generally in situations like these, it's far better to let std::async do a lot of the hard work for you. Also, try and eliminate shared mutable state as much as possible, because it makes life a lot harder. Firstly, let's write a simple search function that'll return an index if we find a match:

const std::size_t not_found = -1;

template <typename Iterator, typename T>
std::size_t find_index(Iterator begin, Iterator end,
                       const T& value, std::size_t start)
{
    std::size_t s = start;
    Iterator it = begin;

    while(it != end) {
        if(*it == value) {
            return s;
        }
        ++s;
        ++it;
    }

    return not_found;
}

Ok, this is just our normal linear search, but it allows us to start from anywhere in our search range since we're only passing in a few iterators.

Next, we define a vector to search over:

std::vector<int> v { 4, 5, 1, 3, 9, 8, 7, 6, 11, 42, 13 };
typedef typename std::vector<int>::iterator iter_t;

Create our vector of future to hold results:

std::vector<std::future<std::size_t>> futures;

And then populate it:

futures.emplace_back(
    std::async(std::launch::async, find_index<iter_t, int>,
               v.begin(), v.begin() + 6, 8, 0));
futures.emplace_back(
    std::async(std::launch::async, find_index<iter_t, int>,
               v.begin() + 6, v.end(), 8, 6));

Note the std::launch::async flag specifies we want to create a new thread to run our function in. I've hardcoded the search start and end places here, but you can obviously add some logic to split things up evenly if you'd like. Also, note that we need to explicitly define the template types here, since they won't be deduced.

Now we'll set up a vector to store our results in, and get the results back:

std::vector<std::size_t> results;
for(auto& future : futures) {
    results.emplace_back(future.get());
}

Finally, print out anything that was found:

std::for_each(results.begin(), results.end(),
              [](std::size_t index)
              { if(index != not_found) std::cout << index << "\n"; });
}

The whole thing can be found here.

Hopefully this gives you an idea of a different way of approaching your problem.


Return input as a partial fraction I need to write a function that, when given an input (string, float, or int), returns that input as a reduced partial fraction, and it needs to accept a wide array of inputs:

1.5 =>  "1 1/2"
5/2 => "2 1/2"
"1/3" => "1/3"
5 => "5"
"6 1/3" => "6 1/3"
0 => "0"

I've written this function using the Rational class, but was wondering if there were a better, more elegant solution:

def format_partial_fraction(fraction)
  if fraction.include? "/" # to handle "6 1/3" => "6 1/3" (or maybe I'll just trust user input in this case)
    fraction = fraction.split(" ").inject{|sum,x| sum.to_r + x.to_r }
  end

  rational = fraction.to_r
  if rational == 0
    return "0"
  elsif rational < 1 # e.g. "1/3"
    rational.to_s
  else # e.g. "3 1/2" or just "3"
    rational.to_i.to_s + ( rational%1 == 0 ? "" : " " + (rational%1).to_s)
  end
end

Ruby 2.0.0, Rails 3.2.13

Non-string input can lead to surprising results:

p (5/2).to_r #=> (2/1), integer division performed first
p (1.1).to_r #=> (2476979795053773/2251799813685248),  float can not be represented in binary

The last case is prevented by:

p 1.1.to_s.to_r #=> (11/10)

So I'd change rational = fraction.to_r to rational = fraction.to_s.to_r.  But the first case is only handled correctly if it is a string to begin with:

p "5/2".to_r #=> (5/2)

I don't think there is a remedy for that, except accepting string or float input input only.


Extract changesets between last two tags in Mercurial I have written the following script that I'd like to have reviewed:

#!/usr/bin/python

import os

os.system('hg tags > tags.txt')
file = 'tags.txt'
path = os.path.join(os.getcwd(), file)
fp = open(path)
for i, line in enumerate(fp):
    if i == 1:
        latestTagLine = line
    elif i == 2:
        previousTagLine = line
    elif i > 4:
        break
fp.close()
revLatestTag = latestTagLine.split(':')
l = revLatestTag[0].split(' ')
revPreviousTag = previousTagLine.split(':')
p = revPreviousTag[0].split(' ')
command = 'hg log -r {}:{} > diff.txt'.format(l[-1],p[-1])
os.system(command)

Output of hg tags command:

tip                         523:e317b6828206
TOOL_1.4                    522:5bb1197f2e36
TOOL_1.3                    515:7362c0effe40
TOOL_1.1                    406:33379f244971

You didn't specify that you wanted to leave a tags.txt file as an intentional side-effect of your script.  I'm going to assume that it's an unwanted temporary file.  In that case, you can read the output of hg directly through a pipe.  Furthermore, the file object can be used as an iterator to fetch just two lines, with no for-loop.

from subprocess import Popen, PIPE

# Read the first two lines of `hg tags`
with Popen(['hg', 'tags'], stdout=PIPE).stdout as hg_log:
    latestTagLine = hg_log.next()
    previousTagLine = hg_log.next()

The code to interpret each line is repeated, and therefore deserves to be put into a reusable function.  Instead of splitting, use a capturing regular expression.

import re

def parse_tag_line(line):
    match = re.match(r'^(.*) *(\d+):(.*)', line)
    return dict([
        ('tag', match.group(1)),
        ('rev', match.group(2)),
        ('hash', match.group(3)),
    ])

Finish up by generating the output:

log_range = '%s:%s' % (latestTag['hash'], previousTag['hash'])
with open('diff.txt', 'w') as diff:
    Popen(['hg', 'log', '-r', log_range], stdout=diff)

  Personally, I would prefer to just dump the output to sys.stdout, and use the shell to redirect the script's output to a file (script > diff.txt) instead of hard-coding diff.txt in the script itself.  Then, the epilogue would become

log_range = '%s:%s' % (latestTag['hash'], previousTag['hash'])
Popen(['hg', 'log', '-r', log_range])

Putting it all together:

import re
from subprocess import Popen, PIPE

def parse_tag_line(line):
    match = re.match(r'^(.*) *(\d+):(.*)', line)
    return dict([
        ('tag', match.group(1)),
        ('rev', match.group(2)),
        ('hash', match.group(3)),
    ])

# Read the first two lines of `hg tags`
with Popen(['hg', 'tags'], stdout=PIPE).stdout as hg_log:
    latestTag = parse_tag_line(hg_log.next())
    previousTag = parse_tag_line(hg_log.next())

log_range = '%s:%s' % (latestTag['hash'], previousTag['hash'])

# Write `hg log -r ...:...` to diff.txt
with open('diff.txt', 'w') as diff:
    Popen(['hg', 'log', '-r', log_range], stdout=diff)


Which solution of the two is the most efficient? Is there a better one? (IP Address Defang) I just started doing preparation for interviews, but im having trouble recognizing the optimal solution
Problem: For a given IP Address, replace every "." with "[.]"
Example:
Input: address = "1.1.1.1"
Output: "1[.]1[.]1[.]1"

My solutions:
First:
def defang(x):
    lst = []
    for i in x:
        if i == ".":
             lst.append("[" + i + "]")
        else:
            lst.append(i)
    return lst

adress = "255.100.50.0"
tmp = defrang(adress)
x=''
for i in tmp:
    x+=i
print(x)

Second:
adress = "255.100.50.0"
print("[.]".join(adress.split('.')))

Which one you think is better? The first doesnt rely on built-in methods but it creates a new list (more memory wasted?)
The second seems better but is it a problem that it relies on built-in methods?
In general idk what level of abstraction is requested by the interviewers, so any advice is welcome (regarding this specific solution or whatever else)

If you are in an interview using Python, assume that the interviewer is
reasonable and wants you to write fluent Python code. That means, among
other things, taking advantage of the language's built-in powers:
def defrang(ip):
    return ip.replace('.', '[.]')

But let's suppose that the interviewer explicitly directs you to ignore
built-ins and write your own implementation that does the work
character-by-character. Your current implementation is reasonable, but I would
suggest a few things. (1) Especially at interface points (for example, a
function's signature) use substantive variable names when they make sense. (2)
Don't make the caller re-assemble the IP address. (3) There is no need to
use concatenation to create a literal: just use '[.]' directly. (4) The logic is
simple enough for a compact if-else expression (this is a stylistic judgment, not a firm opinion).
def defang(ip_address):
    chars = []
    for c in ip_address:
        chars.append('[.]' if c == '.' else c)
    return ''.join(chars)

A final possibility is to distill further and just use a comprehension.
def defang(ip_address):
    return ''.join(
        '[.]' if c == '.' else c
        for c in ip_address
    )


Handling all error conditions on success or failure response I'm working on a screen where I sent a request and I get response in two types.
One is success and contains the below format:

{
    "searchNames": [
        {
            "CustomerName": "John",
            "CustomerType": "Business",
            "Profession": "Businessman",
            "address": {
                "address1": null,
                "address2": null,
                "city": null,
                "state": null,
                "country": null
            },
            "ContactNumber": null
        }
    ],
    "Count": 0
}

The other is failure response and the format differently every time, there can be no error code or message in the format.

{
status: "FAILURE"
error: {
errorMsg: [1]
0:  {
fieldName: "CustomerName"
errorCode: null
message: ""
}
errorNo: 12
}
}

I have tried validating the response and displaying the response, but I have no idea whether it is the right way to do the same.

This function is called in a promise:

function(response)
{
if(response.searchNames)
{
console.log("result", response.searchNames );
}
else
                          {
                            var errorString = ["Error : Invalid Data"];
                              if (response.status && response.status.toLowerCase() ===
                                  'failure' && response.error && response.error.errorMsg &&
                                  response.error.errorMsg.length > 0) {
                                  var errorString = [];
                                  for (var errMsg in response.error.errorMsg) {
                                      errorString.push(response.error.errorMsg[
                                          errMsg].message);
                                  }
}

My questions are:

Is this the right way to do the error validations, or are there any best practices for this? The error response may or may not contain error message. If there is error message, I should show the error message, otherwise I should show the default error message stored in another variable.
In the success response as well is it the right way to check the success response.
What is the right way in handling all the conditions and how can I do it?

The response from the server is not best practice. If you have any influence over the service, it should generate an http error (4XX range) on an exceptional condition and include the exception details in that response.

Assuming you have no control over the service, I would recommend wrapping the service  call in a method returning a promise which can be resolved or rejected upon checking the response.

If you only have one piece of code making the call then I would leave it as is -- wrapping for a single call would add unnecessary complexity.