Trimming unicode whitespace, control characters, and line breaks with regex I'm getting user submitted text from a <textarea></textarea>, and I want to edit it a bit before storing it in a database. Here are my 3 regexes, with explanations below them:

/(?:^((\pZ)+|((?!\R)\pC)+)(?1)*)|((?1)$)|(?:((?2)+|(?3)+)(?=(?2)|(?3)))/um

For every line...

Match all leading whitespace and unicode control characters ("WS/CC") (EXCEPT LINE BREAKS).
Match all trailing WS/CC (EXCEPT LINE BREAKS).
Match all but one WS/CC between non-whitespace characters.

regex101

/(\pZ+)|((?!\R)\pC)/u

Match remaining WS/CC (EXCEPT LINE BREAKS).
regex101

/(^\R+)|(\R+$)|(\R(?=\R{2}))/u

Match all leading line breaks.
Match all trailing line breaks.
Match all but 2 consecutive line breaks.
regex101

This is pretty much how I'm planning on using these regexes in my PHP script

// ...
// [Get user input from $_POST
// check its length, etc.,
// if everything looks O.K., store it in $str]
// ...

// Trim unnecessary whitespace (leave line breaks)
$str = preg_replace("/(?:^((\pZ)+|((?!\R)\pC)+)(?1)*)|((?1)$)|(?:((?2)+|(?3)+)(?=(?2)|(?3)))/um", '', $str);
// Convert remaining whitespace to regular spaces (leave line breaks)
$str = preg_replace("/(\pZ+)|((?!\R)\pC)/u", ' ', $str);
// Trim line breaks
$str = preg_replace("/(^\R+)|(\R+$)|(\R(?=\R{2}))/u", '', $str);

// Sanitize for safe printing between html tags
$str = htmlspecialchars($str, ENT_HTML5, 'UTF-8');

// ...
// [Store $str in DB with prepared statements]
// ...

And here's a test case which I've executed on regex101:

Input
(with some WS/CC in it):

a b  c
 d    e
fghi  j

k l m n
op

Output after 1st regex
(still has some WS/CC, but none consecutive, except for line breaks):

a b c
d e
fghi j

k l m n
op

Output after 2nd regex
(all previous WS/CC - except for line breaks - are now regular spaces):

a b c
d e
f g h i j

k l m n
o p

Output after 3rd regex
(max 2 consecutive line breaks, and no whitespace other than non-consecutive regular spaces):
a b c
d e
f g h i j

k l m n
o p

Based on my testing, this process seems to work as intended. I would however be very interested if you can think of any cases in which it would fail to behave as expected.

Also - in addition to possible fail-cases - I'd like to know if there are ways I could make these regexes perform better (faster), while still keeping the functionality as is. This is pretty much the first time I've ever done anything in regex, so I'm assuming there's some room for optimizations.

And if you see something else that is done incorrectly/could be done better, do let me know.

EDIT: Added regex101 links
EDIT2: Also, I've heard there's something called ReDoS, or Regex DoS. If you can tell that these regexes are susceptible to those, and which ones specifically, I'd like to know how to avoid them.

So I've toiled away at this for a while and although I am feeling a little out of my depth I'll submit my approach which employs what I do understand about preg_replace and regex.

preg_replace() allows mixed type pattern and replacement parameters, so one call can execute multiple patterns/replacements.
Speeding up regex can be achieved by minimizing capture groups and alternatives (pipes) as well as using character classes and negated characters classes where appropriate.

I've prioritized accuracy and endeavored to provide the most robust patterns considering all possible character sequences that may be submitted from the textarea.  Then I've attempted to write each pattern with greater efficiency (without impacting accuracy).  Finally, I've tried to reduce the overall length and convolution of the regex patterns.

I want to be clear that I don't have any experience with recursive patterns.  I don't know how/when to use them effectively and so none of my patterns employ them.

This is the sample input that I used which has whitespaces, newlines (single and greater than double), and control characters:

$text="\0\n\n\1\n\r\n\13\27a ab\r\n\t\t\r\r\ncà\1êß€\0\0abcbc   d\n\n\t\r\n\5   e\2\n\3\n\4\n";

This input string was designed to be as difficult as possible to best test my method.

$patt_repl=[
    '/^[\s\pC]+|[\s\pC]+$/u'=>'',  // aka mb_trim() -- remove all leading and trailing whitespace and control characters. DO NOT ADD m FLAG TO PATTERN, THAT WILL DAMAGE THE STRING
    '/[\s\pC]*?(\R)[\s\pC]*?(\R)[\s\pC]*/u'=>"\n\n",  // cleans 2 or more new lines (uninterupted by non-white space characters)
    '/(?!\R)[\h\pC]*\R[\h\pC]*(?!\R)/u'=>"\n",    // cleans single new lines
    '/\n+(*SKIP)(*FAIL)|[\h\pC]+/u'=>' '  // convert 1 or more non-newline white-spaces and control characters to single space
];

$text=htmlspecialchars(preg_replace(array_keys($patt_repl),$patt_repl,$text), ENT_HTML5, 'UTF-8');

echo "This is the post-replace/result string dump:\n";
var_dump($text);

Yes, my method uses 4 separate patterns rather than 3 like the OP.  This is for improved accuracy.  I believe the OP's method had some gaps in accuracy, so that is the justification for the additional pattern.

The output from the above method is:

This is the post-replace/result string dump:
string(28) "a ab

cà êß€ abcbc d

e"

...which is what I believe the output should be.

It trims all leading and trailing whitespace and control characters from the input string.
It reduces two or more newline characters (which may have leading, in-between,  trailing whitespace or control characters) to \n\n.
It trims all leading and trailing horizontal whitespace or control characters from all single newline characters.
It converts 1 or more consecutive horizontal whitespace or control characters to  (a single space).  This, during my development, required me to use the (*SKIP)(*FAIL) technique to disqualify newline characters becausse \h wasn't doing what I expected.  I have read on SO and elsewhere that there can be some quirky/untrustworthy behaviors when dealing with \h and \v while processing certain characters on certain systems -- but I do not possess the necessary wisdom to explain this.

For my own confidence, I wanted to have a way to confirm the existence/absense of whitespace and control characters in the string before and after the method.  I found a very helpful function in the top comment at the php manual's ord() page.

Here is a PHP demo that iterates the string and identifies each and every character to prove effectiveness.

Here are a few relevant regex links that I visited during my research:

https://www.regular-expressions.info/unicode.html
https://stackoverflow.com/questions/3230623/filter-all-types-of-whitespace-in-php
https://stackoverflow.com/questions/5471644/what-are-the-whitespaces-matched-by-s-in-php

On the matter of ReDoS and Catastrophic Backtracking, the threat is minimal (if not non-existent) because of my use of character classes and the structure of my alternatives.  In case you haven't already visited it, here is a good page to read if you want to investigate further.

Finally, I humbly ask that if anyone has a string that breaks my method, please provide it to me so that I can update my answer.  And if any of my assertions are incorrect, please correct me.


Weak pointers to vector elements idea I want to store some objects in linear memory, but sometimes I want to use these objects in some place for some time, but in the meantime object can be deleted, so I want to use weak_ptr's. This is my idea how to handle this, what you think ?
Class which have vector of my object.

world3d.h

class World3d
{
public:
    void Render();
    std::weak_ptr<Object3d> CreateObject3d();

private:
    std::vector<Object3d> m_objects;
};

world3d.cpp

void World3d::Render()
{

}

std::weak_ptr<Object3d>  World3d::CreateObject3d()
{
    m_objects.emplace_back();
    return m_objects.back().GetWPtr();
}

void World3d::SetCamera(std::shared_ptr<Camera> pCamera)
{
    m_pCamera = pCamera;
}

object3d.h

class Object3d
{
public:
    Object3d();
    virtual ~Object3d();
    std::weak_ptr<Object3d> GetWPtr();

private:
    std::shared_ptr<Object3d> thisPtr;

    Matrix m_xform;
};

object3d.cpp

void null_deleter(Object3d *) {}
Object3d::Object3d()
{
    thisPtr.reset(this, &null_deleter);
}

Object3d::~Object3d()
{
}

std::weak_ptr<Object3d> Object3d::GetWPtr()
{
    return thisPtr;
}

Very suspicious approach

Lifetime of elements in std::vector is manager by the vector. I doubt that std::shared_ptr can intervene here at all. The idea is both hard to understand and hard to prove its correctness. Though, even if std::shared_ptr would be able to intervene...

Reference invalidation

Imagine the container already gave out some pointers, then someone puts more elements into the container and it reallocates elements into new location. The handed out pointers will not be able to get lock()ed, and the whole thing will fall. Even if nobody puts elements into it, problem description mentions deletion. On deletion pointers will get invalidated too.

Solution

I see two ways of solving this problem:

Use std::list. The problem requires references that are not invalidated in case of deletion. If you're not traversing the list a lot, and doing some heavy computation on it, probably cost of segmented memory layout will be negligible/affordable.
Block the thread that does deletion/resizing of the vector when you're performing some operations on already existing ones. This is the hard way, but arguably can be win-win situation if thread synchronization is done correctly.

Code Review

Initialize members on construction where possible

It is possible to pass this, &null_deleter to thisPtr in member initialization list:

Object3d::Object3d():
                   thisPtr(this, &null_deleter)
{}

Redundant destructor.

If it is empty and is not virtual, just don't declare/define it. If it is virtual, you can just = default it.
Double copy.

void World3d::SetCamera(std::shared_ptr<Camera> pCamera)
{
    m_pCamera = pCamera;
}

Copies pCamera twice. Pass by const reference to avoid second copy. May be compiler will figure it out, but in more complex case it might not be able to.

Testing

Testing is important.

Build script with testing.

There should be some script (CMake, make, python, whatever is available) to build the project and test it. It will provide confidence in claims.
It is better in the long run

Tests are like an investment. The time spend will come back when changes are made to the code.

Conclusion

Usually code has some places where it is slower, and some places that are optimized well. You could trade performance of some components for the other, or for easier implementation and correctness.


Rails conditional sorting based on params The following code sorts the current users bid model items based on a param condition :bid_status. It starts to get messy when checking if the project space (:real_property_sale_project) :failed_date is .present? or .nil? to exclude certain items from the returned results. How can I improve this?

def index
  @bids = current_user.bids.select { |bid| bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? }

  if params[:bid_status] == 'in_progress'
    @bids = @bids.select { |bid| bid.real_property_sale.status == 'published' }
  elsif params[:bid_status] == 'awarded'
    @bids = @bids.select { |bid| bid.status == 'accepted' && bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).nil? }
  elsif params[:bid_status] == 'lost'
    @bids = @bids.select { |bid| bid.status == 'rejected' }
  elsif params[:bid_status] == 'history'
    @bids = current_user.bids.select { |bid| bid.status == 'accepted' && bid.real_property_sale.try(:real_property_sale_project).try(:failed_date).present? }
  end

  @bids = @bids.paginate(:page => params[:page], :per_page => 9)
