A small printf-like function using variadic templates I'm looking to get some feedback on a small printf-like function I wrote that provides simplified behavior similar to boost::format:

#ifndef EXT_FORMAT_HPP__
#define EXT_FORMAT_HPP__
// ext::format
// Implements a variadic printf-like function providing behavior similar to
// boost::format
#include <regex>
#include <sstream>
#include <string>
#include <utility>

namespace {

inline std::string format_helper(
    const std::string &string_to_update,
    const size_t) {
  return string_to_update;
}

template <typename T, typename... Args>
inline std::string format_helper(
    const std::string &string_to_update,
    const size_t index_to_replace,
    T &&val,
    Args &&...args) {
  std::regex pattern{"%" + std::to_string(index_to_replace)};
  std::string replacement_string{(std::ostringstream{} << val).str()};
  return format_helper(
      std::regex_replace(string_to_update, pattern, replacement_string),
      index_to_replace + 1,
      std::forward<Args>(args)...);
}

} // namespace

namespace ext {

template <typename... Args>
inline std::string format(const std::string &format_string, Args &&...args) {
  return format_helper(format_string, 1, std::forward<Args>(args)...);
}

} // namespace ext

#endif

Usage example:

#include "format.hpp"
#include <iostream>

struct foo {
  int value;
  foo(int val) : value{val} {
  }
};

std::ostream &operator<<(std::ostream &os, const foo &f) {
  os << "foo(" << f.value << ")";
  return os;
}

int main() {
  double tmp = 37.382;
  std::cout << ext::format("%1 + %2 * %1 = %3", 5, tmp, 5 + tmp * 5) << std::endl;

  // Support user defined types provided the appropriate operator<< overload is defined.
  foo a_foo(55);
  std::cout << ext::format("Here is a foo constructed with 55: %1", a_foo) << std::endl;
};

This is not written to replace boost::format; it's intended to be used as a header-only include with a very limited scope. As currently written, it is possible to supply too few or too many arguments to the parameter pack. Passing too few arguments will return a string that still has format specifiers. Passing excess arguments are simply ignored and you pay the price of excess calls to std::regex_replace, which will do nothing. With that in mind, I'm looking for any and all feedback.

I find this to be a sweet demo, but I don't like the traps in it for the unwary. Like you mention, there is little validation of parameter counts. There is also a chance that the same location can be replaced multiple times, e.g. with a call to ext::format("%1", "%2", "%3, "%4", "hi!") resulting in "hi!". Perhaps you might consider that a bonus.

What I find most surprising in the code is this:

std::string replacement_string{(std::ostringstream{} << val).str()};

I would probably have tended towards auto replacement_string{std::to_string(val)}; or, more likely, just embedded that in the call to regex_replace. Do you do this for compatibility with existing code that provides an overload for operator<< but not to_string?

Finally, I'm torn about the use of regex. It seems like a pretty big hammer without a lot of need, here, however it does keep the code short and sweet, and implicitly avoids an infinite loop that a flawed implementation might have with ext::format("%1", "%1").

Reviewing the tests, I find the Does_Not_Alter_Format_String test to be surprising as well. I would have thought the const qualifier would indicate this well enough.


Improving a filter array function with Set and objects, to match elements and conditions I've posted a similar question on: Link
With the help of the other users, I was able to improve the code, but I don't think is the "ultimate" form.
Thats my code now, any help?
Conditions entries example:
[1,2],
[9,6,4],
[3]
[7,1,5,2,3]

private filterElements(userSelectedConditions: Set<number>): Element[]{
const meetConditions: Element[] = [];

this.arrayWithAllElements.forEach(element => {

  let isPossibility: boolean = true;

  for (let ev of userSelectedConditions ) {
    if( !element.condition_matrix.includes( ev ) ){
      isPossibility = false;
      break;
    }
  }

  if(isPossibility){
    meetConditions.push(element);
  }
});

if(meetConditions.length === 0){
  meetConditions.push( { <a_filler_object> } );
}

return meetConditions;
}

The user select some conditions, and this code has to iterate on the full elements list, filter the elements that meets all the conditions passes. So if the user selects ONE condition, every element with that condition should be filtered. IF the user selects THREE conditions, every element that meet those conditions should be filtered, if they don't match ALL the selected conditions, they should be out of the list.
this.selectedConditions is already maped by ID
Elements list example:
elements: [
    {
        name: 'el1',
        condition_matrix: [1,2]
    },
    {
        name: 'el2',
        condition_matrix: [2,3]
    },
    {
        name: 'el3',
        condition_matrix: [2,1]
    }
]

Conditions list example:
conditions: [
    {
        id: 1
        name: 'cond1'
    },
    {
        id: 2
        name: 'cond2'
    },
    {
        id: 3
        name: 'cond3'
    }
]

The Array methods filter() and every() come in very handy here. First you could define a function to check wheter a given element matches all conditions:
function meetsAll(element, conditions) {
    return conditions.every(c => element.condition_matrix.includes(c));
}

Now it's almost trivial to filter out the matching elements using Array.filter():
const meetConditions = this.arrayWithAllElements.filter(
    element => meetsAll(element, userSelectedConditions)
);

I recommend you also have a look at the other functional methods defined on Array.


Hi-Lo guessing game with numbers I am new to Java in general, but wanted to get some input on the code for a CS class I am taking.  It is a program that plays the Hi-Lo guessing game with numbers. A user may choose to quit mid-game by entering 0 and then they are prompted to play again if they choose.  If they guess correctly, they are also prompted to play again.

The code works fine, however, my original direction was to use the while loop as the main conditional instead of the do statement but I couldn't get the game to start again after the user entered 0 to exit the loop. I'm curious to know how I can use the while loop as the main conditional vs the do statement.

import java.util.Scanner;
import java.util.Random;

public class HiLo {
    public static void main(String[] args)
    {
        int guess, answer, count = 0;
        String restart;

        Random generator = new Random();
        Scanner scan = new Scanner(System.in);

        System.out.print("What number do you guess (enter 0 to quit)? ");
        guess = scan.nextInt();

        answer = generator.nextInt(100) + 1; // randomly generates a number between 1-100

        do // loops through game until user gets answer correct or quits
        {
            count++;

            if (guess == 0) // if user enters 0 to quit game, this message is displayed
            {
                System.out.println("\nQuitting game");
                System.out.println("\nWould you like to play again?");
                restart = scan.next();
                if (restart.equalsIgnoreCase("Y"))
                {
                    System.out.print("Enter a number to start playing");
                    guess = scan.nextInt();
                }
                else
                    break;
            }
            if (guess == answer)
            {
                System.out.println("That's correct! You made " + count + " guesses.");
                System.out.println("Would you like to play again?");
                restart = scan.next();
                if (restart.equalsIgnoreCase("Y"))
                {
                    System.out.print("Enter a number to start playing");
                    guess = scan.nextInt();
                }
                else
                    break;
            }
            if (guess < answer)
            {
                System.out.print("Your guess is too low, try again.");
                guess = scan.nextInt();
            }
            if (guess > answer)
            {
                System.out.print("Your guess is too high, try again.");
                guess = scan.nextInt();
            }

        }
        while (guess != 0);

        System.out.print("Thanks for playing!");
    }
}

Don't repeat yourself

You twice say

                System.out.println("\nWould you like to play again?");
                restart = scan.next();
                if (restart.equalsIgnoreCase("Y"))
                {
                    System.out.print("Enter a number to start playing");
                    guess = scan.nextInt();
                }
                else
                    break;

And five times say

                    guess = scan.nextInt();

Including twice in the previous repeated code.  Don't do that.  Instead organize the code so that you say these things once each.

Consider the following

        String restart = "Y";
        while (restart.equalsIgnoreCase("Y")) {
            int answer = generator.nextInt(100) + 1;

            System.out.print("What number do you guess (enter 0 to quit)? ");
            while (true) {
                int guess = scan.nextInt();
                if (guess == 0) {
                    System.out.println("\nQuitting game");
                    break;
                }

                count++;
                if (guess == answer) {
                    System.out.println("That's correct! You made " + count + " guesses.");
                    break;
                }

                if (guess < answer) {
                    System.out.println("Your guess is too low, try again.");
                } else {
                    System.out.println("Your guess is too high, try again.");
                }
            }

            System.out.println("\nWould you like to play again (Y/N)?");
            restart = scan.next();
        }

This won't necessarily work any better.  But this pattern will generally be easier to maintain.  One of the problems with academic code is its low reuse.  You never have to maintain it, so it's easy to slip into unmaintainable habits.  Outside the academic context, most of your time will be spent on maintaining existing code.

I used a while for the outer loop even though a do/while would have been more natural (since you always loop once).  Notice though that it is gated by restart rather than guess.  I find this to be a cheesy solution and would prefer the do/while.

The inner loop will loop forever until explicitly exited (with the break statements).  Notice how the first thing that it does is collect the next guess.  This saves collecting it in the five different places.  Collect it once and then process it once.

When processing guess, you don't need to do three comparisons.  There are only three states.  If it's not equal (otherwise we break out of the loop) and not less than, it must be greater than.  So use an else rather than the third comparison.

We could also use an else if after guess == answer, but we don't need to do so since we break.

I added a (Y/N) to your play again query.  As is, it works if you know how.  But someone who doesn't know what to do might type "Yes." which seems like a reasonable response but won't work.  Give a little extra guidance as to what to type there.  You also might consider accepting "Yes" as a positive answer.

