Beginner PHP calculator I've spent the last few evenings learning PHP, CSS and HTML through Codecademy and now on Team Treehouse. My goal is to progress with PHP over the next year or two and perhaps obtain Zend cert.

Anyway, I've worked on a simple calculator to test what I've learned. I'd like to see, (given the same 'functionality')what more experienced programmers would come up with.  I think this would enhance my learning curve.

I've used a switch statement, because I'm not sure if it's possible to do something like this:

Variable = Math Operator

Var1 = User Input
Var2 = User Input

Variable3 = Var1 Variable Var2

Example, if you entered *, 2, 2, Variable 3 would = 4.

I tried a few ways but could not make it work, so had to settle with a switch which seems cumbersome.

<?php
$input1    = $_GET["num1"];
$input2    = $_GET["num2"];
$input3    = $_GET["symbol"];
$mathinput = "Not Selected";

switch ($input3) {
    case "*":
        $input3    = "*";
        $mathinput = "Multiplication";
        $result    = $input1 * $input2;
        break;
    case "/":
        $input3    = "/";
        $mathinput = "Division";
        $result    = $input1 / $input2;
        break;
    case "+":
        $input3    = "+";
        $mathinput = "Addition";
        $result    = $input1 + $input2;
        break;
    case "-":
        $input3    = "-";
        $mathinput = "Subtraction";
        $result    = $input1 - $input2;
        break;
    default:
        $input3    = "0";
        $mathinput = "Invalid Operator";
}
if ($input3=="0"){
    echo '

You have entered a bad operator

' ;
}
else{
    echo '

Result: ' . $result . '

';
}
?>

No it's not possible writing

Variable3 = Var1 Variable Var2

With the Variable as an operator, i decided to use call_user_func to call the BC Math functions http://php.net/manual/en/ref.bc.php

I removed the $mathinput since you were not using it anyway.

  Result

<?php
$input1    = $_GET["num1"];
$input2    = $_GET["num2"];
$operator  = $_GET["symbol"];

$operators = [
    '+' => 'bcadd',
    '-' => 'bcsub',
    '*' => 'bcmul',
    '/' => 'bcdiv',
    '%' => 'bcmod',
];

$result = array_key_exists($operator, $operators)
        && is_numeric($input1)
        && is_numeric($input2)
        ? call_user_func($operators[$operator], $input1, $input2)
        : null;
?>

<?php if (!$result) : ?>

Error: Invalid input or arithmetic operator.

<?php else: ?>

Result: <?= $result; ?>

<?php endif; ?>

  EDIT

The array_key_exists function checks if a given key exists in the array.
http://php.net/manual/en/function.array-key-exists.php

The following is a shorthand if / ternary logic

$var = array_key_exists($operator, $operators)
&& is_numeric($input1)
&& is_numeric($input2)
? 'all true' : 'conditions not met';

Simply checks if all the conditions are true, if so the above example would set $var to a string with a value of 'all true', if one of the conditions would return false, the $var would be set to 'conditions not met'

You could write the above snippet using regular if / else statements like this:

if(array_key_exists($operator, $operators) && is_numeric($input1) && is_numeric($input2)) {
    $var = 'all true';
}
else {
    $var = 'conditions not met';
}

Using ternary logic could have some of the following advantages when used properly.

Makes coding simple if/else logic quicker
Makes code shorter
Makes maintaining code quicker, easier


line-endings detection I would like to detect which is the line-endings type of a file. I wrote this function:

 open my $fh, '<afile.txt';
 seteol($fh);

Here the eol detection subroutine:

 sub seteol {
    my $fh = shift;
    my $byte;
    my $i = 0;
    while (sysread($fh, $byte, 1)) {
        if($byte eq "\n") {
            $/ = "\n";
            return 0;
        }
        elsif($byte eq "\r") {
            sysread($fh, $byte, 1);
            if($byte eq "\n") {
                $/ = "\r\n";
                return 0;
            } else {
                $/ = "\r";
                return 0;
            }
        }
        last if $i++ > 65535;
    }
    return -1;
    seek($fh,0,0);
}

I am wondering if I can do a better job with only core-modules.

It reads file by 4096 chars in hope to find EOL,

local $/ = seteol($fh) // $/;

sub seteol {
    my $fh = shift;

    local $/ = \4096;
    my $line = "";

    while (my $b = <$fh>) {
      $line .= $b;
      last if $line =~ /(\R)/;
    }
    seek($fh,0,0);
    return $1 if $1;

    warn "Unknown EOL";
    return;
}


IP4 strings validator Trying to write fast non dependency IP4 string validator for embedded system, It should counts leading zeros, values over 255 and other corrupted strings. Not sure if I missed something. Сan you guys take a look:

// Return 0 on error, store result to segs, if segs != 0.
int is_valid_ip4(const char *str, unsigned char* segs)
{
    int dots = 0;   // Dot count.
    int chcnt = 0;  // Character count within segment.
    int accum = 0;  // Accumulator for segment.

    // Catch NULL pointer.
    if (str == 0)
        return 0;

    // Process every character in string.
    for (;;)
    {
        // Check numeric.
        if (*str < '0' || *str > '9')
            return 0;

        // Prevent leading zeros.
        if (chcnt > 0 && accum == 0)
            return 0;

        // Accumulate and check segment.
        accum = accum * 10 + *str - '0';
        if (accum > 255)
            return 0;

        // Store result.
        if (segs)
            segs[dots] = (unsigned char)accum;

        // Advance other segment specific stuff.
        ++chcnt;
        ++str;

        // Finish at the end of the string when 3 points are found.
        if (*str == '\0' && dots == 3)
            return 1;

        // Segment changeover.
        if (*str == '.')
        {
            // Limit number of segments.
            ++dots;
            if (dots == 4)
                return 0;

            // Reset segment values.
            chcnt = 0;
            accum = 0;
            ++str;
        }
    }
}

<stdbool.h>

Unless you need compatibility with C89 for some reason, I would use bool for the return type of the function and true and false as the possible values.

It helps readability.

sscanf()

This one depends on your performance needs.  A solution using sscanf() will be much easier to understand with a simple look and also shorter in code, but may also be much slower (a benchmark would be appropriate).

Update: I would discard sscanf() because it would accept spaces before the numbers; better use inet_pton() as @vnp suggested.

<stdint.h>

Same as with bool: segs seems at first glance to be some string, but after some time I realized it's just an array of 8-bit unsigned integers.  You should use uint8_t for that.

In case of <stdint.h> I would say that if you don't have it, you should do the typedefs yourself (enclosed in some #if) anyway.  Don't do that with bool, however, which is very dangerous.

Unneeded cast

unsigned char *p;
int i;

...
*p = (unsigned char)i;

This cast is not needed.

Usually casts are very dangerous: they can hide bugs that otherwise the compiler would catch easily.  Don't ever cast, unless you know a very good reason to.

BUG! (No, I was wrong)

// Check numeric.
if (*str < '0' || *str > '9')
        return 0;

This line returns 0 at the first dot it finds.  The function doesn't work!

This if that comes later is always false due to that:

// Segment changeover.
if (*str == '.') {
        /// Unreachable code!!!
}

<ctype.h>

There are some functions in <ctype.h> that you should use:

// Check numeric.
if (*str < '0' || *str > '9')
        return 0;

That code above can be simplified to this one (and the comment is now even more obvious and therefore removed ;-):

if (!isdigit((unsigned char)*str))
        return 0;

  NOTES: The standards require that the argument c for these functions is either EOF or a value that is representable in the type unsigned char.  If the argument c is of type char, it must be cast to unsigned char.

  This is necessary because char may be the equivalent of signed char, in which case a byte where the top bit is set would be sign extended when converting to int, yielding a value that is outside the range of unsigned char.

See man isdigit.

If you're going to use these functions, I would create an unsigned char pointer to alias the char * parameter, so that you don't need to cast all the time.


Checking whether an image is part of another I have the following code (slightly modified for simplicity). It checks whether a given image A (the template) is part of another image B. To handle different sizes, image B is scaled as long as a match is found or some minimum/maximum image size is reached.

The naive implementation for this is quite straight forward but I'm not happy with it:

The code within the two while loops is almost identical.
Handling scaling down and scaling up in two different loops is inefficient in a lot of cases (e.g. when the first iteration of the second while loop would find a match but we still need to go through all iterations of the first one).

Can you offer some suggestions on how to improve my code?

double bestMatch = 0;

while (image.width() > MIN_IMAGE_WIDTH) {
   match = match(image, template); // for the sake of simplicity let this return a double
   if (match > bestMatch) {
      bestMatch = match;
   }
   if (match.maxVal >= threshold) {
      foundMatch = true;
      break;
   }
   image = scaleImage(image, 0.9);
}

image = originalImage;
while (image.width() < MAX_IMAGE_WIDTH) {
   match = match(image, template);
   if (match > bestMatch) {
      bestMatch = match;
   }
   if (match.maxVal >= threshold) {
      foundMatch = true;
      break;
   }
   image = scaleImage(image, 1.1);
}

// do something with bestMatch

To answer your second point:

Why not include the foundMatch in the condition of the while?