end

Don't use select - that requires loading all records, and then filtering/sorting them in Ruby. But sorting and filtering is what databases and SQL is for.

For the first part, something like this should work (my Rails query-fu is rusty, so no guarantees):

current_user.bids
  .joins(real_property_sale: :real_property_sale_projects)
  .where(real_property_sale_project: { failed_date: nil })

On a side note, it seems like your models are overly complicated. They share the same prefix ("real property sale") which would seem to suggest they're been split up in a strange way. Not saying that's the case, but it does seem... off, somehow.

Whatever your app is doing, I'd wager that a user's bids are pretty important. So it seems strange that it'd take this much work to load them. A "bid status" should probably be front-and-center in the models; not a complex derived value, requiring loading not just 1st order but 2nd order associations. Just seems suspect to me.

Anyway, that code should probably be hidden away in a scope on the Bid model, so you can just say:

current_user.bids.active

or whatever name makes sense for this particular collection of bids.

Also, in your current code it seems that for the "awarded" value, you redo the same filtering you've already done once (checking failed_date).

Anyway, I imagine something like this:

@bids = case params[:bid_status]
        when 'in_progress'
          current_user.bids.active.where(real_property_sale: { status: "published" })
        when 'awarded'
          current_user.bids.active.where(status: "accepted")
        when 'lost'
          current_user.bids.active.where(status: "rejected")
        when 'history'
          current_user.bids
            .joins(real_property_sale: :real_property_sale_projects)
            .where("real_property_sale_project.failed_date IS NOT NULL")
        else
          current_user.bids.active
        end

Again, I can't guarantee it'll work as-is, but it illustrates the structure.

Note also that scopes can be chained, so you could conceivably make scopes for each of the filters, and do things like bids.active.accepted, bids.active.rejected, etc..

If you have named scopes for each filter, you could even do stuff like:

current_user.bids.active.send(params[:bid_status])

provided that a) you're sure it's safe to send the param (i.e. it's been whitelisted), and b) you use the same param values as the scope names.

Right now, it's a little odd that none of your param values actually match anything in the data models; "awarded" really means "accepted", "lost" really means "rejected", "in progress" actually means "published", etc..

Again, it seems like your data modelling is little off.


Putting read data into a combined file based on certain attributes I have a pretty big method where I read data from files and then put them into a combined file in a specific format based on an ID, first and end timestamp.

I already refactored some of my code to reduce the code complexity of my method, but it's still over 11 right now. So I was wondering if I should leave the method as is or try to find other ways to make it less complex.

This code will run on hardware which is designed by the company I work for so it's important to be as efficient with memory usage as possible.

Can you have a look at the code and help me out with reducing the code complexity by extracting pieces of code and putting them into separate methods?

protected File getSensorData(String sensorId, long first, long end) throws IOException, NumberFormatException {
  /*
   * This method gets data collected from files named after timestamps in seconds
   */
  String[] idArray = new String[0];
  String[] valArray = new String[0];
  long timestamp = 0;
  File formatFile = File.createTempFile("temp", "bufferedsensordata");
  PrintWriter formatPWriter = getFilePrintWriter(formatFile);

  // This sorts numerical instead of alphabetical
  File[] fileArray = sortFiles(file.listFiles());

  for(File f : fileArray) {
    long longTimestamp = getFileTimestamp(f); // Converts filename into timestamp
    if(longTimestamp >= first && longTimestamp <= end) {
      BufferedReader br = new BufferedReader(new FileReader(f));
      String line;
      while(null != (line = br.readLine())) {
        StringTokenizer tokens = new StringTokenizer(line, ";");
        String timestampString = tokens.nextToken();
        String id = tokens.nextToken();
        String val = tokens.nextToken();
        boolean foundId = false;
        int index = 0;

        if(id.startsWith(sensorId) && (Long.parseLong(timestampString) >= first && Long.parseLong(timestampString) <= end)) {
          // id should be used and timestamp falls between the requested range
          if(!Arrays.asList(idArray).contains(id)) {
            idArray = growArray(idArray); // Increases the array size by 1 to make room for missing id
            valArray = growArray(valArray);
          } else {
            foundId = true;
            index = getIdIndex(idArray, id); // Returns location of the id in the array
          }

          if(foundId) {
            valArray[index] = val; // Puts value in valArray at same spot as id in idArray
          } else {
            idArray[idArray.length -1] = id; // Adds new id & val to end of both arrays
            valArray[valArray.length -1] = val;
          }

          // Timestamps are based on seconds so data should only be written if more than a second has passed
          // But data is recorded per millisecond so there can be multiple pieces of data per second
          if(timestamp != Long.parseLong(timestampString)) {
            if(timestamp != 0) {
              String dataLine = String.valueOf(timestamp);
              for(int i = 0; i < valArray.length; i++) {
                dataLine = dataLine + ";" + valArray[i];
                valArray[i] = "";
              }
              formatPWriter.println(dataLine);
            }

            // Timestamp is update to the next second
            timestamp = Long.parseLong(timestampString);
          }
        }
      }
    }
  }

  File finalFile = newFinalFile();
  PrintWriter finalPWriter = getFilePrintWriter(finalFile);
  // Write idArray as first line (header)
  writeHeader(idArray, finalPWriter);
  // Write valArray as the rest of the file (body)
  writeContent(formatFile, finalPWriter);

  formatFile.delete();

  return finalFile;
}

I have to scroll to read it all, so without looking at the code: Yes, split it.

And now with looking at the code:
I would try to extract the content of your for loop and give the method a nice name.

Afterward you can check how to split this method.

Maybe you want to have a look at one-thing-extract-till-you-drop (also check the comments)


Get the first item in a sequence that matches a condition I've been trying to figure out what's the best, most Pythonic way to get the first element of a sequence that matches a condition. Here are three options:

I don't like that I've written caller[0].f_locals.get("runner") twice, but it feels most Pythonic otherwise.
runner = next(
    caller[0].f_locals.get("runner")
    for caller in inspect.stack()
    if isinstance(caller[0].f_locals.get("runner"), AbstractRunner)
)

Is it weird to rely on an assignment during the loop?
for caller in inspect.stack():
    runner = caller[0].f_locals.get("runner")
    if isinstance(runner, AbstractRunner):
        break

The second line is Pythonic and all, but the first line is only there to set up a generator where you no longer have to compute the expression (only apply the conditional in the second line).
generator = (caller[0].f_locals.get("runner") for caller in inspect.stack())
runner = next(x for x in generator if isinstance(x, AbstractRunner))

There's no performance difference at all from my tests.

This is a use case for the walrus operator :=, which was introduced in Python 3.8. It allows you to write 1) without repeating yourself:
runner = next(
    r
    for caller in inspect.stack()
    if isinstance(r := caller[0].f_locals.get("runner"), AbstractRunner)
)

Whether or not you find this more readable or if this is too much nestedness in the if part is up to personal taste.


Direction array to support multiple keydowns in 4-directional movement To get a simple, responsive, 4-directional movement in a 2D game, I've implemented the following. When a key is pressed, the add method adds the direction to the keys array. When the key is released, the remove method removes the direction from the keys array. In my Update method, I call getLast to get the last direction and move the player in that direction.

I want to restrict movement to one direction, but when I press multiple keys, I want the last key's direction to be used. Basically, if I'm going left and press up, but then release up while still holding down left, I want the character to go left, up, and continue going left.

I'm rather inexperienced when it comes to this kind of Javascript, so I'm hoping anyone has some tips / best practices recommendations for this small code snippet. I know naming is a bit iffy - I wanted to name it 'stack', but it's hardly a stack implementation.

function Keys() {
    this.keys = [];
}

// Only add the direction if it doesn't exist yet
Keys.prototype.add = function (dir) {
    for (var i = 0; i < this.keys.length; i++) {
        if (this.keys[i] == dir) {
            return;
        }
    }

    this.keys[this.keys.length] = dir;
};

// Find the index of the first occurrence and splice the array there without it
Keys.prototype.remove = function (dir) {
    for (var i = 0; i < this.keys.length; i++) {
        if (this.keys[i] == dir) {
            this.keys.splice(i, 1);
        }
    }
};

Keys.prototype.getLast = function () {
    if (this.keys.length > 0) {
        return this.keys[this.keys.length - 1];
    }
};

In my main file, I use it as: var keys = new Keys();

If you don't need to support IE8 and under, you could cut out those for-loops by using indexOf().

// Only add the direction if it doesn't exist yet
Keys.prototype.add = function (dir) {
    if (!~this.keys.indexOf(dir)) {
        // you could also do this.keys.indexOf(dir) == -1
        this.keys[this.keys.length] = dir;
    }
};

// Find the index of the first occurrence and splice the array there without it
Keys.prototype.remove = function (dir) {
    // you could also save this in a variable to use the second time
    if (~this.keys.indexOf(dir)) {
        this.keys.splice(this.keys.indexOf(dir), 1);
    }
};

Instead of this.keys[this.keys.length] = dir;, you could just push():

this.keys.push(dir);

Your check in Keys.prototype.getLast is redundant. It'll either return an item from the array, or nothing (undefined). A nonexistant array index also returns undefined, so you could just cut it out:

Keys.prototype.getLast = function () {
    return this.keys[this.keys.length - 1];
};

If you need to support oldIE, you could at least optimize Keys.prototype.remove by adding a return statement after removing the item:

Keys.prototype.remove = function (dir) {
    for (var i = 0; i < this.keys.length; i++) {
        if (this.keys[i] == dir) {
            this.keys.splice(i, 1);
            return;
        }
    }
};


Objective-C Stack implementation I just started coding in Objective-C and would like to know if my simple implementation of a Stack is acceptable & what ways would you improve the Stack code, or the Main code? i'm curious of things such as (but not limited to):