The Java standard is to put { on the same line as a method declaration or control structure, so I switched to that.  It's more important to be consist though.  So the other way is OK.  It's just not how I normally write Java.

Bugs

You don't manage the count correctly.  I fixed a small mistake for you, but I left the bigger one.  The small mistake was that you incremented the count even if the guess was 0, which is not a real guess but the exit criterion.  Without the larger mistake, this doesn't really matter.  You don't display the incorrect count anyway.

The larger mistake comes when you play repeated games.  To my mind, the correct way to fix this would be to move the count declaration and initialization to a different location.  Note that in Java, variable declarations do not need to be at the beginning of a block.  They can appear anywhere.  Figure out what's wrong with the current behavior (win repeated games until it becomes obvious).  Then think about where it should be.

Use methods

I would prefer to move generator and scan into class fields.  This means inside the class but outside any method.

    private static final Random GENERATOR = new Random();
    private static final Scanner SCAN = new Scanner(System.in);

That would allow them to be shared across multiple methods.

I would expect main to look more like

    public static void main(String[] args) {
        String restart;
        do {
            play();

            System.out.println("Would you like to play again (Y/N)?");
            restart = SCAN.next();
        } while (restart.equalsIgnoreCase("Y"));
    }

Perhaps you haven't gotten to methods yet.  When you do, consider going back to this and writing a play method.


Acquiring and formatting data from a JSON file I have a JavaScript function that is getting data out of a JSON file and then doing some formatting to it.  The code works but I feel like it just an awful way of doing it.  All criticism and help appreciated.

function MCScrap(ZWA, ZWB, ZWC, ZWD, Number) {

$(document).ready(
        function() {
            setInterval(function() {

                $.getJSON("mccount.json", function(machines) {
                    console.log(machines);
                    var Tcount = machines.Machines[Number].Tcount;
                    var Ycount = machines.Machines[Number].Ycount;
                    var TScount = machines.Machines[Number].TScount;
                    var YScount = machines.Machines[Number].YScount;
                    document.getElementById(ZWA).innerHTML = Ycount;
                    document.getElementById(ZWB).innerHTML = Tcount;
                    document.getElementById(ZWD).innerHTML = TScount + "%";
                    document.getElementById(ZWC).innerHTML = YScount + "%";
                    if (TScount < 3) {
                        document.getElementById(ZWB).style.backgroundColor = 'lime';
                    } else if (TScount < 5) {
                        document.getElementById(ZWB).style.backgroundColor = 'orange';
                    } else {
                        document.getElementById(ZWB).style.backgroundColor = 'red';
                    }

                    if (YScount < 3) {
                        document.getElementById(ZWA).style.backgroundColor = 'lime';
                    } else if (YScount < 5) {
                        document.getElementById(ZWA).style.backgroundColor = 'orange';
                    } else {
                        document.getElementById(ZWA).style.backgroundColor = 'red';
                    }
                }
                );

            }, 3000);
        });
}

I'd suggestion cleaning it up like this:

function MCScrap(ZWA, ZWB, ZWC, ZWD, Number) {
    $(document).ready(function () {
        setInterval(function () {
            $.getJSON("mccount.json", function (machines) {
                var zwa, zwb, zwc, zwd, TScount, YScount;

                zwa = document.getElementById(ZWA);
                zwb = document.getElementById(ZWB);
                zwc = document.getElementById(ZWC);
                zwd = document.getElementById(ZWD);

                zwa.innerHTML = machines.Machines[Number].Ycount;
                zwb.innerHTML = machines.Machines[Number].Tcount;

                YScount = machines.Machines[Number].YScount;
                zwc.innerHTML = YScount + "%";
                TScount = machines.Machines[Number].TScount;
                zwd.innerHTML = TScount + "%";

                function getColor(count) {
                    if (count < 3) {
                        color = 'lime';
                    } else if (count < 5) {
                        color = 'orange';
                    } else {
                        color = 'red';
                    }
                    return color;
                }

                zwa.style.backgroundColor = getColor(YScount);
                zwb.style.backgroundColor = getColor(TScount);

            });

        }, 3000);
    });
}

Types of changes:

Get a DOM element once into a variable and then use it from there rather than calling document.getElementById() more than one for the same DOM element.
Put the color code into a reusable function rather than having two copies which also makes it easier to set the color in just one place rather than three places.
Put zwa, zwb, zwc, zwd blocks of code in order to make it easier to read and follow.

Some other things that seem a little off to me:

Putting $(document).ready() inside another function like this seems a little odd.  Usually, you would call MCScrap() inside a $(document).ready() as that would make your function a bit more reusable.
Setting up an ajax call to run every 3 seconds in perpetuity is generally not a good idea.  For starters, it's horrible for battery life (phone, tablet, laptop).  And, it can get messy if you every get in a situation where the ajax call takes longer than 3 seconds to finish or timeout.  Then, you have multiple ajax calls in flight at the same time and they could be building up.  The safer way to do this is to launch the next ajax call from a setTimeout() that is set in the completion of the previous ajax call.  Then, they can never build up.


Printing a week range string in Python For some reporting, I need to print a human readable date string for the week covered by the report, using just the standard python module library.  So the strings should look like:

  Dec 29, 2013 - Jan 4, 2014

  Jan 26 - Feb 1, 2014

  Jan 19 - 25, 2014

Below is a script I threw together to achieve this.  But is there a simpler way to do this?

from datetime import (datetime, date, timedelta)

def week_string(dt):
    (year, week, weekday) = dt.isocalendar()
    week_0 = dt - timedelta(days=weekday)
    week_1 = dt + timedelta(days=(6-weekday))

    month_0 = week_0.strftime("%b")
    day_0 = week_0.strftime("%e").strip()
    year_0 = week_0.strftime("%Y")

    month_1 = week_1.strftime("%b")
    day_1 = week_1.strftime("%e").strip()
    year_1 = week_1.strftime("%Y")

    if year_0 != year_1:
        return "%s %s, %s - %s %s, %s" %(
            month_0, day_0, year_0,
            month_1, day_1, year_1)
    elif month_0 != month_1:
        return "%s %s - %s %s, %s" %(
            month_0, day_0,
            month_1, day_1, year_1)
    else:
        return "%s %s - %s, %s" %(
            month_0, day_0, day_1, year_1)

print week_string(date(2013, 12, 30))
print week_string(date(2014, 01, 30))
print week_string(datetime.date(datetime.now()))

Since the report script is going to be shared with other people, I want to avoid adding dependencies on anything they'd need to install.

You have a bug: if dt is a Sunday, then it outputs the date range starting from the previous Sunday.  That is because datetime.isocalendar() represents the day of week as a number from 1 (= Monday) to 7 (= Sunday), but you want to subtract 0 to 6 days.

>>> print week_string(date(2014, 01, 19))
Jan 12 - 18, 2014

My recommendation:

def week_string(dt):
    # Use underscore to indicate disinterest in the year and week
    _, _, weekday = dt.isocalendar()
    week_0 = dt - timedelta(days=weekday % 7)  # Sunday
    week_1 = week_0 + timedelta(days=6)        # Saturday

Then, I would write the following six lines more compactly:

    day_0, month_0, year_0 = week_0.strftime('%e-%b-%Y').lstrip().split('-')
    day_1, month_1, year_1 = week_1.strftime('%e-%b-%Y').lstrip().split('-')

The rest of it seems fine.


Unit test for a function that adds users I'm new to unit testing and I'm curious if anyone can see any problems with my approach below.  Basically I want to test the Add function.  To do this I am creating a new Account object, passing that to the Add function, retrieving that account and finally comparing the retrieved objects values to hard coded values.

I'm wondering if anyone can see any thing that would not be considered best practice for unit tests?  Specifically, should I be using the testUser object in my asserts rather than the hard coded values?  Anything else I should consider?

[TestMethod]
public void TestAddUser()
{
    AccountAccess accountActions = new AccountAccess();
    Account testUser = new Account("abe",1000m);
    accountActions.Add(testUser);
    Account retrievedUser = accountActions.GetAccount("abe");
    Assert.AreEqual("abe", retrievedUser.name, "Added user does not have corret name: 'abe' - '" + retrievedUser.name + "'");
    Assert.AreEqual(1000m, retrievedUser.limit, "Added user does not have corret limit: '1000' - '" + retrievedUser.limit + "'");
}

A unit test is split in 3 steps. Arrange-Act-Assert. Usually, to make it clear what is being tested, you write as a comment in which step you're at.

[TestMethod]
public void TestAddUser()
{
    //Arrange
    AccountAccess accountActions = new AccountAccess();
    Account testUser = new Account("abe",1000m);

    //Act
    accountActions.Add(testUser);

    //Assert
    Account retrievedUser = accountActions.GetAccount("abe");
    Assert.AreEqual("abe", retrievedUser.name, "Added user does not have corret name: 'abe' - '" + retrievedUser.name + "'");
    Assert.AreEqual(1000m, retrievedUser.limit, "Added user does not have corret limit: '1000' - '" + retrievedUser.limit + "'");
}

Now, to make your test as good as possible, it needs to be as small as possible but still crystal clear about what you're testing.

What are your Assert testing? If the name and limits are equals.
What do you want your `Assert to test? That the same account that was added is returned. That both accounts are equal.

It's not a big difference, but what if one day the equality of an Account is based on an Id? You might need to refactor this test because the assertions would be false. Ex : If I add twice the same Account with the same name and limit, which is supposed to be the one that is returned?

What I mean is, your test should look like this :

[TestMethod]
public void AddUser()
{
    //Arrange
    AccountAccess accountActions = new AccountAccess();
    Account expected = new Account("abe",1000m);

    //Act
    accountActions.Add(new Account("abe",1000m));

    //Assert
    Account actual = accountActions.GetAccount("abe");
    Assert.AreEqual(expected, actual);
}

Now, this test might fail, if so, it's because your Account class doesn't override the Equal and GetHashCode methods. It probably should as they both  define the identity of your object.

Let's get back to my last example. What is different from your test?

Expected/Actual : This way, I know what are the expected results (I have a variable for that) and what was actually returned (I also have a variable for that!)
Only one assertion. It's a best practice, you can't always achieve one assertion, but that should be your goal. This way, it's easy for anyone to understand what your test... tests. It's now very clear I'm testing that it's the same account that is returned, because internally Assert.AreEqual will use the Equal property of your class. And it seems reasonable that the Account class should know if it's equal to another much better than your unit test.

Now, if you don't want to override the equals, there's another solution for you. Use of constants

Ex :

[TestMethod]
public void AddUser()
{
    //Arrange
    const string accountName = "abe";
    const decimal accountLimit = 1000m;
    AccountAccess accountActions = new AccountAccess();
    Account testUser = new Account(accountName,accountLimit);

    //Act
    accountActions.Add(testUser);

    //Assert
    Account retrievedUser = accountActions.GetAccount(accountName);
    Assert.AreEqual(accountName, retrievedUser.name);
    Assert.AreEqual(accountLimit, retrievedUser.limit);
}

This way, there's no confusion about "abe" being everywhere, heck, I don't even need to know what is inside the accountName variable, I just need to know it is equals to the retrieved one because I used it to retrieve the account. Actually, the use of constants should be heavily considered in my first example too!!

Note : The string messages, aren't necessary. The default assertion message for Assert.AreEqual will be very clear that the actual value isn't equal to the expected one. And your test name shouldn't include Test. You know it's a test, there's the attribute juste above :) (You should take a look at @Konrad's comment about naming!)


Disproving Euler proposition by brute force in C I wrote a little bit of code to brute force disprove the Euler proposition that states:

  $$a^4 + b^4 + c^4 = d^4$$

  has no solution when \$a\$, \$b\$, \$c\$, \$d\$ are positive integers.

I'm no mathematician, but reading around this, at least one solution was found by Noam Elkies in 1987 (a = 95800; b = 217519; c = 414560; d = 422481). I wanted to get an idea of how much firepower it would take to solve by brute force, so I wrote the following in C:

#include <stdio.h>
#include <time.h>
#include <math.h>

int prop(long int A, long int B, long int C, long int D) {
    return (pow(A, 4) + pow(B, 4) + pow(C, 4) == pow(D, 4));
}

int main() {
    long int a, b, c, d;
    clock_t t;
    t = clock();

    for (a = 1; a < 100000; a++) {
        for (b = 1; b < 300000; b++) {
            for (c = 1; c < 500000; c++) {
                for (d = 1; d < 500000; d++) {
                    if (prop(a, b, c, d))
                        printf("FOUND IT!\na = %ld\nb = %ld\nc = %ld\nd = %ld\n", a, b, c, d);
                }
                if (!(c%1000))
                    printf("a = %ld, b = %ld, c = %ld, time = %fs\n", a, b, c, ((double)(clock() - t))/CLOCKS_PER_SEC);
            }
            printf("a = %ld, b = %ld, time = %fs\n", a, b, ((double)(clock() - t))/CLOCKS_PER_SEC);
        }
        printf("a = %ld, time = %fs\n", a, ((double)(clock() - t))/CLOCKS_PER_SEC);
    }

}

I ran it for a while, and worked out that it would take roughly \$85 \times 10^6\$ years for it to get to the answer above on my current machine.

I mean, maybe if I waited a few thousand years, I'd have a slightly better machine, but my question is (and I am new to C and computer science in general - so please be gentle); what strategies could I take to make the above code run faster? I thought about threading, and using some sort of bit shifting in place of the pow() calls.

Would there be any way (on my current machine) to get this to run in my lifetime?

Note: At some point, this review drifted into the realm of assembler and GMP. An actual review is at the end of this post, whereas the first section discusses the runtime-problems concerning pow, wrong data types and arbitrary large integers.

No life time for run time

  Would there be any way (on my current machine) to get this to run in my lifetime?

There's a great saying in carpentry: measure twice, cut once. It concerns cutting wood or other material, where you have to throw away your resources if you accidentally cut at the wrong place.

A similar saying is there for software engineers: you can't optimize what you can't measure. There are several ways to measure your code, e.g. benchmarking, profiling, or looking at the generated assembler to see how many instructions a certain part of your code will take.

Here, we will take the latter route, start with the assembler, take considerations step by step and see where we end up.

A study in assembly

Lets have a look at your code. Well, not yours, but the assembler the compiler generates. You can use gcc -S -O3. On my platform, this results in the following "hot" section in main:

.L6:
        add     rbx, 1
        cmp     rbx, 500000
        je      .L18
.L8:
        mov     rax, QWORD PTR .LC0[rip]
        movsd   xmm0, QWORD PTR [rsp+40]
        movq    xmm1, rax
        call    pow                        ; (1)
        mov     rax, QWORD PTR .LC0[rip]
        movsd   QWORD PTR [rsp+8], xmm0
        movsd   xmm0, QWORD PTR [rsp+48]
        movq    xmm1, rax
        call    pow                        ; (2)
        mov     rax, QWORD PTR .LC0[rip]
        movsd   QWORD PTR [rsp+16], xmm0
        movsd   xmm0, QWORD PTR [rsp+32]
        movq    xmm1, rax
        call    pow                        ; (3)
        mov     rax, QWORD PTR .LC0[rip]
        movsd   QWORD PTR [rsp+24], xmm0
        pxor    xmm0, xmm0
        cvtsi2sd        xmm0, rbx
        movq    xmm1, rax
        call    pow                        ; (4)
        movsd   xmm2, QWORD PTR [rsp+8]
        addsd   xmm2, QWORD PTR [rsp+16]
        movapd  xmm1, xmm0
        movsd   xmm0, QWORD PTR [rsp+24]
        addsd   xmm0, xmm2
        ucomisd xmm0, xmm1
        jp      .L6
        jne     .L6

Even though you might not know assembler, you can see those four calls to pow. The first thing you need to know is that call is slow compared to those other operations. Those four calls happen in the innermost loop. The compiler removed the call to prop and instead replaced it by its code (that's faster).

mov* assigns values to registers, add* adds values, and so on. The registers with xmm* are double precision registers, meant for double variables. So we're basically calling pow with the right values and then add, subtract and modify our small little double values.

Double trouble

But wait a second. We're trying to solve a completely integral problem! Why does our generated program use those registers at all?

This should raise a red flag. And indeed, if we remember pow's signature, it should be clear that it's not the right tool. It takes a double base and exponent, which indicates that it's suitable for terms like \$15.12151^{3.1415926}\$. This is a total overkill for your problem.

Using proper functions

So let's use another pow version instead:

long int pow4(long int x){
    return x * x * x * x;
}

Note that your compiler should create something like this from that:

movq    %rdi, %rax
imulq   %rdi, %rax
imulq   %rax, %rax
ret

but if your compiler doesn't recognize this potential (micro) optimization, you can use

long int pow4(long int x){
    const long int t = x * x;
    return t * t;
}

instead.

We also need to change prop:

int prop(long int A, long int B, long int C, long int D) {
  return (pow4(A) + pow4(B) + pow4(C) == pow4(D));
}

Allright. Now, before I show the times of the new program, let's check the output of your old one:

a = 1, b = 1, c = 1000, time = 114.156000s

That's when I hit ^C. How does the one using pow4 hold up?

a = 1, b = 1, c = 1000, time = 0.296000s
a = 1, b = 1, c = 2000, time = 0.578000s
a = 1, b = 1, c = 3000, time = 0.859000s
a = 1, b = 1, c = 4000, time = 1.140000s
a = 1, b = 1, c = 5000, time = 1.421000s
a = 1, b = 1, c = 6000, time = 1.703000s
a = 1, b = 1, c = 7000, time = 1.984000s
a = 1, b = 1, c = 8000, time = 2.265000s
a = 1, b = 1, c = 9000, time = 2.546000s
a = 1, b = 1, c = 10000, time = 2.828000s
a = 1, b = 1, c = 11000, time = 3.109000s
a = 1, b = 1, c = 12000, time = 3.390000s
a = 1, b = 1, c = 13000, time = 3.687000s
a = 1, b = 1, c = 14000, time = 3.968000s
a = 1, b = 1, c = 15000, time = 4.250000s
a = 1, b = 1, c = 16000, time = 4.531000s

Which is 0,2% of your original time, or a 500x speedup.

However, this comes at a cost: pow4(500000) is too large for a int64_t, since \$\log_2(500000^4) \approx 76\$. The greatest number you could check with a uint64_t is 65535, \$2^{16}-1\$, which shouldn't be very surprising. As the standard does not provide int128_t or similar, you should make sure that your numbers don't exceed those bounds.

You can either write your own large integer logic for this, or use GMP.

Proper bounds and parameter estimation

Next up, you can increase the lower bounds of b and c, so that \$a \le b \le c\$. And for d, well, if we have a, b, c, then there is only one solution for d. We can directly search for that solution with binary search.

The binary search makes a \$\mathcal O (n^3 \log n)\$ algorithm from your current \$\mathcal O (n^4)\$ one, which should provide a lot more speed than the previous speedup.

Even better, if you used the appropriate bounds for a, b and c, we can bound d by

$$d^4 = a^4 + b^4 + c^4 \le c^4 + c^4 + c^4 = 3c^4$$

and therefore get

$$c \le d \le \sqrt[4]{3}\,c.$$

With the proper binary algorithm,you can finish the first a=1,b=1 case quickly:

…
a = 1, b = 1, c = 481000, time = 0.031000s
a = 1, b = 1, c = 482000, time = 0.031000s
a = 1, b = 1, c = 483000, time = 0.031000s
a = 1, b = 1, c = 484000, time = 0.031000s
a = 1, b = 1, c = 485000, time = 0.031000s
a = 1, b = 1, c = 486000, time = 0.031000s
a = 1, b = 1, c = 487000, time = 0.031000s
a = 1, b = 1, c = 488000, time = 0.031000s
a = 1, b = 1, c = 489000, time = 0.031000s
a = 1, b = 1, c = 490000, time = 0.031000s
a = 1, b = 1, c = 491000, time = 0.031000s
a = 1, b = 1, c = 492000, time = 0.031000s
a = 1, b = 1, c = 493000, time = 0.031000s
a = 1, b = 1, c = 494000, time = 0.031000s
a = 1, b = 1, c = 495000, time = 0.031000s
a = 1, b = 1, c = 496000, time = 0.031000s
a = 1, b = 1, c = 497000, time = 0.031000s
a = 1, b = 1, c = 498000, time = 0.031000s
a = 1, b = 1, c = 499000, time = 0.031000s
a = 1, b = 1, time = 0.031000s

Which brings us back into the realm of your lifetime.

Exercise

Write a function, that given a, b and c checks whether there exist a d, such that your property holds. It should return -1 if there does not exist such a d, and the d otherwise.

Use that function in your code. Make sure that you need roughly \$\log d_{\text{max}}\$ iterations in that function.

Important remark about integer sizes

Keep in mind that long int is usually just a 64 bit integer, which means that the largest integer you can store is \$2^{63}-1\$. Integer types with more bits have greater bounds, but are platform specific. Also, multiplication can be a tad slower, since multiplying 128bit numbers isn't as easy as multiplying 64bit numbers.

See the next section how to get multiplications down.

An actual review

Our pow4 is now essentially two multiplications. However, we're still using pow4 too often. After all, we don't need to recalculate \$a^4\$ in every iteration. The compiler happily does, since it doesn't optimize aggressively enough.

Which brings us to the actual review: your code is cleanly written, easy to read and to understand. Unfortunately, well-written, modular code often doesn't squeeze the last bit (heh) out of your hardware, unless your compiler/runtime is very smart (and thus often expensive).

So let's get back to the drawing board for a final review of your code:

Includes

#include <stdio.h>
#include <time.h>
#include <math.h>

I would sort them by name, but that's fine. You don't include anything that's not necessary, nor did you forget something (and got saved by a non-standard compliant compiler).

Declaration

int main() {
    long int a, b, c, d;
    clock_t t;
    t = clock();

Depending on whether you write ANSI-C or C99, I would defer declaration of variables as long as possible. For example, at the moment it's easy to accidentally change c to some bogus value, or forget a { and accidentally check the prop after the loops or similar:

for (a = 1; a < 100000; a++)
    for (b = 1; b < 300000; b++)
        for (c = 1; c < 500000; c++)
            for (d = 1; d < 500000; d++)
                printf("inner loop");
                if (prop(a,b,c,d))
                    printf("FOUND IT!\na = %ld\nb = %ld\nc = %ld\nd = %ld\n", a, b, c, d);

Whoops. The if doesn't get checked, and you don't get a warning (in older compiler versions; new ones do warn about possible whitespace issues). If you declare your variables later (e.g. C99-style), errors like that cannot happen (although it introduces possible shadowing):

for (long int a = 1; a < 100000; a++)
    for (long int b = 1; b < 300000; b++)
        for (long int c = 1; c < 500000; c++)
            for (long int d = 1; d < 500000; d++)
                printf("inner loop");
                if (prop(a,b,c,d))
                    printf("FOUND IT!\na = %ld\nb = %ld\nc = %ld\nd %ld\n", a, b, c, d);

This will now lead to a compiler error, since a, b and so on are out of scope. Either way, that depends on the language standard you want to use. Some people prefer one way, others the other one. Choose yours.

Types

Given that all values should be strictly greater than zero, long int is not the appropriate type, as it can be negative. We should accommodate that. However, instead of using long unsigned int throughout our code, let's use a type synonym in case we want to change it later to a type with a greater range:

 typedef long unsigned int Number;

You can probably come up with a better name.

Cache results (by hand)

One thing that strikes me most is that you recalculate \$a^4\$ and so on every time. We can easily treat this with more variables (using your declaration style):

int main() {
    long int a, b, c, d;
    long int a4, b4, c4, d4; // new variables
    clock_t t;
    t = clock();

    for (a = 1; a < 100000; a++) {
        a4 = pow4(a);                             // remember
        for (b = a; b < 300000; b++) {
            b4 = pow4(b);                         // remember
            for (c = b; c < 500000; c++) {
                c4 = pow4(c);                     // the fourth power
                for (d = c; d < 500000; d++) {
                    d4 = pow4(d);                 // of this member
                    if (a4 + b4 + c4 == d4)
                        printf("FOUND IT!\na = %ld\nb = %ld\nc = %ld\nd = %ld\n", a, b, c, d);
…

Remember how I said that nicely written, modular code isn't often optimal? This is one of those unfortunate examples where you have to help the compiler (unless you know exactly what optimization flags you have to use or your compiler is overly aggressive). The prop is gone, the calls to pow4 are now in your loop.

But the compiler cannot make a mistake here anymore: it's now very clear that a4 doesn't need to be recalculated 300000*500000*500000 times.

That being said, we should apply the other suggestions like the type synonym and the late declaration:

typedef long unsigned int Number;

int main() {
    clock_t t;
    t = clock();

    for (Number a = 1; a < 100000; a++) {
        const Number a4 = pow4(a);                         // remember
        for (Number b = a; b < 300000; b++) {
            const Number b4 = pow4(b);                     // remember
            for (Number c = b; c < 500000; c++) {
                const Number c4 = pow4(c);                 // the fourth power
                for (Number d = c; d < 500000; d++) {
                    const Number d4 = pow4(d);             // of this member
                    if (a4 + b4 + c4 == d4)
                        printf("FOUND IT!\na = %ld\nb = %ld\nc = %ld\nd = %ld\n", a, b, c, d);
…

While const isn't necessary here, it will make sure that we don't change our cached results accidentally.

The time has come

Although our code is now more verbose, there is one small piece of code that repeats itself three times throughout your main:

((double)(clock() - t))/CLOCKS_PER_SEC)

That's quite hard to read, isn't it? It's a perfect candidate for a function:

static inline seconds_since(clock_t t){
    return ((double)(clock() - t))/CLOCKS_PER_SEC;
}

This changes your printf from

printf("a = %ld, b = %ld, c = %ld, time = %fs\n", a, b, c, ((double)(clock() - t))/CLOCKS_PER_SEC);

to

printf("a = %ld, b = %ld, c = %ld, time = %fs\n", a, b, c, seconds_since(t));

Ah. Much easier to read. That's what inline functions are for. Note that any sophisticated compiler should inline that function anyway, so you may also drop inline if you don't want to use C99.


Building multiple parts of formula strings from different inputs in R I have been building a function that creates more advanced formula snippets using simple formula code. For example, a user may write:
$$Given\ dependent\ variable\ Y,\ and\ indepdentent\ variables\ X\  and \ U$$
$$Argument\ 1:\ Y_t = \alpha_t +  X_t,\ where\ X_t\ \to\  X_t + \sum_{i = -k}^k \Delta X_{t-i}$$
$$Argument\ 2: \ =U_t ,\ where\ U_t \to\ +\sum_{I = 1}^k U_{t-i} $$

in R: y ~ 1 + x becomes y ~ 1 + x + L(diff(x),-k:k) and

in R: ~ u becomes L(u,1:k)

I have built the following but am curious for feedback as (1) the code itself feels lengthy and (2) it is not super intuitive for a user to start Argument 2 with a ~.

buildFormula(Arg1 = y ~ x, data = dat)
#[1] "y ~ 1 + x + L(diff(x),-k:k)"

buildFormula(Arg1 = y ~ x + u + z, data = dat)
#[1] "y ~ 1 + x + u + z + L(diff(x),-k:k) + L(diff(u),-k:k) + L(diff(z),-k:k)"

buildFormula(Arg1 = y ~ x, data = dat, Arg2 = ~ u)
#[1] "y ~ 1 + x + L(diff(x),-k:k) + L(u,1:k)"

buildFormula(Arg1 = y ~ x, data = dat, Arg2 = ~ u + z)
#[1] "y ~ 1 + x + L(diff(x),-k:k) + L(u,1:k) + L(z,1:k)"

buildFormula(Arg1 = y ~ x, data = dat, Arg2 = ~u)
#[1] "y ~ 1 + x + L(diff(x),-k:k) + L(u,1:k)"

buildFormula(Arg1 = y ~ x, data = dat, Arg2 = u + z)
#Error in ... :
#  object 'u' not found

buildFormula(Arg1 = y ~ x, data = dat, Arg2 = u + z)
#Error in ... :
#  object 'u' not found

(3) I would also be appreciative if someone would recommend how to catch this error and warn the user to add a ~ in Arg2.

Function Code

# generate data
dat <- cbind(y = rnorm(30), x = rnorm(30), u = rnorm(30), z = rnorm(30))
# function
buildFormula <- function(Arg1, data, Arg2 = NULL){
  ff <- Arg1
  all_names <- dimnames(attr(terms(ff), "factors"))
  y_names <- all_names[[1]][!(all_names[[1]] %in% all_names[[2]])]
  x_names <- all_names[[2]][all_names[[2]] %in% colnames(data)]
  ff_LHS <- y_names
  # building the right hand side ( ~ X1 + X2 + L(diff(X1),-k:k) + L(diff(X2),-k:k) )
  ff_RHS <- paste(c(ifelse(attr(terms(ff), "intercept") == 1, "1", "-1"), # intercept
                    x_names, # independent variables
                    paste0("L(diff(", x_names, "),-k:k)")), # sum of lagged differences of independent variables
                  collapse = " + ")
  # if extra was indicated, add it on but treat it differently (~ + L(X3, 1:k) )
  ff_RHS <- ifelse(is.null(Arg2), ff_RHS, paste0(ff_RHS, " + ",
                                                   paste0("L(", unlist(strsplit(as.character(Arg2)[-1], " \\+ ")),
                                                          ",1:k)",
                                                          collapse = " + ")))
  ff_k <- paste(ff_LHS, "~", ff_RHS)
  ff_k
}

1
The function is a bit lengthy indeed. I have some general remarks, but I will hold them off, because I have some suggestions that would fix points 2 and 3.
2
The source of the error is because u + z is not interpreted as a formula, but rather as the addition of two variables. So when you interact with Arg2, an error is thrown. Adding the ~ fixes this, because it is recognized as a formula then.
A workaround for this problem is to combine both parts of the formula into one. This is something other packages in R do as well. (mlogit::mFormula is one example from the top of my head) The combined formula would look like so: y ~ x | u + z. You can then split the RHS at the | and do your work on each part of the formula.
3
Catching the problem is really quite hard, because any interaction with Arg2 will trigger the evaluation of u+z, which in turn throws an error if u or z don't exist.
What you can do is use tryCatch. This function allows you to evaluate an expression and define a function that is executed if an error or warning is thrown.
> tryCatch(is.formula(1+1), error = function(e)stop("bla"))
[1] FALSE
> tryCatch(is.formula(u + z), error = function(e)stop("bla"))
 Error in value[[3L]](cond) : bla
> tryCatch(is.formula(~u + z), error = function(e)stop("bla"))
[1] TRUE

You could add this to the top of your function:
if(!tryCatch(is.formula(1+1),
    error = function(e)FALSE))){
    stop("Arg2 is not a formula, be sure to add \"~\" to the start.")
}

Back to 1
String operations will quickly look very wordy, but that's all fine. You are working with words after all.
You spend 4 lines dissecting the formula for the LHS and the RHS. This can be done a lot more easily by coercing the formula to a string:
> as.character(a+b~c+d)
[1] "~"     "a + b" "c + d"

Before you do coercion, you should test whether the thing you are coercing is actually what you expect it to be. So add a test to the top of the function:
stopifnot(is.formula(Arg1))

We still should separate removing the intercept from the rest of the formula. Because we don't want to see L(diff(-1,-k:k)). So we test whether -1 is in the RHS, remove it if it is and at the end, add it back if necessary:
  removeIntercept <- grepl("-1", RHS)
  if(removeIntercept){
    splitRHS[[1]] <- splitRHS[[1]][!grepl("-1", splitRHS[[1]])]
  }

Putting it back:
  if(removeIntercept){
    ff_RHS <- paste("-1 +", ff_RHS)
  }

If we run with the suggestion in part 2, about combining the formulae, we can drop Arg2. We then split the RHS on |. Because we coerced the formula to character, we can do this with strsplit. We then want to split the formula for each of the terms. Again with strsplit.
The argument data is not used, so we can omit it.
Right now we are returning a string, as per the function in OP, but the function is called buildFormula, so it would be nice to wrap the output in as.formula, so that the name fits the output (or you can rename the function).
The new function would be:
buildFormula <- function(formula){
  stopifnot(is.formula(formula))

  ffAsChar <- as.character(formula)
  LHS <- ffAsChar[2]
  RHS <- strsplit(ffAsChar[3], "[|]")[[1]]

  splitRHS <- strsplit(RHS, "[+]")
  removeIntercept <- grepl("-1", RHS[1])
  if(removeIntercept){
    splitRHS[[1]] <- splitRHS[[1]][!grepl("-1", splitRHS[[1]])]
  }

  ff_RHS <- paste(splitRHS[[1]],
                  paste0("L(diff(",
                         splitRHS[[1]],
                         "),-k:k)"),
                  collapse = " + ",
                  sep = " + ")
  if(length(splitRHS) > 1){
    ff_RHS <- paste0(ff_RHS, "+",
                     paste("L(", splitRHS[[2]], ",1:k)",
                           collapse = " + "))
  }
  if(removeIntercept){
    ff_RHS <- paste("-1 +", ff_RHS)
  }
  paste(LHS, "~", ff_RHS)
}

This will produce the following output:
> buildFormula(Arg1 = y ~ x, data = dat)
[1] "a ~ x + L(diff(x),-k:k)"
>
> buildFormula(Arg1 = y ~ x + u + z, data = dat)
[1] "a ~ x  + L(diff(x ),-k:k) +  u  + L(diff( u ),-k:k) +  z + L(diff( z),-k:k)"
>
> buildFormula(Arg1 = y ~ x | u, data = dat)
[1] "a ~ x  + L(diff(x ),-k:k)+L(  u ,1:k)"
>
> buildFormula(Arg1 = y ~ x | u + z, data = dat)
[1] "a ~ x  + L(diff(x ),-k:k)+L(  u  ,1:k) + L(  z ,1:k)"


Repository of trainers This is my first asp.net core web api and i did followed the Repository pattern but i don't know whether this approach is good practice.

public class TrainerRepo : IRepoTrainer
{
    private readonly IConfiguration _config;

    public TrainerRepo(IConfiguration config)
    {
        _config = config;
    }

    public IDbConnection dbConnection
    {
        get
        {
            return new SqlConnection(_config.GetConnectionString("MyConnString"));
        }
    }

    public List<Trainer> GetTrainers()
    {
        string sql = "select trainer_id,trainer_name from trainer";
        using(IDbConnection conn = dbConnection)
        {
            conn.Open();
            var res = conn.Query<Trainer>(sql).ToList<Trainer>();
            conn.Close();
            return res;
        }

    }

    public int NewTrainer(Trainer trainer)
    {
        string sql = "insert into trainer ( trainer_name) values ( @name)";
        using(IDbConnection conn = dbConnection)
        {
            conn.Open();
            var res = conn.Execute(sql, param: new
            {
                //id = trainer.trainer_id,
                name = trainer.trainer_name
            });
            conn.Close();
            return res;
        }
    }

    public async Task<int> updateTrainer(Trainer trainer)
    {
        string sql = "update trainer set trainer_name = @tname where trainer_id = @tid";
        using(IDbConnection conn = dbConnection)
        {
            conn.Open();
            var res = await conn.ExecuteAsync(sql, param: new { tname = trainer.trainer_name, tid = trainer.trainer_id });
            conn.Close();
            return res;
        }
    }

    public int deleteTrainer(int id)
    {
        string sql = "delete from trainer where trainer_id=@trainerId";
        using(IDbConnection conn = dbConnection)
        {
            conn.Open();
            var res =  conn.Execute(sql, param: new { trainerId = id });
            conn.Close();
            return res;
        }
    }

}

IRepoTrainer interface

public interface IRepoTrainer
{
    List<Trainer> GetTrainers();
    int NewTrainer(Trainer trainer);
    Task<int> updateTrainer(Trainer trainer);
    int deleteTrainer(int id);
}

my problem is that reuse of following code block, is it good practice ?

 using(IDbConnection conn = dbConnection)
   {
    conn.Open();
    //
    conn.Close();
    return res;
   }

It has been suggested in docs to not pass IConfiguration around for injection. Try creating a IConnectionFactory or just have a connection injected directly into the repo

Something like

public interface IDbConnectionFactory {
   IDbConnection CreateConnection();
}

and its implementation for your class would look something like this

public class SqlConnectionFactory : IDbConnectionFactory {
    private readonly string dbConnectionString;

    public SqlConnectionFactory(string dbConnectionString) {
        this.dbConnectionString = dbConnectionString;
    }

    public IDbConnection CreateConnection() {
        return new SqlConnection(this.dbConnectionString);
    }
}

it would be registered in startup

var connectionString = Configuration.GetConnectionString("MyConnString");
services.AddScoped<IDbConnectionFactory, SqlConnectionFactory>(_ => new SqlConnectionFactory(connectionString));

//...

and injected

public class TrainerRepo : IRepoTrainer {
    private readonly IDbConnectionFactory factory;

    public TrainerRepo(IDbConnectionFactory factory) {
        this.factory = factory;
    }

    public List<Trainer> GetTrainers() {
        string sql = "select trainer_id,trainer_name from trainer";
        using(IDbConnection connection = factory.CreateConnection()) {
            connection.Open();
            var res = connection.Query<Trainer>(sql).ToList<Trainer>();
            connection.Close();
            return res;
        }
    }

    //...

The repeated code can also be generalized

T Invoke<T>(Func<IDbConnection, T> func) {
    using(IDbConnection connection = factory.CreateConnection()) {
        connection.Open();
        var res = func(connection);
        connection.Close();
        return res;
    }
}

async Task<TResult> InvokeAsync<TResult>(Func<IDbConnection, Task<TResult>> func) {
    using(IDbConnection connection = factory.CreateConnection()) {
        connection.Open();
        var res = await func(connection);
        connection.Close();
        return res;
    }
}

Which changes the previous examples to

public List<Trainer> GetTrainers() {
    string sql = "select trainer_id,trainer_name from trainer";
    return Invoke(connection => connection.Query<Trainer>(sql).ToList<Trainer>());
}

public Task<int> updateTrainer(Trainer trainer) {
    string sql = "update trainer set trainer_name = @tname where trainer_id = @tid";
    return  InvokeAsync(connection => connection.ExecuteAsync(sql, param: new { tname = trainer.trainer_name, tid = trainer.trainer_id }));
}


How can I avoid all this code duplication? As you can see I have to populate the ViewModel with the List of Tags so that the dropdown can have all the options for the user to select.
And I have to do this 2 times for the Create Action and 2 times for the Edit action.

[Authorize]
public ActionResult Create()
{
    var tags = from i in _articleService.GetAllTags()
               select new SelectListItem {Text = i.Name, Value = i.ID.ToString()};
    var model = new CreateArticleViewModel {Tags = tags};
    return View(model);
}

[HttpPost, Authorize]
public ActionResult Create(CreateArticleViewModel model)
{
    if (ModelState.IsValid)
    {
        try
        {
            var currentUser = _userService.GetByUserName(User.Identity.Name);
            var tagIds = model.TagIds.Select(int.Parse);
            var article = _articleService.Create(currentUser, model.Title, model.Body, tagIds);
            return RedirectToAction("ViewArticle", new { id = article.ID });
        }
        catch (Exception ex)
        {
            ModelState.AddModelError("Error", ex.Message);
        }
    }
    var tags = from i in _articleService.GetAllTags()
               select new SelectListItem { Text = i.Name, Value = i.ID.ToString() };
    model.Tags = tags;
    return View(model);
}

[Authorize]
public ActionResult Edit(int id)
{

    var article = _articleService.GetById(id);
    var tags = from tag in _articleService.GetAllTags().ToList()
               select new SelectListItem { Text = tag.Name, Value = tag.ID.ToString(), Selected = article.HasTag(tag)};
    var model = new CreateArticleViewModel {Title=article.Title, Body = article.Body, Tags = tags };

    return View(model);
}

[HttpPost, Authorize]
public ActionResult Edit(CreateArticleViewModel model)
{
    if (ModelState.IsValid)
    {
        try
        {
            //handle article update
        }
        catch (Exception ex)
        {
            //Add ModelState error
        }
    }
    var tags = from tag in _articleService.GetAllTags().ToList()
               select new SelectListItem { Text = tag.Name, Value = tag.ID.ToString(), Selected = article.HasTag(tag)};
    model.Tags= tags;

    return View(model);
}

Is there a better way?

You could refactor out your tagging into a helper method, as below:

[Authorize]
public ActionResult Create()
{
   var model = new CreateArticleViewModel {};
   TagModel (model, _ => false);
   return View(model);
}
[HttpPost, Authorize]
public ActionResult Create(CreateArticleViewModel model)
{
   if (ModelState.IsValid)
   {
      try
      {
         var currentUser = _userService.GetByUserName(User.Identity.Name);
         var tagIds = model.TagIds.Select(int.Parse);
         var article = _articleService.Create(currentUser, model.Title, model.Body, tagIds);
         return RedirectToAction("ViewArticle", new { id = article.ID });
      }
      catch (Exception ex)
      {
         ModelState.AddModelError("Error", ex.Message);
      }
   }
   TagModel (model, _ => false);
   return View(model);
}

[Authorize]
public ActionResult Edit(int id)
{
   var article = _articleService.GetById(id);
   var model = new CreateArticleViewModel {Title=article.Title, Body = article.Body};
   TagModel (model, tag => article.HasTag (tag));

   return View(model);
}

[HttpPost, Authorize]
public ActionResult Edit(CreateArticleViewModel model)
{
   if (ModelState.IsValid)
   {
      try
      {
            //handle article update
      }
      catch (Exception ex)
      {
            //Add ModelState error
      }
   }
   TagModel (model, tag => article.HasTag (tag));
   return View(model);
}

private void TagModel (CreateArgicleViewModel model, Predicate<Tag> isSelected)
{
   var tags = from tag in _articleService.GetAllTags()
              select new SelectListItem { Text = tag.Name, Value = tag.ID.ToString(), Selected = isSelected(tag)};
   model.Tags= tags;
}


Simple one-heap Nim game I wrote this game to practice some simple OOP. You play against the computer, but be aware that the computer has no real AI, as it just generates its moves randomly. The loser is the one whose turn it is when there is only one match left in the stack.

import java.util.Random;
import java.util.Scanner;

class Game {

    private boolean gameOver;
    Scanner input;
    Player computer, human;

    public Game() {

        computer = new Computer();
        human = new Human();
        input = new Scanner(System.in);

    }

    public void runGame(){

        System.out.println("Welcome to the game of Nim! How many matches would you like to play with?");
        human.setMatchesLeft(input.nextInt());

        while (!gameOver){

            if (computer.getMatchesLeft() != 1){
                System.out.print("Computers move: ");
                computer.playerTurn();
                computer.printStatus();

                if (computer.getMatchesLeft() == 1) {
                    gameOver = true;
                    System.out.println("Human loses, computer wins.");
                }
            }

            if (human.getMatchesLeft() != 1){
                System.out.print("Humans move: ");
                human.playerTurn();
                human.printStatus();

                if (human.getMatchesLeft() == 1) {
                    gameOver = true;
                    System.out.println("Computer loses, human wins.");
                }
            }

        }

    }

}

abstract class Player {

    private static int matchesLeft;

    public abstract void playerTurn();
    public abstract void printStatus();

    public void setMatchesLeft(int x){
        matchesLeft = x;
    }

    public int getMatchesLeft(){
        return matchesLeft;

    }

}

class Human extends Player {

    private int humanTurn;
    private Scanner input;

    public Human(){

        input = new Scanner(System.in);
    }

    public void playerTurn(){

        humanTurn = input.nextInt();

        while ( humanTurn > (getMatchesLeft() / 2) || humanTurn < 1 ) {
            System.out.print("Illegal move. Please pick at least 1 matches.\n");
            humanTurn = input.nextInt();
        }

        setMatchesLeft(getMatchesLeft() - humanTurn);

    }

    public void printStatus(){
        System.out.println("Human removes " + humanTurn + " matches. " + getMatchesLeft() + " matches left. ");
    }
}

class Computer extends Player {

    private int computerTurn;
    Random computerGen;

    public Computer(){
        computerGen = new Random();
    }

    public void playerTurn(){

            computerTurn = computerGen.nextInt(((getMatchesLeft() / 2) - 1) + 1) + 1;

        setMatchesLeft(getMatchesLeft() - computerTurn);

    }

    public void printStatus(){
        System.out.println(computerTurn);
        System.out.println("Computer removes " + computerTurn + " matches. " + getMatchesLeft() + " matches left. ");
    }

}

public class Nm {
    public static void main (String[] args){

        Game newgame = new Game();
        newgame.runGame();

    }
}

System.out.print("Illegal move. Please pick at least 1 matches.\n");

You've got a typo there, should be 1 match.

I like how you've structured your code, but there's a couple naming points I want to address...

When a player takes a turn, you call it a "player turn". Which sounds pretty okay... but then the code reads like this: human.playerTurn(); which doesn't sound all that great. "I am now doing my player turn". There is no verb in your function name. Functions do stuff, so they should have a verb in there, or else they might be not doing stuff. Or maybe they're doing vague stuff.

Try "player takes a turn". So you get this:

computer.takeTurn();

human.takeTurn();

Next up, you've got duplicated code.

        if (computer.getMatchesLeft() != 1){
            System.out.print("Computers move: ");
            computer.playerTurn();
            computer.printStatus();

            if (computer.getMatchesLeft() == 1) {
                gameOver = true;
                System.out.println("Human loses, computer wins.");
            }
        }

        if (human.getMatchesLeft() != 1){
            System.out.print("Humans move: ");
            human.playerTurn();
            human.printStatus();

            if (human.getMatchesLeft() == 1) {
                gameOver = true;
                System.out.println("Computer loses, human wins.");
            }
        }

This is pretty much the same thing twice. If you could make computer and human print "xxx's move: " then you'd be pretty close to merging the two sections of code into one...

Specifically, the parts that are different are...

if (computer.getMatchesLeft() != 1){
if (human.getMatchesLeft() != 1){

The human and computer in statements like this;

            System.out.print("Computers move: ");
            System.out.print("Humans move: ");

These two pieces of text;

                System.out.println("Human loses, computer wins.");
                System.out.println("Computer loses, human wins.");

And these two pieces of text. If you could remove those differences, or somehow extract them from the section of code to another place, you could merge the two sections of code. Having less duplicated code is good, because if you would have to make a change, you wouldn't have to make it in two places.

For this, I'd add a method to Player: getName(). That way, you could print player.getName()+"'s move: ". Implement in Computer as return "Computer"; and in Human as, well... your choice, maybe it just returns Human, maybe it returns a string the player has entered at the start of the game.

Then, you could use an array or a List to do this:

    Player[] players = {computer, human};
    Player victor = null;
    while (!gameOver){
        for(Player player : players){ //for each item in collection
            if(player.getMatchesLeft() != 1){
                System.out.print(player.getName()+"'s move: ");
                player.takeTurn();
                player.printStatus();
                if(player.getMatchesLeft() == 1){
                    victor = player;
                    gameOver = true;
                    break;
                }
            }
        }
   }

   if(victor == human){
      System.out.println("Computer loses, human wins.");
   } else {
      System.out.println("Human loses, computer wins.");
   }

Which would simplify your game code quite a bit. You could make it even smaller by remembering that you don't need the matchesLeft check due to the existence of the break; statement.

I'd also take a look at moving the whole "Game" to a separate class. That way you could ask the player "do you want to play again" and start at the top. Aim for a structure like this:

  do {
      int matches = askForMatchCount();//maybe pass in Scanner, maybe use it from member variable
      Game game = new Game(human, computer);//Game(Player players...)
      game.setMatches(matches);
      Player victor = game.playGame();
      printVictoryMessage(victor);
 } while(askForRematch());

I've used some tricks there you might not have seen before. You'll have to google to find out what they are, but I'll help you with the search terms and short descriptions:

 do { /*code*/ } while( condition );

That's a do-while loop. It works like a while loop, but always runs at least once.

 Game(Player players...)

That ... in the argument list is called "varargs". It's useful if you have a number of arguments but don't know how many. In this case, I'm making it possible to give you a three player game.

Last point I have for now is misuse of static.

In Player, you have this:

 private static int matchesLeft;

And at first I was like "Why would you do this?" It makes no sense! The amount of matches left doesn't belong to 1 player, does it? Do they have their own matches? Isn't this a game where the matches are shared between the players... Oh.

You've made them shared between the players! Makes sense, I guess...

Except you've just made them shared between ALL the players. Which is not so good if you ever wanted there to be multiple games happening at the same time.

If you instead relocated the matches to the game (like I did a bit earlier with game.setMatches(int matches), then you can change the takeTurn to take a Game object and obtain the matches that way (takeTurn(Game game)).


Reusable vector module with tests I'm learning rust with mostly a background in managed languages. I have a little C background.

As a learning project I am re-implementing a linear-transformations     library I've been working on in C#.

This is my first reusable library I've tried to make in Rust and I'd like feedback on creating and using modules, as well as rust programming in general.

Project structure

+ transforms
    + src
        - lib.rs
        - tests.rs
        - transforms.rs
        - vectors.rs
    + target
    - Cargo.toml

I haven't made any changes to the automatic cargo.toml.
transforms.rs is empty and have the linear transformation code added to it in the future.

lib.rs

pub mod vectors;
mod tests; // tests don't get run if I don't have this, not sure why?

vectors.rs

use std::fmt;

pub struct KVector3{
    pub x : f64,
    pub y : f64,
    pub z : f64,
}

pub trait Vector {
    fn magnitude_squared(&self) -> f64;
    fn magnitude(&self) -> f64;
}

pub trait Vector3 {
    fn dot(&self, v : KVector3) -> f64;
    fn cross(&self, v : KVector3) -> KVector3;

    fn zero() -> KVector3;
    fn i_hat() -> KVector3;
    fn j_hat() -> KVector3;
    fn k_hat() -> KVector3;
}

impl Vector for KVector3 {
    fn magnitude_squared(&self) -> f64 {
        self.x * self.x + self.y * self.y + self.z * self.z
    }

    fn magnitude(&self) -> f64 {
        (self.x * self.x + self.y * self.y + self.z * self.z).sqrt()
    }
}

impl Vector3 for KVector3 {
    fn dot(&self, v : KVector3) -> f64 {
        self.x * v.x + self.y * v.y + self.z + v.z
    }

    fn cross(&self, v : KVector3) -> KVector3 {
        KVector3 {
            x : self.y * v.z - self.z * v.y,
            y : self.x * v.z - self.z * v.x,
            z : self.x * v.y - self.y * v.x
        }
    }

    fn zero() -> KVector3 { KVector3 { x: 0., y: 0., z: 0. } }

    fn i_hat() -> KVector3 { KVector3 { x: 1., y: 0., z: 0. } }

    fn j_hat() -> KVector3 { KVector3 { x: 0., y: 1., z: 0. } }

    fn k_hat() -> KVector3 { KVector3 { x: 0., y: 0., z: 1. } }
}

impl PartialEq<KVector3> for KVector3 {
    fn eq(&self, other : &KVector3) -> bool {
        self.x == other.x && self.y == other.y && self.z == other.z
    }
}

impl fmt::Debug for KVector3 {
    fn fmt(&self, f : &mut fmt::Formatter) -> fmt::Result {
        write!(f, "<{},{},{}>", self.x, self.y, self.z)
    }
}

tests.rs

#[cfg(test)]
mod tests {

    use ::vectors::{Vector, Vector3, KVector3};

    #[test]
    fn unit_magnitude_squared() {
        let mag_squared = KVector3::i_hat().magnitude_squared();
        assert_eq!(1., mag_squared);
    }

    #[test]
    fn v3_magnitude_squared() {
        let triangle = KVector3 { x : 3., y : 4., z: 0. };
        let mag_squared = triangle.magnitude_squared();
        assert_eq!(25., mag_squared);
    }

    #[test]
    fn v3_magnitude() {
        let triangle = KVector3 { x : 3., y : 4., z: 0. };
        let mag = triangle.magnitude();
        assert_eq!(5., mag);
    }

    #[test]
    fn cross_product() {
        let product = KVector3::i_hat().cross(KVector3::j_hat());
        assert_eq!(KVector3::k_hat(), product);

    }
}

Unit tests should be put in the file they're testing

It's recommended to put the unit tests into the same file as the functions you're testing. This prevents your code from getting out of sync with your tests more easily. Also, you don't want one single large tests.rsfor all your unit tests.

Use proper types in traits

At the moment, as soon as we use any function from Vector3, we end up with a KVector3:

pub trait Vector3 {
    fn dot(&self, v : KVector3) -> f64;
    fn cross(&self, v : KVector3) -> KVector3; // here

    fn zero() -> KVector3;  // here
    fn i_hat() -> KVector3; // here
    fn j_hat() -> KVector3; // here
    fn k_hat() -> KVector3; // here
}

That's probably not what you intended. Use Self instead here. Furthermore, all your functions are fixed to f64, but one can implement all functions from Vector3 for any number type. The following interface encapsulates that:

pub trait Vector3 {
    type Output;

    fn dot(&self, v: Self) -> Self::Output;
    fn cross(&self, v: Self) -> Self;

    fn zero() -> Self;
    fn i_hat() -> Self;
    fn j_hat() -> Self;
    fn k_hat() -> Self;
}

Use derive for canonical implementations

If you use #[derive(PartialEq, Debug)] you don't have to implement both variants yourself.

Consider generics for your structs (and traits)

Your KVector3 only supports f64. However, we can imagine a situation where we want to store f32, for example for GPU calculation. We can use KVector3 also in that circumstance if we make it generic.

Add documentation and examples

Since you're working on a reusable library you want to have some documentation at hand. Furthermore, examples in your documentation are automatically checked with cargo test.


4 different implementations of modulo with fully defined behavior I've implemented the 3 variants of the modulo operation described on this Wikipedia page.
The goal is to have fully defined behavior for all inputs.
Code
Implementation with truncated division (result has the same sign as the dividend)
int32_t rem(int32_t x, int32_t y) {
    if (y == 0) exit(-1);
    if (y == -1) return 0;
    return x % y;
}

This one is the simplest since it's already how the % operator is implemented in C.
We have to take care of two edge cases though:

If y == 0 it's an invalid operation; exit(-1).
INT32_MIN % -1 is undefined behavior, so I've hard-coded the result for y == -1.

Implementation with floored division (result has the same sign as the divisor)
int32_t mod(int32_t x, int32_t y) {
    if (y == 0) exit(-1);
    if (y == -1) return 0;
    return x - y * (x / y - (x % y && (x ^ y) < 0));
}

This one is a bit trickier.
We still have to handle the two previous edge cases.
However, the modulo is computed differently: x - y * floor(x / y).
We can't use floor here because we're working with integers, so the formula looks like this:
x - y * (x / y - {1 if x isn't a multiple of y and the result of x / y is negative, 0 else})
x - y * (x / y - {1 if x % y and the result of x / y is negative, 0 else})
x - y * (x / y - {1 if x % y and (x or y, but not both, is negative), 0 else})
x - y * (x / y - {1 if x % y and (x ^ y) < 0, 0 else})
x - y * (x / y - (x % y && (x ^ y) < 0))
Implementation with Euclidean division (result is always positive)
Portable implementation
int32_t euc1(int32_t x, int32_t y) {
    if (y == 0) exit(-1);
    if (y == -1) return 0;
    if (y == INT32_MIN) return x >= 0 ? x : INT32_MAX + x + 1;
    y = abs(y);
    return x - y * (x / y - (x < 0 && x % y));
}

It's basically the same implementation as the previous one, except that we use the absolute value of y.
Also, (x ^ y) < 0 was simplified to x < 0 because y is always positive (we use the absolute value).
However abs(INT32_MIN) is undefined behavior (because abs(INT32_MIN) = INT32_MAX + 1 which is not representable) so we have to take care of this special case:

If x >= 0, we have x % (INT32_MAX + 1) = x because x is always inferior to INT32_MAX + 1.
If x < 0, we have x % (INT32_MAX + 1) = INT32_MAX + 1 + x, then we reorder operations to avoid overflow on signed integers which is undefined behavior: INT32_MAX + x + 1 (remember that x is negative).

GCC implementation
int32_t euc2(int32_t x, int32_t y) {
    if (y == 0) exit(-1);
    if (y == -1) return 0;
    if (y != INT32_MIN) y = abs(y);
    int32_t tmp = x / y - (x < 0 && x % y);
    __builtin_mul_overflow(y, tmp, &tmp);
    __builtin_sub_overflow(x, tmp, &tmp);
    return tmp;
}

This implementation is the same as the portable one, except for the handling of the y = INT32_MIN edge case: there is less conditional logic, making it potentially easier to optimize.
If y == INT32_MIN we don't compute the absolute value because it's undefined behavior, then we continue the execution as nothing happened. A multiplication and a subtraction were replaced by their built-in functions counterparts to avoid undefined behavior on overflow.
Because of black magic it works:

If x >= 0 we have x / INT32_MIN - (x < 0 && x % INT32_MIN) = 0, then INT32_MIN * 0 = 0, then x - 0 = x, so the result is x as expected.
If INT32_MIN < x < 0 we have x / INT32_MIN - (x < 0 && x % INT32_MIN) = -1, then INT32_MIN * -1 = INT32_MIN (built-in function defined behavior), then x - INT32_MIN = INT32_MAX + x + 1 (built-in function defined behavior), so the result is INT32_MAX + x + 1 as expected.
If x == INT32_MIN we have INT32_MIN / INT32_MIN - (INT32_MIN < 0 && INT32_MIN % INT32_MIN) = 1, then INT32_MIN * 1 = INT32_MIN, then INT32_MIN - INT32_MIN = 0, so the result is 0 as expected.

Questions
Is there an error somewhere, something that i missed? Like a set of inputs that don't give the right result.
If this code really undefined behavior free?
Can it be simplified?

Is there an error somewhere, something that i missed? Is there an error somewhere, something that i missed?

Certainly have most of main street covered.
if (y == -1) return 0; is a nice trick.
y = abs(y); fails 32-bit math when int is 16-bit.  Could use labs() or conditional code or an if() block.  (Only real portability bug I see.)
Pedantic: int32_t is an optional type.  It is missing and code fails to compile on select (dinosaur) machines that do not have 32-bit unpadded 2's complement.
Pedantic: On a select (unicorn) machine where int is 64-bit and int32_t exist, promotions occur.  I do not see a problem though.
Pedantic: Usage of x ^ y obliges 2's complement encoding for correct functionality which is the case with intN_t here , but not all integers on all machines.

Adding a test harness would help.

Can it be simplified?

A shot at simplifying Euclidean division (result is always non-negative).  Similar code may apply to the other 2 cases.
// Substitute `int` for `int32_t`, `long long`, etc.
int modulo_Euclidean(int x, int y) {
  if (y == 0) exit(-1);
  if (y == -1) return 0;
  int m = x % y;
  if (m < 0) {
    m = (y < 0) ? m - y : m + y;
  }
  return m;
}

Does not require intN_t math*, 2's complement, no padding, range constants, nor extended math.

It would be interesting to see if 64-bit math was really slower or a reasonable candidate.
int32_t mod_rem_or_euc(int32_t x, int32_t y) {
  if (y == 0) exit(-1);
  int_fast64_t m = (int_fast64_t) x % y;
  ...
}

* intN_t types are 2's complement, no padding.


Class handler system Since I had a lot of classes to store inside 1 main class I decided to create a handler system. I was having to create 30 - 50 private readonly fields for private instances of classes, then assigning them all in the constructor, then creating a public method to get them via a return. I decided instead of having up to 150 lines of code that I would try and sort out a more organised system and put it in place.

I start by declaring an empty interface for all my handlers to inherit  that interface. This interface is in a totally seperate .cs file.

namespace Panda.Root.Core.Other
{
    public interface IPandaClass { }
}

Now, I declare a new dictionary in my main class (PandaServer.cs)

private readonly Dictionary<string, IPandaClass> _handlers;

I then assign the dictionary:

public PandaServer()
{
    _handlers = new Dictionary<string, IPandaClass>(StringComparer.InvariantCultureIgnoreCase);
}

I create two methods, one for registering new handlers (or replacing them) and one for returning the handler by name.

private void RegisterHandler(string handlerName, IPandaClass handler)
{
    _handlers[handlerName] = handler;
}

public IPandaClass GetHandler(string handlerName)
{
    return _handlers[handlerName];
}

And then I can register as many hadlers as I can, pretty easily:

RegisterHandler("messages", Utilities.CreateInstanceOf<MessageHandler>());
RegisterHandler("configuration", Utilities.CreateInstanceOf<ConfigurationManager>());
RegisterHandler("console_worker", Utilities.CreateInstanceOf<ConsoleWorker>());
RegisterHandler("base_handler", Utilities.CreateInstanceOf<BaseHandler>());

I then declare them where I need them like this

var panda = Program.GetPandaServer();
var pandaBase = panda.GetHandler("base") as BaseHandler;

if (pandaBase == null)
{
    return;
}

var playerHandler = pandaBase.GetHandler("player") as PlayerHandler;

I was told declaring all your classes there instead of making multiple calls in future was better as you would always have that variable there incase you wanted to make multiple calls to the same method or methods of that method's return type.

The only major bad things I see with this system is you're hardcoding the name of the handlers but I guess there isn't a way around that, its how it works. If anyone can think of any other improvements please let me know.

IServiceProvider
So you decided to reinvent the IServiceProvider Interface :-) which

Defines a mechanism for retrieving a service object; that is, an object that provides custom support to other objects.

Your public interface IPandaClass { } is exactly the same. I guess you forgot to mention that it has the GetHandler method or otherwise it would be completely meaningless.
Then we read:

This interface is implemented by a class or value type that provides a service to other objects.

This is the missing GetHandler method.

Generics vs strings

RegisterHandler("messages", Utilities.CreateInstanceOf<MessageHandler>());

Instead of hardcoding strings you could use generics and derive the name from the class name or just use the type as a key.

Dependency Injection
Do you really need all classes in one handler? Have you tried something like Autofac etc.?


Defining a new Delphi Class with built-in Connection Using Delphi XE2, I'm wondering if it would be usable to define a base object something like below.
Class is prone to be used in multiple projects.

I've chosen TSQLConnection as my starting point - since it's using the dbxpress framework and should be able to connect to a wide range of databases and other. But if a better solution exists - please feel free to comment on this.

TMyObject = class
private
  fConnection : TSQLConnection;
public
  procedure SetupConnection(aConn : TSQLConnection);
  property Connection : TSQLConnection read fConnection write fConnection;
  ... // a lot of other properties and methods ...
end;

procedure TMyObject.SetupConnection(aConn : TSQLConnection);
begin
  fConnection := aConn; // Should we clone here? ... or is a reuse ok?
end;

This way it would be possible to use a connection from any of my inherited classes.

Questions:

Is this a good design? (from a general point of view)
Is the use of TSQLConnection OK, if we have like 200 inherited instances?
Making the assignment of the Connection - is it ok to re-use from the datamodule?
Perhaps the SQLConnection was better located in an Interface?

Intentions

My intention is to create a "base class structure" for all my classes (in all my projects), and make it easier for co-workers to develop new applications, based on this hierarchy.

Is this the best design?

Best design for what? and the answers of yes, no and maybe are all correct.

What you want to research is design patterns and more specifically Patterns of Enterprise Application Architecture, Active Record, Business Objects, Repository pattern Design etc. Also lookup ORM.


Caesar cipher implementation in C++ I've implemented a Caesar cipher in C++ and would like for you to review it. It works as expected, at least with the inputs I've tried.

Feel free to suggest improvements, warn about bad practices or mistakes that I might be doing or give general tips.

// Caesar Cipher implementation

#include <iostream>
#include <string>
#include <algorithm>
#include <fstream>
#include <vector>
#include <cassert>
#include <iterator>

std::string encodeStr(const std::string& str, int shift, bool decodeFlag = false)
{
    std::string str_enc;
    // not sure if this function is doing to much
    std::transform(str.cbegin(), str.cend(), std::back_inserter(str_enc), [&](char ch) -> char
    {
        if (ch == 'z' || ch == 'Z')
        {
            return ch - 25;
        }
        else if (isspace(ch))
        {
            return ' ';
        }
        else
        {
            if (islower(ch))
                ch ^= 0x20; // XOR 6th bit don't care if it's lower case.
            if (decodeFlag)
            {
                return ((ch - 'A' + 26) - shift) % 26 + 'A';
            }
            else
            {
                return ((ch - 'A') + shift) % 26 + 'A';
            }
        }
    });
    return str_enc;
}

std::vector<std::string> encodeVec(const std::vector<std::string>& strVector, int shift, bool decodeFlag = false)
{
    std::vector<std::string> tempMsg;
    tempMsg.reserve(strVector.size());

    std::transform(strVector.cbegin(), strVector.cend(), std::back_inserter(tempMsg),
        [&](const std::string& s) { return encodeStr(s, shift, decodeFlag); });

    return tempMsg;
}

int main(int argc, char *argv[])
{
    int choice;
    std::cout << "What do you want to do? 1.Encrypt, 2.Decrypt: ";
    std::cin >> choice;

    if (!(choice == 1 || choice == 2)) {
        return EXIT_FAILURE;
    }

    int key;
    std::cout << "Enter desired shift: ";
    std::cin >> key;

    std::ifstream inFile("test.txt"); // Might ask for the file instead of hard coding it...
    if (!inFile)
    {
        std::cout << "There was a problem with the file!";
        return EXIT_FAILURE;
    }

    std::string line;
    std::vector<std::string> lines;

    while (std::getline(inFile, line))
    {
        lines.push_back(line);
    }

    const std::vector<std::string> finalResult = [&]()
    {
        return choice == 1 ? encodeVec(lines, key) : encodeVec(lines, key, true);
    }();

    std::ofstream outFile("test.txt");
    std::copy(finalResult.cbegin(), finalResult.cend(),
        std::ostream_iterator<std::string>(outFile, "\n"));

    return EXIT_SUCCESS;
}

You can accept arguments from the command line.

You don't need to differentiate between decode and encode, you can simply provide negative shift.

You can hide the big lambda into a char -> char function.

You actually don't need the string -> string nor vector -> vector implementations.

encodeStr is a terrible name :)

You can leverage the char -> char implementation and copy istream_iterator<char> to ostream_iterator<char> char by char, and doing it over std::cin and std::cout will give you the ability to change the files as needed (through pipes) or use standard input / output by default.

#include <iostream>
#include <algorithm>
#include <iterator>
#include <sstream>

const unsigned char caesar_range = 'Z' - 'A' + 1;

char caesar_cipher(char c, int shift)
{
    char offset;
    if (c >= 'a' && c <= 'z') offset = 'a';
    else if (c >= 'A' && c <= 'Z') offset = 'A';
    else return c;

    shift = shift % caesar_range;
    auto base = c - offset + shift;
    if (base < 0) {
        base = caesar_range + base;
    }
    return base % caesar_range + offset;
}

void caesar_usage()
{
    std::cout << "Usage caesar <shift>\n";
    std::cout << "\tshift must be integer and providing -shift decodes string encoded with shift.\n";
}

int main(int argc, char *argv[])
{
    if (argc != 2) {
        caesar_usage();
        return EXIT_FAILURE;
    }

    int shift;
    std::stringstream ss(argv[1], std::ios_base::in);
    ss >> shift;
    if (ss.fail() || !ss.eof()) {
        caesar_usage();
        return EXIT_FAILURE;
    }

    // turn off white space skipping
    std::cin >> std::noskipws;

    std::transform(
        std::istream_iterator<char>(std::cin),
        std::istream_iterator<char>(),
        std::ostream_iterator<char>(std::cout),
        [&](char c) -> char {
          return caesar_cipher(c, shift);
        }
    );

    return EXIT_SUCCESS;
}

Encode | decode:
./caesar 5 <in.txt | ./caesar -5


Count numbers with atleast one common factor, excluding one I have been trying to teach myself some python 3 lately by working through some problems at geeksforgeeks.org. A common issue I am having is that my code runs on my machine, and in their test environment, but my code ultimately exceeds their time limits upon submission. I was wondering if anyone had any general advice on making my code more efficient so I could continue to make use of the practice problems. Also, if there are any concerns about using geeksforgeeks.org and folks could recommend other practice problem sites that would be appreciated. The problem I have been working on is:

http://practice.geeksforgeeks.org/problems/mathematical-manipulation/0

  Given a number \$N\$, find the total numbers, less than or equal to \$N\$ which have at-least one common factor with \$N\$ other than \$1\$.

  Input:
  The first line of input contains an integer \$T\$ denoting the no of test cases. Then \$T\$ test cases follow. Each test case consist of an integer \$N\$.

  Output:
  For each test case output an integer denoting the count of numbers, less than or equal to \$N\$ having at-least one common factor between the number and \$N\$ except \$1\$.

  Constraints:
  \$1 \le T \le 100000\$
  \$1 \le N \le 1000000\$

  Example:
  Input:

3
1
6
15

  Output:

0
4
7

And my code:

def myfun(num_in):

    new = num_in

    pos = []
    test = []
    int_list = []
    temp_list = []
    final_list = []

    if num_in == 1:
        print(0)
    elif num_in > 1:
        # list all possibilities
        for i in range(2, num_in):
            pos.append(i)

        # check remainder from division
        for i in range(0, len(pos)):
            test.append(num_in % pos[i])

        # record if remainder is 0, if it is, keep that because it will be lowest common denominator
        for i in range(0, len(test)):
            if test[i] == 0:
                int_list.append(pos[i])

        # multiply factor my integers until it exceeds input
        for i in range(0, len(int_list)):
            temp_val = int_list[i]
            j = 1
            while temp_val * j < num_in:
                final_list.append(temp_val * j)
                j += 1

        # add 1 for the number itself since we don't need to check that
        print(len(set(final_list)) + 1)

master_list = []

tot_num = int(input())

for p in range(tot_num):
    temp_val = int(input())
    master_list.append(temp_val)

for q in range(tot_num):
    myfun(master_list[q])

You write:

  A common issue I am having is that my code runs on my machine, and in their test environment, but my code ultimately exceeds their time limits upon submission. I was wondering if anyone had any general advice on making my code more efficient so I could continue to make use of the practice problems.

Here's that general advice: measure your code before you submit it! These kind of programming challenges normally give bounds on the size of the possible inputs, for example here it says:

  Constraints: \$1≤T≤100000\$, \$1≤N≤1000000\$

So in the worst case there would be 100,000 test cases, each of which has a number that might be as large as 1,000,000. So you need to ensure that your code runs quickly enough when run against a test case of this size.

Creating a test case is easy, for example you could write:

import random
T = 100000
N = 1000000
with open('worst-case.txt', 'w') as f:
    print(T, file=f)
    for _ in range(T):
        print(random.randrange(1, N + 1), file=f)

And then run your code using worst-case.txt as the input and see how long it takes. If the time limit is 5 seconds on the challenge site, then you probably want to make sure your code runs a bit quicker than that on your computer, in case the challenge site is running on a slower computer.

When you try this you'll find that the code in the post takes so long that you would be waiting a very long time for it to finish! I find that with \$T\$ reduced to 10, the code in the post takes about 3 seconds. So, extrapolating to the worst case, it would seem that when \$T = 100000\$, the code in the post would take 30,000 seconds, or about 8 hours.

Why does it take so long? Well, for every number \$n\$ in the input, it calls myfun. And then myfun constructs a list pos of all the numbers between 1 and \$n\$. But remember that \$n\$ might be as large as 1,000,000, which means that pos might have 1,000,000 elements, and a new list pos gets created for each number in the input. But there might be as many as 100,000 numbers in the input, so the line

pos.append(i)

might need to be executed as many as 100,000,000,000 times!

In this case you need to think clearly about all the steps involved in your algorithm. Try working through your algorithm step-by-step on a piece of paper. Can you spot any steps that seem useless? Can you spot any places where you needlessly compute the same thing many times?


Playing with radio input I think my code has too many functions... can it be written better?

Please help reviewing my jQuery functions.

HTML

        xxx
        <input type="radio" name="event_owner" id="xxx">

        yyy
        <input type="radio" name="event_owner" id="yyy">

<input type="text" name="_event_owner[name]" autocomplete="off" value="">

jQuery

$(".event_owner").click(function(){
    $(".event_owner input[type='text']").val($('.event_owner :radio:checked').attr("id"));
    $('.event_owner p').css({"opacity":"1"});
    $('.event_owner input').not(":checked").parent("p").css({"opacity":"0.3"});
});

// when page loaded and input(radio) saved

if($(".event_owner input[type='text']").length) {
    $("input[id="+$(".event_owner input[type='text']").val()+"]").attr("checked","checked");
    $('.event_owner input').not(":checked").parent("p").css({"opacity":"0.3"});

}

Playground

http://jsfiddle.net/Zf4UE/

There are a few points worth mentioning:

Your code does not actually function the way you want it to.
$(".event_owner input[type='text']").length checks for the existence of that element, not whether its value is set.
Your radio buttons don't have a value attribute on them. They really should.
You should cache your selectors. You're querying for .event_owner a total of 7 times in your code, and an additional 4 times on every click.
Instead of listening for every click inside .event_owner, you should instead only listen for the change event of the radio buttons.
On page load, if there's a value in the input text field, all we have to do is check the correct radio button. The change event listener will take care of the rest.

With the above in mind, here's some sample code you might consider:

var $wrapper = $('.event_owner'),
    $textInput = $wrapper.find('input[type="text"]'),
    $radios = $wrapper.find('input[type="radio"]'),
    owner = $textInput.val();

$radios.on( 'change', function () {
    $textInput.val( $radios.filter(':checked').val() );
    $radios.each(function () {
        $(this).closest('p').css( 'opacity', this.checked ? 1 : .3 );
    });
});

if ( owner ) $radios.filter('#' + owner).prop('checked', true);

Here's the fiddle: http://jsfiddle.net/Zf4UE/8/


Secret Santa - Pick Pairs Interested to hear your thoughts on this Secret Santa pairs picker.

It's pretty standard but I'll explain the rules all the same:

All names go into a hat
Each person picks a name from the hat and buys a small present for that person as a Christmas gift

No one should pick their own name from the hat so in this circumstance they should replace the name and pick another one

This becomes problematic in real life because if someone picks their own name and puts it back they have more information about who can or cannot have their name. As such, it's a prime candidate for some computational intervention.

The Worksheet (codename Sheet1) is simple. It contains a ListObject named "Names" and a button linked to the following Sub.

Option Explicit

Public Sub GenerateSecretSantaPairs()

    Dim namesRange As Range
    With Sheet1.ListObjects("Names")
        Set namesRange = Intersect(.DataBodyRange, .DataBodyRange.Columns(1))
    End With

    Dim hat As Scripting.Dictionary
    Set hat = New Scripting.Dictionary

    'fill the hat with names
    Dim r As Range
    For Each r In namesRange
        hat.Add Key:=r.Value, Item:=r.Value
    Next r

    'retain original names (in order) in an array
    Dim originalNames As Variant
    originalNames = hat.Keys

    Dim partnerNames As Variant
    ReDim partnerNames(0 To hat.Count - 1)

    Dim i As Long
    For i = 0 To hat.Count - 1
        Do
            'pick a random person from the hat
            Dim selectedName As String
            selectedName = hat.Keys(Application.WorksheetFunction.RandBetween(0, hat.Count - 1))

        Loop While originalNames(i) = selectedName And hat.Count <> 1 'try again if picked yourself unless you are the only person left in the hat

        'assign partner and remove from hat
        partnerNames(i) = selectedName
        hat.Remove selectedName

        'if last person has picked themselves then run the sub again
        If partnerNames(i) = originalNames(i) Then
            GenerateSecretSantaPairs
            Exit Sub
        End If
    Next i

    'ouput pairs
    namesRange.Offset(0, 1).Value = Application.Transpose(partnerNames)
End Sub

Very cool ideal.

Variable Declaration

Variables should be declared at the top of the subroutine. Variables should also be instantiated following the variable declarations when it makes sense to do so.  Separating variable declaration and instantiation from the body of the subroutine improves readability and allows you to better focus on the "Business Logic".

Names, Name, Name2

Naming is an essential skill that you need to develop to write good code.

Compare the following

Sheet1.ListObjects("Names") or Sheet1.ListObjects("Names") : Sheet1.ListObjects("SecretSantaTable")
Name column: Santas column
Name2 column: Children column

Random Stuff

The two lines of ↓code below↓ both target the same range

  Set namesRange = Intersect(.DataBodyRange, .DataBodyRange.Columns(1))
  Set namesRange = .ListColumns("Names").DataBodyRange

Why use a WorksheetFunction when you can use a VBA function?

  selectedName = hat.Keys(Application.WorksheetFunction.RandBetween(0, hat.Count - 1))
  selectedName = hat.Keys(Int(Rnd * hat.Count))

I would use an ArrayList instead of a Dictionary because you don't need Key/Value pairs.

Here is how I would do it. But it's still not quite right...I'll be back!

Public Sub Refactored_GenerateSecretSantaPairs()
    Dim key As Variant
    Dim n As Long, x As Long
    Dim tbl As ListObject, source As Range
    Dim Santas As Object, Children As Object
    Set tbl = Sheet1.ListObjects("SecretSantaTable")
    Set Santas = CreateObject("System.Collections.ArrayList")
    Set source = tbl.ListColumns("Santas").DataBodyRange

    If source Is Nothing Then Exit Sub
    'Remove Duplicates and Reset Source Range
    source.RemoveDuplicates (1)
    Set source = tbl.ListColumns("Santas").DataBodyRange

    For Each key In source.Value
        Santas.Add key
    Next

    Set Children = Santas.Clone

    For x = 0 To Santas.Count - 1
        Do
            n = Int(Rnd * Children.Count)
            key = Children(n)
        Loop Until key <> Santas(x) Or Children.Count = 1

        If Children.Count = 1 And Santas(x) = key Then
            key = Santas(x - 1)
            Santas(x) = Santas(x - 1)
        End If

        Santas(x) = key
        Children.Remove key
    Next

    tbl.ListColumns("Children").DataBodyRange.Value = Application.Transpose(Santas.ToArray)

End Sub

I recommend that you further simplify the Macro, reducing it to 3 lines of code:

Public Sub GenerateSecretSantaPairs()
    Sheet1.ListObjects("Names").ListColumns("Name2").DataBodyRange.Value = "Thomas Inzina"
End Sub