while (!foundMatch && image.width() < MAX_IMAGE_WIDTH) {

That way you don't run the second loop at all if a match was found in the first.

Having a couple of duplicate lines isn't a bad thing. It's obvious what the lines do and what the difference is between the 2 loops (1 scaling up, the other scaling down). So at least in my opinion there is nothing wrong with having both loops like this.

I'm actually most surprised that you need to handle scaling at all though. One of the most popular image comparison algorithms these days uses SIFT (Scale invariant feature transform) which, as the name implies is scale invariant.


Resize image with good quality and less weight I want to resize an image with good quality, and the most important thing is having less weight for an image so that webpages will load it faster.

I am using the below method for this. Please check it out, if it is proper or let me know if it needs any modifications. If there is a better method than this, then also please tell me.

public static void ResizeImage(int newWidth, int newHeight, Stream sourcePath, string targetPath)
{
    using (var image = System.Drawing.Image.FromStream(sourcePath))
    {
        Image imgPhoto = Image.FromStream(sourcePath);

        int sourceWidth = imgPhoto.Width;
        int sourceHeight = imgPhoto.Height;

        if (sourceWidth > newWidth || sourceHeight > newHeight)
        {
            //Consider vertical pics
            if (sourceWidth < sourceHeight)
            {
                int buff = newWidth;

                newWidth = newHeight;
                newHeight = buff;
            }

            int destX = 0, destY = 0;
            float nPercent = 0, nPercentW = 0, nPercentH = 0;

            nPercentW = ((float)newWidth / (float)sourceWidth);
            nPercentH = ((float)newHeight / (float)sourceHeight);
            if (nPercentH < nPercentW)
            {
                nPercent = nPercentH;
                destX = System.Convert.ToInt16((newWidth -
                      (sourceWidth * nPercent)) / 2);
            }
            else
            {
                nPercent = nPercentW;
                destY = System.Convert.ToInt16((newHeight -
                      (sourceHeight * nPercent)) / 2);
            }
        }

        var thumbnailImg = new Bitmap(newWidth, newHeight);
        var thumbGraph = Graphics.FromImage(thumbnailImg);
        thumbGraph.CompositingQuality = CompositingQuality.HighQuality;
        thumbGraph.SmoothingMode = SmoothingMode.HighQuality;
        thumbGraph.InterpolationMode = InterpolationMode.HighQualityBicubic;
        var imageRectangle = new Rectangle(0, 0, newWidth, newHeight);
        thumbGraph.DrawImage(image, imageRectangle);
        thumbnailImg.Save(targetPath, image.RawFormat);
    }
}

Variable type choice. Why do you use Int16 and float? Most likely you'll be running on a 32-bit or 64-bit machine, and even the 32-bit machines will be optimized for math operations using double. I know you don't need perfect accuracy, but using the smaller types is not likely to save you any space or time. The optimizer will probably convert them to Int32 and double anyway and then just round them off/truncate/extend.
You should declare variables as late as possible. Ideally right before they're initialized. In particular, there's no point in declaring a variable, setting its initial value to 0, and then on the next line setting its value to what you really wanted.
What is the purpose of nPercent, destX, and destY? You calculate them then throw them away.
I don't think you tested this code. You have an if block that swaps newWidth and newHeight, but nothing that ever swaps them back. What's the code intended to do?


TCP connection and reconnection Is there anyway to make this piece of code more elegant? It's not a nice view, but I can't really see a way to improve the looks of it or shorten it.

private void Send(CancellationToken CTSSend)
{
    try
    {
        NativeMethods.Rect rc;
        NativeMethods.GetWindowRect(hwnd, out rc);
        IntPtr dc1;
        IntPtr dc2 = NativeMethods.GetWindowDC(hwnd);
        using (tcp = new TcpClient())
        {
            tcp.NoDelay = true;
            while (!tcp.Connected && !CTSSend.IsCancellationRequested)
            {
                try
                {
                    if (!tcp.Connected)
                        tcp.Connect(adress);
                }
                catch (Exception e)
                {
                    if (e is SocketException) { Console.WriteLine(e.Message); }
                    else
                        MessageBox.Show(e.Message + " Tcp Connect : Send");
                }
            }
            using (EncoderParameters JpegParam = new EncoderParameters())
            using (JpegParam.Param[0] = new EncoderParameter(System.Drawing.Imaging.Encoder.Quality, 50L))
            using (Bitmap bmp = new Bitmap(rc.Width, rc.Height, System.Drawing.Imaging.PixelFormat.Format32bppArgb))
            using (Graphics g = Graphics.FromImage(bmp))
            using (var ms = new MemoryStream())
            using (var tcpStream = tcp.GetStream())
                while (tcp.Connected && !CTSSend.IsCancellationRequested)
                {
                    try
                    {
                        dc1 = g.GetHdc();
                        NativeMethods.BitBlt(dc1, 0, 0, rc.Width, rc.Height, dc2, 0, 0, 13369376);
                        g.ReleaseHdc(dc1);
                        bmp.Save(ms, jpeg, JpegParam);
                        if (bsize != ms.Length)
                        {
                            bsize = ms.Length;
                            tcpStream.Write(BitConverter.GetBytes(bsize), 0, 4);
                            tcpStream.Write(ms.GetBuffer(), 0, (int)bsize);
                        }
                        ms.SetLength(0);
                    }
                    catch (Exception e)
                    {
                        if (e is IOException)
                        {
                            try { connect.Invoke(new Action(() => { connect.Text = "Connect"; })); }
                            catch (InvalidOperationException) { return; }
                            capcon = false;
                        }
                        else
                            MessageBox.Show(e.Message + ": Send Error");
                    }
                }
        }
    }
    catch (Exception e)
    {
        MessageBox.Show(e.Message + "SEND");
    }
}

I am trying to find a nice way to put TCP reconnecting in another class, but I found a limitation where I have to call return in the main class.

internal static void TCPReconnect(CancellationToken CTS, TcpClient tcp, IPEndPoint adress)
{
    tcp.NoDelay = true;
    while (!tcp.Connected && !CTS.IsCancellationRequested)
    {
        try
        {
            if (!tcp.Connected)
                tcp.Connect(adress);
        }
        catch (Exception e)
        {
            if (e is SocketException) { Console.WriteLine(e.Message); }
            else
                MessageBox.Show(e.Message + " Tcp Connect : Send");
        }
    }

}

So, when it fails, it goes out of the loop. I can also make it call Return, but it will do the same thing.

In the class Caller I have to use:

FastMethods.TCPReconnect(CTS, tcp, adress);
if (CTS.IsCancellationRequested)
    return;

As you can see, I have to recheck for CTS. Is it possible to have it inside the other class somehow?

If I use:

if (CTS.IsCancellationRequested)
    return;

It will simply return to the main class and continue.

A couple of comments:

Shortening code is not a goal - readability and maintainability is (which may, but not neccessarily, shorten it).
The code mixes net code, graphics code, encoder code and UI code. Separating these into separate functions/classes will be very good. Just remember that (as a general rule), one function should do one thing.
Always use braces, even when it's for a single statement where it's not required.

if (!tcp.Connected)
{
    tcp.Connect(adress);
}

Similarly, I'd suggest putting brackets around the !xxx in the if statement. Not needed, but can aid understandability.

while ((!tcp.Connected) && (!CTS.IsCancellationRequested))

What does 13369376 represent?  Define the set of constants (SRCCOPY etc) that you're using and then use those.
TcpClient.Connect is a blocking call - depending on your use, you may want to consider rewriting using BeginConnect instead. One problem is that when in the middle of a connect, you can not check CTSSend for a cancellation.
You need to rethink your exception handling. At the moment, your code puts up a message box, requiring a response. You do not want to mix UI and non-UI code like this
The exception-catching occurs in the middle of a loop, so if it happens once, it's likely to happen again and again...each time requiring acknowledgement of the message box.
Catch specific exceptions (SocketException) rather than general ones (Exception). You can have multiple catches if you need them.
If you're using bsize to determine if the screen has changed, it'll be unreliable. You might end up with two images that have the same size even if the images are different.
The code grabs images as quickly as possible, possibly discarding most of them (if my understanding is correct) - this seems uneccessary. Why not grab an image (say) every 1 second? In which case you can set up something like a System.Timers.Timer instance and get the image when that is triggered.
You should probably allocate a new MemoryStream, rather than reusing it. If you're doing that to "save time", it 'probably' doesn't matter compared with the speed of network traffic and image conversion.
What happens when the window size changes/minimized etc, or is it guaranteed to have a fixed size?
In regards to your edit: Rather than a void return, either return a bool or (preferably) define an enum (CancelRequested, Connected) and return that.

Edit in response to (first) comment:

If you have 10 stacked braces at the end of the method, the method is too long. Break it up rather than remove the braces.
re MemoryStream - it'll gain readability. One thing though that you may want to do is to set an initial capacity (say, rc.Width*rc.Height) - MemoryStream itself reallocates memory/copies data on resizing.
Have you tried using a different image format, say png, rather than jpeg. It may be quicker to compress or smaller, depending on the type of image.
You could also try spliting the image up into quandrants/etc, compressing & sending those and reassembling them at the other side. It may be overall quicker.
And of course, if you want 'quicker', always base it on actual timings rather than feelings :)


powershell md5 compare too slow I'm trying to compare several files using md5 hash with powershell 2.0. The code works, the problem is that it's to slow. It takes longer in step 3. It's about 500 files to compare. Can you see a way to get it faster, like, don't do the step 3 every time?

write-host "1"
    $COMP_ORI=$LOCAL_HOME+"\"+$PROG+"\"+$COMPARE
    $file_ori = Get-ChildItem -Path $COMP_ORI -name
write-host "2"
    $COMP_DEST="\\"+$HOSTIP[$i]+"\"+$PROG_PATH
    $file_dest = Get-ChildItem -PATH $COMP_DEST -name