formatting
edge cases
run time (like one part where try to manipulate strings in description)
general good practice
memory management practices (i'm using ARC in this code)

Stack.h:

#import <Foundation/Foundation.h>

@interface Stack : NSObject

-(void)push:(id)obj;
-(id)pop;
-(NSUInteger)size;
-(id)peek;
-(BOOL)isEmpty;

@end

Stack.m file:

#import "Stack.h"

@implementation Stack{
    NSMutableArray *stack;
}

-(id)init{
    self = [super init];
    if(self!=nil){
        stack = [[NSMutableArray alloc] init];
    }
    return self;
}

-(void)push:(id)obj{
    [stack addObject:obj];
}

-(id)pop{
    id lastObj = [stack lastObject];
    [stack removeLastObject];
    return lastObj;
}

-(NSUInteger)size{
    return stack.count;
}

-(id)peek{
    return [[stack lastObject] copy];
}

-(BOOL)isEmpty{
    return stack.count == 0;
}

-(NSString *)description{
    NSMutableString *result = [[NSMutableString alloc] initWithString:@"["];
    for (id s in stack) {
        [result appendFormat:@"%@, ",[s description]];
    }
    if (stack.count>0) {
        result = [[result stringByTrimmingCharactersInSet:[NSCharacterSet characterSetWithCharactersInString:@", "]] mutableCopy];
    }
    [result appendString:@"]"];
    return result;
}

@end

Lastly, this is how i verify my code is "working":

#import <Foundation/Foundation.h>

#import "Stack.h"

int main(int argc, const char * argv[])
{

    @autoreleasepool {
        Stack *s1 = [[Stack alloc] init];
        assert([s1 size]==0);
        assert([s1 pop]==nil);
        assert([s1 peek]==nil);
        assert([s1 isEmpty]==YES);
        NSLog(@"first set of tests passed");
        [s1 push:[NSNumber numberWithInt:0]];
        [s1 push:[NSNumber numberWithInt:3]];
        [s1 push:[NSNumber numberWithInt:5]];
        NSLog(@"%@",[s1 description]);
        assert(![s1 isEmpty]);
        assert(s1.size == 3);
        assert([[s1 peek] isEqual:[NSNumber numberWithInt:5]]);
        assert([[s1 pop] isEqual:[NSNumber numberWithInt:5]]);
        assert(s1.size == 2);
        assert([[s1 pop] isEqual:[NSNumber numberWithInt:3]]);
        assert(![[s1 pop] isEqual:[NSNumber numberWithInt:3]]);
        assert([s1 pop] == nil);
        assert(s1.isEmpty);
        NSLog(@"second set of tests passed");
    }
    return 0;
}

-(id)pop{
    id lastObj = [stack lastObject];
    [stack removeLastObject];
    return lastObj;
}

Not sure if you are using ARC, but without ARC this is a potential use-after-free bug.  If you are not using ARC you should consider:

-(id)pop{
    id lastObj = [stack lastObject];

    if (lastObj)                         // < -- now checking for nil
    {
       [[lastObj retain] autorelease];   // < -- this line added

       [stack removeLastObject];
    }
    return lastObj;
}

(This also assumes that the objects in the stack are NSObjects, but I believe NSMutableArray makes the same assumption...)

This bumps the ref count up so that the next line (which will cause the NSMutableArray to release its reference to the object) does not end up deallocating the object.

Update on second reading:

Also, what do you do when lastObj is nil (i.e. too many pops)?  Doesn't seem like you've handled that case.


Shopping cart login and coupon code validation I am figuring out how I could make the following query more secure:
I have already used mysqli_real_escape_string but I doubt that adds much to security.

I have looked into the using prepared statements but I also have to take into account the following line:

$check_customer = mysqli_num_rows($run_c);

My solution:

   $c_email = mysqli_real_escape_string($con,$_POST['email']);
        $c_pass = mysqli_real_escape_string($con,$_POST['pass']);
        $couponCodeLogin = mysqli_real_escape_string($con,$_POST['couponCodeLogin']);
$couponCodeLoginAmount = mysqli_real_escape_string($con,$_POST['couponCodeLoginAmount']);

        $sel_c = "select * from customers where customer_pass='$c_pass' AND customer_email='$c_email'";

        $run_c = mysqli_query($con, $sel_c);
        $check_customer = mysqli_num_rows($run_c);
        if($check_customer==0){

        echo "<script>
        document.getElementById('loginError').innerHTML = 'Password or email is incorrect, please try again.'
        </script>";
        exit();
        }

        if($check_customer>0){
                    $crs_id = $_GET['crs_id'];

     $insert_c = "insert into customers (customer_email,coupon_code_login,coupon_code_login_amount) values ('$c_email','$couponCodeLogin','$couponCodeLoginAmount')";
                $run_c = mysqli_query($con, $insert_c);

                $_SESSION['userCoupon'] = $_POST['couponCodeLoginAmount'];

        $_SESSION['customer_email']=$c_email;
                        $_SESSION['userCouponName'] = $_POST['couponCodeLogin'];

        echo "<script>window.open('coursePayment.php?crs_id=$crs_id','_self')</script>";

        }
        else {
        $_SESSION['customer_email']=$c_email;

        echo "<script>window.open('coursePayment.php?crs_id=$crs_id','_self')</script>";

        $_SESSION['userCoupon'] = $_POST['couponCodeLoginAmount'];
                $_SESSION['userCouponName'] = $_POST['couponCodeLogin'];

        }
    }

and

        $fname= mysqli_real_escape_string($con,$_POST['fname']);
        $lname = mysqli_real_escape_string($con,$_POST['lname']);
        $email = mysqli_real_escape_string($con,$_POST['email']);
        $pnumber = mysqli_real_escape_string($con,$_POST['pnumber']);
 $couponCodeRegister=mysqli_real_escape_string($con,$_POST['couponCodeRegister']);
$couponCodeRegisterAmount = mysqli_real_escape_string($con,$_POST['couponCodeRegisterAmount']);

        $pass = mysqli_real_escape_string($con,$_POST['pass']);
        $cname = mysqli_real_escape_string($con,$_POST['cname']);
        $cposition = mysqli_real_escape_string($con,$_POST['cposition']);

         $insert_c = "insert into customers (customer_fname,customer_lname,customer_email,customer_number,customer_pass,customer_cname,customer_cposition,coupon_code_register,coupon_code_register_amount) values ('$fname','$lname','$email','$pnumber','$pass','$cname','$cposition','$couponCodeRegister','$couponCodeRegisterAmount')";

        $run_c = mysqli_query($con, $insert_c);

        $insert_email = "select * from customers";
        $run_email = mysqli_query($con, $insert_email);

        $find_email = mysqli_fetch_array($run_email);
        $demail = $find_email['customer_email'];

        if($email!= $demail)
       {
         $crs_id = $_GET['crs_id'];

         $_SESSION['userCoupon'] = $_POST['couponCodeRegisterAmount'];
         $_SESSION['userCouponName'] = $_POST['couponCodeRegister'];
         $_SESSION['customer_email']=$email;

         echo "<script>
         document.getElementById('registerError').innerHTML = 'Account has been created successfully, Thanks!'
         </script>";
         echo "<script>window.open('coursePayment.php?crs_id=$crs_id','_self')</script>";

       }

You need to learn how to use prepared statements, aka queries. That way, user input is never part of the SQL command.

https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet

http://prash.me/php-pdo-and-prepared-statements/


Quickly passing and parsing variable length float arrays I have this code for parsing for parsing a list of floats of variable length.

on_message = function(event_data) {
    var data = event_data.split(" ").map(parseFloat)
    var size = data.shift() + 1;
    while (data.length >= size) {
        // put the data in a buffer
        // could be replace by console.log() for testing purposes
        // each data chunk needs to be pushed separately
        this.data_store.push(data.slice(0, size));
        data = data.slice(size);
        size = data.shift() + 1;
    }
    // start displaying stuff to the user ASAP
    this.schedule_update();
}

At this point, the data is formatted in the following manner when a single "chunk" is sent:

[length data_1 data_2 etc etc]

However, because the back-end that sends these updates can sometimes be faster than the front-end, multiple "chunks" can be sent at the same:

[length_1 data_1_1 data_1_2 etc_1 etc_1 length_2 data_2_1 data_2_2 etc_2]

As mentioned before, all values in event_data are floats, except for the length which is an int.

The format of the data packet is not set, although the fact that more than one "chunk" can be sent at a time and the chunks are different sizes cannot be changed. I would just like to be able to send a variable length array of floats to a buffer quickly and efficiently. Right now, this code is taking really long. What can I do to speed it up?

You can get some improvements by using JSON and moving the length increment to the back-end like so:

Nengo.SpaSimilarity.prototype.on_message = function(event) {
    var data = JSON.parse(event.data);
    var size = data.shift();
    while (data.length >= size) {
        this.data_store.push(data.slice(0, size));
        data = data.slice(size);
        size = data.shift();
    }
    this.schedule_update();
}


Implementation of Counting Sort using C++ Please review my code. If any improvement needed please point out. I have used std::map. Is there any better alternative?

#include <bits/stdc++.h>

class count_sort
{
  std::map<int, int> m;
  std::map<int, int>::iterator it;

public:

  void counting_sort(std::vector<int>& a){
   for(int i=0; i<a.size(); i++)
   {
    m[a[i]]++;
   }
  }

  void display()
  {
    it = m.begin();
    while(it != m.end())
    {
      for(int i = 0; i < it->second; i++)
      {
        std::cout<<it->first<<" ";
      }
     it++;
   }
   std::cout<<"\n";
  }
};

int main()
{
    count_sort c1;
    std::vector<int>v {2,5,3,0,2,3,0,3};
    std::cout<<"The entered values are \n";
    for(int i = 0; i < v.size(); i++)
    {
      std::cout<<v[i]<<" ";
    }
    std::cout<<"\n";
    c1.counting_sort(v);
    std::cout<<"The sorted values \n";
    c1.display();
}

It says add more details. I don't know what to add.

#include <bits/stdc++.h>

You normally shouldn't use this. It includes every standard library header. If you compile without precompiled headers, this would make your compilation time much greater. The recommended practice is to #include just what you need, and if you want to use a precompiled header as well, that's just fine.

class count_sort
{

It is a bit strange to use a class for sorting. That your sort function is in a class implies that there's state, which is unexpected. You could use a namespace.

  std::map<int, int> m;
  std::map<int, int>::iterator it;

This state can easily be moved inside the functions, to the smallest scope possible.

