Sanity check for mail code I have the following PHP function, which is admittedly not quite as robust as it could be (I'm missing some sanity checks including a proper one for $sendTo for example; odd choices in text formatting, too) but I don't see why it shouldn't work. A version of it certainly worked the other day, though I made a few small changes (allegedly for the better!) today. When the function executes, my condition if (mail(etc)) indeed appends a success string to my content.

<?php
$FROM_EMAIL = "foo@bar.com";
$email = "foo@bar.com";
$content = "";
$emailMessage = "";
$sendTo = "";

if ($_SERVER['REQUEST_METHOD'] === 'POST') {
  foreach ($_POST as $key => $value) {
    $value = (string) $value;
    if ($key === "DESCRIPTION_OF_ITEMS")
      $value = str_replace("\r", '
', $value);
    if ($value !== "" && $key !== "hiddenField" && $key !== "button2" && $key !== "cloneAdmin")
      $emailMessage .= "$key - $value
\n";
    if ($key === "EMAIL")
      $sendTo = $value;
  }

  if ($emailMessage !== "") {
    $content .= "
Thank you for submitting your registration form... we'll get back to you shortly.

You can expect to receive an email with the following registration information:
";
    $content .= $emailMessage;
    if (mail("foobar@bazbat.com,$sendTo", "Foobar Registration Form", $emailMessage, "MIME-Version: 1.0\r\nContent-type: text/html; charset=iso-8859-1\r\nFrom: $FROM_EMAIL\r\nReply-to: $email")) {
      $content .= "
Email successfully sent!
";
    } else {
      $content .= "
We encountered an error and the email was not successfully sent.
";
    }
  } else {
    $content .= "No Email message";
  }
  $content .= "
";
} else {
  $content .= "

A blank or invalid form was sent; your submission has not been successful.
";
}
?>

For the record, I tried the mail() function in its most basic form as well:

mail('foo@bar.com', 'My Subject', $emailMessage);

and it didn't make a difference. The successful trial earlier this week used the additional header parts.

Unfortunately I don't have access to the PHP logs; I only have FTP access. Also as you may have been able to guess from the coding style, I'm generally more of a JavaScript guy, so I'm not sure what my options are for capturing errors without the PHP log.

Is it possible that there's just a problem (or security measures?) with the mail server? That the PHP code is fine and the problem is elsewhere? Maybe my own mail server is flagging spam and not even passing it into my junk box?

Don't know if you wanted a full review or not so I supplied one since I was going over it anyways. Not really much wrong, mostly aesthetics. I could find no errors in your code, but maybe one of these fixes will ninja your problem away. You never know, the all-powerful PHP god's might be feeling better today :)

POST Data

Not sure if better, but if I'm looking for $_POST variables, I usually just do the following. Up to you though, nothing wrong with what you've got.

if($_POST) { etc... }

You should have some sort of sanitizer for those user inputs though. This is the only real issue I see with your code. Take a look at filter_input if you have PHP 5.2 or greater, otherwise you'll have to take a look around at what others are doing. It is never a good idea to take raw user input and run it in your program. That can lead to all sorts of nasty things.

Ignoring Keys

If you have a list of keys, or other items that you want to exclude, make them into an array and then check against it. Its cleaner than having a bunch of if statements and allows for extension later.

$exceptions = array(
    'hiddenField',
    'button2',
    'cloneAdmin',
);

if ($value !== "" && ! in_array($key, $exceptions)) { etc... }

Also, please use curly braces everywhere and not just on outter or large if statements. Yes PHP can work without them, but debugging your code will become very difficult later. Many might argue this point, but it truly is easier to read.

Since you are already checking if the value is blank, you might as well move that to the beginning and place all other checks inside it. What's the use of running all that code if the value is blank or a key you don't want to process? Better yet, just add the following to the top of the foreach loop and it will skip those records entirely.

if($value === '' || in_array($key, $exceptions)) { continue; }

Multiple If Statements

Don't use if statements for checking variables that you know a range of values for. Switch statements are faster and easier to read.

//Don't do this
if($key === 'DESCRIPTION_OF_ITEMS') { etc... }
if($key === 'EMAIL') { etc... }

//Do this
switch($key) {
    case 'DESCRIPTION_OF_ITEMS':
        //etc...
    break;
    case ''EMAIL':
        //etc...
    break;
}

Long Variables

Heredoc will make your long variables easier to read.

$content .= <<HTML

Thank you for submitting your registration form... we'll get back to you shortly.

You can expect to receive an email with the following registration information:

$emailMessage
HTML;

Long Strings as Arguments

If you are going to use long strings in functions you should replace them with variables, even if you aren't going to use those variables again. It makes it cleaner and easier to read.

$recipients = array();//You'll see why I used an array soon
$recipients[] = "foobar@bazbat.com";
$recipients[] = "$sendTo";
$recipients = implode(', ', $recipients);//See :)

$subject = "Foobar Registration Form";

$headers = array();//Same as recipients
$headers[] = "MIME-Version: 1.0";
$headers[] = "Content-type: text/html;";//This line didn't originally have "\r\n" so maybe this was your problem?
$headers[] = "charset=iso-8859-1";
$headers[] = "From: $FROM_EMAIL";
$headers[] = "Reply-to: $email";
$headers = implode("\r\n", $headers);

mail($recipients, $subject, $emailMessage, $headers);//See much cleaner

Sorry I couldn't be of more use in finding your error. Since you are indeed receiving a success string, I would say that your send to line is just wrong somehow. Try dumping its contents before sending the message to manually check if anything is wrong. Only other thing I can think of is that maybe your server doesn't have the mail() function installed. Though, since you said it succeeded at some point, I don't know what to tell you. Check php_info() anyways to see if its enabled. As for if this is the wrong stackoverflow subsite? Not really. It could have gone on stackoverflow and no one would have complained. Here you just get the added benefit that I reviewed your code too :)


Simple Priority Queue I am trying to implement A* search on a grid in MATLAB. I am currently using a priority queue class I found here, but it's a bit slow. I tried to write this simple priority queue class in MATLAB:

classdef PQ2 < handle

    properties
        nElements
        indx;
        priorityList;
        valueList;
    end

    methods
        function thePQueue = PQ2()
            thePQueue.nElements = 0;
            thePQueue.priorityList = NaN*ones(500,1);
            thePQueue.valueList{500} = [];

            thePQueue.indx = 1;
            thePQueue.nElements = 0;
        end

        function push(thePQueue, value, priority)
            thePQueue.priorityList(thePQueue.indx) = priority;
            thePQueue.valueList{thePQueue.indx} = value;
            thePQueue.indx = thePQueue.indx + 1;
            thePQueue.nElements = thePQueue.nElements + 1;
        end

        function minPriorityElement = pop(thePQueue)
            if ~thePQueue.isEmpty
                thePQueue.nElements = thePQueue.nElements - 1;
                [~, minPriorityIndx] = min(thePQueue.priorityList);
                minPriorityElement = thePQueue.valueList{minPriorityIndx};

                thePQueue.priorityList(minPriorityIndx) = NaN;
            else
                disp('Queue is empty');
            end
        end

        function flagIsEmpty = isEmpty(thePQueue)
        flagIsEmpty = (thePQueue.nElements == 0);
        end
    end
end

The above code is at least 3 times faster than the one I've mentioned above. I'm getting exactly the same output from these 2 classes, and therefore I believe that the code is correct, but I'm not 100% certain. How can I check it? Is there any other way I could implement the same idea and get a faster result?

[This answer comes a couple of years after the question was posted. The Community User keeps bumping the question because there is an answer without upvotes, not worth up-voting. It also has 2k+ views. So maybe a good review is in order.]

This priority queue implementation has an \$O(1)\$ push operation, and an \$O(n)\$ pop operation, but with \$n\$ being the size of the container (500) rather than the number of elements in the queue. However, the push complexity increases once there have been 500 elements in the queue, because the two data arrays will be reallocated when pushing more elements. Note that this is not 500 elements currently in the queue, but 500 elements having gone through the queue. Used array elements are not re-used.

Code review:

Variable names are excellent: it is clear what they mean without any comments.

The class has no documentation. Documentation on how to use a class or a function is as important (IMO) as the code working correctly.

There are a few statements that could be more concise: NaN*ones(500,1) is the same as NaN(500,1). thePQueue.valueList{500} = [] is better written as thePQueue.valueList = cell(1,500). thePQueue.nElements is set to 0 twice in the constructor.

The constructor PQ2() could take an optional input argument specifying how many elements one expects will go through the queue, to give the queue an appropriate initial size.

Timing:

This is a very fast implementation of a priority queue (as long as it is not used for too long, because performance degrades after 500 elements have been pushed. I wrote the following script to time it, and try to improve on it:

t_push = 0;
t_pop = 0;
for jj=1:100
   pq = PQ2;
   tic;
   for ii=1:500
      pq.push(ii,randi(10000));
   end
   t_push = t_push + toc;
   tic;
   for ii=1:500
      pq.pop;
   end
   t_pop = t_pop + toc;
end
t_push
t_pop

Sure, it's a little simplistic, but so be it. It pushes 500 elements, then pops 500 elements. This is repeated, with a fresh queue, 100 times. The OP's priority queue implementation gives me t_push = 0.1822 and t_pop = 0.2930. I could not make this significantly faster.

I compared this against the classical \$O(\log n)\$ priority queue as implemented in the link provided by the OP. That one gave t_push = 5.9049 and t_pop = 28.1718. No wonder OP thinks it's slow. That implementation has many problems, one is that it tries to do memory management. If we let MATLAB do the memory management (MATLAB will double the storage when appending elements to an array, and does so much more efficiently than this user's code), and store the priorities in a numeric array rather than a cell array, I get t_push = 0.5715 and t_pop = 1.8866, 10 times faster for pushing and 15 times faster for popping. But still nowhere near the speed of the code in this question.

Next, I increased the size of the queue to 5000, and changed the test code to run 10 iterations of pushing and popping 5000 elements. Push times remained similar for both queues, and pop times increased to 0.9861 for OP's code, and 2.8028 for the other implementation. Here one can see the \$O(n)\$ vs \$O(\log n)\$ characteristic of these two implementations. But it is clear that n times a small number is much less than log(n) times a large number.

For very large queues, with millions of elements, I'd stick to one of the other solutions proposed in the Stack Overflow question (the built-in Java queue or a C++ MEX-file implementation).


Conversion from postfix to infix in C Just over half way through in K&R, came across an exercise to convert from postfix ("reverse-Polish") notation to infix (standard) notation.

Basically I'm posting here to see if there is anything excessive in my code, or any way to reduce the amount of storage it takes up. It runs extremely fast and doesn't take up much space, but if space can be reduced I believe it always should be.

Also, I have tried to maintain the clearest readability and consistent style throughout my code, but I am new to C, so if there are certain style conventions I am breaking, let me know as well.

#include <stdio.h>
#include <ctype.h>
#include <string.h>

#define MAXLEN 1000    // Maximum string length

int isOperator(int c);
void addPrecedence(char * infixExpr);
void pushExpression(char * infixExpr);
char * popExpression(void);

int main(int argc, char * argv[])
{
    // Only one argument is allowed
    if (argc != 2) {
        printf("Usage: evaluate [expression] from postfix to infix\n");
        return 1;
    }

    int i;
    char * expression = argv[1];
    char left[MAXLEN], right[MAXLEN], expr[MAXLEN];

    for (i = 0; expression[i] != '\0'; i++) {
        if (isalpha(expression[i]) || isdigit(expression[i])) {
            // Copy character into a string so it can be pushed onto the stack
            expr[0] = expression[i], expr[1] = '\0';
            pushExpression(expr);

        } else if (isOperator(expression[i])) {
            strcpy(right, popExpression());
            strcpy(left, popExpression());

            expr[0] = expression[i], expr[1] = '\0';
            strcat(left, expr);
            strcat(left, right);
            addPrecedence(left);
            pushExpression(left);

        } expr[0] = '\0';    // Reset expr character array
    }
    printf("%s\n", popExpression());
    return 0;
}

// Check if the character is a valid operator
int isOperator(int c)
{
    int i;
    char legalOps[] = {'+', '-', '*', '/'};

    for (i = 0; i < 4; i++)
        if (c == legalOps[i])
            return 1;
    return 0;
}

// Wrap the expression in paranthesis to maintain order of precedence
void addPrecedence(char * infixExpr)
{
    int i, needsPrecedence = 0;

    // Check if parens are even needed
    for (i = 0; infixExpr[i] != '\0'; i++) {
        if (infixExpr[i] == '(') {
            break;

        } else if (infixExpr[i] == '+' || infixExpr[i] == '-') {
            needsPrecedence = 1;
            break;
        }
    }
    // If parens are needed, insert them at the beginning and end
    if (needsPrecedence) {
        infixExpr[strlen(infixExpr)] = ')';
        for (i = strlen(infixExpr)-1; i >= 0; i--)
            infixExpr[i+1] = infixExpr[i];
        infixExpr[0] = '(';
    }
}

#define MAXEXPR 100    // Maximum expression stack value

// Create expression stack
char expressionStack[MAXEXPR][MAXLEN];
int currentExpr = 0;

// Push an expression (character array) onto the expression stack
void pushExpression(char * infixExpr)
{
    int i = 0;

    // Copy each character from infixExpr into the expression stack
    while (*infixExpr != '\0')
        expressionStack[currentExpr][i++] = *infixExpr++;

    // Delete any additional data from previous stack value
    expressionStack[currentExpr][i] = '\0';
    ++currentExpr;
}

// Return expression (character array) from top of stack
char * popExpression(void)
{
    // Return NULL if expression stack is empty
    if (currentExpr == 0 || expressionStack[currentExpr-1][0] == '\0') {
        return 0;

    } else {
        // Save expression to be returned, then delete it from the stack
        char * expr = expressionStack[--currentExpr];
        return expr;
    }
}

Since you are using end-of-line commands, I will assume that you are using at least C99 in this review (it may be a compiler extension to C89 though).

main

There are too many things in your main function. It shouldn't handle the expression parsing, but delegate this task to another function. Also, if you are using C99, you don't need to return 0; at the end of main: the compiler will have your program automagically return 0 when it reaches the end of the function.

isOperator

Whenever it makes sense, try to use boolean values instead of integers. The header <stdbool.h> defines bool, true and false that you can use to make your code clearer. Using booleans instead of integers can help to clarify your intent. For example, here is your function isOperator with booleans instead of integers

bool isOperator(int c)
{
    int i;
    char legalOps[] = {'+', '-', '*', '/'};

    for (i = 0; i < 4; i++)
        if (c == legalOps[i])
            return true;
    return false;
}

By the way, there are many other way to implement this function. Your current function uses a loop and therefore performs in \$ O(n) \$ where n is the number of operators while you could rewrite it to perform in \$ O(1) \$, with a lookup table for example. That said, it shouldn't be a problem since you only have 4 operators.

Variable declarations

You seem to declare your variables at the beginning of blocks. It is not needed anymore with C99, you can declare your variable when you need it. You can also declare a variable in a for loop and therefore write things like this:

for (int i = 0; infixExpr[i] != '\0'; i++) {
    // ...
}


Image placed with username in recent post of WordPress I am using this code to display the recent post from my subdomain to my main domain. I got it after spending 6-7 hours. Now I need to show the avatar image and username of the user of has posted the questions on questions.admissiontimes.com. Right now, it's coming in green and black.

function print_requested_template_part() {
    // Respond only to requests from the same address...
    if ( $_SERVER['REQUEST_METHOD'] == 'GET' && isset($_GET['including_template_part']) && isset($_GET['load_part']) && $_GET['load_part'] != '' ) {
        $part = $_GET['load_part'];
        $func = 'render_' . str_replace('-', '_', $part); // if you have declared a function called "render_footer_include", then "?load_part=footer_include"
        if ( function_exists($func) ) {
            // Allow for passing parameters to the function
            if ( isset($_GET['params']) ) {
                $params = $_GET['params'];
                $params = ( strpos($params, ',') !== false )? explode(',', $params) : array($params);
                call_user_func_array($func, $params);
            } else {
                call_user_func($func);
            }
        }
        exit; // if we don't exit here, a whole page will be printed => bad! it's better to have empty footer than a footer with the whole main site...
    }
}
add_action('init', 'print_requested_template_part', 1);

function render_my_recent_posts( $numberposts = 5 ) { ?>

    <?php
        $args = array( 'numberposts' => '5' );
        $recent_posts = wp_get_recent_posts( $args );
        foreach( $recent_posts as $recent ) {
            echo '
' .   $recent["post_title"].'
 ';
        }
    ?>

<?php
}

Take a look at filter_input(). It allows you to sanitize and validate GET/POST variables whether they exist or not. If no variable in that scope exists it will be set to FALSE/NULL which means all you have to do is set it to a variable and ensure it has a value.

$tempPart = filter_input( INPUT_GET, 'including_template_part', FILTER_SANITIZE_STRING );
$part = filter_input( INPUT_GET, 'load_part', FILTER_SANITIZE_STRING );
if( $tempPart && $part ) {

You should avoid internal comments. Internal comments adds clutter to your code and if overused can make your code difficult to read. If your code is self documenting most comments become unnecessary. Anything else should be limited to doccomments.

Ternary statements are good if used properly, but they can become difficult to read if they get too large or if you nest them. Try to keep your ternary statements short and uncomplicated. Here for instance, it is not even necessary. explode() will return an array with the string as the only element if the delimiter does not exist. So you can just call explode() immediately to the same effect.

$params = explode( ',', $params );

You should avoid using exit or die() to force your program to stop. It is inelegant. If you must stop execution use a return or throw an error.

There is a principle called the arrow anti-patern. It dictates that you should avoid having heavy/unnecessary indentation in your code to aid in legibility. To avoid violating it you should return early from your statements or refactor them so that only small portions of code are indented. Returning early sometimes also makes else statements unnecessary. If we reverse that first if statement we can return early allowing us to remove an entire level of indentation from our function.

if( ! $tempPart || ! ! $part ) {
    return FALSE;//or throw error
}

//rest of function here...

Your second function has a $numberposts argument that is unused. You immediately create an array with the same values though. If you need to do something like this you should either immediately inject it that way or you can use compact() to the same effect.

$args = compact( 'numberposts' );

Now, as to your problem with the images. Since they are originating on another part of your domain, I would hazard to guess that the path to said images is not being updated for the new domain path. This means that you will need to somehow modify the path you are given to reflect the differences. Seen as none of the code you provided has images in it I have no idea what the culprit is.


Implementing a thread-safe factory with caching I have a ConnectorFactory that creates Connector objects based on parameters like URLs, username and passwords.

The Connector object itself implements a HTTP connection pool internally, and can handle multiple HTTP requests concurrently.

I would like to do is modify my factory to cache the Connector objects. So it will get an existing Connector object if one already exists for the particular URL/username/password combination, or a new instance otherwise.

The newConnection() method needs to be thread safe, but rather than declaring it synchronized I have opted for a Double-checked locking-style solution.

Just about every reference I found on double checked locking relates to the singleton pattern, and requires use of the volatile keyword. Since I'm not using my Factory in this way, I don't find those relevant.

Here are the relevant parts of the ConnectorFactory I have come up with. I would like for an expert to tell me if it is correct, and if not, what the correct way to do it would be.

private static Map<ConnectionSettings, Connector> connectorCache = new ConcurrentHashMap<ConnectionSettings, Connector>();

public static Connector newConnection(String url, String user, String psswd) throws ConfigurationException {
    ConnectionSettings settings = new ConnectionSettings(url, user, psswd);
    Connector connector = connectorCache.get(settings);
    if (connector == null) {
        synchronized (ConnectorFactory.class) {
            connector = connectorCache.get(settings);
            if (connector == null) {
                connector = new Connector(settings);
                connectorCache.put(settings, connector);
            }
        }
    }
    return connector;
}

I have overridden the equals() and hashCode() methods on ConnectionSettings.

Finally, if only this method accesses connectorCache, can I change it to a normal HashMap rather than a ConcurrentHashMap?

If you are on Java 8, you can use a cool new feature of ConcurrentMap: computeIfAbsent.

It lets you defer the actual construction of the Connection until it becomes necessary and makes your code more concise:

private static ConcurrentMap<ConnectionSettings, Connector> connectorCache =
    new ConcurrentHashMap<ConnectionSettings, Connector>();

public static Connector newConnection(String url, String user, String psswd)
        throws ConfigurationException {

    ConnectionSettings settings = new ConnectionSettings(url, user, psswd);
    return connectorCache.computeIfAbsent(settings, Connector::new);
}

Note the two arguments to computeIfAbsent:

The first is your ConnectionSettings instance, which is used as a key.
The second is an instance of the interface Function. If called, it gets the settings as an argument and must return an instance of Connector. The function is defined by using a lambda expression, in this case a constructor reference. This is possible because the constructor of Connector does exactly that: it takes the settings as an argument and returns a Connector instance.

The call tells your connectorCache to check for the specified settings. If they exist the corresponding connector is returned. If not (and only then!) is the function executed, which creates a new Connector. (Of course in that case the new instance is put into the map.)

So you see the constructor is only invoked if needed. All necessary locking for concurrency is done by the map and you don't have to worry about it.


Getting the grandparent directory of the current code I have this example:

from os.path import dirname, realpath
dirname(dirname(dirname(realpath(__file__)))

And the chained dirname() applications look a bit odd to me. Is there a way to improve my code? There is of course something like Better Function Composition in Python, but that seems too complicated to me.

Maybe, some solution with function composition "∘" would be better?

reduce(∘, [dirname] * 3)(realpath(__file__))

But that doesn't look any better.

Do you have any ideas for this concrete example?

This will be short one, but as we all know, in most cases, "explicit is better than implicit".

Indeed, the second one is ugly and not easily understandable at the first sight. Doing something like this might be better:

from os.path import dirname, realpath

filepath = realpath(__file__)

dir_of_file = dirname(filepath)
parent_dir_of_file = dirname(dir_of_file)
parents_parent_dir_of_file = dirname(parent_dir_of_file)

OFC that's in no way simpler than your first solution, altough it might be clear what the end-result will look like.

More, from here:

  os.path.abspath doesn't validate anything, so if we're already
  appending strings to __file__ there's no need to bother with
  dirname. Just treat __file__ as a directory and start climbing:

# climb to __file__'s parent's parent:
os.path.abspath(__file__ + "/../../")

Climbing more than two levels starts to get ridiculous.

  But, since we know how many levels to climb, we could clean this up
  with a simple little function:

uppath = lambda _path, n: os.sep.join(_path.split(os.sep)[:-n])

# __file__ = "/aParent/templates/blog1/page.html"
>>> uppath(__file__, 1)
'/aParent/templates/blog1'
>>> uppath(__file__, 2)
'/aParent/templates'
>>> uppath(__file__, 3)
'/aParent'


Dividing an arbitrary dollar amount into the fewest bills and coins I am taking a class in mobile application development this fall. I have finished my first assignment of creating a class to divide an arbitrary dollar amount into the minimum number of bills and coins. I have completed the assignment and everything works as intended (outside of an occasional error I'm having with computing pennies, but I am working on that). My main reason for posting is I would like to try and make my code more Swift like. I am coming from a mostly C background so I am learning the ins and outs of Swift and OOP on the fly. So if you have any suggestions to making my code more "Swift like" I would greatly appreciate it.

public class Cash
{
    //    Denominations of monies
    let bills = [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01]

    //    Backing variable
    var value:Double

    //    Computed Property of minimum number of bills to satisfy the value sent in
    public var minBills:[Int]? {
        var tempArray = [Int]()
        var tempValue: Double
        if value >= 0 {
            for index in 0..<bills.count {
                tempValue = value / bills[index]                  //Calculating the amount of bills
                value -= Double(Int(tempValue)) * bills[index]    //Recalculating the value
                tempArray.append(Int(tempValue))                  //Adding number of bills needed to tempArray
            }
            return tempArray                                      //Returns entire tempArray to minBills
        }
        else {
            return nil
        }
    }

    //Constructor
    public init(value: Double) {
        self.value = value
    }
}

The computed property minBills modifies the value property, which would
be unexpected to the caller:

let cash = Cash(value: 50.0)
print(cash.minBills!) // [1, 0, 0, 0, 0, 0, 0, 0, 0]  OK!
print(cash.minBills!) // [0, 0, 0, 0, 0, 0, 0, 0, 0]  What?

Better make value a constant property:

let value: Double

and create a mutable copy only for the computation.

let bills = [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01]

can be made a type property

static let bills = [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01]

because it is the same for each instance.

But why do you need a class at all? This could be just a (free) function

func minimumBills(for value: Double, withDenominations: [Double]) -> [Int]?

instead. Or, if you plan to add more functionality to the Cash type,
consider defining it as a struct unless the reference semantics of a class
are explicitly needed.

Another possible design would be to define a CoinSystem type with
“factory methods” for known coin systems:

public struct CoinSystem {
    let denominations: [Double]

    public init(denominations: [Double]) {
        self.denominations = denominations
    }

    public func optimalBills(for value: Double) -> [Int]? {
        // ... computation here ...
    }

    public static var usDollar = CoinSystem(denominations:
        [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01])
}

This would be used as

let usCoins = CoinSystem.usDollar
let optimal = usCoins.optimalBills(for: 12.34)

and allows adding more coin systems easily.

Now let us consider the “main code,” i.e. the computation of

public var minBills:[Int]?

tempArray and tempValue are bad variable names because they do not
indicate what the variable stands for. Also tempValue is only needed
inside the scope of the for-loop.

Returning early on error conditions simplifies the code and saves one
indentation level:

if value < 0 { return nil }

or

guard value >= 0 else { return nil }

Iterating of the bills array is better done with

for bill in bills {
    // ...
}

instead of subscripting.

Summarizing these suggestions, the code could look like this:

public struct CoinSystem {
    let denominations: [Double]

    public init(denominations: [Double]) {
        self.denominations = denominations
    }

    public func optimalBills(for value: Double) -> [Int]? {
        guard value >= 0 else { return nil }

        var remainingValue = value
        var result = [Int]()
        for denomination in denominations {
            let count = Int(remainingValue/denomination)
            remainingValue -= Double(count) * denomination
            result.append(count)
        }

        return result
    }

    public static var usDollar = CoinSystem(denominations:
        [50.00, 20.00, 10.00, 5.00, 1.00, 0.25, 0.10, 0.05, 0.01])
}

You said

  .... outside of an occasional error I'm having with computing pennies ...

That is to be expected, because Double is  binary floating point number,
and cannot represent all (decimal) fractions exactly. Good resources for
this topic are

What Every Computer Scientist Should Know About Floating-Point Arithmetic
Is floating point math broken? on Stack Overflow.

Possible remedies are

Work with integers only (representing cents, or whatever the lowest
possible denomination is).
Use the Decimal
type to represent the monetary values.

Finally: What you are solving is the Change-making problem,
and your approach is the “greedy method:” Always pick the largest denomination
which is not greater than the remaining amount.
This gives the optimal result for so-called “canonical coin systems,”
like the one used in the US, but not necessarily for arbitrary coin systems.

The general problem can be solved with dynamic programming, and you'll find
further information in the above referenced Wikipedia page.


Optimal way to annihilate a list by removing items from the ends I'm stuck with a contest problem.

Description:

What is the minimum number of moves to annihilate an int array where the possible moves are:

Remove both the first and last elements if they are equal to each other, or
Remove the first element, or
Remove the last element.

Input

The first line is the number of test cases (less than or equal to 200).

For any test case there is two lines.

Line 1 has 1 integer with the size of the vector. (The size never exceeds 1000)
Line 2 has n integers with the values of the vector. (Values never greater than 109)

Ouput

The output must show the test case number with the minimum value of remove steps.

Input Example:

3
5
1 3 1 3 2
5
1 2 3 2 1
7
1 1 2 3 3 4 2

Output Example:

Case #1: 4
Case #2: 3
Case #3: 5

My solution is brute-force and recursive, with memorization:

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

int minOf2(int x, int y){
    return (x < y) ? x : y;
}

int minOf3(int x, int y, int z){
    return minOf2(z,minOf2(x,y));
}

int memoization[1010][1010];

int recur(unsigned long long *arr,int i,int j){
    if(i > j){
        return 0;
    }
    int aux = INT_MAX;
    if(arr[i] == arr[j]){
        aux = recur(arr,i+1,j-1);
    }
    if(memoization[i][j])
        return memoization[i][j];
    else
        return memoization[i][j] = 1 + minOf3(aux,recur(arr,i+1,j),recur(arr,i,j-1));
}

int main()
{
    int t,n,i,j;
    unsigned long long arr[1010];
    scanf("%d",&t);
    for (i = 1; i <= t; i++) {
        memset(memoization,0,sizeof(memoization));
        scanf("%d",&n);
        for (j = 0; j < n; j++) {
            scanf("%llu",&arr[j]);
        }
        printf("Case #%d: %d\n",i,recur(arr, 0, n-1));
    }
    return 0;
}

This solution is still getting "Time Limit Exceeded".

Missing use of memoization
Your program is almost right, but you aren't using your memoization to the fullest potential.  When the array ends match, you don't check for the memoized value.  Therefore, a stress test of an array of 1000 elements with all the same values causes your program to run for a very long time.
Also, another optimization is that when the array ends match, you can always remove 2 instead of also trying to remove just 1 end (which can only be equal or worse than removing both ends).
Other things to note
Your use of 1010 as an array size is a bit odd.  First of all, you can use exactly 1000 as the problem stated.  Maybe you were afraid of a buffer overflow, but there isn't any.   Also you could use a #define instead of copy/pasting the value all over the place.  That way you could easily change the test size without accidentally missing one place or another.
You don't need to use long long for your array.  An int can hold values up to 10^9.
Corrected function
Here is your function with the memoization corrected:
int recur(unsigned int *arr,int i,int j){
    if(i > j){
        return 0;
    }
    if(memoization[i][j])
        return memoization[i][j];
    if(arr[i] == arr[j])
        return memoization[i][j] = 1 + recur(arr,i+1,j-1);
    return memoization[i][j] = 1 + minOf2(recur(arr,i+1,j),recur(arr,i,j-1));
}

I made an array of 1000 elements all the same value and ran the function 10 times.  With the original code, it took 9.6 seconds.  With the corrected code it took 0.04 seconds.


List 130. Surrounded Regions leetcode solution using bfs Given a 2D board containing 'X' and 'O' (the letter O), capture all
  regions surrounded by 'X'.

  A region is captured by flipping all 'O's into 'X's in that surrounded
  region.

Input:

X X X X
X O O X
X X O X
X O X X

Output:

X X X X
X X X X
X X X X
X O X X

All I am doing is to traverse all the edges and set all of them to x_not_possible and also there connected neighbors. Connected neighbors are found by doing bfs.

Traverse once more and set x_not_possible back to O and all the nodes not set to x_not_possible to X.

Code:

class Solution(object):
    def solve(self, board):
        if not board:
            return

        row_len, col_len = (len(board), len(board[0]))
        if row_len == 1 and col_len == 1:
            return

        def get_neighbours((i, j)):
            directions = []
            map_direction = {"left":(0, -1), "right":(0, 1), "up":(-1, 0), "down":(1, 0)}
            for _i, _j in map_direction.values():
                __i, __j = _i + i, _j + j
                if __i < row_len and __i >= 0 and __j < col_len and __j >= 0:
                    yield __i, __j

        increment_col, decrement_row, decrement_col, increment_row = (False, False, False, True)
        queue = []
        stop = [0, 0]
        while True:
            row, col = 0, 1
            if increment_row:
                if stop[row] + 1 >= row_len:
                    increment_col, increment_row = True, False
                    continue
                else:
                    stop[row] += 1
            elif increment_col:
                if stop[col] + 1 >= col_len:
                    decrement_row, increment_col = True, False
                    continue
                else:
                    stop[col] += 1
            elif decrement_row:
                if stop[row] - 1 < 0:
                    decrement_col, decrement_row = True, False
                    continue
                else:
                    stop[row] -= 1
            elif decrement_col:
                if stop[col] - 1 < 0:
                    break
                else:
                    stop[col] -= 1
            i, j = stop
            if board[i][j] == "O":
                queue.append((i, j))

        while len(queue):
            i, j = queue.pop()
            board[i][j] = "x_not_possible"
            for k, l in get_neighbours((i, j)):
                if board[k][l] == "O":
                    board[k][l] = "x_not_possible"
                    queue.append((k, l))

        for i in range(row_len):
            for j in range(col_len):
                if board[i][j] == "O":
                    board[i][j] = 'X'
                elif board[i][j] == "x_not_possible":
                    board[i][j] = 'O'

Review

There are a few things that could be improved upon.

1 .Your get_neighbours could use some love

map_direction = {"left":(0, -1), "right":(0, 1), "up":(-1, 0), "down":(1, 0)}

I might understand using a dictionary so you can see which value corresponds to which direction, but this may be simplified to just using a tuple of directions.

_i and __i are variables badly named I believe, maybe new_i or something would be better. _i and __i implies that these variables are private/protected, and in your case they are not.

Secondly I think with this:

increment_col, decrement_row, decrement_col, increment_row = (False, False, False, True)

You reinvent your get_neighbours

2. Use enumerate

I think this was said in your last question you posted on CR too, and it is still valid. You should defenitly use enumerate when possible, see the corresponding PEP article

3. Doing this in one loop

You have:

A while True: loop
Another while loop

This could be done in one loop, by tracking if any of the current visited vertex in the dfs, are in either the last or first (row or collumn)

Alternative code

def get_neighbours(board, position):
    directions = ((0, -1), (0, 1), (-1, 0), (1, 0))
    for direction in directions:
        new_x, new_y = (p+d for d, p in zip(direction, position))
        if new_x in range(0, len(board) - 1) and new_y in range(0, len(board[0]) - 1):
            yield new_x, new_y

def dfs(board, start):
    visited, stack = set(), [start]
    change = True

    while stack:
        vertex = stack.pop()
        if vertex not in visited:
            if change and (vertex[0] in (0, len(board) - 1) or vertex[1] in (0, len(board[0]) - 1)):
                change = False
            visited.add(vertex)
            for x, y in get_neighbours(board, vertex):
                if (x, y) not in visited  and board[x][y] == "O":
                    stack.append((x, y))
    return visited, change

def solve(board):
    visited = set()

    # Group the O's
    for x, row in enumerate(board):
        for y, col in enumerate(row):
            if col == 'O' and (x, y) not in visited:
                group, change = dfs(board, (x, y))
                visited |= group
                if change:
                    for o in group:
                        board[o[0]][o[1]] = "X"

    return board

if __name__ == '__main__':
    board = [["X", "X", "X", "X"],
             ["X", "O", "O", "X"],
             ["X", "X", "O", "X"],
             ["X", "O", "X", "X"]]
    print(solve(board))


Prim's algorithm via Priority Queues to print the minimum spanning tree of an adjacency matrix undirected graph My adjacency matrix looks like this, where non-zero weights denote edges.
[0, 2, 0, 0, 3]
[2, 0, 2, 4, 0]
[0, 2, 0, 3, 0]
[0, 4, 3, 0, 1]
[3, 0, 0, 1, 0]
Here's my attempt:
//nested class, only showing the relevant parts of code
class Edge implements Comparable<Edge> {
    int source;
    int destination;
    int weight;

    public Edge(int source, int destination, int weight) {
        this.source = source;
        this.destination = destination;
        this.weight = weight;
    }

    public String toString() {
        return source + "---" + weight + "---" + destination;
    }

    @Override
    public int compareTo(Edge other) {
        return Integer.compare(this.weight, other.weight);
    }
}

public void printPrimsMST(int[][] matrix) {
    // Start with any vertex.
    Set<Integer> visited = new HashSet<>();
    Queue<Edge> edgeQueue = new PriorityQueue<>();

    int currentNode = 0;
    visited.add(0);
    while (visited.size() < matrix.length) {

        for (int col = 0; col<matrix[currentNode].length; col++) {
            // Take all edges from this node.
            if (col == currentNode || matrix[currentNode][col] == 0) {
                //skip self as well as 0-weight edges.
                continue;
            }
            edgeQueue.add(new Edge(currentNode, col, matrix[currentNode][col]));
        }

        if (edgeQueue.isEmpty())
            return;

        //find cheapest edge to a not already visited destination.
        Edge edge = edgeQueue.remove();
        while (visited.contains(edge.destination)) {
            if (edgeQueue.isEmpty())
                return;
            edge = edgeQueue.remove();
        }

        System.out.println(edge.source + "--" + edge.weight + "--" + edge.destination);
        // Now that you've reached this edge as a destination, record it.
        currentNode = edge.destination;
        visited.add(edge.destination);
    }
}

I'll comment on code style, mainly.
printPrimsMST() method
It both computes the MST and prints it to System.out. This way you can't reuse it if you want to do some additional processing with its results.
So, change it to return Collection<Edge> without printing anything, and introduce a print() method that takes Collection<Edge>.
For printing, you wrote:
System.out.println(edge.source + "--" + edge.weight + "--" + edge.destination);

That's unneccessarily complex, as you defined a toString() method in the Edge class, giving exactly that format. So it's enough to use
System.out.println(edge);

Curly braces
I'd recommend to always surround blocks with curly braces (you do most of the time, but not always):
    while (visited.contains(edge.destination)) {
        if (edgeQueue.isEmpty())
            return;
        edge = edgeQueue.remove();
    }

Someone editing the code might be tempted to insert an additional instruction before returning, e.g.
    while (visited.contains(edge.destination)) {
        if (edgeQueue.isEmpty())
            doneIt = true;
            return;
        edge = edgeQueue.remove();
    }

That's a nasty trap. By indentation, it looks like a block of two statements, but it isn't (in your specific case, it's not that bad, as the compiler will flag the edge = edgeQueue.remove(); statement as unreachable, but you get the idea). So, make it a habit to always write
    while (visited.contains(edge.destination)) {
        if (edgeQueue.isEmpty()) {
            return;
        }
        edge = edgeQueue.remove();
    }

Restricting field access
class Edge implements Comparable<Edge> {
    int source;
    int destination;
    int weight;
    ...
}

The internals of a class should only be visible to the class itself. The idea of OOP is that a class defines an API visible to the outside world, and is free to decide on (and change) its inner workings. That's close to impossible if outside code directly accesses instance fields.
For that reason, make all fields private, unless you have good reason otherwise. And add only the necessary public getters and setters. And if you don't intend to change a field's value after construction of the instance, make it final as well.
You might argue that Edge is a class only meant for local use. Then make Edge a private class.
Argument checks
You are talking about undirected graphs represented as adjacency matrix. Depending on who will be calling your method, you might want to include a test whether the matrix is valid:

All rows must have the same length.
The matrix must be square.
The matrix must be symmetric.

If any of these conditions are violated, you should throw an IllegalArgumentException.


How to collect reference values from hashmap? I have written a code snippet to use HashMap<String, Vec<&String>> as the main data structure, and finally, I want to return a collected Vec<Vec<String>>, I can archive it by a for-loop, it there any pure iterator way to do this? (I have tried several times, but got tons of
confused compiler errors)
pub fn group_anagrams(strs: Vec<String>) -> Vec<Vec<String>> {
    let mut map: HashMap<String, Vec<&String>> = HashMap::new();
    for s in &strs {
        let mut key = s.clone();
        unsafe {
            key.as_bytes_mut().sort();
        }
        (*map.entry(key).or_insert(vec![])).push(s);
    }
    let mut ans: Vec<Vec<String>> = Vec::new();
    // I don't like these 2 lines
    for v in map.values() {
        ans.push(v.iter().cloned().cloned().collect::<Vec<String>>())
    }
    ans
}

I also tried with this code:
    pub fn group_anagrams(strs: Vec<String>) -> Vec<Vec<String>> {
        let mut map: HashMap<String, Vec<String>> = HashMap::new();
        for s in strs {
            let mut key = s.clone();
            unsafe {
                key.as_bytes_mut().sort();
            }
            (*map.entry(key).or_insert(vec![])).push(s);
        }
        map.values().cloned().collect::<Vec<Vec<String>>>()
    }

In the last line, I want to move all the values from hashmap into result vector but also failed because of the lifetime error(then I gave up with a cloned vector), I am wondering how to make it.

unsafe {
        key.as_bytes_mut().sort();
    }

This works as long as you are working in Ascii, but will have undefined behavior if there is any unicode in the string.
 (*map.entry(key).or_insert(vec![])).push(s);

You don't need the dereference *, calling a method will automatically take care of that.
Its a bit tricky to help with your main question since I don't know what exactly you tried and why that didn't work.
A pretty direct translation of what you wrote into iterator syntax works:
map.values()
    .map(|v| v.iter().cloned().cloned().collect::<Vec<String>>())
    .collect()

I would however suggest doing something more like your second code block, and avoid cloning the strings:
    map.values().cloned().collect::<Vec<Vec<String>>>()

I'd do this:
 map.into_iter()
    .map(|(_key, values)| values)
    .collect()

By consuming the iterator we avoid cloning the vectors.
Alternatively, take the strings as input by reference and return them by reference:
pub fn group_anagrams(strs: &[String]) -> Vec<Vec<&String>> {
    let mut map: HashMap<String, Vec<&String>> = HashMap::new();
    for s in strs {
        let mut key = s.clone();
        unsafe {
            key.as_bytes_mut().sort();
        }
        (*map.entry(key).or_insert(vec![])).push(s);
    }

    map.into_iter()
        .map(|(_key, values)| values)
        .collect()
}


Reading of named settings from the XML Usually it is a good style in Java to create a named constant for a string (and other) hardcoded values. Using the example below I want to know how pedantic I should be in it.

I use a Java class as a wrapper of XML settings. Each function is suppose to return a value from a related XML file.

I defined a lot of constants with keys names on the top and each function use one single constant. Is it a good idea to remove constants and use hardcoded strings directly in each function?

Here is my settings file:

package com.art.backend.configs;

import com.art.backend.configs.inherited.Config;

public class ConfigStorage extends Config {

    private static final String SETTINGS_FILENAME = "storage.cfg.xml";

    private static final String KEY_HOST = "host";
    private static final String KEY_PORT = "port";
    private static final String KEY_USERNAME = "username";
    private static final String KEY_PASSWORD = "password";
    private static final String KEY_CONNECTION_TYPE = "connection_type";
    private static final String KEY_SEPARATOR= "separator";
    private static final String KEY_REMOTE_ROOT = "remote_root";
    private static final String KEY_SUBDIR_UPLOADED = "subdir_uploaded";

    public ConfigStorage() {
        super(SETTINGS_FILENAME);
    }

    public String getHost(){ return getString(KEY_HOST); }

    public int getPort(){ return getInt(KEY_PORT); }

    public String getUsername(){ return getString(KEY_USERNAME); }

    public String getPassword(){ return getString(KEY_PASSWORD); }

    public String getSeparator(){ return getString(KEY_SEPARATOR); }

    public String getConnectionType(){ return getString(KEY_CONNECTION_TYPE); }

    public String getRemoteRoot(){ return getString(KEY_REMOTE_ROOT); }

    public String getSubdirUploaded(){ return getString(KEY_SUBDIR_UPLOADED); }
}

Also I will attach file "Config" as a base class for my settings and XML settings file just in case, but they are not really important.

package com.art.backend.configs.inherited;

import java.io.IOException;
import java.io.InputStream;
import java.util.Properties;

public abstract class Config {
    private static final String MSG_SETTING_NOT_FOUNT = "Settings file not found";

    private final String filename;
    private Properties properties = null;

    public Config(String filename){
        this.filename = filename;
        init();
    }

    private void init(){

        //Reading properties file in Java example
        try {
            properties = new Properties();
            InputStream fis = getClass().getClassLoader().getResourceAsStream("storage.cfg.xml");
            properties.loadFromXML(fis);
        } catch (IOException e) {
            e.printStackTrace();
            throw new IllegalArgumentException(MSG_SETTING_NOT_FOUNT);
        }
    }

    public String getFilename(){ return filename; }

    public String getString(String key){
        return properties.getProperty(key);
    }

    public int getInt(String key){
        return Integer.parseInt(properties.getProperty(key));
    }

    public float getFloat(String key){
        return Float.parseFloat(properties.getProperty(key));
    }

    public boolean getBoolean(String key){
        return Boolean.parseBoolean(properties.getProperty(key));
    }
}

XML settings

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!DOCTYPE properties SYSTEM "http://java.sun.com/dtd/properties.dtd">
<properties>
    <entry key="host">***</entry>
    <entry key="port">***</entry>
    <entry key="username">***</entry>
    <entry key="password">***</entry>
    <entry key="connection_type">sftp</entry>
    <entry key="separator">/</entry>
    <entry key="remote_root">/home/adzhus/art_storage/</entry>
    <entry key="subdir_uploaded">uploaded/</entry>
</properties>

In your use case, where the constants are private and only ever used in one place, they are pretty much boilerplate. One could argue that the constants group the keys into one condense place where they can be referred to easily when reading code.


RubyKoans: Greed dice scoring The Greed game, from Ruby Koans, where you roll up to 5 dice:

A set of three ones is 1000 points. A set of three numbers (other than ones) is worth 100 times the number.
A one (that is not part of a set of three) is worth 100 points. A five (that is not part of a set of three) is worth 50 points. Everything else is worth 0 points.

I broke my solution into four methods.
def score(dice)
  (1..6).reduce(0) { |sum, n| sum + points(dice, n) }
end

def points(dice, num)
  ((dice.count(num) / 3) * triple(num)) + ((dice.count(num) % 3) * bonus(num))
end

def triple(num)
  num == 1 ? 1000 : (num * 100)
end

def bonus(num)
  [1, 5].include?(num) ? 50 + (50 * (num % 5)) : 0
end

I suspect there's a way to avoid having to pass the dice array into points(), but I can't figure it out. Maybe using a yield somewhere?

I think your code is very nice, succinct, and ruby-like.

Two minor issues -

Your bonus code is a bit over-sophisticated, which makes it not very readable, and quite brittle. Although it won't fit in one line - a case solution will be more suitable here:

def bonus(num)
  case num
  when 1 then 100;
  when 5 then 50;
  else 0;
  end
end

Also, in score I personally think that separating the score calculation from its aggregation is more easy on the eyes:

(1..6).map { |n| points(dice, n) }.inject(:+)

although, I admit, it is more a matter of taste than anything else...

As for your suspicion about not having to pass dice to points, I don't see a straight-forward way of doing that, or a proper motivation for doing it.

Perhaps you want to be able to simplify your block into a straight forward method call (using a syntax like .map(&some_method)) - for that you can use proc's curry method, but it actually results in a very awkward code, which does not seem to be worth it:

def score(dice)
  (1..6).map(&lambda(&method(:points)).curry[dice]).inject(:+)
end

Perhaps you want to decouple the point calculation methods from the dice array altogether. This can be done easily enough by passing dice.count(n) instead of dice, as that is all the points actually cares about, although this solution will break as soon as point calculation will need more information...

def score(dice)
  (1..6).reduce(0) { |sum, n| sum + points(n, dice.count(n)) }
end

def points(num, count)
  ((count / 3) * triple(num)) + ((count % 3) * bonus(num))
end


Add two numbers represented by linked lists I've just encountered this question and am trying to solve it using Java.

Here is my solution to it, which may not be optimized or might not be right way to do it. Someone please review whether it is correct or there is some good way to do it. For focusing on logic, I have hard coded List creation and code repetition is present.

package linkedlist.singly;

//Add two numbers represented by linked lists
// 245   :  5 -> 4 -> 2
// 99789 :  9 -> 8 -> 7 -> 9 -> 9
// Ans   :  99341
public class Add2NumbersInLinkListType2 {

    static int carry=0;

    public static void main(String[] args) {

        Node templ11 = new Node(5);
        Node templ12 = new Node(4);
        Node templ13 = new Node(2);

        Node templ21 = new Node(9);
        Node templ22 = new Node(8);
        Node templ23 = new Node(7);
        Node templ24 = new Node(9);
        Node templ25 = new Node(9);

        templ11.setNext(templ12);
        templ12.setNext(templ13);

        templ21.setNext(templ22);
        templ22.setNext(templ23);
        templ23.setNext(templ24);
        templ24.setNext(templ25);

        Node res = findSum(templ11, templ21, 0);
        if(carry==1){
            Node tempNode = new Node(carry);
            tempNode.setNext(res);
            res = tempNode;
        }
        while(res!=null){
            System.out.print(res.getData());
            res=res.getNext();
        }
    }

    private static Node findSum(Node l1, Node l2, int diff){

        int length1 = findLength(l1);
        int length2 = findLength(l2);

        if(length1>length2){
            //l1 having more nodes
            Node res = findSum(l1.getNext(), l2, diff--);
            int data = l1.getData() + carry;
            if(data>9){
                carry=1;
                Node tempNode = new Node(data%10);
                tempNode.setNext(res);
                res = tempNode;
            }else{
                carry=0;
                Node tempNode = new Node(data);
                tempNode.setNext(res);
                res = tempNode;
            }
            return res;

        }else if(length2>length1){
            //l2 having more nodes
            Node res = findSum(l1, l2.getNext(), diff++);
            int data = l2.getData() + carry;
            if(data>9){
                carry=1;
                Node tempNode = new Node(data%10);
                tempNode.setNext(res);
                res = tempNode;
            }else{
                carry=0;
                Node tempNode = new Node(data);
                tempNode.setNext(res);
                res = tempNode;
            }
            return res;

        }else{
            //both have same length
            Node res = findSumForListOfSameSize(l1, l2);
            return res;
        }
    }

    private static Node findSumForListOfSameSize(Node l1, Node l2){
        if(l1==null && l2==null)
            return null;

        Node head = findSumForListOfSameSize(l1.getNext(), l2.getNext());

        int temp = l1.getData() + l2.getData() + carry;
        if(temp>9){
            carry=1;
        }else{
            carry=0;
        }

        if(head==null){
            head = new Node(temp % 10);
        }else{
            Node tempNode = new Node(temp % 10);
            tempNode.setNext(head);
            head = tempNode;
        }
        return head;
    }

    private static int findLength(Node node){
        int count=0;
        while(node!=null){
            count++;
            node = node.getNext();
        }
        return count;
    }
}

few notes on your implementation:

if(data>9){
            carry=1;
            Node tempNode = new Node(data%10);
            tempNode.setNext(res);
            res = tempNode;
        }else{
            carry=0;
            Node tempNode = new Node(data);
            tempNode.setNext(res);
            res = tempNode;
        }

can be rewritten as

 carry=data/10;
 Node tempNode = new Node(data%10);
 tempNode.setNext(res);
 res = tempNode;

findSum(Node l1, Node l2, int diff)

you call int length1 = findLength(l1);int length2 = findLength(l2); in every recursive call. It's enough to call it once, before you enter the recursion. The result won't change in the middle. it'll reduce the complexity of your solution from O(N^2) to O(n)
All you need to do afterwards is decrement the value of diff until it becomes 0

I would rewrite the whole method as follows:

private static Node findSum(Node longer, Node shorter, int diff){
    if(diff==0){
         Node res = findSumForListOfSameSize(longer, shorter);
         return res;
    }
        Node res = findSum(longer.getNext(), shorter, diff--);
        int data = longer.getData() + carry;

        carry=data/10;
        Node tempNode = new Node(data%10);
        tempNode.setNext(res);
        res = tempNode;

        return res;
}

and the main would become

...
int length1 = findLength(l1);
int length2 = findLength(l2);
int diff = length1-length2;
if(diff<0){
    diff = diff * -1;
    Node res = findSum(templ21, templ11, diff);
}else{
    Node res = findSum(templ11, templ21, diff);
}
...

you could remove the need for static variable carry by doing this:

Node res = findSum(longer.getNext(), shorter, diff--);
int prevSum = res.getData()
int carry = prevSum / 1;
res.setData(prevSum % 10)

int sum = longer.getData() + carry;
Node tempNode = new Node(sum);

you're 'delegating' the calculation of the carry (and update of the vaue) to the next digit calculation). Also note, I renamed the data variable to be prevSum and sum


Understanding my permalink service tests I started writing unit tests cases recently. For now, I created only "perfect case" where there is no error. However, the test seems to me difficult to maintain and difficult to understand.

How can I make it better for other people to understand?

All permalink attributes are set within the service itself I'm testing (PermalinkService.java).

My test

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(classes = Configuration.class)
@FixMethodOrder(MethodSorters.NAME_ASCENDING)

public class PermalinkServiceTest {

    @Autowired
    PermalinkService permalinkService;

    private static Permalink PERMALINK;
    private static Validacao VALIDATION;
    private static DBObject PECA;
    private static BasicDBObject COMMENTS;

    @BeforeClass
    public static void init(){
        PERMALINK = new Permalink();
        VALIDATION = new Validacao().setId("55534e63ccf2f879efcbd2a3");
    }

    @Test
    public void testPermalinkServiceA_createPermalinkWithoutValidation_permalinkWithAllFilesAndWithoutValidation() throws Exception {
        PERMALINK = permalinkService.createPermalink(VALIDATION.getId(), "Permalink sem validação");
        Assert.notNull(PERMALINK);
    }

    @Test
    public void testPermalinkServiceB_createPermalinkWithValidation_permalinkWithAllFilesAndValidation() throws Exception {
        PERMALINK = permalinkService.createPermalink(VALIDATION.getId(), "Permalink com validação");
        PECA = (DBObject) JSON.parse(PERMALINK.getPecas().get(0).toString());
        Assert.notNull(PERMALINK);
    }

    @Test
    public void testPermalinkServiceB_getAllPermalinks_getAllPermalinks() throws Exception {
        Assert.notNull(permalinkService.getPermalinks(VALIDATION.getId()));
    }

    @Test
    public void testPermalinkServiceC_getPermalink_getPermalink() throws Exception {
        Assert.notNull(permalinkService.getPermalinks(VALIDATION.getId(), PERMALINK.getId()));
    }

    @Test
    public void testPermalinkServiceD_setStatus_setNokStatus() throws Exception {
        String status = "nok";
        String email = "daniela.morais@sofist.com.br";
        permalinkService.setStatus(VALIDATION.getId(), PERMALINK.getId(),
                String.valueOf(PECA.get("id")), status, email);
    }

    @Test
    public void testPermalinkServiceE_addComment_addComment() throws Exception {
        String comment = "comentario teste";
        String email = "daniela.morais@sofist.com.br";
        PERMALINK = permalinkService.addComment(VALIDATION.getId(), PERMALINK.getId(), String.valueOf(PECA.get("id")),
                comment, email);
        DBObject pecas = (DBObject) JSON.parse(PERMALINK.getPecas().toString());
        DBObject peca = (DBObject) JSON.parse(pecas.get("0").toString());
        DBObject comentarios = (DBObject) JSON.parse(peca.get("comentarios").toString());
        COMMENTS = (BasicDBObject) JSON.parse(comentarios.get("0").toString());
        Assert.notNull(PERMALINK);
    }

    @Test
    public void testPermalinkServiceF_removeComment_removeComment() throws Exception {
        permalinkService.removeComment(VALIDATION.getId(), String.valueOf(COMMENTS.get("id")), PERMALINK.getId(), "");
    }

    @AfterClass
    public static void testPermalinkService_removeFromMongo_removeFromMongo() throws Exception {
        MongoCollection mongoCollection = DBSingleton.getJongo().getCollection("validacao_permalinks");
        mongoCollection.remove("{ _id : #}", PERMALINK.getId());
    }
}

Naming

Good names are very important for readability. For example, this:

testPermalinkServiceA_createPermalinkWithoutValidation_permalinkWithAllFilesAndWithoutValidation

is just way too long a name.

You can reduce it's size by removing all references to permalinks (it's in the PermalinkServiceTest class, so it's assumed that the tests test permalinks). It's also not very clear what the A stands for, but it seems to be only necessary because the tests need to be executed in a specific order, which doesn't seem like a good idea

The name also contains more duplication, as it contains WithoutValidation twice.

A shorter name might be testCreateLinkAllFilesNoValidation, or even just allFilesNoValidation (if that accurately described what is tested, which I'm not sure it does; It's hard to say without the actual code, but I'm eg unsure what allFiles are, or how they are used in the test).

Tests building on each other

I don't really like how the tests seem to build on each other; I would prefer if each test is self-contained.

Self-contained tests seem a lot more stable and maintainable, and would also result in easier to understand code. If you are worried about duplication, just extract that code to a method.

This would also get rid of many of the fields, which I think also results in nicer code.

Misc

Many of your tests don't actually contain any tests, why is that?


Stack implementation using Swift I would like to request some code review and feedback on a stack implementation using Swift.

Disclaimer: I might have not used protocols, optional unwrapping and error and nil check correctly (still trying to figure out the essence of protocols - but that's the drill I'm going through now).

import Foundation

/* Node conformance */
protocol NodeProtocol {

    typealias IDType;
    typealias IDNode;
    func insertNode(element : IDType, nextNode : IDNode?);
}

/* Stack's conformance prototol */
protocol StackProtocol {

    typealias IDType;
    /* Function to push element on stack */
    func push(element : IDType);
}

class IDNode<IDType> : NodeProtocol
{
    var element: IDType?;
    var nextNode: IDNode?;

    /* Sets the element and updates next pointer */
    func insertNode(element: IDType, nextNode: IDNode?) {

        self.element = element;
        self.nextNode = nextNode;
    }
}

class IDStack<IDType> : StackProtocol
{
    private var stackHead: IDNode<IDType>?;

    init() {

        /* Constructor to initialize the stack */
        stackHead = IDNode<IDType>();
    }

    func push(element: IDType) {

        /* If stackHead is empty - just create a new stack
        * Ideally the constructor should have taken care of this.
        * but in our case - because of optional. stackHead can be NULL.
        * So we don't want to take any chance while pushing element to stack.
        */
        if(stackHead == nil) {
            stackHead = IDNode<IDType>();
        }

        /* if stack's first element is empty - Insert as the first element
        * At this point stack is guaranteed not NULL. Right?
        * What if memory allocation fails from above?
        */
        if(stackHead!.element == nil) {
            stackHead!.insertNode(element, nextNode: nil);
        }else {
            /* create a new node and insert the new element at top */
            var nodeToPushInStack: IDNode<IDType>! = IDNode<IDType>();
            /* I'm assuming memory allocation always passes from above.
            * Is it correct?
            */
            nodeToPushInStack.insertNode(element, nextNode: stackHead);
            /* Update stack's head to the new Node */
            stackHead = nodeToPushInStack;
        }

    }

    func popElement() -> IDType? {

        /* Remove from top and return the element. */
        var itemPoppedFromStack : IDType?;

        if(self.stackHead == nil) {
            /* Stack is empty / not initialized */
            return nil;
        }

        itemPoppedFromStack = self.stackHead!.element!;

        if(itemPoppedFromStack == nil) {
            return nil;
        } else {
           /* restore pointer order */
            stackHead = stackHead!.nextNode;
        }
        return itemPoppedFromStack;
    }

    func getLength() -> Double {

        var length: Double = 0;
        var headNode: IDNode<IDType>! = self.stackHead;

        if(headNode == nil) {
            /* stack is probably empty, just return 0 */
            return 0;
        }

        while(self.stackHead != nil) {

            length++;
            self.stackHead = self.stackHead!.nextNode;
        }
        /* restore back stackHead to original head */
        self.stackHead = headNode;

        return length;
    }
}

Let's start with some general remarks:

Swift does not require semicolons after statements (but they are allowed). From what I have seen since Swift was introduced last year,
most people do not write semicolons in Swift.
Classes are reference types, and the properties of an instance of a class
can be modified even if the instance is declares as a constant with let.
Therefore you can replace var by let at many places in your code,
e.g.

let nodeToPushInStack: IDNode<IDType>! = IDNode<IDType>()

Often an explicit type annotation is not necessary because the compiler
can infer the type of an expression automatically. E.g. the above statement can be shortened to

let nodeToPushInStack = IDNode<IDType>()

Now the "major" points. To simplify things, I'll consider the
implementation without protocols first, so in the first part
of this answer I'll ignore your protocol definitions and assume that
the classes are defined as

class IDNode<IDType> { ... }
class IDStack<IDType> { ... }

First the IDNode class: You have defined element as a variable
and optional, but actually node elements are cannot be nil and
are never mutated once a node has been been pushed on the stack.

You probably did that because you create "empty" nodes first and then
call the insertNode() method to set the element and next pointer.
Another reason is that you use nodes "without an element", but I'll come
back to that later.

I would suggest to make element a constant and non-optional
and replace the insertNode() method by an init method:

class IDNode<IDType>
{
    let element: IDType
    let nextNode: IDNode?

    init(element : IDType, before: IDNode?) {
        self.element = element
        self.nextNode = before
    }
}

Which leads us to the IDStack class: The push and pop methods are
a bit too complicated. The reason is that you use a single node
with element == nil for an empty stack, a single node with
element != nil for a stack with a single element, and then
a linked list of nodes for two or more elements.

It becomes much simpler if you use a linked list with one node for each element in each case, and stackHead points to the front node,
or is nil for an empty stack. Then you don't need an init() method
because the optional property

private var stackHead: IDNode<IDType>?

is automatically initialized to nil, and pushing an element
simplifies to

func push(element: IDType) {
    let newNode = IDNode(element: element, before: stackHead)
    stackHead = newNode
}

Popping an element becomes simpler as well. Note that the preferred
way to check an optional (here: stackHead) for nil is optional
binding:

func popElement() -> IDType? {
    if let firstNode = stackHead {
        let element = firstNode.element
        stackHead = firstNode.nextNode
        return element
    } else {
        return nil
    }
}

Remark: Your comment

        /* Stack is empty / not initialized */

is misleading: Variables cannot be uninitialized in Swift. This is
ensured by the compiler and one of the major design goals in Swift.

Finally your getLength() method: Using Double as return type seems
quite strange to me, Int would be appropriate. And I would use
a computed property count instead, similar to the count property
of Swift's sequence types.

Modifying self.stackHead inside the method temporarily is a bad idea
and I can see no reason to do so. Use a local variable instead which
traverses through the linked list, and again use optional binding
to check if you have hit the end of the list:

var count : Int {
    var numItems = 0
    var currentNode = self.stackHead
    while let node = currentNode {
        numItems++
        currentNode = node.nextNode
    }
    return numItems;
}

Back to the protocols: Defining a protocol for "Stack" makes only sense
if it contains all required methods:

protocol StackProtocol {

    typealias IDType
    func push(element : IDType)
    func popElement() -> IDType?
    var count : Int { get }
}

Then you can create an instance of IDStack and pass it to a function
that expects a StackProtocol. Simple example:

func test<S : StackProtocol where S.IDType == String>(stack : S) {
    stack.push("foo")
    stack.push("bar")
    stack.push("baz")
    print(stack.count)

    while let element = stack.popElement() {
        print(element)
    }
}

test(IDStack<String>())

Of course you can also define protocols for both the node and the
stack type. Now the node protocol should contain all required
properties and methods:

protocol NodeProtocol {
    typealias IDType
    var element : IDType { get }
    var nextNode : Self? { get }
    init(element : IDType, before: Self?)
}

and a concrete implementation would be

final class IDNode<IDType> : NodeProtocol
{
    let element: IDType
    let nextNode: IDNode?

    init(element : IDType, before: IDNode?) {
        self.element = element
        self.nextNode = before
    }
}

(Please don't ask me why the final is necessary here :)

To make the stack independent of the used node class, the stack protocol
uses  node protocol as generic placeholder (and  not IDType  as you did):

protocol StackProtocol {

    typealias NodeType : NodeProtocol
    func push(element : NodeType.IDType)
    func popElement() -> NodeType.IDType?
    var count : Int { get }
}

and a concrete implementation (in terms of a generic node type)
is

class IDStack<Node : NodeProtocol> : StackProtocol
{
    typealias NodeType = Node
    private var stackHead: Node?

    func push(element: Node.IDType) {
        let newNode = Node(element: element, before: stackHead)
        stackHead = newNode
    }

    func popElement() -> Node.IDType? {
        if let firstNode = stackHead {
            let element = firstNode.element
            stackHead = firstNode.nextNode
            return element
        } else {
            return nil
        }
    }

    var count : Int {
        var numItems = 0
        var currentNode = self.stackHead
        while let node = currentNode {
            numItems++
            currentNode = node.nextNode
        }
        return numItems;
    }
}

As an example, a stack of strings could now be created with

let stack = IDStack<IDNode<String>>()


Checking if a line in a file contains user.Handle and '-r'+any single digit number The title pretty much explains everything. I'm try to check if a line in a file contains user.Handle and '-r'+any single digit number, AND that the aforementioned user.Handle does not contain '-r'. I'm sure there is an easier way to do this. And I'm sorry if this made your eyes bleed.

if (line.Contains(user.Handle) && !user.Handle.Contains("-r") &&
line.Contains("-r1") || line.Contains(user.Handle) &&
!user.Handle.Contains("-r") && line.Contains("-r2") ||
line.Contains(user.Handle) && !user.Handle.Contains("-r") &&
line.Contains("-r3") || line.Contains(user.Handle) &&
!user.Handle.Contains("-r") && line.Contains("-r4") ||
line.Contains(user.Handle) && !user.Handle.Contains("-r") &&
line.Contains("-r5") || line.Contains(user.Handle) &&
!user.Handle.Contains("-r") && line.Contains("-r6") ||
line.Contains(user.Handle) && !user.Handle.Contains("-r") &&
line.Contains("-r7") || line.Contains(user.Handle) &&
!user.Handle.Contains("-r") && line.Contains("-r8") ||
line.Contains(user.Handle) && !user.Handle.Contains("-r") &&
line.Contains("-r9"))

If I see it correctly, you could already simplify your code by saying

if (line.Contains(user.Handle) && !user.Handle.Contains("-r") &&
    (line.Contains("-r1") || line.Contains("-r2") || ... line.Contains("-r9"))

That should be basically the same thing as the above. Now when you look at the last part, you could simplify it to a regular expression -r\d (not sure if - needs to be escaped here) So the final version could look like this:

if (line.Contains(user.Handle) && !user.Handle.Contains("-r") &&
    Regex.IsMatch(line, "-r\\d"))

did I get your question? Did I miss anything?


Implementing `randoms` Learn You a Haskell presents the randoms function.

I implemented it as follows:

-- randoms that takes a generator and returns an infinite sequence
-- of values based on that generator
import System.Random

randoms' :: StdGen -> [Int]
randoms' g = num : randoms' gen
   where (num, gen) = random g

Please critique it.

Straight forward. Easy to follow. But this is clearly some kind of fold.

randoms' = unfoldr (Just . random)

works nicely.


How to verify if HTML table sort is stable or not? I have implemented a HTML table sorting. I actually implemented a sort first using jQuery and then modified it based on the answer to this question for performance as I had done the same mistake. My question is how to make sure if the code I have written is stable or not?

I know that the stability of sorting depends on the sort method's algorithm and that changes with the browser, and according to this page, mozilla's implementation of sorting is stable, but when I checked with my code, it doesn't seem to be stable. Could someone have a look at my code and let me know how to determine whether the sort performed is stable or not?

                    Month
                    Savings

                    March
                    180

                    January
                    100

                    February
                    80

                    Januray
                    40

                    March
                    200

                    February
                    90

And if there can be any improvements for efficiency, can anyone point that out as well?

From a once over;

The easiest way to be sure that you sort algorithm is stable, is to write you own sort algorithm.
The second easiest way to be sure is to pass in each array entry object it's position in the array ( you could set this after your arr[i][j] assignment ). If (a[c]==b[c]) then you would not return 0, but you would return -1 or +1 based on their original position in the array
You did not declare c and asc with var
var arr = []; is preferred to var arr = new Array();
One single comma separated var statement is preferred over many var statements
You declare var i twice, since var i declares i for the scope of the function, you do not need to declare it again.
Your indenting is off, you should look into that
Assigning a function to document.ready is old skool, you should look into using addEventListener or simple using $( handler );.
You should cache the value of a<$("tbody tr:eq(0) td").length

 for (var a = 0, count = $("tbody tr:eq(0) td").length; a < count ; a++)

On my second point, imagine you are sorting people by their age, in a stable manner:

Bob, 23 years
John, 12 years
Tricia, 23 years

becomes

Bob, 23 years
Tricia, 23 years
John, 12 years

it would be unstable if the order was

Tricia, 23 years
Bob, 23 years
John, 12 years

because Tricia was initially below Bob, she should stay below Bob.
You can accomplish this by comparing the original position of the 2 records and making sure that if the age is the same, you make sure the original order is respected.

I hope this example made it clearer.


Portably get one character from standard input using standard library only While translating an old Basic game to C, I found myself needing a function to get one character from the keyboard.  You can't do this with common standard C library functions like getchar() because the standard input stream is line-buffered (i.e. it will store whole line of input, including the terminating \n, in its internal buffer although getchar() only uses its first character. Subsequent calls to getchar() will consume buffer's remaining characters until exhausted and only then will it resume accepting user's new inputs).  This causes problems because if you need inputting twice, the second time you will get some unexpected value instead of letting user entering second character.

Now, I know the way around this is to use operating system calls such as ioctl and read to set input to an unbuffered state and read a character (or better yet use a library like Curses which abstracts away all this in a cross-platform way.) but I started wondering if it was possible to do this entirely via the standard C library.

My first attempt was to empty out the remaining characters in stdin's buffer by adding a call to:

fflush(stdin);

...after calling getchar() but this didn't do anything.

My next attempt was to try and take stdin out of line buffer mode like this:

setvbuf(stdin, NULL, _IONBF, 0);

I had high hopes this would work but actually it seems calling setvbuf on stdin is undefined behavior.  It certainly does not work on Linux.

So finally I came up with this function.  It works but I have a nagging feeling it could be improved.  What do you think?

int getkey(const char* prompt = "") {
    /* Print the prompt message if there is one */
    if (strcmp(prompt, "") != 0) {
        puts(prompt);
    }

    /* Get a character and examine it.  If it is a newline from a previous
       call to this function eat it otherwise put it back in the buffer. */
    int c = getchar();
    if (c != '\n') {
        ungetc(c, stdin);
    }

    /* This is the character we really want. */
    c = getchar();

    /* Drain the input buffer so any extra characters which were pressed are
       discarded except for newline which is needed to actually send the input
       to stdin. */
    int next;
    while(!feof(stdin) && next != '\n') {
        next = getchar();
    }

    return c;
}

Your strcmp can be replaced with:

if (*prompt) {

Your last loop has issues. You effectively have both a precondition (feof) and a postcondition (next). You can replace the lot with:

while (!feof(stdin))
    if (getchar() == '\n')
        break;


Fetch and display products from a SOAP call I'm relatively new to php and any advice is appreciated.

I retrieve some data from an external source and process them in a foreach loop to display the items. Now I'm not sure if the way I choose is the right way or if there is a better way for a situation like this.

And here the code I use:

define ("WSDL_SERVER",
"XXX");
$oSmartFeed= new SoapClient(WSDL_SERVER);
$oSessionHash= $oSmartFeed->login('XXX', 'XXX');
if(!$oSessionHash->HasError){
$sSessionHash= $oSessionHash->Records['sessionHash'];
$aConfig= array('limit'=>5, 'offset'=>2, 'feeds'=>[XXX]);
$aResult= $oSmartFeed-> getProductData ($sSessionHash, $aConfig);

echo '
';

foreach ($aResult->Records AS $aRecord) {

    $produktName = $aRecord['productname'];
    $imageUrl = $aRecord['imagebigurl'];
    $price = $aRecord['currentprice'] . $aRecord['currency'];
    $affLink = $aRecord['deeplinkurl'];
    $productDescription = $aRecord['productdescriptionlong'];
    $versandKosten = $aRecord['shipping'] . ' €';

    ?>

           <a href="<?php echo $affLink;?>">
               <img src="<?php echo $imageUrl; ?>" alt="<?php echo $produktName; ?>">

                   <?php echo $produktName; ?>

                   <?php echo $productDescription;?>

                       Price: <?php echo $price; ?>

                       Shipping: <?php echo $versandKosten; ?>

    <?php
}
echo '
';
$oSmartFeed->logout($sSessionHash);

Have you considered using a template engine vs. writing old-style PHP spaghetti code?

if(!$oSessionHash->HasError){

Reverse this conditional to get rid of the huge section of code within this conditional (whihc is actually not even closed with the code shown.  So something like:

if($oSessionHash->HasError) {
    // die, exit, redirect or similar
}
// rest of code, now no longer in a conditional.

This strategy of existing early from scripts/function/methods will help you write much less fragile code.

A number of stylistic concerns:

Drop the whole $o*, $a*, $s* variable naming (which looks like you are trying to infer variable type in name).  Your variables should have meaningful and specific names which convey their meaning. Also, if you use a good IDE, you won't need this.
Your spacing around assgniment operators, flow control structures, etc. is wildly inconsisten, making your code hard to read.  If you are strating in PHP, I would recommend getting to know the PSR-1 and PSR-2 coding standards as well as other PSR recommendations which are most widely accetped ways of writing code in the profession PHP development community. A good IDE will be able to help you enforce these styles.
Don't split your define across two lines unnecessarily like you are.

Why not perform logout immediately after your API call response is returned, rather than wait until after displaying results?

This code is very happy path oriented.  What happens if API doesn't respond? What if it responds with an error code? What if it returns empty result set?


SPOJ solution of GRAVITY I am solving problem GRAVITY on SPOJ. Each test case is a rectangular ASCII grid of dimensions m × n, and you have to determine whether it is possible to find a path from the 'S' square to the 'T' square, moving using the space squares and avoiding '#' obstacles, in at most O steps (diagonal steps allowed).

I am using normal BFS but I am getting TLE. The time limit is 2 sec and my code should easily run within this limit.

#include <bits/stdc++.h>
using namespace std;
#define ll long long
typedef pair <int , int > pii;

#define INF (1000000000)
#define ull unsigned long long
#define s(n) scanf("%d",&n)
#define s2(a,b) scanf("%d %d",&a,&b)
#define s3(a,b,c) scanf("%d %d %d",&a,&b,&c)

int O,m,n;
int dr[]={0,0,-1,1,1,1,-1,-1}; //right,left,up,down,down-right,down-left,up-right,up-left
int dc[]={1,-1,0,0,1,-1,1,-1};
int inRange(int a,int b){ if(a>=0 && a<m && b>=0 && b<n) return 1;return 0;}

char arr[101][101];map <pii,int > dist;

int bfs(int a,int b)
{
    queue <pii> q;
    dist[pii(a,b)]=0;
    q.push(pii(a,b));
    while(!q.empty())
    {
        pii front=q.front();q.pop();
        a=front.first;b=front.second;
        for(int i=0;i<8;i++)
        {
            int x=a+dr[i],y=b+dc[i];

            if(inRange(x,y) && arr[x][y]!='#' && dist[pii(x,y)]==INF)
            {
                dist[pii(x,y)]=dist[pii(a,b)]+1;
                q.push(pii(x,y));
            }
            if(inRange(x,y) && arr[x][y]=='T')
            {
                dist[pii(x,y)]=dist[pii(a,b)]+1;
                return dist[pii(x,y)];
            }
        }
    }
    return INF;
}

int main()
{
    int t;
    s(t);
    while(t--)
    {

        int Sx=-1,Sy=-1,Tx=-1,Ty=-1;
        s3(O,m,n);
        char ch;
        scanf("%c",&ch);    //for end of line character

        dist.clear();
        for(int i=0;i<m;i++)
        {
            for(int j=0;j<n;j++)
            {
                scanf("%c",&ch);
                arr[i][j]=ch;
                dist[pii(i,j)]=INF;
                if(arr[i][j]=='S')
                    Sx=i,Sy=j;
                if(arr[i][j]=='T')
                    Tx=i,Ty=j;
            }
            scanf("%c",&ch);   //for end of line character
        }

        if(Sx==-1 || Sy==-1 || Tx==-1 || Ty==-1)
        {
            printf("Impossible\n");
            continue;
        }
        int val=bfs(Sx,Sy);
        if(val <= O)
            printf("Possible\n");
        else
             printf("Impossible\n");
    }
    return 0;
}

You should pass O to your bsf and terminate it, once you reach zero oxigen. Big map with low O can cause TLE.
You don't need the map, you can reuse the array and just mark visited spots (e.g. with '*'). This would be wave algorithm (handle all nodes in the queue at the start of the step, reduce counter O and proceed to next step, unless T or zero O reached). This is simplification of Dijkstra algorithm for graph with all weights=1.
Your code is hard to read. You are on Code Review! You are using C++ but program like in old C. Use typedef instead of #define when possible, avoid super-shortcuts like s and s2, please.


Filtering and processing computers listed in a CSV file I am trying to make my script run a lot faster. I have a csv that has about 330k of lines on it that I plan on running my script with and it has been taking an extremely long time. I just did ran the script and it 50 mins to generate the new csv. What the code does is it adds 2 more fields and filters only the administrators and The windows operating systems. Below are before and after pictures of the output.

$csv = Import-Csv 'U:\Local Group Members.csv' |
Where-Object {($_.Name0 -eq "administrators") -and ($_.caption0 -match "Microsoft Windows 10 Enterprise|Microsoft Windows 7 Enterprise|Microsoft Windows 7 Professional|Microsoft Windows 8 Enterprise|Microsoft Windows 8 Pro|Microsoft Windows 8.1 Enterprise|Microsoft Windows 8.1 Pro")} |
Select-Object "Netbios_name0", "Name0", "Account0","category0","Domain0","Unique Account Name","Type0","caption0", "Excluded"
#Modify each line
Foreach ($row in $csv) {
If ($row.Type0 -eq 'Domain') {
    $row."Unique Account Name" = "$($row.Domain0) - $($row.Account0)"
    If ($row."Unique Account Name" -in @('ACCOUNTS - DODSCAN'.'ACCOUNTS - Domain Admins','ACCOUNTS - LADM_WS_Admins','ACCOUNTS - Tech Enterprise'))
         {$row."Excluded" = "True"}
    Else {$row."Excluded" = "False"}
}
Else {
    $row."Unique Account Name" = "$($row.Netbios_name0) - $($row.Account0)"
    If ($row."Account0" -in @('esrxadm1n_esi','#Update','medco_tech','medco_admin'))
         {$row."Excluded" = "True"}
    Else {$row."Excluded" = "False"}
}
    Write-Host $row."Unique Account Name"
    Write-Host $row."Excluded"
}

#Export CSV
$csv | Export-Csv U:\$(get-date -f yyyy-MM-dd-hh-mm)-TestOutput1.csv -NoTypeInformation

Can someone help me make my script faster?

As far as performance goes there are a few places to make changes.

Make better use of the pipeline

You read in the csv to a variable then process. Convert the script to process these records directly. There would be performance increases this way. Insanely brief example...

Import-Csv 'U:\Local Group Members.csv' |
   Where-Object {($_.Name0 -eq "administrators")}
   Select-Object "Netbios_name0", "Name0" |
   ForEach-Object {
       # Something happens.
} | Export-Csv ....

Wasted console output

I cannot imagine you are checking ~1000's of lines of console output. You are wasting processing time but putting that data to console. Remove the write-host lines. Even if you have this running as a task.

Where-Object is slow

This is really easy to use but is not very performance friendly. I can tell you have at least PowerShell 3.0 but if you have at least v4 then you have access to the where and foreach methods of collections. The syntax as you will see is very similar but there should be performance boons here as well. Example:

(Import-Csv -Path C:\temp\devices.csv).where({$_.DeviceType -ne "BlackBerry"})

I think you are already doing this but order your conditions in order of least likely to most likely. PowerShell will not evaluate the RHS of -and if the LHS is already false.

I tried to improve the logic in your regex but it was at a cost of performance everytime. As redundant as that string is it seems to be a faster choice given other options.


Ping Pong game in SFML I am learning C++ and SFML so I want to know if I am laying my code out neatly and if my code is efficient. I know this is not a full game yet but I want to know if I am on the right tracks.

#include <SFML/Graphics.hpp>
#include "ResourcePath.hpp"

#include <iostream>

class Paddel {
    int speed = 3;
    sf::Vector2f border;
    sf::Vector2f size;
    sf::Vector2f position;
    sf::Vector2f screenSize;
public:
    sf::RectangleShape shape;

    Paddel(sf::Vector2f screenSize,int player)
    {
        this->screenSize = screenSize;
        this->size = sf::Vector2f(20,100);
        this->border = sf::Vector2f(8,6);

        if(player == 1)
            this->position = border;
        else if(player == 2) {
            position.x = (screenSize.x - size.x) - border.x;
            position.y = border.y;
        }

        this->shape.setSize(this->size);
        this->shape.setPosition(this->position);
    }

    void moveUp()
    {
        this->position.y -= this->speed;
        if(this->position.y < this->border.y)
            this->position.y += this->speed;
        this->shape.setPosition(this->position);
    }

    void moveDown()
    {
        this->position.y += this->speed;
        //100 - (600 + 5)
        if(this->position.y > (this->screenSize.y - this->border.y) - this->size.y)
            this->position.y -= this->speed;
        this->shape.setPosition(this->position);
    }
};

using std::cout;
using std::endl;

int main()
{
    sf::Vector2f screenSize(800,600);
    sf::RenderWindow Window(sf::VideoMode(screenSize.x,screenSize.y),"Ping Pong");
    Window.setFramerateLimit(60);
    Window.setVerticalSyncEnabled(true);

    Paddel playerOne(screenSize,1);
    Paddel playerTwo(screenSize,2);

    const sf::Time TimePerFrame = sf::seconds(1.f/60.f);
    sf::Clock clock;
    sf::Time timeSinceLastUpdate = sf::Time::Zero;
    sf::Time elapsedTime;

    bool play = true;

    while(Window.isOpen())
    {
        Window.clear();
        sf::Event Event;

        while(Window.pollEvent(Event))
        {
            if(Event.type == sf::Event::Closed)
                Window.close();
        }

        elapsedTime = clock.restart();
        timeSinceLastUpdate += elapsedTime;
        if(play){
            if(timeSinceLastUpdate > TimePerFrame){
                if(sf::Keyboard::isKeyPressed(sf::Keyboard::W))
                {
                    playerOne.moveUp();
                }
                if(sf::Keyboard::isKeyPressed(sf::Keyboard::S))
                {
                    playerOne.moveDown();
                }

                if(sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
                {
                    playerTwo.moveUp();
                }
                if(sf::Keyboard::isKeyPressed(sf::Keyboard::Down))
                {
                    playerTwo.moveDown();
                }

                if(sf::Keyboard::isKeyPressed(sf::Keyboard::P))
                    play = false;
            }

            Window.draw(playerOne.shape);
            Window.draw(playerTwo.shape);

            Window.display();
        }
    }
}

your code is fine but I would go with some thing like this, i have commented the changes

#include <SFML/Graphics.hpp>
#include <cstdint>

// for eliminating magic numbers
enum class Players
{
    PlayerOne,
    PlayerTow
};

enum class ScreenSize
{
    Width = 800,
    Height = 600
};

// SFML has tiny class for rendering purpose, it has only a one virtual fucntion
class Paddel : public sf::Drawable
{
    // for member data perfered tobe started with m prefix
    float mSpeed; // here is an optional you may use integer value
    sf::Vector2f mBorder;
    sf::Vector2f mPosition;
    sf::Vector2u mScreenSize;
    sf::RectangleShape mShape; // <-- now it's private
public:

    Paddel(sf::Vector2u screenSize, Players player)
        // member data perfered be initilaized as contructor's initilaized list
        : mScreenSize(screenSize)
        , mBorder(8, 6)
        , mSpeed(3.f)
    {
        sf::Vector2f size = sf::Vector2f(20, 100);

        switch (player)
        {
        case Players::PlayerOne:
            mPosition = mBorder;
            break;
        default:
            mPosition.x = (mScreenSize.x - size.x) - mBorder.x;
            mPosition.y = mBorder.y;
        }

        mShape.setSize(size);
        mShape.setPosition(mPosition);
    }

    void moveUp()
    {
        mPosition.y -= mSpeed;
        if (mPosition.y < mBorder.y)
            mPosition.y += mSpeed;
        mShape.setPosition(mPosition);
    }

    void moveDown()
    {
        mPosition.y += mSpeed;
        //100 - (600 + 5)
        if (mPosition.y >(mScreenSize.y - mBorder.y) - mShape.getSize().y)
            mPosition.y -= mSpeed;
        mShape.setPosition(mPosition);
    }

    // sf::Drawable has singe virtual fucntion for rendering, override it
    void draw(sf::RenderTarget& target, sf::RenderStates states) const override
    {
        // sf::RectangleShape has its own defualt RenderStates
        target.draw(mShape, states);
    }
};

int main()
{
    // unfortunately strong type enumeration doesn't decay to integer value, so that we need to static cast it to integer type
    // here we don't want to have a negative value for size, we should use unsigned int or use std::uint32_t which is declared
    // in <cstdint> header file if you considered C++11 features.
    sf::Vector2u screenSize(static_cast<std::uint32_t>(ScreenSize::Width), static_cast<std::uint32_t>(ScreenSize::Height));
    sf::RenderWindow Window(sf::VideoMode(screenSize.x, screenSize.y), "Ping Pong");

    Window.setFramerateLimit(60);
    Window.setVerticalSyncEnabled(true);

    Paddel playerOne(screenSize, Players::PlayerOne);
    Paddel playerTwo(screenSize, Players::PlayerTow);

    const sf::Time TimePerFrame = sf::seconds(1.f / 60.f);
    sf::Clock clock;
    sf::Time timeSinceLastUpdate = sf::Time::Zero;
    sf::Time elapsedTime;

    bool play = true;

    while (Window.isOpen())
    {
        Window.clear();
        sf::Event Event;

        while (Window.pollEvent(Event))
        {
            if (Event.type == sf::Event::Closed)
                Window.close();
        }

        elapsedTime = clock.restart();
        timeSinceLastUpdate += elapsedTime;

        if (play)
        {
            if (timeSinceLastUpdate > TimePerFrame)
            {
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::W))
                {
                    playerOne.moveUp();
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::S))
                {
                    playerOne.moveDown();
                }

                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Up))
                {
                    playerTwo.moveUp();
                }
                if (sf::Keyboard::isKeyPressed(sf::Keyboard::Down))
                {
                    playerTwo.moveDown();
                }

                if (sf::Keyboard::isKeyPressed(sf::Keyboard::P))
                    play = false;
            }

            Window.draw(playerOne); // <-- we can draw Paddel object directly
            Window.draw(playerTwo);

            Window.display();
        }
    }
}