write-host "3"
    for ($i=0; $i -lt $file_ori.Count; $i++) {
write-host "compare md5" $i
    if ( Get-ChildItem -PATH $COMP_DEST -name -Include $file_ori[$i] ) {
        $md5 = New-Object -TypeName system.Security.Cryptography.MD5CryptoServiceProvider
write-host "4"
        $hash_ori = [System.BitConverter]::ToString($md5.ComputeHash([System.IO.File]::ReadAllBytes($COMP_ORI+"\"+$file_ori[$i])))
write-host "5"
        $hash_dest = [System.BitConverter]::ToString($md5.ComputeHash([System.IO.File]::ReadAllBytes($COMP_DEST+"\"+$file_ori[$i])))
write-host "6"
    if ($hash_ori -ne $hash_dest) {
        $out=$file_ori[$i]
        $out=$out+" IS DIFFERENT"
        output message_solo $out
    }
}

}

There are two obvious optimisations.

The first is to remove the line

    $file_dest = Get-ChildItem -PATH $COMP_DEST -name

since $file_dest is unused. I assume that it was previously used instead of

Get-ChildItem -PATH $COMP_DEST -name -Include $file_ori[$i]

The second is to do the remote hashing remotely. Rather than fetch 500 files (of unknown size) across the network in order to calculate their MD5 sums, use remote PowerShell to calculate their MD5 sums and just return the filenames and the hashes. You might even be able to manage to get the two computers to calculate the hashes in parallel, giving a two-fold speedup.


Implementation of disjoint set in Java with linked-list I have just wrote a linked list implementation of disjoint sets in Java for the Rainfall Problem. Can you take a look and tell me if there is something missing and also make a judgement in performance?

public class DisJointSet<E>
{
    int rank;
    DNode<E> head;
    DNode<E> tail;

    public DisJointSet(E data)
    {
        DNode<E> node = new DNode<>(data);
        head = node;
        tail = node;
        rank = 1;
        node.represantive = this;
    }

    public void union(DNode<E> x, DNode<E> y)
    {

        DisJointSet<E> xSet = findSet(x);
        DisJointSet<E> ySet = findSet(y);
        if (xSet.rank>= ySet.rank)
            append(xSet, ySet);
        else
            append(ySet, xSet);
    }

    public void append(DisJointSet<E> first, DisJointSet<E> second)
    {

        if (first.rank == 0 || second.rank == 0)
            throw new IllegalArgumentException();

        // Set the representative of each node in the second set to be
        // the first set.
        DNode<E> curr = second.head;

        while(curr!=null)
        {
            curr.represantive = first;
            curr = curr.next;
        }

        first.tail.next = second.head;
        first.tail = second.tail;
        first.rank += second.rank;

        // Invalidate the second set, just to be sure.
        second.head = null;
        second.tail = null;
        second.rank = 0;
    }

    public DisJointSet<E> findSet (DNode<E> x)
    {
        return x.represantive;
    }
}

class DNode<E>
{
    DNode<E> next;
    DisJointSet<E> represantive;
    E data;

    DNode(E val)
    {
        data = val;
    }
}

Nitpicks

Disjoint is a single word, your class should be named DisjointSet
The correct spelling for the "sink" is representative

Formatting and Style

It's generally recommended to always put braces around blocks in if-else constructs. Yes, even when they only throw an Exception.
Binary operators usually have a space on each side. (applies curr != null and xSet.rank >= ySet.rank)

OOP vs. Functional-Style
Your way of writing OOP is... strange. union, append and findSet are defined as instance methods, meaning they need to be called on a certain instance of DisjointSet, but they don't actually use the instance at all.
Overall these could all be completely extracted into a static utility class, and nothing much would change. I strongly advise you to actually do exactly that. It should clarify responsibilities and make the code clearer to follow.
Alternatively you could make these functions actually start working in an OOP way. If you do that I'd furthermore suggest to make DisjointSet something like an immutable class (see String or Path) that will create a new instance when you union or append another Set to it. In fact I recommend you do that, even when you don't rewrite the code into a more OOP fashion.
Additionally your DNode's data could (and should) be declared final to avoid modification.


LinkedIn work history time The code I have works. I was just curious if there was an easier way to write the function.

Note: It is referenced inside of an object so you may need to change the $this variables.

public function workTime() {

    $startDate = new DateTime($this->start_date);
    $startPretty = date("F Y", strtotime($this->start_date));
    $endDate = new DateTime($this->end_date);
    $endPretty = date("F Y", strtotime($this->end_date));
    $current = $this->current; // boolean true/false

    if($current) // currently works at this job
    {
        $endDate = new DateTime(); // current date/time
        $endPretty = 'Present';
    }

    $diff = $endDate->diff($startDate);

    if($diff->y = 0)
    {
        if($diff->m > 1)
        {
            $string = $diff->m . ' months';
        }
        else
        {
            $string = '1 month';
        }
    }
    elseif ($diff->y = 1)
    {
        if($diff->m > 1)
        {
            $string = '1 year ' . $diff->m . ' months';
        }
        else
        {
            $string = '1 year 1 month';
        }
    }
    else
    {
        if($diff->m > 1)
        {
            $string = $diff->y . ' years ' . $diff->m . ' months';
        }
        else
        {
            $string = $diff->y . ' years 1 month';
        }
    }
    $output = $startPretty . ' - ' . $endPretty . ' ( ' . $string . ' )';
    return $output;

}

Here is some sample data:

$this->start_date = '2013-05-01';
$this->end_date = '2014-12-01';

or you can make $this->end_date = null; and $current = true;

It currently outputs:

  May 2013 - Present ( 1 year 7 months )

which is what it should do. I'm just curious if this could be done cleaner/simpler.

If you already parse the date string using DateTime, then it's better to format using that resulting date object instead of parsing again with strtotime, for example:

$startDate = new DateTime($this->start_date);
$startPretty = $startDate->format('F Y');

If $current is true, then there's no need to parse $this->end_date.
You can move that to the else block instead:

if($current) // currently works at this job
{
    $endDate = new DateTime(); // current date/time
    $endPretty = 'Present';
}
else
{
    $endDate = new DateTime($this->end_date);
    $endPretty = $endDate->format('F Y');
}

These conditions don't do what you think they do:

if($diff->y = 0)
{
    // ...
}
elseif ($diff->y = 1)
{
    // ...
}

The = operator assigns. This is not a check for equality.
The result of an assignment is the assigned value.
So the above statements are actually evaluated like this:

if(0)  // always false, $diff->y was set to 0, no matter what it was before
{
    // ...
}
elseif (1)  // always true, $diff->y was set to 1, no matter what it was before
{
    // ...
}

That is, first $diff->y is set to 0, the expression evaluates to false,
then in the elseif, $diff->y is set to 1, the expression evaluates to true.
To see the problem, set start_date to 2 or more years before end_date.
The result will always be 1 year difference instead of 2, 3 or more.

What you really wanted is the equals operator == like this:

if($diff->y == 0)
{
    // ...
}
elseif ($diff->y == 1)
{
    // ...
}


reformatting string to two string values in javascript I have the following code that converts a string that looks like aaa-bbb|ccc-ddd|eee-fff to two strings that look like aaa|ccc|eee and bbb|ddd|fff.

var id = "HEL-CAS|MAD-STO|XXX-YYY"
var splittedId = id.split('|');
var from = "", to = "";
$(splittedId).each(function(a,b) {
    from = from + b.split("-")[0]+"|";
    to = to + b.split("-")[1] + "|";
});
from = from.substring(0,from.length-1);
to = to.substring(0,to.length-1);

The code works fine, but I am wondering if it's possible to do this in fewer operations / or with faster runtime.

Yes, there are few built-in array functions you may use to simplify your code. I don't think you will ever notice any performance difference but code will also be easier to read:

var id = "HEL-CAS|MAD-STO|XXX-YYY"

// Format is fixed then it's easy to use a regex
var splittedId = id.split(/[\|-]/);

// Now we can pick "every other element" and then join strings
var from = splittedId.filter(function (v, i) {
    return i & 1 == 0;
}).join('|');

var to = splittedId.filter(function (v, i) {
    return i & 1 == 1;
}).join('|');


Local image cache for external images I have written a simple image cache script, the purpose is to load the image locally, so i can control the cache times of the images, and i have better load times than the host server, (battle.net serves 350 ms, i serve 65ish ms) this times 20 images is a pretty good performance step.

But being the performance freak i am. I was wondering if i can make it run faster!

Please also include the following in the review:

Better suggestions for function/variable names.
Better writing of code.
Alternate formatting.
Something that could be changed for something better / more correct.
Suggestions about how to make this generic / reusable.
An explanation about why you are right.
An explanation why you down vote this post.

So we all can learn from this.

Comments are only for Code Review and not actually in the file.

<?php
// Checks if the img parameter is set.
if(!isset($_GET['img'])){
    die;
}
// Saves it as a variable
$url = $_GET['img'];
// Compiles the local file string
$img = dirname(__FILE__) . '/media/cache/' . get_image_from_url($url);

// Check if the file exists, if not it dowloads a version of the image.
if(!file_exists($img)){
    // log_message('info', 'file does not exists');
   download_image($img, $url);
}

// Checks if the image existing is more than 24 hours old if it is gets a new image.
if(time()-filemtime($img) > 24 * 3600){
  //  log_message('info', 'file is too old');
    download_image($img, $url);
}

header('Content-Type: image/jpeg');
$handle = imagecreatefromjpeg($img);
imagejpeg($handle);

/*
 * FUNCTIONS
 */

function get_image_from_url($url){
    $url = str_replace('http://eu.battle.net/static-render/','', $url);
    $url = str_replace('/','-',$url);
    return $url;
}

function download_image($img, $url){
    $headers = get_headers($url);
    $response_code = substr($headers[0], 9, 3);

    if($response_code == '404'){

        // Pleas also advice here to make the file more generic.
        $url = 'http://localhost/warlords' . '/media/images/anonymous.jpg';
    }

    $thisurl = file_get_contents($url);
    file_put_contents($img, $thisurl);
}

When I want to cache some file, I'm typically doing it similar to your solution.

However, you're able to do most of this with built-in functionality, making the code a lot shorter:

<?php
// Some URL for testing
$url = 'https://www.google.de/favicon.ico';

// Time to cache the files (here: 10 minutes)
define('time_to_cache', 600);

// Create a local file representation
$local = './cache/' . urlencode($url);

// Determine whether the local file is too old
if (@filemtime($local) + time_to_cache < time()) {
    // Download a fresh copy
    copy ($url, $local);

    // Store headers in case we need them (see alternative below)
    file_put_contents($local . '.hdr', join($http_response_header, "\n"));
}

// Solution 1: Redirect to the local cache file
header('Location: ' . urlencode($local));
exit();

// Alternative: Send headers and the actual file
// Note that this might cause problems, e.g. due
// to cache fields and the like.

// Read and send headers
foreach(file($local . '.hdr') as $line)
    header($line);

// Read and send the actual file
readfile($local);

This doesn't include any error handling though.

For error handling, you might just want to check the results of the copy() operation. There's no need to request headers in advance or doing anything like that, since that will essentially delay you twice.

Regarding your implementation above:

Don't check the headers first. Just try downloading the file. This will take the same time. If you request headers first, you'll do two request, each of which might delay you by the mentioned 200-300 ms.
I've used urlencode() to create a working local filename. This might not be perfect depending on your actual file system though.
Don't check whether your local file exists first; you don't have to. You can just prepend filemtime() by an @, which will keep it from outputting any error messages and still return 0 in case the file doesn't exist (which will simply be interpretes as the file being ancient).
Why do you read the JPEG from your local disk and then reencoding it as a new image to be sent to the browser? Just send the actual file contents (or redirect the browser). The following will do the same without reencoding the image:

header('Content-type: image/jpeg');
readfile($local);


Writting day_of_month and month_day with pointers instead of array indexing Rewrite the routines day_of_year and month_day with pointers instead of indexing.

The exercise is quite simple, this is the solution:

int day_of_year(unsigned int year, int month, int day) {
    int leap, i;

    leap = ((year % 4 == 0 && year % 100 != 0) || (year % 400 == 0));

    if((month >= 1 && month <= 12) && ((day >= 1) && (day <= *(*(daytab + leap) + month)))) {
        for(i = 1; i < month; i++) {
            day += *(*(daytab + leap) + i);
        }

        return day;
    }

    return -1;

}

int month_day(unsigned int year, int yearday, int *pmonth, int *pday) {
    int leap, i;

    leap = ((year % 4 == 0 && year % 100 != 0) || (year % 400 == 0));
    if((leap == 1 && (yearday >= 1 && yearday <= 366)) || (leap == 0 && (yearday >= 1 && yearday <= 365))) {

        for(i = 1; yearday > *(*(daytab + leap) + i); i++) {
          yearday -= *(*(daytab + leap) + i);
        }
        *pday = yearday;
        *pmonth = i;

        return 1;
    }
    return -1;
}

The expression (daytab + leap) is a pointer to the leap-th row and (*(daytab + leap) + i) is a pointer to the i-th element of the leap-th row.

It would have been nice to see your daytab definition as well.  I'll just assume that you're satisfied with the way it's defined, and that the functions work as intended.

I find this condition in month_day() a bit monstrous:

if((leap == 1 && (yearday >= 1 && yearday <= 366)) || (leap == 0 && (yearday >= 1 && yearday <= 365)))

I would shorten it to the following.  Also, I've inverted the check, so that the function returns early if the parameters fail validation.  Returning early is a better habit, as it reduces indentation, and reduces mental load when reading the code by getting the simpler branch out of the way.

int leap = (year % 4 == 0) && ((year % 100 != 0) || (year % 400 == 0));
if (yearday < 1 || yearday > 365 + leap) {
    return -1;
}
for (…) {
    …
}


Object oriented, runtime expandable Sieve of Eratosthenes In answering this question, it seemed to me that it would be nice to have a runtime expandable Sieve of Eratosthenes.  This is my implementation of that notion.

Design

I've created a class named Sieve which has two member functions:

unsigned Sieve::expand(unsigned upperlimit);

This expands the existing sieve up to the passed upper limit and attempts to minimize rework.  That is, if the current sieve has an upper limit of 10 and the expansion request is 15, the code only processes the difference, i.e. the half open range (10, 15].  If the new requested limit is smaller, the code simply returns without doing anything.

The other significant function is:

bool isPrime(unsigned n) const;

As one would expect, if n is not greater than the current upper limit, the returned bool is true if n is prime.  If n is greater than the current limit, this function throws a std::range_error.  The intent is that one could either catch an error or, better, always call expand before isPrime.  The reason for not combining those within the class code is that I wanted to create a complete but minimal interface.

The test code is simply a reinterpretation of the original code's interface.  Specifically, it expects one integer to be the number of test cases, T, and then for each test case, a lower and upper integer (m, and n).  For each test case, the code prints a list of prime numbers in the range [m, n] separated by spaces.  The test code assumes correct input and does not, for example, check to assure that m < n.

primes.cpp

#include <iostream>
#include <cmath>
#include <vector>
#include <algorithm>
#include <stdexcept>

class Sieve {
public:
    Sieve();
    unsigned expand(unsigned upperlimit);
    bool isPrime(unsigned n) const;
private:
    unsigned limit;
    std::vector<bool> composite;
};

Sieve::Sieve() :
    limit{0},
    composite{}
{}

bool Sieve::isPrime(unsigned n) const {
    if (n > limit)
        throw std::range_error("n must be less than limit");
    return n==2 || (n%2 && !composite[n/2]);
}

unsigned Sieve::expand(unsigned upperlimit) {
    if (upperlimit > limit) {
        composite.resize(upperlimit);
        composite[0] = true;  // 1 is not prime
        const unsigned sqrtLimit = std::ceil(std::sqrt(upperlimit));
        // std::cout << "old limit = " << limit << ", new limit = " << upperlimit << "\n";
        for (unsigned i = 3; i <= sqrtLimit; i+=2) {
            if (!composite[i/2]) {
                const unsigned start = i * std::max(i, (((limit+i)/i)|1));
                // std::cout << "Starting with i = " << i << ", from " << start << "\n";
                for (unsigned j = start; j <= upperlimit; j += i+i) {
                    composite[j/2] = true;
                    // std::cout << j << " is composite because it's a multiple of " << i << "\n";
                }
            }
        }
        limit = upperlimit;
    }
    return limit;
}

int main() {
    int T;
    int m, n;
    Sieve sieve;

    for (std::cin >> T; T; --T) {
        std::cin >> m >> n;
        sieve.expand(n);
        for (int i = m; i <= n; ++i) {
            if (sieve.isPrime(i)) {
                std::cout << i << ' ';
            }
        }
        std::cout << '\n';
    }
}

Results

When compiled on my 64-bit Linux machine with this command:

g++ -O2 $CXXFLAGS primes.cpp -o primes

I can then run and time the program with this:

time echo 4 1 13 40 50 80 90 10000000 10000100| ./primes

Here is the output:

2 3 5 7 11 13
41 43 47
83 89
10000019 10000079

real  0m0.044s
user  0m0.041s
sys   0m0.004s

Initialize your class members in the declaration

class Sieve {
public:
    unsigned expand(unsigned upperlimit);
    bool isPrime(unsigned n) const;
private:
    unsigned limit = 0;
    std::vector<bool> composite{};
};

is easier to read because you don't need to jump to the constructor to see the initial values and now you don't even need a constructor anymore.
In case you do write constructors and use the initialization list syntax it will override the declaration initialization for those members, so your constructors can concentrate on initializing members that are different than the default from the declaration, again increasing readability.

Limit your nesting depth

Sieve::expand is moving quite far to the right. A way to avoid that is to use early exits instead of nesting:

unsigned Sieve::expand(unsigned upperlimit) {
    if (upperlimit <= limit)
        return limit;
    composite.resize(upperlimit);
    composite[0] = true; // 1 is not prime
    const unsigned sqrtLimit = std::ceil(std::sqrt(upperlimit));
    // std::cout << "old limit = " << limit << ", new limit = " << upperlimit << "\n";
    for (unsigned i = 3; i <= sqrtLimit; i += 2) {
        if (composite[i / 2])
            continue;
        const unsigned start = i * std::max(i, (((limit + i) / i) | 1));
        // std::cout << "Starting with i = " << i << ", from " << start << "\n";
        for (unsigned j = start; j <= upperlimit; j += i + i) {
            composite[j / 2] = true;
            // std::cout << j << " is composite because it's a multiple of " << i << "\n";
        }
    }
    limit = upperlimit;
    return limit;
}

Declare variables as late as possible

Inside main you don't need m and n until you are inside the for loop.

Do at least basic input error checking

You wrote that the program expects valid input, but that is not really acceptable. When a user enters something that cannot be parsed as an int you read uninitialized memory which is undefined behavior and bad things happen.
if (!(std::cin >> m >> n)) return -1; at least gives your program defined behavior without much effort. You could also throw an exception or print a proper message and let the user try again, but having std::cin outside an if is usually a problem.

Better name-giving-ness

limit and upperlimit seem less nice than size and requested_size.
T is usually a template parameter and it probably shouldn't be the only variable name that is capitalized. m and n don't tell me much either. I would prefer T-> test_cases, n->upper, m->lower or something more descriptive.

Consistent parenthesis for ifs

Arguably it is a matter of style if you prefer

if (n > limit)
        throw std::range_error("n must be less than limit");

or

if (sieve.isPrime(i)) {
    std::cout << i << ' ';
}

but you should pick a style and stick to it.
Now, going through the code and adding brackets around one-line if-statements is a stupid waste of time, fortunately clang-tidy -fix *.cpp "-checks=-*,readability-braces-around-statements" -- -std=c++14 can automatically fix that for you.


Logging out users securely Is there anything I need to add or is this the best security possible?

<?php

//Get Sessions Prepared;
session_start();

//If logged in, Logout;
if(!empty($_SESSION['username'])) {

    //Remove Sessions to Logout the user;
    $params = session_get_cookie_params();
    setcookie(session_name(), '', time() - 42000,
        $params["path"], $params["domain"],
        $params["secure"], $params["httponly"]
    );
    session_destroy();

} else {

    //If not logged in redirect to Homepage;
    header("Location: http://gameshare.io/login");

}

?>

Is there anything I need to add or is that the best security im looking at?

There really isn't much that you can do wrong here. You correctly tell the browser to delete the cookie, and there isn't more that you can do.

But if this is all your logout code, you are vulnerable to CSRF logout. It is debatable if this is a security issue; some argue that it is not, as cookies may be forcefully deleted by overflowing the cookie jar, but that's really a browser issue that may or may not exist on the client side, so to be on the save side, you should add CSRF protection.

As a matter of best practice, you should also always die after a header redirect, as a client doesn't have to follow the redirect and code afterwards will be executed. This doesn't seem to be an issue now, but it may be in the future.

Other than that, your code is fine, but I would remove all your comments, they are just repeating what the code already says. If you want structured and named code blocks, just add functions.


Displaying the multiples of a given number Here's a method I made that's supposed to print out multiples of any numbers up to any quantity:

def DisplayMultiples(multiplesOf, count)
  i = multiplesOf
  while i <= count
    if i % multiplesOf == 0
      puts i
    end
    i += 1
  end
end

Any suggestions on how to improve the code to something more fitting to the Ruby style? I'm coming from a C# background so I'd like to switch things up a bit.

One important note regarding naming in ruby:

The syntax in ruby is that constants (including class and module names) have to start with capital letters and local variables have to start with lower case letters. Instance, class and global variables must start with their respective sigil followed by a letter in any case. Method names must start with a letter in any case, but if they start with a capital letters, they must be called with () when there are no arguments.

The naming convention in ruby is to use snake_case for method names and variables, PascalCase for class and module names and ALL_CAPS for constants other than class and module names.

For method names the convention is to use snake_case for all methods except "constructor"-style functions (e.g. Integer(), Array(), Hash()) where the name of the function is the same as that of the constructed class.

Generally using PascalCase for method names is a bad idea because:

It's against convention and will make people think it's a constructor-style function
If you define a method without arguments, you'll trip yourself up because you'll try to call it as MyMethod instead of MyMethod() which does not work if the method name starts with a capital letter.

General notes on your code:

Using a while loop is usually unidiomatic ruby. Using a while loop to iterate from one number to another number with a constant step-size is always unidiomatic ruby. If the step-size is 1, you can just use each on a range and if it is something else you can use step. So your code would just be written as:

def display_multples(multiples_of, count)
  (multiples_of .. count).each do |i|
    if i % multiples_of == 0
      puts i
    end
  end
end

Now as pdr points out, this is not the optimal algorithm to do this. Instead of iterating between all integers between multiples_of and count and printing those which are divisible by multiples_of, you can just iterate between the numbers between multiples_of and count with a step size of multiples_of, i.e. only iterate over the multiples of multiples_of. However his code contains an unnecessary each (which will also make the code not work on ruby 1.8.6, though that's thankfully becoming less of an issue these days). So the code should just be:

def display_multiples(multiples_of, count)
  (multiplesOf..count).step(multiples_of) do |i|
    puts i
  end
end

However this code still contains one major, non-ruby-specific design-smell: You're mixing the logic to generate the multiples with the code printing it. Generally IO and logic should be separated as much as possible. So what you should do is define a method multiples which generates the multiples and a method print_multiples which prints them.

In ruby the best way to write a method which generates a sequence of values is to write an "iterator method", i.e. a method which yields the elements. That would look like this:

def multiples(multiples_of, count)
  (multiples_of .. count).step(multiples_of) do |i|
    yield i
  end
end

def print_multiples(multiples_of, count)
  multiples(multiples_of, count) do |i|
    puts i
  end
end

You can also define multiples as:

def multiples(multiples_of, count, &blk)
  (multiples_of .. count).step(multiples_of, &blk)
end

In 1.8.7+ this also has the benefit of returning an Enumerator if no block is given. so if you don't just want to print it, you can also use Enumerable methods like map, select etc. on it.


Finding two numbers that add to a given total Given there's a list of random number and the amount I want to find.

For example:

  [80, 98, 83, 92, 1, 38, 37, 54, 58, 89]

And I want two numbers that add up to a given total in this case it's 181. So, it's going to be (92+89). The list could be really big.

Here's the solution I come up with. But it's kinds brute force and I was wondering if there's a better way of doing this.

for i, item in enumerate(numbers):
    for j in range(i+1, len(numbers)):
        total_of_two_items = numbers[i] + numbers[j]
        if(total_of_two_items == total_number):
            print '{first_item} {second_item}'.format(first_item=i+1, second_item=j+1)
            print '\n'

Here is an alternative approach using set logic, which is O(n) in the average case:

n = 181
n2 = n//2
numbers = [80, 98, 83, 92, 1, 38, 37, 54, 58, 89]
goodnums = {n-x for x in numbers if x<=n2} & {x for x in numbers if x>n2}
pairs = {(n-x, x) for x in goodnums}

What this does is first filter out values that are greater than 1/2 the target value, since one number in each pair must be that way.  Then it subtracts the remaining numbers from the target (in this case 181).  This gets the other value from the pair.  Then it uses set logic to extract only those values where the other value is present in the original list of numbers.

So to put it more briefly, it finds all values x such that 181-x is also present in the list.

Edit: If you don't want to include cases where both members of the pair are equal and it only exists once, such as n=2 and numbers = [1], as Gareth pointed out, add this to the end:

if not n%2 and (n2, n2) in pairs and numbers.count(n2) == 1:
   pairs.remove((n2, n2))

This will check if n is even and, if so, if there is exactly one value where x==n//2, if so remove (n//2, n//2) from the results.


Flux architecture implementation (vanilla JS) I'm trying to better understand flux-style architecture, so I wrote this snippet to clarify how a simple flux system can be designed. I was wondering if my implementation is valid, or if I've missed something.

const application = () => {
  /* Init the model */
  let model = {
    page: null
  };

  /* redirects the reference from the old state to the new */
  const updateState = newState => model = newState;

  /* a reducer method, more are easily added */
  const UPDATE_PAGE = (model, newPage) => {
    let newState = Object.assign({}, model);
    newState.page = newPage;
    updateState(newState);
  };

  /*
   * dispatch is a core function, directing every action
   * to the proper reducer depending on the `type`
   */
  const dispatch = action => {
    action.type === 'UPDATE_PAGE' ?
      UPDATE_PAGE(model, action.page)
      : null;
  };

  /* An action mutating state */
  let updatePageAction = {
    type: 'UPDATE_PAGE',
    page: 1
  };

  console.log('initial model: ', model);

  /* simply dispatch the action */
  dispatch(updatePageAction);

  /* and the state is updated! */
  console.log('updated model: ', model);
};

application();

First of all, Flux is not to be confused with Redux. While both have similarities, the main difference is that Flux allows multiple stores while Redux advises only using one store per app for simplicity. The following will talk about Redux.

/*
 * dispatch is a core function, directing every action
 * to the proper reducer depending on the `type`
 */
const dispatch = action => {
  action.type === 'UPDATE_PAGE' ?
    UPDATE_PAGE(model, action.page)
    : null;
};

Dispatch doesn't actually "direct" the action to the right reducer. It simply invokes the reducer, supplying it the current state and the dispatched action. It's actually the reducer that does the pattern matching/selection of what logic runs with which action.

/* a reducer method, more are easily added */
const UPDATE_PAGE = (model, newPage) => {
  let newState = Object.assign({}, model);
  newState.page = newPage;
  updateState(newState);
};

A reducer is a pure function which takes 2 arguments, the current state and the dispatched action and returns the new state, based on the state and the passed action. You can think of it as the "equation" part of the app (yes, like a math equation). Reducers look like:

const initialState = { count: 0 }

const reducer = (state = initialState, action) => {
  switch(action.type){
    case 'INCREMENT':
      return {...state, count: state.count + 1 }
    case 'DECREMENT'
      return {...state, count: state.count - 1 }
    default:
      return state
  }
}

There's several reasons why reducers are designed this way.

The reducer is pure. Its result is not affected by anything other than its inputs, making it stupid easy to test.
It's just a data transformer. You can easily test the logic without fancy mocks, stubs or spies because there's nothing to mock, stub or spy.
You can easily compose reducers. You can divide the state into sections, each with its own reducer. Redux provides a handy combineReducers utility function for it.
At the end of the day, reducers are just functions. Not a fancy service class, not a magical method, not some fancy concept invented by someone, it's just a function.

Redux is so simple, that if you just overheard someone in the pub talking about what it does, you can easily implement it yourself. It's effectively a pub-sub library, except that it also manages and holds state, and only ever emits one kind of event - a state change.

// my-redux.js - The only part that is "Redux"
export const createStore = reducer => {
  const subscribers = []
  let currentState = null

  return {
    getState(){
      return currentState
    },
    subscribe(fn){
      subscribers.push(fn)
      fn(currentState)
    },
    dispatch(action){
      currentState = reducer(currentState, action)
      subscribers.forEach(fn => fn.currentState)
    }
  }
}

// reducer.js - Where your app's logic goes.
const initialState = { count: 0 }

export const reducer = (state = initialState, action) => {
  switch(action.type){
    case 'INCREMENT':
      return {...state, count: state.count + 1 }
    case 'DECREMENT'
      return {...state, count: state.count - 1 }
    default:
      return state
  }
}

// app.js - How the app is brought together
import {createStore} from './my-redux'
import {reducer} from './reducer'
import {YourComponent} from './your-component'

const store = createStore(reducer)

YourComponent({
  mount: 'somewhere',
  onmount(){
    store.subscribe(state => {
      this.setYourState(state)
    })
  },
  render(state) {
    return (<button onclick="store.dispatch({ type: 'STH', data: 'NOT VALID JSX BUT WHATEVER' })">Click Me</button>)
  }
})

You can read more about the three principles of Redux and how it's just a guiding concept rather than an actual implementation.


Validate that brackets are balanced I've done a test for a job (which I failed) and I'd like to know in which ways my code could've been better.

Here are the questions and my answers, it's not very long.

  1) Make a program that checks if a string has balanced parentheses or
  brackets.

  Braces: (), [], <>
  For every closing brace, there must've been an opening, and for every open there must be a closure
  return 1 if it's balanced, or 0 if it isn't

My code:

const verify = (string) => {

    // Array of the type of braces
    var braces = ["(", ")", "[", "]", "<", ">"];
    // Array half the length of braces Array
    var balanced = Array.apply(null, {length: braces.length/2}).map(function() { return 0; });

    for(let i = 0; i < string.length; ++i) {
        let char = string[i], braceIndex = braces.indexOf(char);

        if ( braceIndex == -1 ) {
            // If current char is not any type of brace, continue.
            continue;
        }

        // Get the index of the brace found halved and floored
        let braceBalancingIndex = braceIndex / 2, braceFloor = Math.floor( braceBalancingIndex );

        // If the halved index is equal to the index, then it must be an opening, else it must be a closing
        if (braceBalancingIndex == braceFloor) {
            balanced[braceFloor]++;
        }
        else {
            balanced[braceFloor]--;
        }

        // If at any type being, the balance is negative, there was a closing without opening
        for(let j = 0; j < balanced.length; ++j) {
            if (balanced[j] < 0) return 0;
        }

    }

    // If at the end the string is not balanced, there where more openings than closings
    for(let i = 0; i < balanced.length; ++i) {
        if (balanced[i] != 0) return 0;
    }

    return 1;
}

At its heart, this is a stack problem.  You can get O(n) worst-case performance and O(n) space complexity by simply iterating the string in place, pushing opening brackets onto a stack and when encountering a closing bracket, popping the last item off stack to compare against.

You need to end up with an empty stack, and you can never have a case where you try to read out of an empty stack.  This in addition, of course to making sure items you are comparing out of the stack match appropriately.  So, there are three failure scenarios.

The solution I present might technically be optimized by hard-coding the opening and closing bracket values into conditionals and such, but I find this solution more flexible and easier to maintain from a code standpoint, as you have decoupled the bracket "configuration" from the code logic.

var balanced = '({[test string](test [string])})';
var unbalanced = '{()}]';
var empty = '';

var bracketConfig = [
    { left: '{', right: '}' },
    { left: '[', right: ']' },
    { left: '(', right: ')' }
];

function isBalanced(subject, bracketConfig) {
    // not shown - perhaps validate subject as string and error out if failing

    // build bracket arrays from config
    var openingChars = [];
    var closingChars = [];
    bracketConfig.forEach( (item) => {
        openingChars.push(item.left);
        closingChars.push(item.right);
    });

    var stack = [];
    for (var i = 0, len = subject.length; i < len; i++) {
        var char = subject[i];
        var openIdx = openingChars.indexOf(char);
        var closeIdx = closingChars.indexOf(char);
        if (openIdx > -1) {
            stack.push(openIdx);
        } else if (closeIdx > -1) {
            if (stack.length === 0) return 0;
            lastIdx = stack.pop();
            if(lastIdx !== closeIdx) return 0;
        }
    }

    if (stack.length !== 0) return 0;
    return 1;
}

// run tests
console.log(isBalanced(balanced, bracketConfig));
console.log(isBalanced(unbalanced, bracketConfig));
console.log(isBalanced(empty, bracketConfig));

JSFiddle of this example

My guess is that, for your interview, they were really looking to see that you could identify this as a stack problem and come up with a reasonable implementation.


Inverted index in Clojure - performance vs. idiomatic code I have this code to create an inverted index from a directory of text files:

(def p (re-pattern #"[^\p{L}&&[^\p{M}]]"))

(defn invert[file]
  (let [f (.getName file)
        tokens (.split p (lower-case (slurp file)))]
        (into {} (mapcat #(hash-map % #{f}) tokens))))

(defn build-index[dirname]
  (reduce #(merge-with union %1 %2) (map invert (.listFiles (java.io.File. dirname)))))

Can you improve it / make it more idiomatic? My concern is performance. I'm testing it with 10k files of 10k in size, and I can make it about 20-30% faster if I use transients like this:

(defn add![idx file]
  (let [f (.getName file)]
    (loop [idx idx
           tokens (.split p (lower-case (slurp file)))]
      (if-not (seq tokens) idx
              (recur (assoc! idx (first tokens) (union (idx (first tokens)) #{f})) (rest tokens))))))

(defn build-index[dirname]
  (loop [files (.listFiles (java.io.File. dirname))
         idx (transient {})]
    (if-not (seq files) (persistent! idx)
            (recur (rest files) (add! idx (first files))))))

Full code including test file generator here:

https://github.com/dbasch/closearch

Any feedback is welcome.

You should be able to use reducers to get a good speed boost as the consumption and building of your index can be parallelized.

I have found the best docs for reducers to be the doc strings themselves:
Reducers -- Github

(ns ...
  (:require [clojure.core.reducers :as r]
            [clojure.java.io :as io]
            [clojure.string :as str]))

(defn invert [file]
  (let [f (.getName file)
        tokens (str/split (str/lower-case (slurp file)) p)]
        (->> tokens
             (r/mapcat #(hash-map % #{f}))
             (into {}))))

(defn build-index
  [dirname]
  (->> dirname
       io/file
       (.listFiles)
       (r/map invert)
       (r/fold (r/monoid (partial merge-with union) hash-map))))


Mergesort that has `man 3 qsort` ANSI C prototype I have taken a look at these review questions: one, two, three and four and would still like to ask you to review my implementation of mergesort that follows the man 3 qsort prototype.

void *malloc_or_exit(size_t size) {
    void *tmp = malloc(size);

    if (tmp == NULL) {

        if (size == 0) {
            return tmp;
        }

        fprintf(
            stderr, "Failed to malloc %zu bytes: %s",
            size, strerror(errno)
        );

        exit(EXIT_FAILURE);
    }

    return tmp;
}

void mergesort0(
    void *xs, size_t n, size_t s,
    int (*cmp) (const void *, const void *)
) {
    if (n == 0 || n == 1) return;

    const size_t nlft = n / 2;
    const size_t nrgt = n - nlft;

    void *ys = (char *)xs + nlft * s;

    mergesort0(xs, nlft, s, cmp);
    mergesort0(ys, nrgt, s, cmp);

    void *const as = malloc_or_exit(n * s);
    void *const bs = (char *)as + nlft * s;
    void *const cs = (char *)as + n * s;

    memcpy(as, xs, n * s);

    void const *i = as;
    void const *j = bs;
    while (i != bs && j != cs) {
        if (cmp(i, j) <= 0) {
            memcpy(xs, i, s);
            i = (char *)i + s;
        } else {
            memcpy(xs, j, s);
            j = (char *)j + s;
        }

        xs = (char *)xs + s;
    }

    for (; i != bs; i = (char *)i + s, xs = (char *)xs + s) {
        memcpy(xs, i, s);
    }

    for (; j != cs; j = (char *)j + s, xs = (char *)xs + s) {
        memcpy(xs, j, s);
    }

    free(as);
}

#include <stdio.h>
int main() {
    int xs[] = {1, 2, 3, 4, 5};
    int ys[] = {5, 4, 3, 2, 1};
    int zs[] = {3, 1, 2, 4, 5};
    int as[] = {1, 1, 1};
    int bs[] = {3, 2, 2};

    int len = 5;

    qsort(zs, len, sizeof(int), compare_int);

    for (int i = 0; i < len; ++i) {
        printf("%d ", *(zs + i));
    } printf("\n");
}

Better show your includes too, that makes evaluating your code easier and more meaningful.
Anyway, it's <stdlib.h>, <stdio.h> and <string.h>.
You know you can combine multiple conditions into a single expression with &&?
You can avoid all those casts if you use char* instead of void* internally.
Why don't you reuse a single allocation instead of doing n*log(n) ones, each local?
You know doing a single combined memcpy is far more efficient than multiple consecutive smaller ones?
If you only move the first nlft elements out of the way, you only need half the extra-space and save on copying.

Your test-program:

Your test-program doesn't seem to exercise your code...
You lost a newline at the end of your test-program.

Modified code:

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

void* malloc_or_exit(size_t size) {
    void* result = malloc(size);

    if (!result && size) {
        fprintf(
            stderr, "Failed to malloc %zu bytes: %s",
            size, strerror(errno)
        );
        exit(EXIT_FAILURE);
    }

    return result;
}

static void mergesort0_impl(
    char* as, char* xs, size_t n, size_t s,
    int (*cmp) (const void*, const void*)
) {
    if (n < 2) return;

    const size_t nlft = n / 2;
    const size_t nrgt = n - nlft;

    char* ys = xs + nlft * s;

    mergesort0_impl(as, xs, nlft, s, cmp);
    mergesort0_impl(as, ys, nrgt, s, cmp);

    char* const bs = as + nlft * s;

    memcpy(as, xs, nlft * s);

    const char* i = as;
    const char* j = ys;
    for (; i != bs && j != xs; xs += s) {
        if (cmp(i, j) <= 0) {
            memcpy(xs, i, s);
            i += s;
        } else {
            memcpy(xs, j, s);
            j += s;
        }
    }
    if (i != bs)
        memcpy(xs, i, bs - i);
}

void mergesort0(
    void* xs, size_t n, size_t s,
    int (*cmp) (const void*, const void*)
) {
    if(n < 2) return;
    void* as = malloc_or_exit(n / 2 * s);
    mergesort0_impl(as, xs, n, s, cmp);
    free(as);
}

#include <stdio.h>

void test(int nr, const int xs[], size_t n) {
    int* a = malloc_or_exit(2 * n * sizeof *xs);
    int* b = a + n;
    memcpy(a, xs, n * sizeof *xs);
    memcpy(b, xs, n * sizeof *xs);
    qsort(a, n, sizeof *a, compare_int);
    mergesort0(b, n, sizeof *b, compare_int);
    printf("Test %d %s: ", nr, !memcmp(a, b, n * sizeof *xs) ? "OK" : "FAILED");
    for (size_t i = 0; i < n; ++i)
        printf("%d%c", xs[i], " \n"[i == n - 1]);
    free(a);
}

int main() {
    int xs[] = {1, 2, 3, 4, 5};
    int ys[] = {5, 4, 3, 2, 1};
    int zs[] = {3, 1, 2, 4, 5};
    int as[] = {1, 1, 1};
    int bs[] = {3, 2, 2};
    int nr = 0;
    #define TEST(a) test(nr++, a, sizeof a / sizeof *a)
    TEST(xs);
    TEST(ys);
    TEST(zs);
    TEST(as);
    TEST(bs);
    #undef TEST
}


Remove all html special chars Before saving the input from users in the database, I am using the below function to replace all HTML special chars. All my users will always be using the English Language.

Option 1

function clean($var) {
  $regEx="/[^a-zA-Z0-9 -_]/";
  $var = preg_replace($regEx, "", $var);
  return $var;
}

I want users to be able to only store

Letters (a to z) (Case Insensitive)
Numbers (0 to 9)
Space, Dash, Underscore

Will the above function is good for this job, or should I be using a more efficient/inbuilt function in PHP?

This is how I am using the function.

$userInput = htmlspecialchars(clean($userInput));

Option 2

function h($str_to_encode = ""){
    // Pregmatch will replacte all HTML characters with Empty string
    return preg_replace("/&#?[a-z0-9]{2,8};/i","", htmlspecialchars($str_to_encode));
}

Regex from : https://stackoverflow.com/a/657670/4050261

Similar to Toto's answer, I recommend:

function clean($var) {
    return preg_replace("~[^\w -]+~", "", $var);
}

This will replace all occurrences of one or more consecutive forbidden characters.

Adding the "one or more" (+) quantifier means longer potential matches and fewer total replacements.  IOW, imagine a carton of a dozen eggs on yhe ground.  If the task was to pick up 12 eggs, you could squat 12 times picking them up one at a time, or just squat once and pickup the carton.

I have eliminated the unnecessary inclusion of "single-use variables" as there is no benefit in retaining them for readability.

Following this custom function call, the call of htmlspecialchars() is useless because there won't be any chars to convert.

On the other hand, if you wanted to call htmlspecialchars_decode() prior to clean() there is reasonable logic to that decision, but it depends on the input that you are expecting.


Summing up ints in vectors for education To get started with C++11/14/17, I decided to write a program that brute forces the problem described in this video (and gives a progress update every few seconds):

  Divide the numbers [0, 31] into two sets A and B such that

  \$\sum_A {a} = \sum_B{b}\$
  \$\sum_A {a^2} = \sum_B{b^2}\$
  \$\sum_A {a^3} = \sum_B{b^3}\$
  \$\sum_A {a^4} = \sum_B{b^4}\$

How can I make this code more C++14-ish? My goal is not to make the code more mathematically clever, or necessarily faster. I mainly wanted to use a reasonable to high number of modern C++ standard features to brute-force-solve this problem.

#include <vector>
#include <iostream>
#include <numeric>
#include <chrono>
#include <iomanip>
#include <cstdlib>

int main() {

    uint32_t split {0};
    uint32_t print_mask { 2048 };
    auto lastPrintTime = std::chrono::system_clock::now();
    auto const SLOWER_THRESHOLD = std::chrono::seconds(5) ;
    auto const FASTER_THRESHOLD = std::chrono::seconds(10) ;

    while (split < std::numeric_limits<uint32_t>::max()) {
        if (split % print_mask == 0) {
            double progress_percentage { (split/(double)std::numeric_limits<uint32_t>::max()) * 100.0 };
            std::cout << "Currently checking " << std::setw(20) << std::setfill(' ') << split << ", " << std::fixed << progress_percentage << '%' << std::endl;

            auto now = std::chrono::system_clock::now();
            auto diff = std::chrono::duration_cast<std::chrono::seconds>(now - lastPrintTime);
            lastPrintTime = now;

            if (diff < SLOWER_THRESHOLD) print_mask *= 2;
            if (diff > FASTER_THRESHOLD && print_mask > 10) print_mask /= 2;
        }

        std::vector<int> va;
        std::vector<int> vb;

        uint32_t mask {1};
        for (int i=0; i<32; i++) {
            ((mask & split) != 0 ? va : vb).push_back(i);
            mask = mask << 1;
        }

        int sum_a;
        int sum_b;

        sum_a = std::accumulate(va.begin(), va.end(), 0, [](auto x, auto y){return x + y;});
        sum_b = std::accumulate(vb.begin(), vb.end(), 0, [](auto x, auto y){return x + y;});
        if (sum_a != sum_b) goto next;

        sum_a = std::accumulate(va.begin(), va.end(), 0, [](auto x, auto y){return x + y*y;});
        sum_b = std::accumulate(vb.begin(), vb.end(), 0, [](auto x, auto y){return x + y*y;});
        if (sum_a != sum_b) goto next;

        sum_a = std::accumulate(va.begin(), va.end(), 0, [](auto x, auto y){return x + y*y*y;});
        sum_b = std::accumulate(vb.begin(), vb.end(), 0, [](auto x, auto y){return x + y*y*y;});
        if (sum_a != sum_b) goto next;

        sum_a = std::accumulate(va.begin(), va.end(), 0, [](auto x, auto y){return x + y*y*y*y;});
        sum_b = std::accumulate(vb.begin(), vb.end(), 0, [](auto x, auto y){return x + y*y*y*y;});
        if (sum_a != sum_b) goto next;

        std::cout << "A = " << sum_a << " = [";
        for (auto v: va) {
            std::cout << v << ", ";
        }
        std::cout << ']' << std::endl << "B = " << sum_b << " = [";
        for (auto v: vb) {
            std::cout << v << ", ";
        }
        std::cout << ']' << std::endl;
        break;

        next:
        split++;
    }

    puts("Win");

    return EXIT_SUCCESS;
}

I am especially unhappy with the way I print the vectors. I have seen some code examples about ostream_iterators, but these looked even worse to me.

Use Functions
There is a ton of repetition in your code. You could make it much better by just factoring out the common parts.
Printing
You don't like how you print your vector? There's a function for that:
template <typename T>
std::ostream& operator<<(std::ostream& os, std::vector<T> const& xs) {
    bool first = true;
    os << '{';
    for (T const& x : x) {
        if (!first) os << ", ";
        os << x;
        first = false;
    }
    return os << '}';
}

And now you can just do:
std::cout << "A = " << va << ", B = " << vb << std::endl;

Checking Summation
There is a huge amount of repetition here. Effectively, we're summing f(x) on each vector, for differing x. So we can write a function for that:
template <typename F>
int accumulate(std::vector<int> const& xs, F f) {
    int sum = 0;
    for (int x : xs) {
        sum += f(x);
    }
    return sum;

    // aka return boost::accumulate(xs | transformed(f), 0);
}

bool matches(std::vector<int> const& A, std::Vector<int> const& B, F f) {
    return accumulate(A, f) == accumulate(B, f);
}

Don't use goto
There is hardly ever a reason to use goto and this is not it. Once you refactor everything, you can just write out your test:
if (matches(va, vb, [](int x){ return x; }) &&
    matches(va, vb, [](int x){ return x*x; }) &&
    matches(va, vb, [](int x){ return x*x*x; }) &&
    matches(va, vb, [](int x){ return x*x*x*x; }))
{
    std::cout << "Found: A=" << va << ", B=" << vb << std::endl;
    break;
}

No goto needed.
Use a for loop
You are looping on split from 0 to the max uint32_t value. This isn't obvious because you're using the wrong loop:
for (uint32_t split=0; split < std::numeric_limits<uint32_t>::max(); ++split)

Returning from main
You don't have to use return EXIT_SUCCESS; or return 0;. As a special case for main, return 0 is implied.
Better algorithm
Looping over a few billion elements isn't going to be the best approach... especially when there are only ~300 million unique sets to check to begin with (those that have 16 elements on each side, ignoring mirrored duplicates for which one ends up being A).


Returning the result of whichever method finishes first Consider a method (let's call it IsPrimeHybrid) which calls at least two methods (let's call them IsPrimeNaive and IsPrimeBySearch) that calculate an identical result using different approaches and the outer method returns the result when any of the inner methods finishes.

The following is my first attempt to write such a method, but I'd like to hear what issues it may have and what's a good way to fix them. (For example, how to implement thread safety in a scenario like this.)

    public static bool IsPrimeHybrid(BigInteger number)
    {
        var result = default(bool);
        var byPureCalc = Task.Run(() => result = IsPrimeNaive(number));
        var bySearchFirst = Task.Run(() => result = IsPrimeBySearch(number));
        Task.WaitAny(byPureCalc, bySearchFirst);
        return result;
    }

For the sake of simplicity, please assume that none of the called methods will consume any common resources, apart from processing power and memory (that is, there will never be two methods that both use file I/O or network I/O, etc).

Tasks can have results, you should take advantage of that, instead of assigning a local variable from a lambda. And WaitAny() returns the index of the Task that finished first. This means you can do something like:

public static bool IsPrimeHybrid(BigInteger number)
{
    var byPureCalc = Task.Run(() => IsPrimeNaive(number));
    var bySearchFirst = Task.Run(() => IsPrimeBySearch(number));
    var tasks = new[] { byPureCalc, bySearchFirst };
    int firstIndex = Task.WaitAny(tasks);
    return tasks[firstIndex].Result;
}

Also, CPU and memory are also resources that you shouldn't waste. So, you may want to cancel the slower Task, after the faster one finished. For that, you can use CancellationToken, which the two computations will periodically check (you can use ThrowIfCancellationRequested () for that):

public static bool IsPrimeHybrid(BigInteger number)
{
    var cts = new CancellationTokenSource();

    var byPureCalc = Task.Run(() => IsPrimeNaive(number, cts.Token));
    var bySearchFirst = Task.Run(() => IsPrimeBySearch(number, cts.Token));
    var tasks = new[] { byPureCalc, bySearchFirst };

    int firstIndex = Task.WaitAny(tasks);

    cts.Cancel();
    return tasks[firstIndex].Result;
}


Just require a file, or check if_readable first? I'm writing a simple front controller, index.php is the main file.
In my index.php, first thing loaded is bootstrap.php via require_once().

I'm deliberating over two ways to load bootstrap.php:

A)

require_once 'bootstrap.php';

B)

if ( is_readable('bootstrap.php') )
  {
    require_once 'bootstrap.php';
  }
else
  {
    exit( 'Sorry, we have a temporary issue. We are aware and it should hopefully be resolved soon. Please try again in 10 mins' );
  }

I see most well know frameworks (Codeigniter etc) just use A, which is neat and simple. If the file is not found, A provides:

A means for the script to halt via the require_once() returning a
fatal error
Nothing else graceful or useful

However I'm tempted to use B as if the file is not found, B will:

Provides a basic message to the screen, no white screen
Avoids a fatal error which A will produce
B seems more frameworky and robust, controlling what happens via the
application rather than just relying upon PHP which falls back on a
fatal error

This (A or B) is only used a couple of times, once to load  bootstrap.php and then twice within bootstrap.php to load SPL autoloader and error management (etc). Thus dealing with require() and any errors/failures throughout the rest of the system are handled more gracefully (with SPL and error management) and so neither A or B are needed.

However, I'm trying to make the entire system tight and well coded, rather than "it's only used a few times, meh that'll do".

I'm personally drawn towards using B, but I don't want opinions on which you would use for personal reasons, there will be some logical reasons or rationale I have not considered which I'd be grateful for you to throw at me.

Such as B has too much overhead (likely negligible but just an example), major frameworks use A because XYZ, etc.

Questions:

Is A flimsy and a bit unprofessional by just allowing code within a framework to use PHP fall back to a fatal error? When a framework should manage things in the best possible way.
Is A acceptable just allowing a fatal error?
Is B over the top, too much code FWIW, and not really necessary?
Is B much better because it removes a stinky fatal error, and because it controls how the application degrades rather than relying on a PHP fatal error to halt?

I don't recommend B:

A file being readable does not guarantee that it can be successfully included.  The specified path could be a directory (weird and unlikely, but possible), or it might point to a file with a syntax error.
is_readable() does not check the include_path the way require_once would.

Supposing that you could write a function to ensure that it is a regular file, and that it does not contain syntax errors, and checked all directories in the search path, you would have written a significant amount of code that reimplements PHP's built-in functionality.  The built-in functionality isn't broken to begin with.  If you say that a file is required, and PHP is unable to include it, that's a fatal error.  What is the point of prettifying a fatal error and suppressing the error code?

If the file to be loaded is not entirely essential, or if you want your script to commit suicide on its own terms, a better strategy might be to check the return value of include_once.


Check for valid parentheses nesting I found this question on leetcode and that was my answer ... But someone told me it's too bad that it takes \$O(n^3)\$ I am not sure that it takes all that time ... and trying to find better understandable Javascript solution.

  Given a string containing just the characters '(', ')', '{', '}', '['
  and ']', determine if the input string is valid. The brackets must
  close in the correct order, "()" and "()[]{}" are all valid but "(]"
  and "([)]" are not.

/**
 * @param {string} s
 * @return {boolean}
 */
var isValid = function(s) {
    while (s.length != 0 && s.includes("[]") || s.includes("()") || s.includes("{}")) {
        s = s.replace("[]", "");
        s = s.replace("()", "");
        s = s.replace("{}", "");
    }
    return s.length < 1
};

console.log(isValid("({(())})")); // True
console.log(isValid("({((}))})")); // False

Your friend's assertion that it's \$O(n^3)\$ is questionable. For that to be true, either or both of the replace or includes function needs to be \$O(n^2)\$ which is ... unlikely. It's more likely that both are \$O(mn)\$ complexity where m is the length of the source string, and n is the length of the search string ([] or {} or () all of which are "short")

Putting aside the complexity, it is still apparent that your code is not very efficient, even at \$O(n^2)\$.

An \$O(n)\$ solution is possible if you use a state machine and a stack (or recursion) to control the assertions. After any open parenthesis only 4 characters can follow, another open parenthesis ((, [, or {), or the matching close parenthesis. You can simplify the state machine by adding a dummy character, I'll use a . period to illustrate:

const terminator = ".",
    openers = "{[(",
    following = {
        "[": "]",
        "{": "}",
        "(": ")",
    };

/**
 * @param {string} s
 * @return {boolean}
 */
var isValid = function(s) {

    // add terminating character.
    s = s + terminator;

    // seed stack with
    const stack = [terminator];

    for (const c of s) {
        if (openers.includes(c)) {
            // going deeper in to nesting.
            stack.push(following[c]);
        } else {
            // coming out of nesting, check the correct closer.
            // (which may be a "." at the end)
            if (stack.pop() != c) {
                return false;
            }
        }
    }
    return true;

};

console.log(isValid("({(())})")); // True
console.log(isValid("({((}))})")); // False


Class library for processing credit cards I am developing a class library for processing credit cards. There is a thought in management in the short-term future to look for a new credit card processor. So I want to write once and use everywhere (I should be anyway, right?).

We are currently using First Data's Global Gateway Web Service API. I need a separate web reference setup for live and test.

I am trying to do the following:

//using ProcessCreditCard.WebReference;
using ProcessCreditCard.WebReferenceTest;

public class ProcessCreditCard
{
    private FDGGWSApiOrderService oFDGGWSApiOrderService = null;

    #region Intialize the object
    /// <summary>
    /// Initializes a new instance of the <see cref="ProcessCreditCard"/> class.
    /// </summary>
    public ProcessCreditCard()
    {
        ServicePointManager.Expect100Continue = false;
        // Initialize Service Object
        oFDGGWSApiOrderService = new FDGGWSApiOrderService();
        // Set the WSDL URL
        oFDGGWSApiOrderService.Url = @Properties.Settings.Default.CcApiUrl;
        // Configure Client Certificate
        oFDGGWSApiOrderService.ClientCertificates.Add(X509Certificate.CreateFromCertFile(Properties.Settings.Default.CertFile));
        // Set the Authentication Credentials
        NetworkCredential nc = new NetworkCredential(Properties.Settings.Default.CertUser, Properties.Settings.Default.CertPass);
        oFDGGWSApiOrderService.Credentials = nc;
    }

    /// <summary>
    /// Initializes a new instance of the test version of the <see cref="ProcessCreditCard"/> class.
    /// </summary>
    /// <param name="test">if set to <c>true</c> [test].</param>
    public ProcessCreditCard(bool test)
    {
        ServicePointManager.Expect100Continue = false;
        // Initialize Service Object
        oFDGGWSApiOrderService = new FDGGWSApiOrderService();
        // Set the WSDL URL
        oFDGGWSApiOrderService.Url = @Properties.Settings.Default.CcApiUrl_Test;
        // Configure Client Certificate
        oFDGGWSApiOrderService.ClientCertificates.Add(X509Certificate.CreateFromCertFile(Properties.Settings.Default.CertFile_Test));
        // Set the Authentication Credentials
        NetworkCredential nc = new NetworkCredential(Properties.Settings.Default.CertUser_Test, Properties.Settings.Default.CertPass_Test);
        oFDGGWSApiOrderService.Credentials = nc;
    }
    #endregion

    /// <summary>
    /// Charges the credit card.
    /// </summary>
    /// <returns></returns>
    public MktoResponse ChargeCreditCard()
    {
        /// Code here for setting up the transaction
        return BuildResponse(oFDGGWSApiOrderService.FDGGWSApiOrder(oOrderRequest));
    }

So in my calling code, I can create the right environment based on the boolean:

ProcessCreditCard.ProcessCreditCard pcc = new ProcessCreditCard.ProcessCreditCard(true);

How can I get this so that when I reference the oFDGGWSApiOrderService object that I am using the right environment?

First, I hope this question is still relevant in your company. One point that is of great interest to me in the question, is the management's vision to use other 3rd party payment processing providers. This directly implies that the code should accomodate this by abstracting the operation of processing orders and resolving dependecies on external systems.

I will give some simple suggestions intended to decouple the code from a specific implementation (therefore enhancing code re-use and testability)

Step I, Resolve dependency on 3rd party credit services

This is acheived by abstracting the functionalities exposed by the service (of course every service will have a different API, but the processing logic will follow more or less the same workflow):

public interface IPaymentGateWayService
{
    List<X509Certificate> ClientCertificates { get; }
    string Url { set; }
    NetworkCredential Credential { set; }
}

So now your Payment processor is agnostic to which service it is using as such:

class PaymentProessor
{
    private readonly IPaymentGateWayService _paymentGateWayService;

    public PaymentProessor(IPaymentGateWayService paymentGateWayService)
    {
        _paymentGateWayService = paymentGateWayService;
        _propertyProvider = propertyProvider;
    }

    public void ProcessCreditCardPayment()
    {
        _paymentGateWayService.Url = /*service url*/;
        _paymentGateWayService.ClientCertificates.Add(/*X509 certificate*/)
        _paymentGateWayService.Credential = new NetworkCredential(/*username and password*/);

    }
}

You then create a wrapper for your FDGGWApiOrderService that implements the IPaymentGateEayService interface:

class FDGGWS : IPaymentGateWayService
{
    private FDGGWSApiOrderService _fdggwsApiOrderService;

    public FDGGWS()
    {
        _fdggwsApiOrderService = new FDGGWSApiOrderService();
    }

    public List<X509Certificate> ClientCertificates
    {
        /*delegate to _fdggwsApiOrderService*/
    }

    public string Url
    {
        /*delegate to _fdggwsApiOrderService*/
    }

    public NetworkCredential Credential
    {
        /*delegate to _fdggwsApiOrderService*/
    }
}

So now when you create your payment processor, you pass the instance of the service you want to use.

new PaymentProessor(new FDGGWS());

This simply allows you to reuse your code to accomodate different service providers, also, you can support multiple providers at the same time by creating a service factory that returns the desired concrete instance of teh service depending on a configuration setting.

Step II, Handle environment settings

There are tons of techniques that allow you to read values from configuration stores, like App.config or a web.config. But for the purpose of my answer I will leverage the dependency injection technique and resolve the dependency on environment properties using constructor injection, similar to what is used to abstract the service interface.

So we create an interface for the configuration settings:

internal interface IPropertyProvider
{
    string Cert_File { get; }
    string Cer_User { get; }
    string Cer_Password { get; }
    string ServiceUrl { get; }
}

And you can create as many flavours of the interface as there are environments, in your case of for test and one for production, here is an abridged example of either:

class ProductionPropertyProvider : IPropertyProvider
{
    public string Cert_File
    {
        get { return "Path to Production file"; }
    }

    /* other properties */
}

class TestPropertyProvider : IPropertyProvider
{
    public string Cert_File
    {
        get { return "Path to test file"; }
    }

    /* other properties */
}

And here is how you perform the dependency injection:

class PaymentProessor
{
    private readonly IPaymentGateWayService _paymentGateWayService;
    private readonly IPropertyProvider _propertyProvider;

    public PaymentProessor()
        :this(new FDGGWS(), new ProductionPropertyProvider())
    {}
    public PaymentProessor(IPaymentGateWayService paymentGateWayService, IPropertyProvider propertyProvider)
    {
        _paymentGateWayService = paymentGateWayService;
        _propertyProvider = propertyProvider;
    }

    public void ProcessCreditCardPayment()
    {
        _paymentGateWayService.Url = _propertyProvider.ServiceUrl;
        _paymentGateWayService.ClientCertificates.Add(X509Certificate.CreateFromCertFile(_propertyProvider.Cert_File));
        _paymentGateWayService.Credential = new NetworkCredential(_propertyProvider.Cer_User, _propertyProvider.Cer_Password);

    }
}

Notice the default constructor that creates default service and property provider instances. This is just to demonstrate that you can specify the default dependencies in the default constructor

Lastly, all you need is one configuration key in your app.config that specifies what environment is it.. we use that key to determine the type of the IPropertyProvider.

Example

//Read env setting from the application configuration file
var environment = ConfigurationManager.AppSettings["env"];

//create appropriate PropertyProvider
IPropertyProvider propertyProvider;
if (environment == "test")
    propertyProvider = new TestPropertyProvider();
else
    propertyProvider = new ProductionPropertyProvider();

//process the payment
new PaymentProessor(new FDGGWS(), propertyProvider).ProcessCreditCardPayment();


Python exception handling class - Getting as much details printed I'm trying to write a Exception Handling class in Python so that I can re-use.  If you have any ideas on how I can improve this to output more detailed information I would appreciate it:

class EHandle:
    @staticmethod
    def printit():
        exc_type, exc_obj, exc_tb = sys.exc_info()
        fname = os.path.split(exc_tb.tb_frame.f_code.co_filename)[1]
        print(exc_type, fname, exc_tb.tb_lineno)
        frm = inspect.trace()[-1]
        mod = inspect.getmodule(frm[0])
        modname = mod.__name__ if mod else frm[1]
        print 'Thrown from', modname

And I call the class and method like below:

try:
    ggg()
    runafunction()
except:
    EHandle.printit()
    sys.exit()

Python's built-in exception traceback gives us:

Traceback (most recent call last):
  File "D:/code/python/error.py", line 43, in <module>
    error()
  File "D:/code/python/error.py", line 28, in error
    another_level()
  File "D:/code/python/error.py", line 31, in another_level
    return tuple()[0]
IndexError: tuple index out of range

Where your code gives us:

(<type 'exceptions.IndexError'>, 'error.py', 39)
Thrown from __main__

So yours is sparse from the go.
But what makes it bad is line 39 is where I call error().
It's not where the error happens, that's line 31.
It also doesn't give us the path to the file, if I have multiple files with the same name then your program is going to cause me some serious headaches.
And finally the error doesn't throw in __main__ it throws in another_level.

If you are ok with printing the exact same as Python then, as zondo said, you can just change your code to:

def printit():
    traceback.print_exc()

As you probably want to format it yourself, and be able to change the data. You should use sys.exc_info() and traceback.extract_tb(exc_traceback).
And so if you wanted to implement print_exc yourself you can use:

def print_exc():
    exc_type, exc_value, exc_traceback = sys.exc_info()
    print 'Traceback (most recent call last):'
    print '\n'.join([
        '  File "{}", line {}, in {}\n    {}'.format(*level)
        for level in traceback.extract_tb(exc_traceback)])
    print '{}: {}\n'.format(exc_type.__name__, exc_value)

This allows you to format the error however you want.


Macro to generate an enum and an array of strings Often when I find myself working with magic number enum values and I want to know what they represent, so I create an array of strings in order to print out their label.

This macro automates that process.

//#define DEBUG
#define GENERATE_ENUM(ENUM) ENUM,
#define GENERATE_STRING(STRING) #STRING,
#define GENERATE_ENUM_LIST(MACRO, NAME) \
   enum NAME                            \
   {                                    \
      MACRO(GENERATE_ENUM)              \
   };

//#ifdef DEBUG
#define GENERATE_ENUM_STRING_NAMES(MACRO, NAME) \
   const char *NAME##_Strings[] = {             \
       MACRO(GENERATE_STRING)                   \
   };

//#else
//#define GENERATE_ENUM_STRING_NAMES(MACRO, NAME)
//#endif

To use do:

#include <stdio.h>

/* ~ The macro ~ */

#define nuclearLaunchCodes(T) \
  T(ONE) \
  T(TWO) \
  T(THREE)

GENERATE_ENUM_STRING_NAMES(nuclearLaunchCodes, nuclearLaunchCodesData)
#undef nuclearLaunchCodes

int main() {
  printf("%s\n", nuclearLaunchCodesData_Strings[0]);
}

Most macro solutions are not readable nor maintainable. You should avoid macro solutions like this.

The best way is to not use macros at all:

typedef enum
{
  ZERO,
  ONE,
  TWO,
  THREE,
  NUMBERS_N
} numbers_t;

static const char* NUMBER_STR[NUMBERS_N] =
{
  [ZERO]  = "ZERO",
  [ONE]   = "ONE",
  [TWO]   = "TWO",
  [THREE] = "THREE",
};

puts(NUMBER_STR[1]); // prints ONE

This code is perfectly readable and it maintains the integrity between the enum and the array well. It only has one small problem and that is code repetition.

Code repetition should be avoided, but programmers tend to exaggerate how bad it is. It is rarely ever so bad that it justifies turning your whole program into "macro hell".

The reasons why code repetition should be avoided is that is leads to typo-like bugs and problems with maintenance. However, while a "macro hell" solution might rule out the typos, it makes the code difficult to maintain and the increased complexity increases the chance of other more serious bugs.

Famous quote by Brian Kernighan:

  Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?

That being said, there are valid cases where you must centralize data in one place, particularly when maintaining existing code that shouldn't be changed more than necessary. The macro you posted is a flavour of "X macros", which is the preferable way to write such macros - "messy in a standardized way" so to speak. X macros is about defining a list of data as you do, but to pass on that data to localized macros embedded into the code itself. To rewrite the above code with X macros, you'd do:

#define NUMBER_LIST \
  X(ZERO)           \
  X(ONE)            \
  X(TWO)            \
  X(THREE)          \

typedef enum
{
  #define X(name) name,
    NUMBER_LIST
  #undef X
  NUMBERS_N
} numbers_t;

static const char* NUMBER_STR[NUMBERS_N] =
{
  #define X(name) [name] = #name,
    NUMBER_LIST
  #undef X
};

puts(NUMBER_STR[1]); // prints ONE

The advantage here is the flexibility to apply the same data in different ways, on case-by-case basis. A macro such as #define X(name) [name] = #name, is cryptic by itself, but when given the context of the surrounding array initializer list, one can easier understand the meaning. "X macros" can also be used to manually unroll loops:

#define X(name) puts(NUMBER_STR[name]);
  NUMBER_LIST
#undef X

This is equivalent to iterating over the array and printing all items, but the loop is unrolled and we end up with a number of puts calls.


Is this a good/safe way to convert enums? Due to the fact that Entity does not support Enums, I am converting them to strings, string lists, and back again.

The reason is two fold:
type-safety, and consistency in the database.

Now, I have a bunch of these, but I will use Credit Cards as an example:

    public enum CreditCardName
    {
        VISA,
        Master_Card,
        Discover,
        American_Express
    }

Originally, I decided on writing the same method for them, over and over again:

    public static List<string> GetCreditCardTypes()
    {
        var list = Enum.GetNames(typeof(CreditCardName));
        return list.Select(n => n.Replace("_", " ")).ToList();
    }

Then it occurred to me that I could make one private method to do this:

    public static List<string> GetCreditCardTypes()
    {
        return EnumToList(typeof(CreditCardName));
    }

    private static List<string> EnumToList(Type type)
    {
        var list = Enum.GetNames(type);
        return list.Select(n => n.Replace("_", " ")).ToList();
    }

Now I'm also doing something similar in reverse, but will skip the code.  The question I have is if this is a good way to go about this?  I know there are other workarounds for Entity's lack of enum support, but they are a bit cumbersome for my simple needs.  I have never used Types in this way, so I just wanted to check that this is how Type/typeof are intended to be used, and that this will not cause unforeseen runtime issues.

I solved a similar issue by inventing a DisplayName attribute, so I can define my enums like this:

public enum CreditCardName
{
    [DisplayName("VISA")]
    Visa,

    [DisplayName("MasterCard")]
    MasterCard,

    [DisplayName("Discover")]
    Discover,

    [DisplayName("American Express")]
    AmericanExpress
}

This has a number of advantages. For one, apps like Resharper will stop complaining about your naming scheme. Also, your string doesn't have to actually correspond to an English name, it could correspond to an entry in a translation strings file or something like that.

Source for DisplayName:

[AttributeUsage(AttributeTargets.Field)]
public class DisplayName : Attribute
{
    public string Title { get; private set; }

    public DisplayName(string title)
    {
        this.Title = title;
    }
}

Source for the method to retrieve your DisplayName objects

        IEnumerable<DisplayName> attributes = from attribute in typeof(CreditCardName).GetCustomAttributes(true)
                                              let displayName = attribute as DisplayName
                                              where displayName != null
                                              select displayName;

You can roll this into a more complete generic method too:

public string[] GetNames<TEnum>()
{
    return (from attribute in typeof(CreditCardName).GetCustomAttributes(true)
                                                     let displayName = attribute as DisplayName
                                                     where displayName != null
                                                     select displayName.Title).ToArray();
}

Unfortuately, I there isn't a way you can enforce that TEnum is an enum at compile time, but you could make it throw an exception instead.