  void counting_sort(std::vector<int>& a){

The name counting_sort is a bit redundant, seeing as it's a member function of a class called count_sort. You could use sort, or even operator(). Other than that, by taking std::vector<int>& as a parameter, you make this sort function not very general. The way the parameters are usually taken for a sort function (in C++) is as an iterator range:

template <typename Iter>
void counting_sort(Iter begin, Iter end) {

If you don't want to use templates, that's fine, but keep in mind that they make it more general.

One other thing to note is that you never change the vector in this function. That's a sign that it should be taken by const& rather than &. However, the expected semantics of a function called sort is that it modifies its parameter so that, after the function is called, the parameter is sorted. It's unusual and unexpected to have to call another function to obtain the output.

For the actual implementation, you can make use of C++11's range-based for loops (aka for-each loops). This:

for(int i=0; i<a.size(); i++)
{
 m[a[i]]++;
}

Becomes this:

// Read as "`for` each `int i` in `a`, ..."
for (int i : a) {
    m[i]++;
}

Putting together my suggestions, without templates, would look something like this:

namespace counting_sort {
    void sort(std::vector<int>& vec) {
        std::map<int, int> count;

        for (int i : vec) {
            ++count[i];
        }

        // modify the parameter; write the values back out
        auto begin = vec.begin();
        for (const auto& pair : count) {
            while (pair->second > 0) {
                *begin = pair->first;
                ++begin;
                --pair->second;
            }
        }
    }
}


Non static class with static fields I have a class that listens to a file event constantly and reads that json file once it's created and makes of it a mail report and an excel file.

Now, I made the properties that are used for both these tasks static so I could determine their values once from the json and easily access them from all methods:

public class Reporter
{
    public static bool ShouldNotify = false;
    public static string CSVFilePath;
    private static string HTMLReport;
    private string aSerializedObject;
    ...
    public Reporter(string jsonFilePath)
    {
      //Determine static params values.
    }
    ...
    public string BuildHtmlReport() { ... }

    public string WriteCsvFile() { ... }
}

There are a couple of things I feel that aren't best practices:

Is it bad practice to combine static and non static params in the same non static class? Eventually it's a lot like using local and global variables but maybe I should encapsulate them.
Initializing all necessary params in the constructor - I always call the constructor with the jsonFilePath and don't use the default constructor.
I don't dispose the instance when finished since most properties are static and I don't really need to (functionality wise), should I?

Not only is it bad practice, I'd say it's horrible practice. Save yourself, or others which will be dealing with your code, some future headache and use as few static variables as possible, preferably none! (The reason why I say this is horrible could have something to do with the code I have to deal with at work, anyways, it is still not good practice...)

I'd say that not using the default constructor is perfectly fine, if all your Reporter objects needs a jsonFilePath then it's good to pass it in the constructor. Also, if this value shouldn't change then you can make it readonly.

If by disposing you mean "clearing it's state" or something, then no you don't need to. Garbage collector will take care of that. However, if you keep using the static variables then you might run into problems if you want to create another one.

So, how to get rid of these static variables?
It's quite easy actually: Tell, don't ask.
You haven't given much code here so I'll have to try to explain how it generally works:
In your json class (which I will call Foo from now on since I don't know it's actual name), or whatever you have, instead of using Reporter.ShouldNotify, create an instance variable of the Reporter type. Then if you call/create your Foo class from your Reporter class, you tell the Foo object about it's Reporter. For example:
// In your reporter class
Foo foo = new Foo();
foo.setReporter(this); // sorry for the Java naming conventions here, but I hope you get the idea

In Foo you can then use your Reporter object which you retreived from the setReporter method to use that particular object.
Note that one can take the "Tell, don't ask" thing even further, but with the code in your question this is what I think you need for now.


Class Design : Parsers for different file type and ParserFactory I have come up with a class design for parsers (for xml,csv etc) and parser factory. I will be using it in my project.I will be configuring these as beans in the spring context file. I would like to get opinion on the class design.

public interface Parser<T> {
    String fileFormatSupported();
    List<T> parse(File file);
    boolean supports(Class<?> clazz);
    boolean validate(File file);
}
public abstract class AbstractCSVFileParser<T> implements Parser<T>{
    private static final String FILE_FORMAT = "csv";
    @Override
    public final String fileFormatSupported() {
        return FILE_FORMAT;
    }
}
public class CSVToDeviceportServer extends AbstractCSVFileParser<DeviceportServer> {
    @Override
    public boolean supports(Class<?> clazz) {
        return DeviceportServer.class.isAssignableFrom(clazz);
    }
    @Override
    public  List<DeviceportServer> parse(File file) { /* logic*/ }
    @Override
    public boolean validate(File file) { /* logic */  }
}
public class CSVToDeviceParser extends AbstractCSVFileParser<Device> {
    @Override
    public boolean supports(Class<?> clazz) {
        return Device.class.isAssignableFrom(clazz);
    }
    @Override
    public List<Device> parse(File file) {  /* logic */  }
    @Override
    public boolean validate(File file) { /* logic */ }
}
public class ParserFactory {
    @Autowired
    private List<Parser<?>> parsers;

    private Map<String, List<Parser<?>>> parserMap;

    public List<Parser<?>> getParsers() {
        return parsers;
    }
    @PostConstruct
    public void afterPropertiesSet() throws Exception{
        if(this.parsers == null) {
            throw new IllegalStateException("Parsers not initialized");
        }
        parserMap = new HashMap<>(2);
        for(Parser<?> parser : parsers) {
            if (parserMap.containsKey(parser.fileFormatSupported())) {
                parserMap.get(parser.fileFormatSupported()).add(parser);
            }else {
                List<Parser<?>> parserList = new ArrayList<>();
                parserList.add(parser);
                parserMap.put(parser.fileFormatSupported(), parserList);
            }
        }
    }
    public Parser<?> getParser(Class<?> clazz, String format) {
        for(Parser<?> parser : parserMap.get(format)) {
            if(parser.supports(clazz)) return parser;
        }
        throw new IllegalStateException("Could not find parser for the format");
    }
}

I'm adding parser classes only for the CSV type of files, but there are other types of parsers for xml, json. They will have same class design as the CSV file parsers.

The method Parser.fileFormatSupported looks great. Letting each parser declare which file types it can process allows for information hiding. Android uses the same style for deciding the "Which app can handle which file types?" question.
The method Parser.supports uses an inferior style. If you can, you should also switch to a declarative style by replacing this method with Set<Class<?>> getSupportedClasses(). Maybe you only need Class<?> getSupportedClass().
In the supports methods, you currently use isAssignableFrom. This looks wrong to me, since each of these parsers probably only supports exactly this one class. Therefore these methods should return clazz == Device.class.
The CSVToDeviceportServer class is missing the Parser in its name.
In the afterPropertiesSet method, you don't need to check for this.parsers == null. The code will fail in the for loop below. The chances of your custom exception being thrown are close to zero. And even if this.parsers were null, it would be very easy to see in the stack trace.
In the afterPropertiesSet method, you should use a Multimap (from Guava) instead of a HashMap of lists. This makes the code shorter and easier to understand.
Instead of the @PostConstruct method, you may think about having an @Autowired void setParsers(List<Parser> parsers) method that initializes everything. This method is easier to use in unit tests. This method should be package-private since it is not intended to be used by any other code.


"Sharing CANDY" on SPOJ How can I improve it and its running time efficiency?

Problem (SPOJ/CANDY)

  Jennifer is a teacher in the first year of a primary school. She has
  gone for a trip with her class today. She has taken a packet of
  candies for each child. Unfortunately, the sizes of the packets are
  not the same.

  Jennifer is afraid that each child will want to have the biggest
  packet of candies and this will lead to quarrels or even fights among
  children. She wants to avoid this. Therefore, she has decided to open
  all the packets, count the candies in each packet and move some
  candies from bigger packets to smaller ones so that each packet will
  contain the same number of candies. The question is how many candies
  she has to move.

  Input specification

  The input file consists of several blocks of data. Each block starts
  with the number of candy packets \$N (1 <= N <= 10000)\$ followed by
  \$N\$ integers (each less than 1000) in separate lines, giving the
  number of candies in each packet. After the last block of data there
  is the number -1.

  Output specification

  The output file should contain one line with the smallest number of
  moves for each block of data. One move consists of taking one candy
  from a packet and putting it into another one. If it is not possible
  to have the same number of candies in each packet, output the number
  -1.

Solution

 //candy.cpp
#include <iostream>
#include <string>
#include <stdio.h>
using namespace std;

int main()
{
string n;
int avg,t,k; // t = test cases , avg = average
while(true)
{
    scanf("%d",&t);
    int sum = 0; // initialise sum and cnt(next line) to 0
    int cnt = 0;
    if(t == -1) // Last line to end the program(Mentioned in the ques.)
        break;

    else
    {
        for(int i = 0 ; i < t ; i++)
        {
            cin >> n[i];
            sum += n[i];
        }

        if(sum % t != 0) // If it is not possible to have the same number of candies in each packet, output the number -1(mentioned in the question)
            cout << "-1" << endl;

        else
        {
            avg = sum/t;
            for(int i = 0 ; i < t ; i++)
            {
                k = n[i];
                while(k > avg) //Adds the moves
                {
                    k-- ;
                    cnt++ ;
                }
            }
            printf("%d\n", cnt);
        }
    }

}

}

This is really silly:

while(k > avg) //Adds the moves
{
    k-- ;
    cnt++ ;
}

When you can do the same thing in one step:

if (k > avg) cnt += k - avg;

I don't see anything else to make this faster. But there are many many coding style issues.

Limit the scope of variables

Instead of declaring all the variables at the top,
it's better to declare them in the smallest scope, for example:

int avg = sum/t;
for(int i = 0 ; i < t ; i++)
{
    int k = n[i];
    if (k > avg) cnt += k - avg;
}

Use the right types

Why do you read the numbers into a string?
The program won't work if a pocket contains more than 9 candies,
which it might, since the requirement states "less than 1000".
Use an int[] instead.

Misc

You should not using namespace std
You're mixing C++ style input/output using cin/cout and old-style scanf/printf. Since you're in C++, you should use C++ everywhere consistently
It's recommended to use braces even with single-statement ifs and loops
After the break statement, you can simplify a bit by eliminating the else block

Corrected and improved implementation

With the above suggestions, the implementation becomes:

#include <iostream>
#include <stdio.h>

int main()
{
    while(true)
    {
        int t;
        std::cin >> t;
        if (t == -1) {
            break;
        }

        int sum = 0;
        int n[t];
        for(int i = 0 ; i < t ; i++)
        {
            std::cin >> n[i];
            sum += n[i];
        }

        if (sum % t != 0) {
            std::cout << "-1" << std::endl;
            continue;
        }

        int avg = sum/t;
        int cnt = 0;
        for(int i = 0 ; i < t ; i++)
        {
            int k = n[i];
            if (k > avg) cnt += k - avg;
        }
        std::cout << cnt << std::endl;
    }
}

Take special note as to how each variable is declared and initialized right before it's really needed.


Create an array which contains specific values of a specific nested object inside an array So I have this array of objects, which respectively contain other objects:

const template_columns = [
  {
    "id": 38,
    "name": "Ideas",
    "position": 0,
    "template_id": 4,
    "column_type": "Column",
    "workflow": [
      {
        "action": "Next",
        "move_to": "Backlog",
        "assign_to": "none"
      },
      {
        "action": "Prev",
        "move_to": "First column",
        "assign_to": "none"
      }
    ],
    "_t": "template_column"
  },
  {
    "id": 39,
    "name": "Backlog",
    "position": 1,
    "template_id": 4,
    "column_type": "Column",
    "workflow": [
      {
        "action": "Start",
        "move_to": "Current",
        "assign_to": "none"
      }
    ],
    "_t": "template_column"
  },
  {
    "id": 40,
    "name": "Current",
    "position": 2,
    "template_id": 4,
    "column_type": "Column",
    "workflow": [
      {
        "action": "Review",
        "move_to": "Review",
        "assign_to": "none"
      }
    ],
    "_t": "template_column"
  },
  {
    "id": 41,
    "name": "Review",
    "position": 3,
    "template_id": 4,
    "column_type": "Column",
    "workflow": [
      {
        "action": "Approve",
        "move_to": "Approved",
        "assign_to": "none"
      }
    ],
    "_t": "template_column"
  },
  {
    "id": 42,
    "name": "Approved",
    "position": 4,
    "template_id": 4,
    "column_type": "Columns::Done",
    "workflow": [],
    "_t": "template_column"
  }
]

I need to process this array and return an array with all workflow.action values. I have loadash available and I'm also transpiling the es6 code with babel.

I managed to do so, but I'm wondering if there's a more elegant way to do it:

My solution:

const workflow = _.map (
  _.flatten(
    _.map(template_columns, function(col) {
      return col.workflow
    })
  ), 'action'
)

So is there a better way to achieve what I'm looking for?

You could use _.reduce() (or alternatively native Array.reduce()) to achieve this:

const workflow = _.reduce(
    template_columns,
    (colResult, colValue) => {
        return colResult.concat(
            _.reduce(
                colValue.workflow,
                (workflowResult, workflowValue) => workflowValue.action,
                []
            );
        );
    },
    []
);

or

const workflow = template_columns.reduce(
    (colResult, colValue) => {
        return colResult.concat(
            colValue.workflow.reduce(
                (workflowResult, workflowValue) => workflowValue.action,
                []
            );
        );
    },
    []
);

Note, use of arrow functions is optional, but since you indicated you are writing for ES6 and transpiling, and since the callback logic is so simple here, I thought the arrow function made sense.

Alternately, you might perform reduce then map like this:

const workflow = _.map(
    _.reduce(
        template_columns,
        (result, value) => result.concat(value.workflow),
        []
    ),
    val => val.action
 );

This approach is actually similar to your current approach, using _reduce() rather than combination of _.flatten and _.map inside the outer _map() call.

Perhaps try it both ways (as well as your original approach) and see where you get best performance, based on the typical data you are working with.


Find all combinations of numbers of a set that add up to a given target - avoid for loops I am thinking about the following problem:
Consider a 3-numbers lock, i.e. a lock which has three little number wheels that opens whenever the sum of these three numbers equals 75. I know, this doesn't make sense for a lock, but just imagine this. The number wheels have numbers 3, 15, 17, 35, 40, 25, 5, 13. I am allowed to use numbers multiple times, i.e. in order to reach 75, I could use the combination 25, 25, 25.
I want to come up with all possible combinations to solve this puzzle. I have a solution and it works fine for just 3 different number wheels. But since I am using 3 for-loops for this, I know I cannot extend this code to work for, say a 100 number-wheels lock. I don't even know if this was possible (maybe recursively, but I always have troubles understanding recursion). Anyways, I really want to make this code look a little nicer. It is okay for me to leave the number of iterations the same, but I really want to get rid of the 3 for-loops, because I kind of want to extend this code to be a 10-numbers lock and in that case I would have 10 for-loops which looks ridiculous. There should be a way to leave out the for loops, I think
So, enough said. Here is my solution:
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class TestClass {

    public static void main(String[] args) {

        List<List<Integer>> solutions = new ArrayList<>();
        List<Integer> combinations = new ArrayList<Integer>();

        List<Integer> numbers = new ArrayList<Integer>();
        numbers.add(3);
        numbers.add(15);
        numbers.add(17);
        numbers.add(35);
        numbers.add(40);
        numbers.add(25);
        numbers.add(5);
        numbers.add(13);

        int target = 75;

        int var1 = 3;
        int var2 = 3;
        int var3 = 3;

        for (int i = 0; i < numbers.size(); i++) {
            var1 = numbers.get(i);
            for (int j = 0; j < numbers.size(); j++) {
                var2 = numbers.get(j);
                for (int k = 0; k < numbers.size(); k++) {
                    var3 = numbers.get(k);

                    int sum = var1 + var2 + var3;
                    if (sum == target) {
                        combinations.add(var1);
                        combinations.add(var2);
                        combinations.add(var3);

                        Collections.sort(combinations);
                        if (!solutions.contains(combinations)) {
                            solutions.add(combinations);
                        }
                        combinations = new ArrayList<>();
                    }
                }
            }
        }

        for (List<Integer> solution: solutions) {
            System.out.println(solution);
        }

    }
}

Any ideas, please share them with me :-)

you need the function
public static List<Integer> get_next_highest(List<Integer> combinations, List<Integer> numbers) {
    for(int i=0; i < combinations.size(); i++) {
        if(combinations[i]<numbers.size()-1) {
            combinations[i]++;
            return combinations;
        }
        else combinations[i] = 0;
    }
    return null;
}

here, to simplify I've made combinations have indices (otherwise can't just do combinations[i]++, would need to find the next highest number in numbers)...
for eg. if numbers=[25 30 60 80] and combinations=[0 0 2 1]
this corresponds to the wheel positions [25 25 60 30]
if you pass the value of combinations [3 3 3 3] the function will return a null so make sure to do a check and stop when it does.
This will loop you through all possible combinations if you start at [0 0 0 0]
Specifically in the order [0 0 0 0] [1 0 0 0 ] [2 0 0 0] [3 0 0 0] [0 1 0 0] [1 1 0 0] ... [3 3 3 3]
should be fairly easy to sum up numbers[combinations[i]]
since you specified no for loops here's the recursive version
public static List<Integer> get_next_highest(List<Integer> combinations, List<Integer> numbers, int i=1) {
    if(i==combinations.size()) return null;
    if(combinations[i]<numbers.size()) {
            combinations[i]++;
            return combinations;
    }
    else {
        combinations[i] = 0;
        return get_next_highest(combinations, numbers, i+1);
    }
}

it does the same thing
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class TestClass {

    public static void main(String[] args) {

        List<List<Integer>> solutions = new ArrayList<>();
        List<Integer> combinations = new ArrayList<Integer>();

        List<Integer> numbers = new ArrayList<Integer>();
        numbers.add(3);
        numbers.add(15);
        numbers.add(17);
        numbers.add(35);
        numbers.add(40);
        numbers.add(25);
        numbers.add(5);
        numbers.add(13);

        int target = 75;
        int num_wheels = 3;

        for (int i = 0; i < num_wheels; i++) combinations.add(0);

        for (; combinations != null; combinations=get_next_highest(combinations, numbers)) {
            int sum = 0;
            for(Integer x : combinations) sum += numbers[x];
            if (sum == target) {
                 temp = new ArrayList<>();
                 for(Integer x : combinations) temp.add(numbers[x]);
                 Collections.sort(temp);
                 if (!solutions.contains(temp)) solutions.add(temp);
             }
        }

        for (List<Integer> solution: solutions) {
            System.out.println(solution);
        }

    }
}


Add entities to and execute "ExecuteMultipleRequest" I have a method which takes a list of Entity objects, loop through elements in that list and add them to a "ExecuteMultipleRequest" object. After that the ExecuteMultipleRequest will be executed.

However, according to Microsoft Dynamic CRM specs, I can only add at most 1000 entities to ExecuteMultipleRequest at one time, or else that request will throw an error when executed, and my list might have more than 1000 elements.

I have come up with the following solution to make sure that at least 1 request will be executed for every 1000 elements, but I find it to be inelegant. What can I improve here?

private readonly int MAXTOBEPROCESSED = 1000;

public void processEntities (List<Entity> entities)
{
    var toBeProcessed = 0;
    var createMultiple = new ExecuteMultipleRequest()
    {
        Settings = new ExecuteMultipleSettings()
        {
            ContinueOnError = false,
            ReturnResponses = true
        },
        Requests = new OrganizationRequestCollection()
    };

    foreach (var entity in entities)
    {
        var createRequest = new CreateRequest{ Target = entity };
        createMultiple.Requests.Add(createRequest);

        if (++toBeProcessed >= MAXTOBEPROCESSED)
        {
            var responseWithResults = (ExecuteMultipleResponse)service.Execute(createMultiple);

            createMultiple = new ExecuteMultipleRequest()
            {
                Settings = new ExecuteMultipleSettings()
                {
                    ContinueOnError = false,
                    ReturnResponses = true
                },
                Requests = new OrganizationRequestCollection()
            };
            toBeProcessed = 0;
        }
    }

    if (toBeProcessed != 0)
    {
        var responseWithResults = (ExecuteMultipleResponse)service.Execute(createMultiple);
    }
}

Your logic looks fine to me. If you find yourself creating something in two places then feel free to create a helper function to do so.

Your logic can be arguably simpler if you count down rather than up, since we already know the number of requests.

Something like:

private static int MAXTOBEPROCESSED = 1000;

public void processEntities (List<Entity> entities)
{
    var createMultiple = CreateMultipleRequest();

    for (int n = entities.Count; n > 0; n--)
    {
        createMultiple.Requests.Add(new CreateRequest{ Target = entities[n] });

        if (n % MAXTOBEPROCESSED == 0)
        {
            var responseWithResults = (ExecuteMultipleResponse)service.Execute(createMultiple);
            createMultiple = CreateMultipleRequest();
        }
    }
}

private CreateMultipleRequest()
{
    return new ExecuteMultipleRequest()
    {
        Requests = new OrganizationRequestCollection(),
        Settings = new ExecuteMultipleSettings()
        {
            ContinueOnError = false,
            ReturnResponses = true
        }
    };
}


C++ linked list with too many pointers Can someone tell me how I can improve this code? It works ok, but I feel that I'm using too many pointers.

#include <iostream>
using namespace std;
struct link{
    int data;
    link *next;
};
class link_list{
private:
    int boundaries = 0;
    link *tail;
    link *temp;
    link *head = NULL;
public:

    void add_node(int data){

        temp = new link;
        temp->data = data;
        temp->next = NULL;
        if (head == NULL){
            head = temp;
            boundaries++;
        }
        else{
            link *tail = head;
            while (tail->next != NULL){
                tail = tail->next;
            }
            tail->next = temp;
            boundaries++;
        }
    }
    void delete_b(){
        link *end = head;
        link *next_to_end = NULL;
        while (end->next != NULL){
            next_to_end = end;
            end = end->next;
        }
        next_to_end->next = NULL;
        end = NULL;
        boundaries--;
    }
    void add_front(int data){
        link *ptr = new link;
        ptr->next = head;
        ptr->data = data;
        head = ptr;
        boundaries++;

    }
    void delete_front(){
        link *new_head = head;
        new_head = head->next;
        head = NULL;
        head = new_head;
        boundaries--;

    }

    void add_by_position(int data, int pos){
        link *node = new link;
        link *linker = head;
        node->data = data;
        for (int i = 0; i < pos; i++){
            linker = linker->next;
        }
        node->next = linker;
        linker = head;
        for (int i = 0; i < pos - 1; i++){
            linker = linker->next;
        }
        linker->next = node;
        boundaries++;

    }

    void show(){
        link *sh = head;
        while (sh){ cout << sh->data << endl; sh = sh->next; }

    }

};

int main()
{
    link_list obj;
    obj.add_node(7);
    obj.add_node(5);

    obj.add_node(8);

    obj.show();
}

struct link{
    int data;
    link *next;
};

Are you sure you want this to be outside of the class? This could be in the private section in the class link_list since the outside world does not need to know of this class at all.

int boundaries = 0;

What is boundaries? It seems like this is your actual size of the list but I had to check your code to make sure what it is. Call it size or entries or something like this.

link *tail;
link *temp;
link *head = NULL;

There is no reason to have temp as a instance variable. And you do not use tail in your code, so drop them both. And use nullptr instead of NULL.

void add_node(int data){

    temp = new link;
    temp->data = data;
    temp->next = NULL;
    if (head == NULL){
        head = temp;
        boundaries++;
    }
    else{
        link *tail = head;
        while (tail->next != NULL){
            tail = tail->next;
        }
        tail->next = temp;
        boundaries++;
    }
}

Again, use nullptr. And if you had used your tail you wouldn't need to iterate over the whole list to get to the end.

void delete_b(){
    link *end = head;
    link *next_to_end = NULL;
    while (end->next != NULL){
        next_to_end = end;
        end = end->next;
    }
    next_to_end->next = NULL;
    end = NULL;
    boundaries--;
}

What's b? (Oh yeah, I think I don't need to remind you about nullptr again.)

void add_front(int data){
    link *ptr = new link;
    ptr->next = head;
    ptr->data = data;
    head = ptr;
    boundaries++;

}

add_node and add_front? What's the difference? Sure, I know what the difference is; I've seen the code. However, I don't like to look through the code to see what a function does. Call them add_front and add_back or something like this (head and tail might fit better, programmers will understand).

void delete_front(){
    link *new_head = head;
    new_head = head->next;
    head = NULL;
    head = new_head;
    boundaries--;

}

What is this playing around? That's three useless lines of code:

void delete_front(){
    link* tmp = head;
    head = head->next;
    delete tmp;
    boundaries--;
}

void add_by_position(int data, int pos){
    link *node = new link;
    link *linker = head;
    node->data = data;
    for (int i = 0; i < pos; i++){
        linker = linker->next;
    }
    node->next = linker;
    linker = head;
    for (int i = 0; i < pos - 1; i++){
        linker = linker->next;
    }
    linker->next = node;
    boundaries++;

}

by_position sounds silly. Call it add_at. And why do you reset linker and start over again? That's totally unnecessary.

void add_at(int data, int pos){
    link *node = new link;
    node->data = data;
    boundaries++;
    if(pos == 0) {
      node->next = head;
      head = node;
      return;
    }

    link *linker = head;
    for (int i = 0; i < pos - 1; i++){
        linker = linker->next;
    }
    node->next = linker->next;
    linker->next = node;
}

You should also delete the nodes in your delete/remove functions.

That's everything for now. You could've easily made a template out of this class, however, you should first fix your problems and then - if you like - ask another question with your new code.


Is this possibly confusing code? A previous developer had an array of strings like so:

public static const PAGE_ONE:String     = 'PageOne';
public static const PAGE_TWO:String     = 'PageTwo';
public static const PAGE_THREE:String   = 'PageThree';
public static const PAGE_FOUR:String    = 'PageFour';
public static const PAGE_FIVE:String    = 'PageFive';
public static const PAGE_SIX:String     = 'PageSix';

When he wanted to open a new type of page he would write

switch ($page)
{
    case PAGE_ONE:
        // Do stuff
        break;

    ...

    case PAGE_SIX:
        // Do stuff
        break;
}

Which was all well and good, but I thought it could be condensed a little bit, so I created a little more functional approach.

In the constructor, this function is called setting up the table:

    private function setDefaultPages() : void {
        _pages[PAGE_ONE]          = function($xml:XMLList)  { return new PageOne( $xml ); }

        ...

        _pages[PAGE_SIX] = function($xml:XMLList)   { return new PageSix( $xml ); }
    }

Now, when the time comes to open a page, all I have to write is this:

_nextPage = _pages[$page](xml);

However, I am worried that I have hurt readability by doing this. Could anybody suggest a good middle-ground if it is so?

It seems to me that what you need is the State pattern.


Clone a linked list with a random pointer I'm working on a problem to clone a linked list (each node has a regular next pointer), and each node also has a random pointer which could point to any node in the linked list. Each linked list node has a unique ID and new cloned linked list node should have the same node ID.

One major concern in my current implementation is that I create another randomMap to map from a newly cloned linked list node ID to a newly cloned linked list node. I'm wondering if you have any ideas on saving the mapping (so that the additional \$O(n)\$ space could be saved).

from __future__ import print_function
from collections import defaultdict

class LinkedListNode:
    def __init__(self, nodeID):
        self.nodeID = nodeID
        self.nextNode = None
        self.randomNode = None
    def dump(self):
        node = self
        print (node.nodeID, node.nextNode.nodeID if node.nextNode else -1, node.randomNode.nodeID if node.randomNode else -1)
        node = self.nextNode
        while node:
            print (node.nodeID, node.nextNode.nodeID if node.nextNode else -1, node.randomNode.nodeID if node.randomNode else -1)
            node = node.nextNode
    def cloneList(self):
        node = self
        randomMap = defaultdict(LinkedListNode) # key node ID, value LinkedListNode in new list
        preNode = LinkedListNode(node.nodeID)
        randomMap[node.nodeID] = preNode
        head = preNode
        # setup regular part
        while node.nextNode:
            curNode = LinkedListNode(node.nextNode.nodeID)
            randomMap[node.nextNode.nodeID] = curNode
            preNode.nextNode = curNode
            preNode = curNode
            node = node.nextNode
        # setup up random part
        node = self
        while node:
            randomMap[node.nodeID].randomNode=randomMap[node.randomNode.nodeID]
            node = node.nextNode

        return head

if __name__ == "__main__":
    node1 = LinkedListNode('1')
    node2 = LinkedListNode('2')
    node3 = LinkedListNode('3')
    node4 = LinkedListNode('4')
    node5 = LinkedListNode('5')

    node1.nextNode = node2
    node2.nextNode = node3
    node3.nextNode = node4
    node4.nextNode = node5

    node1.randomNode = node3
    node2.randomNode = node4
    node3.randomNode = node5
    node4.randomNode = node1
    node5.randomNode = node2

    newList = node1.cloneList()
    newList.dump()

Expected output

1 2 3
2 3 4
3 4 5
4 5 1
5 -1 2

It is indeed possible to spare the map, by the cost of an additional linear pass. A key is to create a cloned list not independently, but interleaved with the original one:

    while node:
        image = LinkedListNode(node.ID)
        image.next = node.next
        node.next = image
        node = image.next

The second pass sets up the random pointers in the cloned nodes:

    while node:
        image = node.next
        image.random = node.random.next
        node = image.next

And the final pass disconnects interleaved lists:

    try:
        while True:
            image = node.next
            node.next = image.next
            image.next = image.next.next
            node = node.next
    except ValueError:
        pass

The exception comes from the last node: image.next is None.


Optimizing quicksort partition method/function for duplicate elements I'm a newbie and interested in learning different data structures and algorithms. While implementing quicksort, I found this code works pretty well for partitioning.

int partition(int ar[], int lft, int rgt) {

int left = lft;
int right = rgt;
/*median of three is the best pivot value to accomplish in nlogn time*/
int pivotVal = ar[(left+right)/2];
printf("pivot: %d\n", pivotVal);

/*
-this while loop holds the left and right portion of the
-splitted array together to accomplish partitioning.
*/
while (left < right) {

    /*loop for left half array*/
    while (ar[left] < pivotVal) { left += 1; };

    /*loop for right half array*/
    while (ar[right] > pivotVal) { right -= 1; };

    /*
    -if the above conditions are not met the sub arrays are unsorted
    -we need to sort or exchange the values.
    */
    if (left < right) {
      int cache = ar[left];
      ar[left] = ar[right];
      ar[right] = cache;
    }

}

int pivotIndex = left; /*updated left or right is the pivot index*/
return pivotIndex;

}

Notice the code will get stuck in an infinite loop if there are any duplicate values in the given input array. So I hacked the problem just adding an additional line just after the swapping portion that looks like this:

int partition(int ar[], int lft, int rgt) {
int left = lft;
int right = rgt;
/*median of three is the best pivot value to accomplish in nlogn time*/
int pivotVal = ar[(left+right)/2];
printf("pivot: %d\n", pivotVal);

/*
-this while loop holds the left and right portion of the
-splitted array together to accomplish partitioning.
*/
while (left < right) {

    /*loop for left half array*/
    while (ar[left] < pivotVal) { left += 1; };

    /*loop for right half array*/
    while (ar[right] > pivotVal) { right -= 1; };

    /*
    -if the above conditions are not met the sub arrays are unsorted
    -we need to sort or exchange the values.
    */
    if (left < right) {
      int cache = ar[left];
      ar[left] = ar[right];
      ar[right] = cache;
    }

    /*if there are any duplicate value
    the above conditions would not met
    and the algo will go into an infinite loop.
    this if condition checks if there are any
    duplicate value if exixts... increase
    left or decrease right one to break the loop
    */
    if (ar[left] == ar[right]) {
      right--; /*if we increase left then pivot index will be the right one*/
    }

}

int pivotIndex = left; /*updated left or right is the pivot index*/
return pivotIndex;

}

How do you veterans handle the duplicate values ?

Naming

I'm a stickler for variable naming - most of my answers mention it. You should be too - correctly named variables make code much easier to maintain.

Now, while I don't know C that well at all, the principles are the same. In your first three lines you do this:

int partition(int ar[], int lft, int rgt) {
int left = lft;
int right = rgt;

Why bother with the extra two declarations? Name the variables correctly in the method declaration, and you can eliminate them entirely. While we're here, what does ar mean? You should name your variables according to the data they hold, so perhaps a method declaration like this would be better:

int partition(int unsortedArray[], int leftIndex, int rightIndex)

Then update your variable references in the rest of the code.

Formatting

Indent your code correctly. Trust me, it makes it much easier to see what comes in which block. I don't know whether this was a product of how you pasted it into the question (which it might well be), but make sure your method code is indented from the declaration.

Sorting algorithms

There are four major sorting algorithms:

Bubble sort
Shuttle sort
Quicksort
Shell sort

Each of those is useful for a different purpose, and each has a different time complexity. If you read through the Wikipedia articles (links above) for each, you should at least have a basic grounding in each.


Google reCAPTCHA Validator This entire class came out of a chat discussion, and I'm curious on how it looks. (This is like literally 30 minutes of development time.)

The idea is to allow very easy, quick implementations of Google's reCAPTCHA ("I am not a robot") checkbox CAPTCHA algorithm. It only requires minor work from the implementor to make it work properly, and that was the point.

Warning: Minimal implementation effort required.

public class ReCaptchaValidator
{
    private const string _HeadScriptInclude = "";
    private const string _ReCaptchaLocationInclude = "

";

    private readonly string _ReCaptchaSecret;
    private readonly string _ReCaptchaSiteKey;

    ///
    /// Returns the script to be included in the &lt;head&gt; of the page.
    ///
    public string HeadScriptInclude { get { return _HeadScriptInclude; } }

    ///
    /// Use this to get or set any extra classes that should be added to the &lt;div&gt; that is created by the <see cref="ReCaptchaLocationInclude"/>.
    ///
    public List<string> ExtraClasses { get; set; }

    ///
    /// Returns the &lt;div&gt; that should be inserted in the HTML where the reCAPTCHA should go.
    ///
    /// <remarks>
    /// I'm still not sure if this should be a method or not.
    /// </remarks>
    public string ReCaptchaLocationInclude { get { return _ReCaptchaLocationInclude.Replace("%SITEKEY%", _ReCaptchaSiteKey).Replace("%EXTRACLASSES%", string.Join(" ", ExtraClasses)); } }

    ///
    /// Creates a new instance of the <see cref="ReCaptchaValidator"/>.
    ///
    /// <param name="reCaptchaSecret">The reCAPTCHA secret.</param>
    /// <param name="reCaptchaSiteKey">The reCAPTCHA site key.</param>
    public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey)
    {
        _ReCaptchaSecret = reCaptchaSecret;
        _ReCaptchaSiteKey = reCaptchaSiteKey;
    }

    ///
    /// Determines if the reCAPTCHA response in a NameValueCollection passed validation.
    ///
    /// <param name="form">The Request.Form to validate.</param>
    /// <returns>A boolean value indicating success.</returns>
    public bool Validate(NameValueCollection form)
    {
        string reCaptchaSecret = _ReCaptchaSecret;
        string reCaptchaResponse = form["g-recaptcha-response"];
        bool passedReCaptcha = false;

        using (WebClient client = new WebClient())
        {
            byte[] response = client.UploadValues("https://www.google.com/recaptcha/api/siteverify",
                                                    new NameValueCollection() { { "secret", reCaptchaSecret }, { "response", reCaptchaResponse } });

            string reCaptchaResult = System.Text.Encoding.UTF8.GetString(response);

            if (reCaptchaResult.IndexOf("\"success\": true") > 0)
                passedReCaptcha = true;
        }

        return passedReCaptcha;
    }
}

Usage:

string reCaptchaSecret = "";
string reCaptchaSiteKey = "";

ReCaptchaValidator rcv = new ReCaptchaValidator(reCaptchaSecret, reCaptchaSiteKey);
bool passedReCaptcha = rcv.Validate(Request.Form);

It should be pretty self-explanatory. You can use the ReCaptchaValidator.HeadScriptInclude to get the entire

Some quick shots at the code

by retrieving the ReCaptchaLocationInclude property an ArgumentNullException is thrown, because you didn't initialize the ExtraClasses property. I would also like to suggest changing this property from autoimplemented to a normal one, so you can validate any set value.
bool passedReCaptcha = false; is not really a good name here. so I wouldn't use this variable at all. Instead I would replace this

if (reCaptchaResult.IndexOf("\"success\": true") > 0)
    passedReCaptcha = true;

with

    return (reCaptchaResult.IndexOf("\"success\": true") > 0);

and for the IDE's love add a return false; at the end of the method. If you don't want to do this, it is fine too, but you should replace the former if with passedReCaptcha = (reCaptchaResult.IndexOf("\"success\": true") > 0);


Revised: Counting subnumbers divisible by a given prime Assume we are given a number and a prime p. The goal is to count all subnumbers of number that are divisible by p. A subnumber of number is formed of any number of consecutive digits in the decimal representation of number.
Following answers to my first question about code to solve this problem, I improved my code as follows:
from collections import Counter

number = input()
p = int(input())

ans = 0

if p != 2 and p != 5:

    res = (int(number[i:]) % p for i in range(len(number)))
    C = Counter(res)

    for c in C:
        if c == 0:
            ans += (C[c] * (C[c] + 1)) // 2
        else:
            ans += ((C[c] - 1) * C[c]) // 2

elif p == 2:
    for i in range(len(number)):
        if number[i] in ('0', '2', '4', '6', '8'):
            ans += i + 1

else:
    for i in range(len(number)):
        if number[i] in ('0', '5'):
            ans += i + 1

print(ans)

Now, it passes more tests but yet there are several tests with time limit exceeded error.
Can this new solution be made faster?

It is a bit weird to have two special cases and start with the case that it is neither of those. I would re-arrange your cases to
if p == 2:
    ...
elif p == 5:
    ...
else:
    ...

Unless you did this as a performance improvement, because a random prime is much more likely to be neither 2 nor 5, but in that case you should have some measurements to show that this is not a premature optimization and probably add a comment in the code.

If you iterate over something and need both the index and the value, use enumerate:
if p == 2:
    ans = sum(i for i, digit in enumerate(number, 1)
              if digit in {'0', '2', '4', '6', '8'})
elif p == 5:
    ans = sum(i for i, digit in enumerate(number, 1)
              if digit in {'0', '5'})

Note that I used the second (optional) argument of enumerate to start counting at one and I used a set instead of a tuple, because it has faster membership test (not a lot faster for small collections as this, but measurably faster). I also put it all into a generator expression and used sum.

Python has an official style-guide, PEP8, which recommends using lower_case for variables, so your counter should be called c, or better yet, something readable like digit_count. You can also factor out the 0 case to avoid having to check for it up to nine times:
else:
    slices = (int(number[i:]) % p for i in range(len(number)))
    digit_count = Counter(slices)

    # get the count of zeroes, with a default value of zero
    zeroes = digit_count.pop(0, 0)
    ans = (zeroes * (zeroes + 1)) // 2
    ans += sum((count * (count - 1)) // 2
               for count in digit_counts.values())


Implement Queue using fixed size array I came across below interview question and I am working on it:

  Build a queue class with the enqueue and dequeue methods. The queue
  can store an UNLIMITED number of elements but you are limited to
  using arrays that can store up to 5 elements max.

Here is what I was able to come up with. Is this the right way to do it in the interview or is there any better way we should implement in the interview?

class Solution {
  private final List<List<Integer>> array;

  public Solution() {
    this.array = new ArrayList<>();
  }

  public void enqueue(int value) {
    if(array.isEmpty()) {
      List<Integer> arr = new ArrayList<>();
      arr.add(value);
      array.add(arr);
      return;
    }
    if(array.get(array.size() - 1).size() != 5) {
      array.get(array.size() - 1).add(value);
      return;
    }
    List<Integer> arr = new ArrayList<>();
    arr.add(value);
    array.add(arr);
    return;
  }

  public int dequeue() {
    if(array.isEmpty()) {
      return -1;
    }
    for(List<Integer> l : array) {
      for(int i=0; i<l.size(); i++) {
        return l.remove(i);
      }
    }
    return -1;
  }
}

Interview questions are almost always meant to evaluate you by the clarifying questions you ask.

In this case, what data structures were you allowed to use beyond the arrays? Clearly something was allowed, since the requirement for queue and dequeue methods implicitly allow a class to be used. But was the Queue class allowed to contain auxiliary classes?

If not, the solution calls for the queue to be an Object[5] where the fifth element, when not null, is the next Object[5].

Requirement said arrays, not ArrayLists.
The requirement did not specify the type so a generic type should have been used instead of assuming integers are ok.
Use of primitive types and lack of exceptions means you're limited to positive numbers, as half of the valid value range has been used for errors.
You have copy-pasted the sub-array initialization code twice. That should have been a shared method.
When dequeueing, you leave the empty sub-arrays in the outer list, forcing you to perform an O(N) search to find the first element. Should have just removed the empty sub-arrays and gone for the O(1) operation of getting the first element from the first sub-array.
The check for array.isEmpty() in dequeue is redundant and adds a third exit point for that method.

Edit: I was thrown off by the for-loop in the dequeue method. Fixed my answer.


Computing the probability of rolling a given total with five dice I'm trying to compute the probability of rolling a certain dice total with an unknown number of dice.  Below is a brute force example for 5 dice, hence the 5 loops.  I feel like there is a recursive answer here that I can't put my finger on that would work for different numbers of dice.

function computeDiceOdds(dice, desired_result, sides = 6) {
    var successes = 0;
    for (let a = 1; a <= sides; a++ ) {
        for (let b = 1; b <= sides; b++) {
            for (let c = 1; c <= sides; c++) {
                for (let d = 1; d <= sides; d++) {
                    for (let e = 1; e <= sides; e++) {
                        if ((a + b + c + d + e) == desired_result) {
                            successes++;
                        }
                    }
                }
            }
        }
    }
    return successes.toString() + " / " + Math.pow(sides, dice).toString();
}

console.log(computeDiceOdds(5, 21)); // "540 / 7776"

I feel like there is a recursive answer here that I can't put my finger on that would work for different numbers of dice.

Sure. Let's work with an example, let's say the one in the question: how many ways can we roll 5 dice to get 21.
The answer could be computed as the sum of answers of the following questions:

How many ways are there to roll 4 dice to reach 20? (first roll was 1)
How many ways are there to roll 4 dice to reach 19? (first roll was 2)
...
How many ways are there to roll 4 dice to reach 15? (first roll was 6)

And each of these questions can be further broken down:

How many ways are there to roll 3 dice to reach 19? (first roll was 1, 2nd was 1)
How many ways are there to roll 3 dice to reach 20? (first roll was 1, 2nd was 2)
...
How many ways are there to roll 3 dice to reach 9? (first roll was 6, 2nd was 6)

I hope you see how the pattern will reach a point where we are actually able to give an answer, for example:

How many ways are there to roll 0 dice to reach 0 -> 1 :-)
How many ways are there to roll 0 dice to reach 1 -> 0, not possible to reach non-zero without dice
How many ways are there to roll 2 dice to reach 0 -> 0, not possible, the remaining rolls will be greater than 0
...

In other words, the recursive function must first check the terminating condition, as usual. If we know the answer given the input (0 or 1), we return it. Otherwise, we compute the sum of calling the function for each possible path (dice value), with one less dice, and reduced target to reach.


RUST: How can I optimize this password cracking algorithm? I made this project in my free time to try to crack a 4x hashed md5 with a couple of hints but I'm looking for ways I can optimize this algo because it's pretty slow and I know you could probs use threading or some other ways to increase speed.
If you can even point out some minor ways to increase performance I would be eternally grateful.
I was told I could try re-using the memory for the strings but I'm not sure how to do that. I was also told to not generate the wordlist in advance and try  as I go but I dont think the algorithm I used supports that?
Here's the branch to review (more info in the readme): https://github.com/TransmissionsDev/crack_yearn_md5/tree/v1
Here's the main.rs file:
#[cfg(debug_assertions)]
use num_format::{Locale, ToFormattedString};

fn hash_md5(input: String) -> String {
    format!("{:x}", md5::compute(input.into_bytes()))
}

fn main() {
    let goal_hash = "dbba1bfe930d953cabcc03d7b6ab05e";

    let length = 17;

    let alphabet = "bdeilmostu-"
        .split("")
        .map(String::from)
        .filter(|letter| !letter.is_empty())
        .collect::<Vec<String>>();

    let mut words = alphabet.clone();

    for phase in 1..length {
        let mut temp: Vec<String> = Vec::new();

        #[cfg(not(debug_assertions))]
        let loopable_words = words.iter();

        #[cfg(debug_assertions)]
        let loopable_words = words.iter().enumerate();

        for data in loopable_words {
            #[cfg(not(debug_assertions))]
            let word = data;

            #[cfg(debug_assertions)]
            let word = data.1;
            #[cfg(debug_assertions)]
            let index = data.0;

            for letter in alphabet.iter() {
                let new_word = format!("{}{}", word, letter);

                temp.push(new_word);
            }

            #[cfg(debug_assertions)]
            if index != 0 && ((index % 1000000) == 0) {
                println!(
                    "Completed phase {}/{}'s sub-phase {}/{}",
                    phase,
                    length,
                    index.to_formatted_string(&Locale::en),
                    words.len().to_formatted_string(&Locale::en),
                );
            }
        }

        words = temp;
    }

    #[cfg(debug_assertions)]
    let word_list_length = words.len();
    #[cfg(debug_assertions)]
    println!("\n\nLength of word list: {}\n\n", word_list_length);

    #[cfg(not(debug_assertions))]
    let loopable_words = words.iter();

    #[cfg(debug_assertions)]
    let loopable_words = words.iter().enumerate();

    for data in loopable_words {
        #[cfg(not(debug_assertions))]
        let word = data;

        #[cfg(debug_assertions)]
        let word = data.1;
        #[cfg(debug_assertions)]
        let attempts = data.0;

        let merged = format!(
            "{}{}",
            word, "........................................................!1"
        );

        let hash = hash_md5(hash_md5(hash_md5(hash_md5(merged.clone()))));

        #[cfg(debug_assertions)]
        println!(
            "Attempts: {}/{}, Hash: {}, Text: {}",
            attempts, word_list_length, hash, merged
        );

        if hash == goal_hash {
            println!("We found the password: {}", merged);

            return;
        }
    }
}

First, I would change the approach fundamentally: I'd consider the plaintext as a 17-digit number in "base 11" (because there are 11 "letters" in the alphabeth). Then the loop becomes a simple iteration through a range from 0 to 1117-1. On top of that, threading will be easy to add using rayon.
As you were told, we should re-use memory for strings and not generate the wordlist in advance.
To re-use memory for strings, we treat them as long-living containers and call .clear() whenever we need to reuse them for another purpose.
This part in your code
for letter in alphabet.iter() {
    let new_word = format!("{}{}", word, letter);

    temp.push(new_word);
}

is very inefficient as it's calling format!, which is complex machinery, just to append a letter. It would be better to have at least String::push there, but we will take another approach below.

On 4 cores, the all-in-one is roughly as fast as just your non-parallelized first pass. For parallelism, I did parallel fold after Range<u64>. So each thread has a little bit of its own state there.
Make sure you add rayon = "0.6" in the Cargo manifest.
Here is the code:
#[cfg(debug_assertions)]
use num_format::{Locale, ToFormattedString};
use std::str;
use std::io::Write;
use rayon::prelude::*;

static PASS_LENGTH: u32 = 17;

#[derive(Default)]
struct State {
    plaintext: Vec<u8>,
    intermediate: Vec<u8>,
}

impl State {
    fn new() -> Self {
        let mut result = State::default();
        for _ in 0 .. PASS_LENGTH {
            result.plaintext.push(b"x"[0]);
        }
        result.plaintext.extend(b"........................................................!1".iter().copied());
        result
    }
}

fn main() {
    let goal_hash = "dbba1bfe930d953cabcc03d7b6ab05e";

    let num_of_md5 = 4;
    let alphabet = "bdeilmostu-";
    let alphabet_len = alphabet.len() as u64;

    let total = alphabet_len.pow(PASS_LENGTH);

    (0 .. total).into_par_iter().fold(
        || State::new(),
        |mut state, num| {
            for i in 0 .. PASS_LENGTH {
                let letter = ((num / alphabet_len.pow(i)) % alphabet_len) as usize;
                state.plaintext[i as usize] = alphabet.as_bytes()[letter];
            }

            state.intermediate.clear();
            write!(&mut state.intermediate, "{:x}", md5::compute(&state.plaintext[..]));
            for _ in 1 .. num_of_md5 {
                let digest = md5::compute(&state.intermediate[..]);
                state.intermediate.clear();
                write!(&mut state.intermediate, "{:x}", digest);
            }
            let result = &state.intermediate[..];

            if result == goal_hash.as_bytes() {
                println!("We found the password: {}", str::from_utf8(&state.plaintext[..]).unwrap());
            }

            if num % 1_000_000 == 0 && num != 0 {
                #[cfg(debug_assertions)]
                println!(
                    "Attempts: {}/{}, Text: {}",
                    num, total, str::from_utf8(&state.plaintext[..]).unwrap()
                );
            }

            state
        }
    ).for_each(|_| {});
}
```


Fabricate with work dates I am using code like this everywhere. How can I reduce such this code so that my Ruby code looks a lot cleaner?

Fabricate(:tl, :when =>Date.yesterday.to_s,:work => 266,:type => "fast" )
Fabricate(:tl, :when =>Date.yesterday.to_s,:work => 100,:type => "super_fast" )
Fabricate(:tl, :when =>Date.yesterday.to_s,:work => 50,:type => "ludicrous" )
Fabricate(:tl, :when =>Date.yesterday.to_s,:work => 900,:type => "budget" )

I found that I can use hash({}).each method to dry up the code. It is :

{"fast"=>266, "super_fast" => 100, "ludicrous" => 50, "budget" => 900}.each{ |key, value|
  Fabricate(:tl, :when =>Date.yesterday.to_s,:work => value,:type => key )
}

If there is a better technique, please share it.


Longest Alternating Substring Code Challenge I'm still new to C++ and tried one of the harder challenges on a coding practice/challenge website.
The challenge was the following:

Given a string of digits, return the longest substring with alternating odd/even or even/odd digits. If two or more substrings have the same length, return the substring that occurs first.

I was able to solve it in ~ 50 minutes, but I feel like my code is really sloppy. Can anyone give me tips on how to simplify this? I find myself falling into very complex ways of solving things often.

std::string longestSubstring(std::string digits) {
    std::string strOddEven{};
    std::vector<std::string> vecOddEven{};
    bool next{};
    int maxSize{};

    for (int i = 0; i < digits.size() - 1; ++i)
    {
        if ((digits.at(i) % 2 == 0 && digits.at(i + 1) % 2 != 0) ||
            (digits.at(i) % 2 != 0 && digits.at(i + 1) % 2 == 0))
        {
            strOddEven += digits.at(i);
            next = true;
            if (i == digits.size() - 2)            // add to strOddEven and push_back
            {                                      // in case of longest string being at end
                strOddEven += digits.at(i + 1);
                vecOddEven.push_back(strOddEven);
            }
        }
        else if (next)
        {
            strOddEven += digits.at(i);
            vecOddEven.push_back(strOddEven);
            strOddEven = "";
            next = false;
        }
    }

    for (auto& i : vecOddEven)
        if (i.size() > maxSize)
            maxSize = i.size();

    for (auto& i : vecOddEven)
        if (i.size() == maxSize)
            return i;
}

Include the headers we need

#include <string>
#include <vector>

Don't make unnecessary copies

std::string longestSubstring(const std::string& digits) {

Also, consider storing std::string_view objects internally, as these are much lighter than owning strings.

Fix the bug

I get a SIGABRT when I call with empty string as argument:

==1629582==    by 0x4914B54: __cxa_throw (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==1629582==    by 0x490C090: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==1629582==    by 0x10AC0B: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::at(unsigned long) const (basic_string.h:1087)
==1629582==    by 0x10A30F: longestSubstring(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (238652.cpp:12)
==1629582==    by 0x10A725: main (238652.cpp:46)

Use the algorithm library

The standard <algorithm> library has the tools you need, that will save you writing much tedious and error-prone code for yourself.

The most useful function here is std::adjacent_find(), which you can use with a simple predicate such as

auto const both_even_or_both_odd
    = [](auto a, auto b){ return a % 2 == b % 2; };

No need to store all the substrings

We only need to remember the longest substring seen so far, so there's no need for the two vectors.  Just have a single std::string_view variable that you update when you find a longer run than the existing one.

Modified version

Here's a version using std::adjacent_find() as suggested:

#include <algorithm>
#include <string>
#include <utility>
#include <vector>

std::string longestSubstring(const std::string& digits)
{
    std::string longest = "";

    auto const same_mod2 = [](auto a, auto b){ return a % 2 == b % 2; };

    auto start = digits.begin();
    while (start < digits.end()) {
        auto finish = std::adjacent_find(start, digits.end(), same_mod2);
        if (finish != digits.end()) {
            ++finish;
        }
        auto const candidate = std::string{start, finish};
        if (candidate.size() > longest.size()) {
            longest = std::move(candidate);
        }
        start = finish;
    }

    return std::string{longest};
}

And a simple test:

#include <iostream>

int main()
{
    for (auto const& s: { "", "0", "11", "01", "112",
                          "0112", "01224", "01223", "01123" }) {
        std::cout << s << "-->" << longestSubstring(s) << '\n';
    }
}