Project Euler Problem 5: Smallest Multiple of 1 through 20 In continuing to learn C# I solved Problem #5 with my code below. I tried to follow the suggestion to write [my] program so that it looks like the specification. Where can I improve my code?
The problem is as follows:

2520 is the smallest number that can be divided by each of the numbers from 1 to 10 without any remainder.
What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine(GetSmallestEvenlyDivisibleNumber());
            Console.ReadLine();
        }

        static bool isEvenlyDivisible(int value, int lowerBound, int upperBound)
        {
            bool returnValue = true;
            for (int i = lowerBound; i<= upperBound; ++i)
            {
                if (value % i != 0)
                {
                    returnValue = false;
                    break;
                }
            }

            return returnValue;
        }

        static int GetSmallestEvenlyDivisibleNumber(int lowerBound = 1, int upperBound = 20)
        {
            int testNumber = 2521;

            while (!isEvenlyDivisible(testNumber, lowerBound, upperBound))
            {
                testNumber += 1;
            }

            return testNumber;
        }
    }
}

Two focus areas for this review, first some style and code comments, and then a little discussion on a more optimal solution.

Code and Style Comments

Mostly good and clean code – I like to see clean, properly spaced, well named code, as it makes it a whole lot easier to understand (and easier for you to maintain in the long run).
Stick to PascalCase – This is already commented upon in another answer, but having equal naming and standards throughout your code really helps readability and eases the understanding of code. Which in turns, makes your code maintable.
Avoid magic numbers – Within GetSmallestEvenDivisibleNumber() you use 2521. This number pops out of nowhere in the context of the code. Not good. In addition, it introduces a bug if you try to find the solution for any range with upperBound < 10.
Comment your code – It is not given what the IsEvenlyDivisible method actually does, and I would spend some time commenting upon what it does. Something along the lines of Checks whether value is a proper factor of all the numbers in the range lowerBound to upperBound.

Alternate Solution

Most Euler problems have at least two major solution: The brute force solution, and an optimal solution. You've implemented the brute force of testing each and every number to see if it matches the criteria. In some cases the brute force can be improved somewhat, like skipping all the odd numbers, check for ending of 0 or 5, verify that the value is dividable by 3, and so on... This will usually lead to a somewhat faster solution, but when tackling the higher Euler Problems that'll not be enough.

Therefore it is a good idea to start looking for optimal solutions, and what is really asked for. In this case what is the number 2520, and how have they derived at it? So what we now about that number:

Is is dividable by the numbers in the range 1 through 10
By nature of previous statement, it also needs to be dividable by all of the factors of each of numbers in the range, so let us decompose the range:

\begin{align}
1 & \rightarrow 1 \\
2 & \rightarrow 2 \\
3 & \rightarrow 3 \\
4 & \rightarrow 2 * 2 \\
5 & \rightarrow 5 \\
6 & \rightarrow 2 *3 \\
7 & \rightarrow 7 \\
8 & \rightarrow 2*2*2 \\
9 & \rightarrow 3*3 \\
10 & \rightarrow 2 * 5
\end{align}

Grouping these factors we find that we have 3 * 2's, 2 * 3's, 1 * 5, and 1 * 7. Based upon these factors, we can compose any of the range, and as it turns out: \$ 2 * 2 * 2 * 3 * 3 * 5 * 7 = 8 * 9 * 5 * 7 = 2520\$

In other words if you decompose each of the numbers of your range, and then count how many of each you need to create each of the numbers, this can be used to calculate the answer directly.

To help you get on the way for the range 1 through 20 let us decompose the final numbers as well:
\begin{align}
11 & \rightarrow \mathbf{11} \\
12 & \rightarrow 2 * 2 * 3 \\
13 & \rightarrow \mathbf{13} \\
14 & \rightarrow 2 * 7 \\
15 & \rightarrow 3 * 5 \\
16 & \rightarrow 2 * 2 * 2 * \mathbf{2} \\
17 & \rightarrow \mathbf{17} \\
18 & \rightarrow 2 * 3 * 3 \\
19 & \rightarrow \mathbf{19} \\
20 & \rightarrow 2 * 2 * 5  \\
\end{align}

I've bolded the numbers not already present in our set, and those are \$11 * 13 * 17 * 19 * 2 = 92738\$. So the end result will be: \$92738 * 2520 = 232792560\$.

So there you have yourself a new challenge: Implement this alternate, but more efficient algorithm.


Generic memoize function in Swift I need to perform some expensive calculation, such as determining a Fibonacci number:
/// Calculate the Nth Fibonacci number (inefficiently)
func fib(n: Int) -> Int {
    n > 1 ? fib(n: n-1) + fib(n: n-2) : n
}

My project contains a number value types that need to perform calculations like fib based on their properties:
struct Fibber : Hashable {
    /// The index in the sequence
    var n: Int

    /// The calculated Fibonacci number at _n_
    var fibcalc: Int { fib(n: n) }
}

It works fine. But it is slow!
class FibberTests: XCTestCase {
    func testFibCalc() {
        measure { // average: 1.291, relative standard deviation: 1.5%
            var fibber = Fibber(n: 1)
            XCTAssertEqual(1, fibber.fibcalc)
            fibber.n = 25
            XCTAssertEqual(75_025, fibber.fibcalc)
            fibber.n = 39
            XCTAssertEqual(63_245_986, fibber.fibcalc)
        }
    }
}

So I make a single global dictionary that is keyed on source code location, and contains a map from a Hashable instance to the result of some arbitrary calculation:
/// Singleton global memoization cache, keyed on source code location and input hashable
private var memoized = Dictionary<String, Dictionary<AnyHashable, Any>>()

The cache key will be something like: "function:fibcalc file:Fibber.swift line:47".
Any Hashable instance can utilize this function to perform and memoize a calculation based on the key type, and return that cached value on subsequent invocations of the same call:
extension Hashable {
    /// Caches and returns the result of the `calculation` function.
    public func memoize<T>(function: StaticString = #function, file: StaticString = #file, line: Int = #line, _ calculation: (Self) -> T) -> T {
        let cacheKey = "function:\(function) file:\(file) line:\(line)"
        let hashKey = AnyHashable(self)
        if let cached = memoized[cacheKey]?[hashKey] as? T { return cached }
        if memoized[cacheKey] == nil { memoized[cacheKey] = Dictionary() }
        let calculated = calculation(self)
        memoized[cacheKey]?[hashKey] = calculated
        return calculated
    }
}

Memoizing these expensive calculations is now very simple:
extension Fibber {
    /// The cached fib. Repeated calls on the same source instance will return the memoized result for this instance.
    var fibmemo: Int { memoize(\.fibcalc) }
}

And we get an order-of-magnitude speedup!
extension FibberTests {
    func testFibMemo() {
        measure { // average: 0.132, relative standard deviation: 299.9%
            var fibber = Fibber(n: 1)
            XCTAssertEqual(1, fibber.fibmemo)
            fibber.n = 25
            XCTAssertEqual(75_025, fibber.fibmemo)
            fibber.n = 39
            XCTAssertEqual(63_245_986, fibber.fibmemo)
        }
    }
}

Assumptions:

the Hashable key will always be a value type (this isn't currently enforceable in Swift)

Non-Issues:

Thread-safety: locking can be added to the cache later
Unbounded memory growth: memoized Dictionary will be converted to an NSCache

Valid Issues:

Duck typing: the keys are AnyHashable and the values are Any, so runtime type conversion is used (yuck)

My main question is: is this a good idea? Are there any issues with the cache key using the source location?

It sounds like you're on the right track, but the cache key needn't be so specific. Unless you're storing to the same cache/dictionary as other code is using, there won't be collisions.
Since Fibonacci numbers build on the previous 2 numbers, recursion is a natural solution, but obviously gets slower for larger numbers. This post details a recursive memoization approach that seems like exactly what you're looking for:
https://www.hackingwithswift.com/plus/high-performance-apps/using-memoization-to-speed-up-slow-functions


Accessing UserControl's DataContext from another UserControl Given these classes:

public partial class MainPage : UserControl
{
    public MainPage()
    {
       InitializeComponent();
       //...

    }
}
public partial class LoginForm : StackPanel
{
   private LoginRegistrationWindow parentWindow;
   private LoginInfo loginInfo = new LoginInfo();
   public LoginForm()
   {
       InitializeComponent();
       //...
   }

   private void LoginOperation_Completed(LoginOperation loginOperation)
   {

    if (loginOperation.LoginSuccess)
    {
            // Here I need to access MainPages's DataContext property and set it with my ViewModel
    }
   }
}

I want to set MainPage's DataContext property inside LoginFrom. So I created the static instance of the MainPage class in the class itself:

public partial class MainPage : UserControl
{
    public static MainPage Instance { get; private set; }
    public MainPage()
    {
        InitializeComponent();
        Instance = this;
    }
}

Now I can access to the MainPage's DataContext this way:

MainPage.Instance.DataContext = new NotificationItemViewModel();

Now I'd like to hear your ideas about this kind of passing data between classes in a Silverlight application. Do you have any idea?

PS: actually I answered my original question here on Stackoverflow. But I'd like to hear your ideas in terms of code review.

Based on what I understood from your question here are my 5ct.

The MainPage class is the parent class of the LoginClass.

Parents should talk to children by using methods.
Children should talk to parents by using events.

So you should raise an event from the LoginClass to be consumed by the MainPage instead of accessing a property of the MainPage.

In this way you can easily test the code of the LoginForm independently of the MainPage whereas in you current implementation a MainPage always needs to be created before the LoginOperation_Completed() method can be used.

In addition in its current state your implementation has side effects because each time you create a new MainPage the Instance property will be changed.


LeetCode 560: Subarray Sum Equals K - C++/Java According to LeetCode, the following question is one of the most frequent interview questions asked by companies such as Facebook and Google. Here, I'm posting C++/Java codes, if you'd like to review, please do so.
LeetCode 560

Given an array of integers and an integer target (K), you need to find the
total number of continuous subarrays whose sum equals to target.
Example 1:
Input:nums = [1,1,1], target = 2 Output: 2
Constraints:
The length of the array is in range [1, 20,000].
The range of numbers in the array is [-1000, 1000] and the range of the integer target is [-1e7,
1e7].

C++
class Solution {
public:
    int subarraySum(vector<int> &nums, int target) {
        map<int, int> prefix_sum;
        int sum = 0, subarrays = 0;
        prefix_sum[0]++;
        for (int index = 0; index < nums.size(); index++) {
            sum += nums[index];
            subarrays += prefix_sum[sum - target];
            prefix_sum[sum]++;
        }

        return subarrays;
    }
};

Java
class Solution {
    public int subarraySum(int[] nums, int target) {
        int sum = 0, subarrays = 0;
        Map<Integer, Integer> prefixSum = new HashMap<>();
        prefixSum.put(0, 1);

        for (int index = 0; index != nums.length; index++) {
            sum += nums[index];

            if (prefixSum.get(sum - target) != null)
                subarrays += prefixSum.get(sum - target);

            prefixSum.put(sum, -~prefixSum.getOrDefault(sum, 0));
        }

        return subarrays;
    }
}

Reference

LeetCode 560
LeetCode 560 Discussion Board

I have some suggestion for the Java version
Always try to pass the size of the maximum size to the Collection / Map constructor when known
The map has a default size of 16 elements, if you have more elements, the map will have to resize its internal cache. By setting the size, you can prevent the resize and make your code faster.
In this case, you can set the maximum size since it's based on the size of the array + 1.
Map<Integer, Integer> prefixSum = new HashMap<>(nums.length + 1);

Extract the expression to variables when used multiple times
In your code, when you check if the key is present, you can extract the value to a variable to reuse it when present.
Before
if (prefixSum.get(sum - target) != null)
   subarrays += prefixSum.get(sum - target);

After
Integer currentValue = prefixSum.get(sum - target);
if (currentValue != null)
   subarrays += currentValue;

This will be better, since this will prevent the rechecking and the rehashing in the map.
Always add curly braces to loop & if
In my opinion, it's a bad practice to have a block of code not surrounded by curly braces; I saw so many bugs in my career related to that, if you forget to add the braces when adding code, you break the logic / semantic of the code.
Before
if (prefixSum.get(sum - target) != null)
   subarrays += prefixSum.get(sum - target);

After
if (prefixSum.get(sum - target) != null) {
   subarrays += prefixSum.get(sum - target);
}

Extract some of the logic to methods.
In this case, I suggest to extract the map creation to a method; this will allow to group the logic and make the main code shorter.
public int subarraySum(int[] nums, int target) {
   Map<Integer, Integer> prefixSum = buildMap(lengthOfNums + 1);
   //[...]
}

private Map<Integer, Integer> buildMap(int defaultSize) {
   Map<Integer, Integer> prefixSum = new HashMap<>(defaultSize);
   prefixSum.put(0, 1);
   return prefixSum;
}


Create new array by filter origin array I'm getting an array and I needed to find a duplicate ID inside:

var aDuplicateId = [];
            //get the duplicated configuration indexs
            for (var i = 0; i < aConfigurations.length; i++) {
                //in case the index is the duplicate array skip
                if (aDuplicateId.indexOf(i) >= 0) {
                    continue;
                }
                var iId = aConfigurations[i]._metadata.id;
                //find duplicate ids
                var iIndex = _.findLastIndex(aConfigurations, function(o) {
                    return o._metadata.id === iId;
                });
                //add the duplicate id index to array
                if (iIndex >= 0 && iIndex !== i) {
                    aDuplicateId.push(iIndex);
                }
            }

So essentially, you're just getting the unique configuration IDs. This can easily be done by converting your aConfigurations into an array of IDs and using Set to weed out duplicates. Then you can convert the set instance back into an array using Array.from.

const configIds = aConfigurations.map(config => config._metadata.id);
const configIdsSet = new Set(configIds);
const configIdsUnique = Array.from(configIdsSet);

Alternatively, without ES6, you can just use array.reduce to collect IDs as keys of an object and then use Object.keys to extract the keys into an array.

var configMap = aConfigurations.reduce(function(configs, config){
  configs[config._metadata.id] = true;
  return configs;
}, {});

var configIdsUnique = Object.keys(configMap);


Guessing a number, but comments concerning I always get marked down for my comments and I just wanted to see if these comments are acceptable or what I should include/where I should include them.

import java.util.*; //so I can use scanner

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

      Random rand = new Random ();
      int max = 100; //sets limit on the random number generator
      Scanner input = new Scanner(System.in);
      int guess;//so I can compare console input to random number
      boolean play = true;
      int totalGames = 0;
      int totalGuesses = 0;
      int bestGame = Integer.MAX_VALUE;

      System.out.println("Can you guess the word?");
      System.out.println("I am sure you cannot guess!");
      System.out.println("Go ahead and try!");
      System.out.println();

      while (play) { //so game will restart after a win

         System.out.println("I'm thinking of a number between 1 and " + max + "...");
         int numberToGuess = rand.nextInt(max) + 1;
         int numberOfTries = 0;
         //so user can continue guessing until correct
         boolean win = false;
         while (!win) {

            System.out.print("Your guess? " + numberToGuess);
            guess = input.nextInt();
            numberOfTries++;

            if (guess == numberToGuess) {
               win = true;
            } else if (guess > numberToGuess) {
               System.out.println("It's lower.");
            } else if (guess < numberToGuess) {
               System.out.println("It's higher.");
            }
            input.nextLine();
         }

         if (numberOfTries == 1) {
            System.out.println("You got it right in " + numberOfTries + " guess!");
         } else {
            System.out.println("You got it right in " + numberOfTries + " guesses!");
         }

            totalGames++;
            totalGuesses+= numberOfTries;
            System.out.print("Do you want to play again? ");

            //so user can choose to play another game

            String answer = input.nextLine();
            char firstLetter = answer.charAt(0);
            if (firstLetter == 'y' || firstLetter == 'Y') {
            play = true;
            } else {
            play = false;
         }

         bestGame = Math.min(bestGame, numberOfTries);
         System.out.println();
      }

      System.out.println("Overall results:");
      System.out.println("Total games   = " + totalGames);
      System.out.println("Total guesses = " +  totalGuesses);
      System.out.printf("Guesses/game  = %.1f \n", totalGuesses/(double)totalGames);
      System.out.println("Best game     = " + bestGame);
   }
}

Welcome to the hell of comments, and personal taste. There are multiple answers to your core question that may be right:

You have not commented enough.
You have commented all the wrong things.
You have commented too much.
Your comments are in the wrong format.
You are missing the formal comments.

To be clear though, nothing we can say will be in line with what your marks are based on, unless you can tell us why you were marked down... ;-)

About Comments in general
OK, comments are supposed to make plain what the code does not tell us already.
Let me say that again.

Comments are supposed to make plain what the code does not tell us already.

Now, whether you are marked on that, or not, is a different story. I have found that the academic application of marks for comments is often contrary to the need for comments... particularly when the code is good code.
Good code seldom needs comments.
Let me say that again:

Good code seldom needs comments.

So, follow this logic through, and then follow the inverse back...

Comments make plain what the code does not tell us already.
Good code seldom needs comments.

thus....

Good code seldom needs comments.
Good code must make plain what the code does without the need for additional explanation.

Applied to your code
Let's start at the top....

import java.util.*; //so I can use scanner

Now, if your code was:
import java.util.Scanner;

then it would not need the comment.
Your next comment is here:

  int max = 100; //sets limit on the random number generator

Well, that is, actually, not a horrible comment, or code. But, it could be better, if we gave it a decent name:
int randomGeneratorLimit = 100;

It would be even better if we made that magic number a constant:
private static final int RANDOM_GENERATOR_LIMIT = 100;

Now, no need for a comment at all.
Next up, this line:

  while (play) { //so game will restart after a win

Well, this one is interesting.... the comment is actually needed here because of the loop you are using.
If you convert this to a do-while loop, and use some functional extraction... then you can convert your code to a self-documenting, commentless block:
private static boolean playAgain() {
    System.out.print("Do you want to play again? ");
    String answer = input.nextLine();
    char firstLetter = answer.charAt(0);
    return firstLetter == 'y' || firstLetter == 'Y';
}

then you can use that function as follows:
do {

    ....... // game logic

} while (playAgain());

No need for a comment (or a play variable).
Similarly, you have the code:

     //so user can continue guessing until correct
     boolean win = false;
     while (!win) {

This should be a do-while as well, and should look like:
do {
     ..... // game logic
} while (guess != numberToGuess);

Again, that makes the logic clear, no need for a comment.
I believe that has now eliminated all of your comments..... and replaced them with code that does not need a comment, because the code is self-explanatory.
In other news, if you had more functions, with good names, then your code would be simpler to read as well.
Bottom line, though, is that the comments in your code should fill in the blanks that your code does not. In addition, your comments should give details on the motivation, and not the application of your code. You should, in general, comment only on why your code does things, not what your code is doing.
That leads on to the other comments you are missing... JavaDoc.
JavaDoc is documentation that should explain what your code does at an abstract level.
You have no JavaDoc, and you likely should. JavaDoc is where you describe what your code does, because, typically, the people reading the javadoc are not reading the code, so they need something else to tell them what the code does.


Simple Bespoke WordPress Integration Plugin I am in the process of setting up WordPress with WooCommerce, one of the requirements is a bit of integration that takes a person from another website directly to a specific product. I have set the external product references in tags on the products.

An example incoming query would be:

http://localhost/TestSite/search?q=9404

I have written a short plugin that does this.

I am an average programmer, but very new to PHP, WordPress and WooCommerce. Can you see any potential errors, inefficencies or, security issues?

Any insight would be gladly recieved.

    //Tag on Test Product: 98614
    //example URL:
    //http://localhost/TestSite/search?q=98614

    function TagLinker(){

        if (isset($_GET['q']))
        {
            $TagNumber = $_GET['q'];
            $params=array(
            'post_type' => 'product',
            'post_status' => 'publish',
            'tax_query' => array( array(
                                'taxonomy' => 'product_tag',
                                'field' => 'slug',
                                'terms' => $TagNumber
                            )
            ));
            $wc_query = new WP_Query($params);

            if ($wc_query -> have_posts() ) {
                $wc_query -> the_post();

                $product_id = get_the_ID($wc_query->ID);

                $url = get_permalink( $product_id );

                wp_reset_postdata();
                if ( wp_redirect( $url ) ) {exit;}
            };
        }
    }

add_action( 'init', 'TagLinker' );
?>

It's a good idea with frameworks to follow the method calls to see what you're actually doing, Wordpress is one of the most exploited frameworks out there and thus has fairly good security improvements. Here is the Wordpress WP_Query class you're invoking (assuming you have the latest code pulled).

It looks like the __construct calls $this->query on your $params, which calls wp_parse_args(). (If you already have the code pulled, Grep can help finding where in the code the class lives).
https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp-query.php

As far as security is concerned, your primary concern would be with the user supplied data $_GET['q']. If you do not feel that the WP_Query class is doing enough to sanitize the user data you can sanitize it yourself.

In your case, it looks like you only want the user to supply a integer? If that is the case you can filter. Adjust as necessary:

if (filter_var($_GET['q'], FILTER_VALIDATE_INT)) {
   // is int
}

Additional tips:
There is no reason to set $_GET['q'] to $TagNumber in your example, since you're not sanitizing it. Doing so only increases memory usage and complicates code review.

Be consistent about whether or not to clear between an if and "{". Most of the reason for clearing has to do with refactoring and applies mostly to class definitions.

There is no need to ?> close the script, it can add unintended white space.

Don't indent functions unless they are inside classes (looks to be intended for no reason).

Pick a style for naming variables, e.g. snake_case; not $TagName and $params, but $tag_name and $params

I hope this is helpful :-)


Removing doubles from a string I've wrote a function where the doubles from a string are removed:

def removeDoubles(string):
    output = ' '
    for char in string:
        if output[-1].lower() != char.lower():
            output += char
    return output[1:]

For example:

removeDoubles('bookkeeper') = 'bokeper'
removeDoubles('Aardvark') = 'Ardvark'
removeDoubles('eELGRASS') = 'eLGRAS'
removeDoubles('eeEEEeeel') = 'el'

As you see, it will remove every double letter from a string no matter if it's uppercase or lowercase.

I was wondering if this could be more pythonic. As I have to start with a string containing a space, output[-1] does exist. I was also wondering if it's possible to use list comprehensions for this.

Your examples are pretty useful (especially 'Aardvark'), and should be included in the documentation of the function, ideally as a doctest.  However, the problem is still underspecified: what should happen when a streak of three identical characters is encountered?  Should removeDoubles('eeek') return 'eek' (which is how I would interpret "doubles"), or 'ek' (which is what your code actually does)?

As per PEP 8, the official Python style guide, function names should be lower_case_with_underscores unless you have a good reason to deviate.  Therefore, I recommend renaming the function to remove_doubles.

Obviously, initializing output to ' ' and then dropping it with output[1:] is cumbersome and inefficient.

Fundamentally, this operation is a fancy string substitution.  Typically, such substitutions are best done using regular expressions.  In particular, you need the backreferences feature:

  Backreferences in a pattern allow you to specify that the contents of an earlier capturing group must also be found at the current location in the string. For example, \1 will succeed if the exact contents of group 1 can be found at the current position, and fails otherwise. Remember that Python’s string literals also use a backslash followed by numbers to allow including arbitrary characters in a string, so be sure to use a raw string when incorporating backreferences in a RE.

  For example, the following RE detects doubled words in a string.

>>>
>>> p = re.compile(r'(\b\w+)\s+\1')
>>> p.search('Paris in the the spring').group()
'the the'

For my interpretation of "doubles":

import re

def remove_doubles(string):
    """
    For each consecutive pair of the same character (case-insensitive),
    drop the second character.

    >>> remove_doubles('Aardvark')
    'Ardvark'
    >>> remove_doubles('bookkeeper')
    'bokeper'
    >>> remove_doubles('eELGRASS')
    'eLGRAS'
    >>> remove_doubles('eeek')
    'eek'
    """
    return re.sub(r'(.)\1', r'\1', string, flags=re.I)

To preserve your implementation's behaviour:

import re

def deduplicate_consecutive_chars(string):
    """
    For each consecutive streak of the same character (case-insensitive),
    drop all but the first character.

    >>> deduplicate_consecutive_chars('Aardvark')
    'Ardvark'
    >>> deduplicate_consecutive_chars('bookkeeper')
    'bokeper'
    >>> deduplicate_consecutive_chars('eELGRASS')
    'eLGRAS'
    >>> deduplicate_consecutive_chars('eeek')
    'ek'
    """
    return re.sub(r'(.)\1+', r'\1', string, flags=re.I)


Repair orders query in SQL Server with subselects I was wondering if you guys know how to reduce the amount of subselects in this SQL Server Query? I think I'm using bad practices in this:

WITH TotalizedRepairOrders AS
(SELECT SalesOrder,Items,DepartmentName, LastCompletedActivity,NextActivity,Plant,ParallelNextActivity  FROM OrdersSynthesis)

SELECT  Plant,
        DepartmentName,
        BlindsInQuestioning =( SELECT ISNULL(SUM(Items),0) FROM TotalizedRepairOrders WHERE NextActivity = 'NEW ORDER REVIEW' AND DepartmentName  = RO.DepartmentName),
        BlindsReadyToCross =( SELECT ISNULL( SUM(Items),0)  FROM TotalizedRepairOrders WHERE (LastCompletedActivity = 'LBM INVOICE' OR NextActivity  = 'LBM INVOICE' OR NextActivity ='PLANT ARRIVAL')  AND  DepartmentName  = RO.DepartmentName),
        BlindsPendingForShipmentCreation = ( SELECT  ISNULL(SUM(Items),0)  FROM TotalizedRepairOrders WHERE ( NextActivity  = 'RETURN SHIPMENT CREATION')  AND  DepartmentName  = RO.DepartmentName),
        PendingForInstructions = ( SELECT  ISNULL(SUM(Items),0)  FROM TotalizedRepairOrders WHERE ( ParallelNextActivity = 'INSTRUCTIONS' )  AND  DepartmentName  = RO.DepartmentName),
        PendingToBeRepaired = ( SELECT ISNULL( SUM(Items),0)  FROM TotalizedRepairOrders WHERE ( NextActivity = 'PRODUCTION' )  AND  DepartmentName  = RO.DepartmentName),
        PendingForQualityControl = ( SELECT  ISNULL(SUM(Items),0)  FROM TotalizedRepairOrders WHERE ( NextActivity = 'INSPECTION')  AND  DepartmentName  = RO.DepartmentName),
        PendingToBePacked = ( SELECT  ISNULL(SUM(Items),0)  FROM TotalizedRepairOrders WHERE ( NextActivity = 'PACKING LABEL')  AND  DepartmentName  = RO.DepartmentName)

FROM TotalizedRepairOrders RO GROUP BY  Plant ,DepartmentName

Here is a simpler method of your query, which doesn't use a CTE and a bunch of sub-selects.

SELECT
    Plant
    DepartmentName,
    SUM(CASE WHEN NextActivity = 'NEW ORDER REVIEW' THEN ISNULL(Items,0) ELSE 0 END) AS BLindsInQuestion,
    SUM(CASE WHEN LastCompletedActivity = 'LBM INVOICE' OR NextActivity  = 'LBM INVOICE' OR NextActivity ='PLANT ARRIVAL' THEN ISNULL(Items,0) ELSE 0 END) as BlindsReadyToCross
    /*
    etc
    */
FROM
    TotalizedRepairOrders
GROUP BY  Plant ,DepartmentName


Hackerrank Day 9: Recursion Write a factorial function that takes a positive integer, N as a parameter and prints the result of N! (N factorial).

  Note: If you fail to use recursion or fail to name your recursive function factorial or Factorial, you will get a score of 0 .

Here's my solution in C++:

#include <iostream>

int factorial( int n ) {

    if(n<=1)

        return 1 ;

    else{

        return n * factorial (n-1) ;

    }

}

int main() {

    int n;

    std::cin >> n;

    std::cout << factorial (n) ;

}

Is there a more efficient solution to this problem?

Yes, there is a more efficient solution that still uses recursion; specifically, using Tail Recursion.

When you return n * factorial(n-1);, the compiler can’t optimize the call away because it still has more work to do when the call to factorial(n-1) returns; it has to do the multiplication, and return the result.

If, instead, you ended the function with return factorial(arguments);, the compiler can optimize away both the call and the return with a jump to the start of the factorial function, with the new arguments.  No additional stack frames are required.  The arguments you need to pass would be the original argument(s) and any “running total”.

int factorial(int n, int product=1) {
    if ( n <= 1 )
        return product;
    return factorial(n-1, n*product);
}

The difference between your implementation and a tail-recursive one can be seen by trying to calculate factorial(1000000000).  Your implementation will require 1 billion stack frames, which should likely crash the program.  A tail-recursive algorithm uses only 1 stack frame, so it will return the (incorrect) answer of zero.

(Why zero?  Assuming 32-bit integer, 34! has 17 even numbers, 8 multiples of 4, 4 multiples of 8, 2 multiples of 16, and a 32, which means the answer is has 17+8+4+2+1=32 powers of 2.  And any number multiplied by 2^32, modulo 32-bits is zero.  Any higher factorial is a multiple of 34!, so will also evaluate as zero.)


Reduce array of objects into object of arrays I have an array of objects where each object has two keys: key (k) and value (v). I need to make an object that would take values of k and collect corresponding values of v as a value of this key, an array of them.

So, for example, from an array

[{
  k: 'a',
  v: 0
}, {
  k: 'a',
  v: 1
}, {
  k: 'b',
  v: 2
}]

I need an object

{
  a: [0, 1],
  b: [2]
}

It's easy to use outer-scope variables but what I want to do is to perform it in functional manner.

Right now, I have this piece of (ES6) code:

rows.reduce((result, item) => ({
  ...result,
  [item.k]: [
    ...(result[item.k] || []),
    item.v
  ]}),
{});

It works nice but the question is if it is possible to optimize it in terms of SLOC. For one, I don't like the ...(result[item.k] || []) piece but if I leave it as simple as ...result[item.k] then I get an error because, when it's undefined, babel fails to perform a method call of it, since undefined doesn't have any methods.

Did I maybe take a wrong path and there's a more elegant solution that reducing the array?

I would appreciate any suggestions, please throw any ideas at me.

We've got a readability issue here. This is the horror we all have to face when we overuse the new "concise" ES6 syntax. I had to read the code at least 3 times to understand what it was doing (and forgot ... is spread).

If you're worried about code size, let the minifier do its job. Code is for programmers to understand. The verbose version of it isn't that bad. It's not like it's not going to run in older browsers.

var result = rows.reduce(function(result, item){
  result[item.k] = (result[item.k] || []).concat(item);
  return result;
}, {});


An Edit action in ASP.NET MVC, with handling for missing id parameter I have a standard Edit action in Asp.Net MVC 5 and I want to avoid throwing the unhandled exception when is made a get request without the id like ~/food/edit, so I did this.

public ActionResult Edit(int id = 0)
{
    if (id == 0)
    {
        return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
    }

    string result = _foodAppService.GetById(id);
    FoodVm food = string.IsNullOrEmpty(result)
        ? null
        : JsonConvert.DeserializeObject<FoodVm>(result);

    if (food == null)
    {
        return RedirectToAction("Index");
    }

    return View(food);
}

My question is: Is it a good practice to handled it in this way or there are more suitable strategies ?

I'm new to this asking question thing, if a should I ask in another way, just let me know, thank you for your time.

I think "BadRequest" is OK for that case. Alternative options are "PageNotFound" or "redirect to index".

Some other points about your code:

0 is usually a valid ID so I would use -1 as default, even if that is not true in your case.
You could check for null or empty first to simplify the code

.

public ActionResult Edit(int id = -1)
{
    if (id < 0)
    {
        return new HttpStatusCodeResult(HttpStatusCode.BadRequest);
    }

    string result = _foodAppService.GetById(id);
    if (string.IsNullOrEmpty(result))
    {
        return RedirectToAction("Index");
    }

    var food = JsonConvert.DeserializeObject<FoodVm>(result);
    return View(food);
}


PHP - Deleting Directory Contents & SubDirectory Contents I've set up some PHP to delete a directory, it's contents, and any subdirectory and its contents... I'm new to PHP so I'm most definitely doing something WRONG or am doing something in the most inefficient way.

Looking for some references or suggestion on how to do this better...

Using PHP 5.3.8.

First Draft

if ($handle = opendir($main_dir)) {
    while (false !== ($entry = readdir($handle))) {
        $absolute_path = $main_dir.'/'.$entry;
        if ($entry != "." && $entry != "..") {
            chmod($absolute_path, 0755);
            unlink($absolute_path);

            //check if any folders exist, then delete files within
            if (file_exists($absolute_path) && is_dir($absolute_path)) {
                if ($child_handle = opendir($absolute_path)) {
                    while (false !== ($child_entry = readdir($child_handle))) {
                    $child_absolute_path = $absolute_path.'/'.$child_entry;
                        if ($child_entry != "." && $child_entry != "..") {
                            unlink($child_absolute_path);
                        }
                    }
                    closedir($child_handle);
                }
            }
            rmdir($absolute_path);
        }
    }
    closedir($handle);
}
rmdir($main_dir);

how about something like this...

Second Draft

 $dir = "path/to/dir";
 chmod($dir, 0755);
 $di = new RecursiveDirectoryIterator($dir, FilesystemIterator::SKIP_DOTS);
 $ri = new RecursiveIteratorIterator($di, RecursiveIteratorIterator::CHILD_FIRST);
 foreach ( $ri as $file ) {
     $file->isDir() ?  rmdir($file) : unlink($file);
 }
 rmdir($dir);

Any thoughts? Much appreciated!

function empty_dir($dir) {
    $iterator = new RecursiveIteratorIterator(new RecursiveDirectoryIterator($dir),
                                              RecursiveIteratorIterator::CHILD_FIRST);
    foreach ($iterator as $path) {
      if ($path->isDir()) {
         rmdir($path->__toString());
      } else {
         unlink($path->__toString());
      }
    }
    rmdir($dir);
}

from: http://php.net/manual/en/class.recursivedirectoryiterator.php


Using std::unique_ptr and std::move I'm trying out new things (on class Inproc), specifically using std::unique_ptr and std::move, and I'm wondering if I'm doing it right. The code compiles so that gives me hope. Still, what do you think?

Note: I'm working without exceptions for reasons I won't go into. That means the factory functions are required to handle errors. Keep that in mind please.

class Fan {
public:
    static Fan * create(Fan *peer = NULL) { return new Fan(peer); }
    virtual ~Fan();
private:
    Fan(Fan *peer__ = NULL) : peer_(peer__) { };
    Fan *peer_;
};

class Inproc {
public:
    static Inproc * create();
    ~Inproc();
private:
    Inproc(std::unique_ptr<Fan> bound__,
           std::unique_ptr<Fan> connected__);
    std::unique_ptr<Fan> bound_;
    std::unique_ptr<Fan> connected_;
};

Inproc * Inproc::create() {
    std::unique_ptr<Fan> bound(Fan::create());
    if (bound) {
        std::unique_ptr<Fan> connected(Fan::create(bound.get()));
        if (connected) {
            return new(std::nothrow) Inproc(std::move(bound), std::move(connected));
        }
    }
    return NULL;
}

Inproc::Inproc(std::unique_ptr<Fan> bound__,
               std::unique_ptr<Fan> connected__)
    : bound_(std::move(bound__)), connected_(std::move(connected__)) { }

Programming / detail review

Fan(Fan *peer__ = NULL);

  (and some more occurrences)

Identifiers that contain a double underscore are reserved for the compiler/libary ("C++ implementation").

The NULL macro can be thought of as deprecated. Use the nullptr keyword instead. It is more typesafe.

static Fan * create(Fan *peer = NULL);

Since those pointers you return from create own a resource (dynamically allocated memory / an object with dynamic storage duration), you should consider making them smart pointers:

static std::unique_ptr<Fan> create(Fan *peer = nullptr);

The function parameter can remain a raw pointer, since it does not convey/transfer ownership. One could argue that some dumb pointer wrapper that explicitly states it does not own a resource might be more appropriate.

Inproc(std::unique_ptr<Fan> bound__,
       std::unique_ptr<Fan> connected__);

In your original version, you used a std::unique_ptr<Fan>&&. Let's examine the alternatives:

void myfunc(std::unique_ptr<Fan>);
void myfunc(std::unique_ptr<Fan>&&);
void myfunc(std::unique_ptr<Fan>&);
void myfunc(std::unique_ptr<Fan> const&);

Only the first guarantees that the unique_ptr you pass in will be empty afterwards. All others do not, by means of (parameter) types, tell you what happens to your pointer. For more details, see the solution of GotW #105: Smart Pointers, Part 3

If you want to always acquire ownership of the pointer argument, you should define your interface using pass-by-value for unique_ptrs.

Let's see how we can refactor Inproc::create() using these changes:

unique_ptr<Inproc> Inproc::create() {
    if (auto bound = Fan::create() ) {
        if (auto connected = Fan::create(bound.get())) {
            return new(std::nothrow) Inproc(std::move(bound), std::move(connected));
        }
    }
    return nullptr;
}

Note that by returning nullptr in all cases of an error, you lose information about what error occurred.

Design review

private:
    Fan(Fan *peer__ = NULL);

Derived classes can only copy a Fan, they cannot create a new one. This seems contradictory to the virtual dtor, which seems to provide "support" for inheritance.

static Fan * create(Fan *peer = NULL);

From the code shown, it is unclear to me why you need to restrict Fans to be created via this factory function (is there a pattern name for this?). It certainly decreases reusability of this class.

For Inproc::create(), the situation could even be worse: You cannot test Inproc independently of Fan, since you can't provide some kind of mocked Fan to construct it. Also, it doesn't seem to require the dynamic storage duration of its unique_ptr subobjects; but again, we don't see the whole program here.

nwp's answer reminded me that you might use pointers to indicate an error. I think that is not a good reason to require dynamic storage duration (-> new + pointers). One solution could be a factory function that returns an optional, or that creates objects in-place at a location passed as a parameter. The former can even be used easily for aggregation; neither can easily be used for inheritance. Another option is to introduce an empty / error state, that the object is set to if construction fails. I don't really like the idea of "empty states" for a class, since those weaken the guarantees / invariants it can provide. But it's a common sight to be able to implement e.g. default constructors. Look at std::thread, std::Xstream etc. - they all have default constructors, even though they leave the object in an almost unusable state.


Pretty print with unit and physical suffix II Follow up of Pretty print with unit and physical suffix

My first code didn't process negative numbers correctly. Additionally I tried to use all of the helpful input to come up with this solution.

I considered:

precision
testing
spacing tips

P.S.: I use the 'u' instead of U+03BC because I had some difficulties displaying it on my Windows console.

# -*- coding: utf-8 -*-

import math

def eng_format(x, unit='', precision=3):
    UNITS = [' ', ' k', ' M', ' G'] + \
            ([None] * 10) + \
            [' f', ' p', ' n', ' u', ' m']

    power_of_1000 = int(math.floor(math.log10(abs(x)) // 3))
    exponent = 3 * power_of_1000
    prefix = UNITS[power_of_1000]
    if prefix is None:
        prefix = '*10^%d ' % exponent

    predecimal = 1+int(math.floor(math.log10(abs(x)) % 3))
    significand = x * 10**(-exponent + precision - predecimal)
    significand = round(significand) / 10**(precision - predecimal)

    postdecimal = precision - predecimal
    if(postdecimal < 0):
        postdecimal = 0
    FormatStr = '{:.%df}{:s}{:s}'%(postdecimal)
    return FormatStr.format(significand, prefix, unit)

if __name__ == '__main__':
    import unittest

    class TestPrettyPrinting(unittest.TestCase):
        def setUp(self):
            self.seq = list()
            self.seq.append( (0.002777, 'V', 3, '2.78 mV') )
            self.seq.append( (0.02777, 'V', 3, '27.8 mV') )
            self.seq.append( (0.2777, 'V', 3, '278 mV') )
            self.seq.append( (2.777, 'V', 3, '2.78 V') )
            self.seq.append( (27.77, 'V', 3, '27.8 V') )
            self.seq.append( (2./3, 'V', 6, '666.667 mV') )
            self.seq.append( (0.000003221, 'A', 3, '3.22 uA') )
            self.seq.append( (30589, 'A', 4, '30.59 kA') )
            self.seq.append( (60000000, 'W', 6, '60.0000 MW') )
            self.seq.append( (6000000000000000, 'W', 1, '6*10^15 W') )
            self.seq.append( (.0000000000000006, 'W', 4, '600.0*10^-18 W') )

        def test_eng_format(self):
            for value, unit, precission, expected_result in self.seq:
                for si, s in enumerate(['', '-']):
                    new_expected_result = s + expected_result
                    result = eng_format(value * (-1)**si, unit, precission)
                    print "%20s %20s"%(result, new_expected_result)
                    self.assertEqual(result, new_expected_result)

    unittest.main()

The computation of postdecimal can be simplified to postdecimal = max(precision - predecimal, 0).

eng_format needs a docstring.

On self.seq:

It needs a meaningful name like examples.
It should be local to test_eng_format, since that's the only place it's used.
It can be built in one expression, without mutation:

examples = [(0.002777, 'V', 3, '2.78 mV'),
            (0.02777, 'V', 3, '27.8 mV'),
            ...]

However, looping over a list of test cases is less readable than a bunch of self.assertEqual(eng_format(-0.1, 'A', 6), '-100.000 mA') calls.

test_eng_format should not print all its test cases. Tests should be silent unless they fail.


Array inside array (comment system with 2 levels) I have a comment system on 2 levels (like Blizzard http://eu.battle.net/sc2/en/blog/7935264/#c-7257875753), it's working fine but my code is a bit dirty :

// $datacoms array from mysql_query
foreach($datacoms as $com){
    if(!$com->parent_id){
        $level_id = $com->id;
        // include the view (display the comment)

        foreach($datacoms as $com){
            if($com->parent_id == $level_id){
                // include the view (display the comment)
            }
        }
    }
}

Any better way to do it ?

Depends on how your data is modeled. I dont know that, but I can assume that you use three relational tables. Something along these lines:

Post [parent]
+-------------+---------+
|    Field    |  Type   |
+-------------+---------+
| id          | int     |
| value       | varchar |
+-------------+---------+

Comment [parent-child]
+-------------+---------+
|    Field    |  Type   |
+-------------+---------+
| id          | int     |
| foreign_key | int     |
| value       | varchar |
+-------------+---------+

Comment->Comment [child]
+-------------+---------+
|    Field    |  Type   |
+-------------+---------+
| id          | int     |
| foreign_key | int     |
| value       | varchar |
+-------------+---------+

You are looking to potentially run three queries. An easy fix would be to add a has_children field to each parent table. With that you would save a potential checkup and easily release some complexity in the PHP loop.

    [parent-child]
    +--------------+---------+
    |    Field     |  Type   |
    +--------------+---------+
    | id           | int     |
    | foreign_key  | int     |
    | has_children | bool    | <===
    | value        | varchar |
    +--------------+---------+

I can provide the PHP code if you like, but I'm assuming that you get the general idea.


Enum string parse and get custom attribute assign to array I wrote a code which:
- parse string to my enum
- get description from enum
- assign to string[] array

private readonly string[] allowed;

public AuthorizeRoles(params object[] arrayParam)
{
    allowed = (arrayParam.Select(myField => Enum.Parse(typeof (MyEnum), myField.ToString()))
        .Select(temp => temp.GetType().GetField(temp.ToString()))
        .Select(field => GetCustomAttribute(field, typeof (DescriptionAttribute)))).OfType<DescriptionAttribute>()
        .Select(attribute => attribute.Description)
        .ToArray();
}

Could you tell me if it possible to improve this code for better efficiency and clean code.

Overall code seems OK then I highlight just few minor issues (and I propose a slightly bigger change to simplify code).

1.

public AuthorizeRoles(params object[] arrayParam)

arrayParam is not a descriptive name, what is it? List of roles? Make it clear changing name to - for example - roles.

2.

arrayParam.Select(myField => Enum.Parse(typeof(MyEnum), myField.ToString()))

You're converting each arrayParam item to a string however you're not doing any check, if one of them is null then code will fail with NullReferenceException for a long line packed with a lot of code. Not so helpful. You may check in advance with:

if (arrayParam.Any(x => x == null))
    throw new ArgumentException("...");

Otherwise just let Enum.Parse() fail and use Convert.ToString(myField) won't throw for null input. Note that if type is restricted to be string or MyEnum then you may change your function definition accordingly:

public AuthorizeRoles(params string[] arrayParam)
public AuthorizeRoles(params MyEnum[] arrayParam)

Note that a weird user may have this:

object[] pars = null; // It may be a function argument...
AuthorizeRoles(pars);

Then you should also check if arrayParam is null itself:

if (arrayParam == null)
    throw new ArgumentNullException("arrayParam");

3.

.Select(field => GetCustomAttribute(field, typeof(DescriptionAttribute))))
.OfType<DescriptionAttribute>()

In this case you do not need OfType<DescriptionAttribute>() because values are just of one type, it's a minor optimization but I think it's better to don't confuse future readers and Cast<DescriptionAttribute>() or directly (DescriptionAttribute) before GetCustomAttribute().

Now if it's allowed to change little bit your code I think it may be simplified. Pick this part:

arrayParam.Select(myField => Enum.Parse(typeof(MyEnum), myField.ToString()))
    .Select(temp => temp.GetType().GetField(temp.ToString()))

First you convert input values to strings then you parse them to have MyEnum and finally you convert them back to string to use them with GetField(). One step should be omitted:

arrayParam.Select(x => Convert.ToString(x))
    .Select(x => typeof(MyEnum).GetField(x))

GetCustomAttribute() is a custom helper function, if you change it to this prototype:

private static T GetCustomAttribute<T>(FieldInfo field)
    where T : Attribute

Then last few lines of your code may be simplified (cast is not required and last .Select() may be merged in previous one). Note that with the generic argument T we don't even need a Type parameter (you can obtain type using typeof(T)).

Then if we rewrite whole function for an overview we have something like this:

public AuthorizeRoles(params object[] roles)
{
    if (roles == null)
        throw new ArgumentNullException("roles");

    if (roles.Any(x => x == null))
        throw new ArgumentException("...");

    allowed = roles.Select(x => Convert.ToString(x))
        .Select(x => typeof(MyEnum).GetField(x))
        .Select(x => GetCustomAttribute<DescriptionAttribute>(x).Description)
        .ToArray();
}

The two .Select() may be merged but I'd not scarify readability just to keep code shorter and a minor performance gain (after all we're working with Reflection here...) You may also want to add some checks before the second .Select() or inside GetCustomAttribute<T>() to throw a meaningful error message if one of given values is not a valid enum element.


Bubble Sort with tests I've become a bit rusty in Java. This is an attempt at brushing up my skills.

  Bubble sort, sometimes referred to as sinking sort, is a simple sorting algorithm that repeatedly steps through the list, compares adjacent elements and swaps them if they are in the wrong order. The pass through the list is repeated until the list is sorted. The algorithm, which is a comparison sort, is named for the way smaller or larger elements "bubble" to the top of the list.

Code

import java.util.Comparator;
import java.util.Objects;

public class Sorting {
    /**
     * Day 07 - Bubble Sort
     * In place sort given array with bubble sort algorithm
     *
     * @param data       array
     * @param comparator implementation of comparator for given data type
     * @param ascending  sort order
     * @param <T>        data type of array elements
     * @return sorted data array
     */
    public static <T> T[] bubbleSort(T[] data, Comparator<T> comparator, boolean ascending) {
        Objects.requireNonNull(data);
        int len = data.length;
        if (len <= 0) return data;
        T temp;
        boolean swapped;
        for (int i = 0; i < len - 1; i++) {
            swapped  = false;
            for (int j = 0; j < len - 1 - i; j++) {
                if ((comparator.compare(data[j], data[j + 1]) * (ascending ? 1 : -1)) > 0) {
                    temp = data[j];
                    data[j] = data[j + 1];
                    data[j + 1] = temp;
                    swapped = true;
                }
            }
            if (!swapped) break;
        }
        return data;
    }
}

Test cases

import org.testng.Assert;
import org.testng.annotations.Test;

public class TestSorting {
    @Test
    public void testBubbleSort() {
        Assert.assertEquals(asc(1, 2, 3), ar(1, 2, 3));
        Assert.assertEquals(asc(3, 2, 1), ar(1, 2, 3));
        Assert.assertEquals(asc(2, 0, -1, 2, 1), ar(-1, 0, 1, 2, 2));

        Assert.assertEquals(dsc(1, 2, 3), ar(3, 2, 1));
        Assert.assertEquals(dsc(3, 2, 1), ar(3, 2, 1));
        Assert.assertEquals(dsc(2, 0, -1, 2, 1), ar(2, 2, 1, 0, -1));

        Assert.assertEquals(asc(2, 1), ar(1, 2));
        Assert.assertEquals(asc(1), ar(1));
        Assert.assertEquals(asc(), ar());
        Assert.assertEquals(dsc(1), ar(1));
        Assert.assertEquals(dsc(), ar());
    }

    @Test(expectedExceptions = {NullPointerException.class})
    public void testNullExcept() {
        Sorting.bubbleSort(null, Integer::compare, true);
    }

    private static Integer[] ar(Integer... objects) {
        return objects;
    }

    private static Integer[] asc(Integer... objects) {
        return Sorting.bubbleSort(objects, Integer::compare, true);
    }

    private static Integer[] dsc(Integer... objects) {
        return Sorting.bubbleSort(objects, Integer::compare, false);
    }
}

Your code looks generally OK in my book (i.e. not being that rusty ;-))

A few pointers though:

Generally keep variable scope as low as possible. T temp and boolean swapped can be declared at the place where they are needed initially.
Though I see the idea of multiplying with 1 or -1, the expression looks too "magic" for me. As Comparators have a reversed() method these days, I recommend to use that.

Putting this advice to use:

public static <T> T[] bubbleSort(T[] data, Comparator<T> comparator, boolean ascending) {
    Objects.requireNonNull(data);
    int len = data.length;
    if (len <= 0) return data;
    Comparator<T> actualComparator = ascending ? comparator : comparator.reversed();

    for (int i = 0; i < len - 1; i++) {
        boolean swapped  = false;
        for (int j = 0; j < len - 1 - i; j++) {
            if (actualComparator.compare(data[j], data[j + 1]) > 0) {
                T temp = data[j];
                data[j] = data[j + 1];
                data[j + 1] = temp;
                swapped = true;
            }
        }
        if (!swapped) break;
    }
    return data;
}


Running async tasks and cancelling after a timeout if necessary - follow up (1) The previous question was a really bad design where I had all the logic in the Main method. The only way to test it was to look at the output in the console. I now encapsulated it in an extension method and wrote two unit-tests for it - here I'm not sure about the part using the Stopwatch for checking how long the execution took - do you think this is the right way to do it? I guess the implementation of the extension alone should now be better.

The purpose of this extension is to execute multiple jobs in parallel and cancel the ones that take too much time to complete.

public static void WaitAll(this IEnumerable<Action<CancellationToken>> actions, TimeSpan timeout)
{
    using (var cancellationTokenSource = new CancellationTokenSource())
    {
        var tasks = actions.Select(action => Task.Run(() => action(cancellationTokenSource.Token))).ToArray();
        if (!Task.WaitAll(tasks, timeout))
        {
            cancellationTokenSource.Cancel();

            // Wait for cancellation, if necessary.
            Task.WaitAll(tasks);
        }
    }
}

I also have another overload that does not use the timeout.

public static void WaitAll(this IEnumerable<Action<CancellationToken>> jobs) => WaitAll(jobs, TimeSpan.FromMilliseconds(-1));

And the test class:

public class ActionExtensionsTest
{
   public TestContext TestContext { get; set; }

    [TestMethod]
    public void WaitAll_JobsFinishInTime_NoCancellation()
    {
        var jobs = new TestJob[]
        {
        new TestJob { WorkInSeconds = 6, Log = m => TestContext.WriteLine(m) },
        new TestJob { WorkInSeconds = 2, Log = m => TestContext.WriteLine(m) },
        new TestJob { WorkInSeconds = 4, Log = m => TestContext.WriteLine(m) },
        };

        var sw = Stopwatch.StartNew();
        jobs.Select(j => new Func<CancellationToken, Task>(j.Start)).WaitAll();
        Assert.IsTrue(sw.Elapsed.TotalSeconds < 7);
    }

    [TestMethod]
    [ExpectedException(typeof(AggregateException))]
    public void WaitAll_OneJobTimesout_Canceled()
    {
        var jobs = new TestJob[]
        {
        new TestJob { WorkInSeconds = 6, Log = m => TestContext.WriteLine(m) },
        new TestJob { WorkInSeconds = 2, Log = m => TestContext.WriteLine(m) },
        new TestJob { WorkInSeconds = 4, Log = m => TestContext.WriteLine(m) },
        };

        var sw = Stopwatch.StartNew();
        jobs.Select(j => new Func<CancellationToken, Task>(j.Start)).WaitAll(TimeSpan.FromSeconds(5));
        Assert.IsTrue(sw.Elapsed.TotalSeconds < 6);
    }

    class TestJob
    {
        private static int counter;

        static TestJob() => counter++;

        public string Name => $"{nameof(TestJob)}{counter}";

        public int WorkInSeconds { get; set; }

        public Action<string> Log { get; set; }

        public async Task Start(CancellationToken cancellationToken)
        {
            Log(Name + " started.");
            for (int i = 0; i < WorkInSeconds; i++)
            {
                cancellationToken.ThrowIfCancellationRequested();
                await Task.Delay(TimeSpan.FromSeconds(1));
            }
            Log(Name + " finished.");
        }
    }
}

Looks good. One potential improvement is to make WaitAll method async. Maybe:

//return false on timeout
public static async Task<bool> RunAsync(this IEnumerable<Action<CancellationToken>> actions, TimeSpan timeout)

This way you can support two additional scenarios:

actions.RunAsync().Wait(); //sync wait (what you currently have)
await actions.RunAsync(); //await
actions.RunAsync().ContinueWith(...); //async call, no waiting

You will probably have to use Task.WhenAll instead of Task.WaitAll to make this happen.

Also  1s timeout is not something I would expect when I see actions.WaitAll() in code. Default timeout in all BCL classes I can think of is Infinite, you should probably follow this practice for consistancy.


Conway's Game of Life in Python, saving to an image I have attempted to make conways game of life in Python, and then save the output into a picture.

Is there any way to make this more efficient?
Is there anything wrong with the logic? (Most of the pictures don't look quite correct)
Other?

import PIL.Image, random

WIDTH = 1366
HEIGHT = 768
ROUNDS = 10

DEAD = (0, 0, 0)
ALIVE = (0, 64, 255)

print("Creating image")
img = PIL.Image.new("RGB", (WIDTH, HEIGHT))
data = img.load()

print("Creating grid")
grid = []
for y in range(HEIGHT):
    grid.append([])
    for x in range(WIDTH):
        grid[y].append(random.randint(0, 1))

for i in range(ROUNDS):
    print("Starting round", i + 1, "of", ROUNDS)
    for y in range(HEIGHT):
        for x in range(WIDTH):
            n = 0
            for y2 in range(-1, 2):
                for x2 in range(- 1, 2):
                    if x2 != 0 and y2 != 0 and grid[(y + y2) % HEIGHT][(x + x2) % WIDTH] == 1:
                        n += 1
            if n < 2:
                grid[y][x] = 0
            elif n > 3:
                grid[y][x] = 0
            elif grid[y][x] == 1 and n > 1 and n < 4:
                grid[y][x] = 1
            elif grid[y][x] == 0 and n == 3:
                grid[y][x] = 1

print("Rendering image")
for y in range(HEIGHT):
    for x in range(WIDTH):
        if grid[y][x] == 1:
            data[x, y] = ALIVE
        else:
            data[x, y] = DEAD

print("Saving image")
img.save("gofl.png")

Style

Your code reads nice, you made a good job following PEP8, except for the 2 imports on the same line and a line exceeding 80 characters.

Other than that you’re missing Python's higher-level constructs and your logic is slightly off.

List comprehension

Creating the grid

Building a list using append in a for loop is considered an anti-pattern in Python. You should use a list-comprehension instead:

# No
l = []
for i in range(X):
    l.append(get_data(i))

# Yes
l = [get_data(i) for i in range(X)]

In case you want a list of lists (as you do for grid) you can easily combine them:

grid = [[random.randint(0, 1) for _ in range(WIDTH)] for _ in range(HEIGHT)]

Note the use of _ to show that we won't use the iteration variable and that we are only interested in repeating the same operation a certain amount of time.

Counting neighbours

You can use almost the same syntax than list-comprehensions to create generator expressions: simply change the surrounding brackets into parenthesis. The difference being that generator expressions won't produce the results at once and store them in memory. Instead, they will be generated as needed but you’ll be able to read them only once.

This is perfect when you don't need to store an intermediate list just to iterate once over it. For instance, if we were to feed a list into the sum function, and that list was built using a list-comprehension, it's probably better to use a generator expression instead.

And that's what is better to count neighbours of a given cell: use the sum builtin with a generator expression. It will also help removing that annoying bug of yours in that big if that tries to remove the current cell out of the count. Instead you’re only counting the neighbours that are in the corner of the surrounding square (I means, those which have neither x2 == 0 nor y2 == 0.) Instead of trying to come up with broken conditions, you’d better of adding the 9 cells and then removing the value of the current one.

So, how do we turn these for loops into generator expressions:

# Assuming NEIGHBOURS = range(-1, 2)
n = sum(
    sum(row[(x+x2) % WIDTH] for x2 in NEIGHBOURS)
    for row in grid[(y+y2) % HEIGHT] for y2 in NEIGHBOURS
)

Here I used a subexpression within the main expression, pretty much like I did with the two for loops that created grid. Setting NEIGHBOURS = range(-1, 2) is also a good idea as it adds meaning and removes duplication.

To get the actual number of neighbours, you just need to substract grid[y][x] from that.

Iterations

In Python we don't like to iterate over a sequence using indexes. We have constrcuts that allow us to directly iterate over elements and it’s faster to do so. But sometimes, like here, you also need the indexes (to update the original data-structure, for instance.) In such case, it's prefered to use enumerate instead of range(len):

for y, row in enumerate(grid):
    for x, cell in enumerate(row):
        data[x, y] = ALIVE if cell else DEAD

Same logic for each ROUND.

You can also note the use of the ternary operator to simplify the logic a bit. Speaking of that…

Logic simplification

Using the ternary was a first step but I think that the following construct reads good as well:

DEAD_OR_ALIVE = (
    (0, 0, 0),
    (0, 64, 255),
)

...

for y, row in enumerate(grid):
    for x, cell in enumerate(row):
        data[x, y] = DEAD_OR_ALIVE[cell]

It might also be a little bit faster (even though I didn't time it).

Handling birth and death of cells can also be simplified: a neighbour's count of 3 will either keep the current cell alive or create a new one in the current slot: grid[y][x] = 1 in both cases; a count of 2 will leave the current slot untouched; and anything else will kill the current cell:

for y, row in enumerate(grid):
    for x, cell in enumerate(row):
        n = ...
        if n == 3:
            grid[y][x] = 1
        elif n == 2:
            pass
        else:
            grid[y][x] = 0

Logic management

One thing that I don't quit get is why you create the image at the beginning of the program only to populate it after all computation is done. You could create it near the end and have related bits of logic near of each other.

Better, you could wrap each logical piece of stuff into it's own function to ease testing and reusability (you can import you file in an interactive shell and call your functions from there with varying parameters.) Doing that you could simplify your constants by using default values for parameters.

You can also improve the overall layout by adding an if __name__ == '__main__' clause to wrap the remaining of your top-level code into.

Proposed improvements

import PIL.Image
import random

WIDTH = 1366
HEIGHT = 768
ROUNDS = 10
NEIGHBOURS = range(-1, 2)

def random_grid(width=WIDTH, height=HEIGHT):
    return [
        [random.randint(0, 1) for _ in range(width)]
        for _ in range(height)
    ]

def compute_next_generation(grid, width=WIDTH, height=HEIGHT):
    for y, row in enumerate(grid):
        for x, cell in enumerate(row):
            neighbours = sum(
                grid[(y+yy) % height][(x+xx) % width]
                for yy in NEIGHBOURS
                for xx in NEIGHBOURS
            ) - cell
            if neighbours == 3:
                grid[y][x] = 1
            elif neighbours == 2:
                pass
            else:
                grid[y][x] = 0

def convert_grid_to_image(grid, dead_or_alive=((0, 0, 0), (0, 64, 255))):
    image = PIL.Image.new('RGB', (len(grid[0]), len(grid)))
    canvas = image.load()
    for y, row in enumerate(grid):
        for x, cell in enumerate(row):
            canvas[x, y] = dead_or_alive[cell]
    return image

if __name__ == '__main__':
    print("Creating grid")
    grid = random_grid()
    for i in range(ROUNDS):
        print("Starting round", i + 1, "of", ROUNDS)
        compute_next_generation(grid)

    print("Creating image")
    img = convert_grid_to_image(grid)
    print("Saving image")
    img.save("gofl.png")


`r` permutations of `n` I have written this snippet to find r-permutations of n, i.e. if I have an array of n=3 {0,1,2}, then r=2 permutations will be {{0, 1}, {0, 2}, {1, 0}, {1, 2}, {2, 0}, {2, 1}}.
Can somebody review it and help me optimize / reduce its complexity (I don't want to use recursive function):

"_getAllPermutation": function(input, allPermutations, usedIndices, r) {
    var index = 0,
        usedIndex = null;

    for (; index < input.length; index++) {
        usedIndex = input.splice(index, 1)[0];
        r--;
        usedIndices.push(usedIndex);
        if (input.length === 0 || r === 0) {
            allPermutations.push(usedIndices.slice());
        }
        if (r > 0) {
            this._getAllPermutation(input, allPermutations, usedIndices, r);
        }
        input.splice(index, 0, usedIndex);
        r++;
        usedIndices.pop();
    }
}

I know you asked for a non-recursive version, but recursion is a natural way to approach this problem.  Here's a compact, though not high-performance, approach:

function nPr(xs, r) {
    if (!r) return [];
    return xs.reduce(function(memo, cur, i) {
        var others  = xs.slice(0,i).concat(xs.slice(i+1)),
            perms   = nPr(others, r-1),
            newElms = !perms.length ? [[cur]] :
                      perms.map(function(perm) { return [cur].concat(perm) });
        return memo.concat(newElms);
    }, []);
}

All we're doing is taking each element, and then appending it to the (r-1) permutations of all the other elements.

It is possible to do this using while and without recursion, though it will be hard to make that approach as terse.  I'll try to take a shot at it later.


Concurrent dependant routines in a gui application The idea is that, in my application I have 5 routines named long_process_1, long_process_2, long_process_3, long_process_4, long_process_5. These are each long calculations that would hang the GUI and can only be called in that order. When each result is ready, user input is required about the result. So we call ask_user_1, ask_user_2, ask_user_3, ask_user_4, ask_user_5 which must obviously run in the main GUI thread. Thus the whole thing cannot be run in a separate thread in its entirety. Here's how I did it:
void MainWindow::on_button_click() // starts with a button click
{
    QEventLoop eventLoop;
    QFutureWatcher<int> watch;

    // Watcher intercepts eventLoop when the computation is finished
    connect(&watch, &decltype(watch)::finished, &eventLoop, &decltype(eventLoop)::quit);

    auto const res1 = QtConcurrent::run(long_process_1);
    watch.setFuture(res1);
    eventLoop.exec(); // Handles GUI during computation
    auto const user_in1 = ask_user_1(res1);

    auto const res2 = QtConcurrent::run(long_process_2, user_in1);
    watch.setFuture(res2);
    eventLoop.exec();
    auto const user_in2 = ask_user_2(res2);

    auto const res3 = QtConcurrent::run(long_process_3, user_in2);
    watch.setFuture(res3);
    eventLoop.exec();
    auto const user_in3 = ask_user_3(res3);

    auto const res4 = QtConcurrent::run(long_process_4, user_in3);
    watch.setFuture(res4);
    eventLoop.exec();
    auto const user_in4 = ask_user_4(res4);

    auto res5 = QtConcurrent::run(long_process_5, user_in4);
    watch.setFuture(res5);
    eventLoop.exec();
    (void)ask_user_5(res5); // result is not used
}

This works alright. I find this a lot simpler than running the whole thing through "on-finished" signal-slots. It also simplifies ownership of local variables/function results as they are all in a single function. The long routines are clearly decoupled from the function calling them. It seems a bit odd overall though. I got the idea from very old Qt docs. All comments are welcome.

Running non-gui related stuffs using the main event-dispatch(gui-thread) is kind of anti-gui practice.
If first process should be called, and then second process, so it's basically a long synchronous calls (with user prompt between each)
Probably(good practice) is run the first long process using a thread(usually out of event-dispatch/gui-thread).
Meanwhile, the long process could inform the GUI about the process by sending progress events(assuming gui works as a process-progress-listener here).
Now, because the long process is run by a thread, gui is functional, and not freezing, so user could see progress, or control the progress(e.g. abort, etc...)
Once the long thread is finished, ask the gui-thread/event-dispatch for user prompt, and if starting the second long process is a thing, go for it just like first one using a thread.
Overall
Try not to perform excessive non-gui stuffs using gui-thread/event-dispatch. This is actually a good practice.
Try not to perform gui related stuffs(such as updating a progress-bar value/status) using non-gui threads(out of widget/dispatch scope). This may not be valid for all widgets, but usually(possible) any gui change(here like progress-bar change) seems to be ignored.
But actually that out-of-scope thread could not force the main-gui thread to perform a refresh/repaint on target component.
Assuming, the-gui/event-dispatch thread is the guy who sitting next to the widget door, and is waiting for any gui-related requests.
So if a request about updating progress-bar is made correctly by the expected routine, that guy will repaint the target progress-bar since it have to.
But what happens when the request came from out of event-dispatch scope? It's more like getting into the room from windows, rather the door, and the guy next to it.
So technically progress bar will be updated, but might not updated immediately(or at all), since the guy who in charge to keeps ui updated did not realized there is a requests that forces component(s) to get repainted.


DFS-based exhaustive word grid search I have implemented an exhaustive depth-first grid search that searches for all possible words in the 8 connected regions. Is there any way I can make this faster or memory efficient than it is now?
I feel like passing a copy of the seen set in dfs(xx, yy, seen.copy(), word + grid[x][y]) is an overhead that can possibly be avoided.
Or is my algorithm completely wrong and I should use something else?
This is not a code site challenge, I am just trying to implement this out of curiosity.
My first question on code review so let me know if there is something that I can add to make this better.
Also can this be made more "pythonic"?
def possible_directions(x, y):
    """
    Returns at most 8 connected co-ordinates for given `x` and `y`
    """
    for xx in range(-1, 2):
        for yy in range(-1, 2):
            if not xx and not yy:
                continue
            if 0 <= x + xx < len(grid) and 0 <= y + yy < len(grid[x]):
                yield x + xx, y + yy

def dfs(x, y, seen, word = ''):
    """
    Recursive Generator that performs a dfs on the word grid
    seen = set of seen co-ordinates, set of tuples
    word = word to yield at each recursive call
    """
    if (x, y) in seen:
        return
    yield word + grid[x][y]
    seen.add((x, y))
    for xx, yy in possible_directions(x, y):
        yield from dfs(xx, yy, seen.copy(), word + grid[x][y])

grid = [['c', 'f', 'u', 'c'], ['e', 'a', 't', 'e'], ['b', 'h', 'p', 'y'], ['o', 'n', 'e', 'p']] # word grid, can be any size, n x n

for x in range(len(grid)):
    for y in range(len(grid[x])):
        for i in dfs(x, y, set()):
            print(i)

This is fairly tight; I don't think there's much more performance to pull out without dropping into C. That said:
possible_directions
First, I would rename this to neighbors or successors; it's not really about direction but about the nodes in the graph that come next or are adjacent to your current node.
Second, you can reduce the number of branches and arithmetic operations a little:
for xx in range(max(0, x - 1), min(x + 1, len(grid)):
    for yy in range(max(0, y - 1), min(y + 1, len(grid[x])):
        if xx == x and yy = y: continue
        yield xx, yy

Third, you can prevent many no-op calls of dfs by doing your seen check in this function rather than in the dfs base case. That is, pass seen to this function and modify your yield line to something like
if (xx, yy) not in seen: yield xx, yy

dfs
You compute word + grid[x][y] many times (once + once per neighbor). You should memoize this before yielding it (Python may already perform this optimization for you but I doubt it).
I don't think a set of tuples is the best implementation for seen.  You can't really avoid the copy of seen that I can see, and the lookups are not as fast as they could be. You already implicitly require that every row is the same length (0 <= y + yy < len(grid[x]), not 0 <= y + yy < len(grid[x + xx])) and that seems like a sensible restriction in any case. I suggest a bit array of some sort (e.g. bitarray, which I've never used but seems to fit the bill), which will allow fast setting, checking, and copying of seen (instead of seen[x][y], check and set seen[y * len(grid) + x]).
Best practice here would probably be to have a wrapper function that does not take a seen or word argument, but initializes them for a call to your recursive helper, something like
def dfs(x, y):
    return dfs_recursive(x, y, set(), '')


Custom QT widget for QStackedWidget I have a widget like below:

class MainWidget(QWidget):
    #This is grabbed by another function for the name of the page
    name = "First Page"

    def __init__(self, parent, sidebarWidget, customModulesLen):
        QWidget.__init__(self, parent=parent)
        self.customModulesLen = customModulesLen #How many pages there are total
        self.parent = parent #The parent QStackedWidget
        self.sidebarWidget = sidebarWidget #The progress sidebar

        #This the only real content so far, a button to go to the next page
        lay = QVBoxLayout(self)
        pushButton = QPushButton("Next")
        lay.addWidget(pushButton)
        pushButton.clicked.connect(self.onNext)

    def onNext(self):
        """Iterate forward in the QStackedWidget, and update the progress bar"""

        if self.parent.Stack.currentIndex() < self.customModulesLen:
            activeMenu = self.sidebarWidget.menuContainer.findChildren(QLabel)[self.parent.Stack.currentIndex() + 1]
            activeMenu.setPalette(self.sidebarWidget.activeMenuPalette)
            previousMenu = self.sidebarWidget.menuContainer.findChildren(QLabel)[self.parent.Stack.currentIndex()]
            previousMenu.setPalette(self.sidebarWidget.inactiveMenuPalette)

        self.parent.Stack.setCurrentIndex((self.parent.Stack.currentIndex() + 1) % (self.customModulesLen + 1))

The above widget is a page that I'm importing and using for a QStackedWidget. It works just fine, and doesn't appear to be against any major conventions to me atleast.

The problem is that I've never really done this before in PyQT, and I'm having some difficulty determining if this is a good approach at accomplishing this, or if there's some shortcuts or general good idea's I'm missing.

name = "First Page": you retrieve the content of this variable from an other function. Functions returns values vary so from a design principle, we can not assign them to class variables which are supposed to be constant/static.
When you instantiate MainWidget, you need to inject 3 arguments into it because of the way you defined its __init__(). Arguments are a different level of abstraction, so you must use them with parsimony. In your case, there is a better way to get rid of sidebarWidget and customModulesLen described in the next bullet.
You must keep the __init__() clean and outsource the creation of the widegets to an other function that you can call from __init__(). In that function, you can refer to the previous __init__()'s argument which are sidebarWidget and customModulesLen.
You forgot to attach the widget to its parent in lay = QVBoxLayout(self). I mean you must code it this way instead: lay = QVBoxLayout(self.parent). Because objects must own their data members, you should write self.lay = QVBoxLayout(self.master)


Need Loops for PHP Username/Pass Encryption Script I've created this PHP script to print a batch of usernames with encrypted passwords locally on my computer because the user/pass format is always the same.

username = username
password = username + "admin"
example = homer / homeradmin

Then paste them into two different .htpasswd file and upload to an public Apache server to password-protect a directory and a directory within it.
Is there a better way to write the code below?
<?php
// Password to be encrypted for a .htpasswd file
$user1 = 'admin';
$user2 = 'g';
$user3 = 'homer';
$user4 = 'marge';
$user5 = 'bart';
$user6 = 'lisa';
$user7 = 'maggie';
$user8 = 'dog';

//Admin
$admin1 = 'admin';
$admin2 = '23!bseEsF@';
$admin3 = 'homeradmin';
$admin4 = 'margeadmin';
$admin5 = 'bartadmin';
$admin6 = 'lisaadmin';
$admin7 = 'maggieadmin';
$admin8 = 'dogadmin';

// Encrypt password
$pass1 = crypt($user1, base64_encode($user1));
$pass2 = crypt($user2, base64_encode($user2));
$pass3 = crypt($user3, base64_encode($user3));
$pass4 = crypt($user4, base64_encode($user4));
$pass5 = crypt($user5, base64_encode($user5));
$pass6 = crypt($user6, base64_encode($user6));
$pass7 = crypt($user7, base64_encode($user7));
$pass8 = crypt($user8, base64_encode($user8));

$adminpass1 = crypt($admin1, base64_encode($admin1));
$adminpass2 = crypt($admin2, base64_encode($admin2));
$adminpass3 = crypt($admin3, base64_encode($admin3));
$adminpass4 = crypt($admin4, base64_encode($admin4));
$adminpass5 = crypt($admin5, base64_encode($admin5));
$adminpass6 = crypt($admin6, base64_encode($admin6));
$adminpass7 = crypt($admin7, base64_encode($admin7));
$adminpass8 = crypt($admin8, base64_encode($admin8));
?>

<!doctype html>
  <meta charset="utf-8">
  <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
  <title>Encrypt</title>
  <meta name="viewport" content="width=device-width">

<?php
// Print encrypted password
echo $user1 . ":" . $pass1 . "
";
echo $user2 . ":" . $pass2 . "
";
echo $user3 . ":" . $pass3 . "
";
echo $user4 . ":" . $pass4 . "
";
echo $user5 . ":" . $pass5 . "
";
echo $user6 . ":" . $pass6 . "
";
echo $user7 . ":" . $pass7 . "
";
echo $user8 . ":" . $pass8 . "
";
?>

<?php
echo $admin1 . ":" . $adminpass1 . "
";
echo $admin2 . ":" . $adminpass2 . "
";
echo $admin3 . ":" . $adminpass3 . "
";
echo $admin4 . ":" . $adminpass4 . "
";
echo $admin5 . ":" . $adminpass5 . "
";
echo $admin6 . ":" . $adminpass6 . "
";
echo $admin7 . ":" . $adminpass7 . "
";
echo $admin8 . ":" . $adminpass8 . "
";
?>

the first thing that came to my mind to make this cleaner was an array or rather several arrays.

maybe something like this:

$user = array('admin','g','homer','marge','bart','lisa','maggie','dog');
$admin = array('admin','23!bseEsF@','homeradmin','margeadmin','bartadmin','maggieadmin','dogadmin');

$password = array(8); //my syntax may be wrong here
$adminPassword = array(8); //my syntax may be wrong here

for ($x=0; $x < count($user); x$++)
{
    $password[$x] = crypt($user[$x], base64_encode($user[$x]));
    $adminPass[$x] = crypt($admin[$x], base64_encode($admin[$x]));
}

And then your Echo's would look like this.

echo $user1 . ":" . $pass1 . "
";
for ($y=0; $y < count($user); $y++)
{
    echo $user[$y] . ":" . $pass[$y] . "
";
}
for ($z=0; $z < count($admin); $z++)
{
    echo $admin[$z] . ":" . $adminPass[$z] . "
";
}

The only way that this works is if all the arrays are the same length and arrays in PHP are base 0. otherwise you have to change the for declarations a little bit.

Again I say that my Syntax may not be perfect or even well formed,  but this is much cleaner than what you have.

Syntax not Guaranteed


Is calling file write in the dispose acceptable? class HtmlLogger:ILogger,IDisposable
{
    private System.IO.StreamWriter  _file;
    private bool _disposed;

    public HtmlLogger()
    {
        _disposed = false;
        _file = new StreamWriter(@"somepath");
        _file.Write("");
    }

    public void Log(string message)
    {
        _file.Write("
{0}
", message);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                if (_file != null)
                {
                    _file.Write("");
                    _file.Flush();
                    _file.Dispose();
                }

            }
            _file = null;
            _disposed = true;
        }
    }
}

I think it is good idea to use existing classes, I've already made some of the refacorings along with some design choices. For additional info, please read this book:

Design Patterns

EDIT: I think that explicit command calls (open log, write something and close it) before dispose the log object are much more readable, easy to understand and maintain later, than impementing the dispose pattern in another object (so you will get at least two objects to control lifecycle: custom class, and StreamWriter class - that will break the Einstein rule, - it's just not good to control 2 objects, when you could simply control one). In particular, that is going to be true when the new contracts will be applied later to the code. Disposable pattern is to heavy for such simple task. Additionally, as an advantage of using one object to control, and - consequently, - very simplified logic, it's easier to control log writer object's lifetime. At second, simple class with a good and clead design can be extended easily to supprt for an additional logic and do will not likely contains any of the caveats in the future.

public interface IHTMLLogger
{
    ILogWriter CreateHTMLLogWriter(string filePath);
}

public interface ILogWriter : IDisposable
{
    void OpenLog();
    void WriteLog(string message);
    void CloseLog();
}

public class HtmlLogger : IHTMLLogger
{
    public class HTMLLogWriter : StreamWriter, ILogWriter
    {
        public HTMLLogWriter(string filePath)
            : base(filePath)
        {
        }
        public void WriteLog(string message)
        {
            base.Write(string.Format("
{0}
", message));
        }
        public void OpenLog()
        {
            base.Write("");
        }
        public void CloseLog()
        {
            base.Write("");
        }
    }

    public ILogWriter CreateHTMLLogWriter(string filePath)
    {
        return new HTMLLogWriter(filePath);
    }
}

EDIT: As you see, I can easily integrate new encoding logic to encode any string data:

public interface IHTMLEncoder
{
    string Encode(string message);
}

public interface IHTMLLogger
{
    ILogWriter CreateHTMLLogWriter(string filePath);
}

public interface ILogWriter : IDisposable
{
    void OpenLog();
    void WriteLog(string message);
    void CloseLog();
}

public class HtmlLogger : IHTMLLogger
{
    public class HTMLEncoder : IHTMLEncoder
    {
        public string Encode(string message)
        {
            return Convert.ToBase64String(Encoding.UTF8.GetBytes(message.ToCharArray()));
        }
    }
    public class HTMLLogWriter : StreamWriter, ILogWriter
    {
        private IHTMLEncoder _encoder;
        public HTMLLogWriter(string filePath, IHTMLEncoder encoder)
            : base(filePath)
        {
            _encoder = encoder;
        }
        public void WriteLog(string message)
        {
            base.Write(string.Format("
{0}
", _encoder.Encode(message)));
        }
        public void OpenLog()
        {
            base.Write("");
        }
        public void CloseLog()
        {
            base.Write("");
        }
    }
    public ILogWriter CreateHTMLLogWriter(string filePath)
    {
        return new HTMLLogWriter(filePath, new HTMLEncoder());
    }
}

Sample:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using System.Configuration;
using System.Runtime.Serialization;
using System.Security.Permissions;
using System.IO;
using System.Diagnostics;

namespace WindowsFormsApplication1
{
    public partial class Form1 : Form
    {
        private static IHTMLLogger logger = new HtmlLogger();
        private ILogWriter logWriter;

        public Form1()
        {
            InitializeComponent();
        }
        private void Form1_Load(object sender, EventArgs e)
        {
            logWriter = logger.CreateHTMLLogWriter(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "html.log"));
            logWriter.OpenLog();

            this.Location = UserSettings.Default.FormLocation;
            this.Size = UserSettings.Default.FormSize;
            this.textBox1.Text = UserSettings.Default.ProcessPath;
        }

        private void Form1_FormClosing(object sender, FormClosingEventArgs e)
        {
            UserSettings.Default.FormLocation = this.Location;
            UserSettings.Default.FormSize = this.Size;
            UserSettings.Default.ProcessPath = this.textBox1.Text;
            UserSettings.Default.Save();

            logWriter.CloseLog();
            logWriter.Dispose();

            MessageBox.Show(File.ReadAllText(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "html.log")));
        }

        private void textBox1_TextChanged(object sender, EventArgs e)
        {
            UserSettings.Default.ProcessPath = this.textBox1.Text;
            logWriter.WriteLog(this.textBox1.Text);
        }
    }

    [SettingsSerializeAs(SettingsSerializeAs.Xml)]
    [Serializable]
    public sealed class UserSettings : ApplicationSettingsBase
    {
        private const string FormLocationProperty = "FormLocation";
        private const string FormSizeProperty = "FormSize";
        private const string ProcessPathProperty = "ProcessPath";

        private UserSettings() { }

        private static UserSettings _defaultInstance = new UserSettings();

        public static UserSettings Default { get { return _defaultInstance; } }

        [UserScopedSetting()]
        [DefaultSettingValue("0, 0")]
        public Point FormLocation
        {
            get { return (Point)(this[FormLocationProperty]); }
            set { this[FormLocationProperty] = value; }
        }

        [UserScopedSetting()]
        [DefaultSettingValue("300, 300")]
        public Size FormSize
        {
            get { return (Size)this[FormSizeProperty]; }
            set { this[FormSizeProperty] = value; }
        }

        [UserScopedSetting]
        [DefaultSettingValue("")]
        public string ProcessPath
        {
            get { return (string)this[ProcessPathProperty]; }
            set { this[ProcessPathProperty] = value; }
        }
    }
}


Passing an array of values in UrlAction I need to pass an array of values in Url.Action.

Currently I'm doing:

var redirectUrl = Url.Action("search", new {
        q = criteria.Q,
        advanced = criteria.Advanced,
        salaryfrom = criteria.SalaryFrom,
        salaryto = criteria.SalaryTo,
    });

if (criteria.JobTypes != null)
    redirectUrl += criteria.JobTypes.Aggregate(string.Empty, (a, x) => a += "&jobTypes=" + x);

To give me something like:

/search?q=developer&advanced=false&salaryfrom=20000&salaryto=80000&jobTypes=Full%20Time&jobTypes=Contract

Is there a nicer/cleaner approach?

Concerning the comment you got about passing an ID, note that it's better to put search parameters in the URL, since it allows users to link to that search and refresh the page without having issues. All search engines do that.
Make sure to test for null without using type coercion: if(criteria.JobTypes !== null). It's a best practice that will avoid you a few surprises.
As for passing an array of values, I don't know much about ASP.NET but it looks like you can pass an array of values with Html.ActionLink. I don't know what version you're using but this seems specific to ASP.NET MVC 2.


Find the highest result between two elements in an array using their values and indices I got a programming question on an interview where performance was the focus. I couldnt come up with a better solution than the one below which I think has a time complexity of O(n^2 + n) and scored 20/100 on performance.

The question in short was to find the highest result by adding two elements and in an array and the difference between the two indices e.g A[k] + A[j] + (j - k) - (k can be equal to j).

The size of the array = [1...100 000]
The values in the array =[-1000 000 000...1000 000 000]

I know using nested loops usally is a bad idea when It comes to handling larger arrays. So my question is simply how can it be improved to make it faster?

public static int solution(int [] A) {

    Set<Integer> occoured = new HashSet<Integer>();
    int maxAppeal = 0;

    for(int i = 0; i < A.length; i++) {
        if(occoured.add(A[i])) {
            for(int j = i; j < A.length; j++) {
                int appeal = A[i] + A[j] + (j - i);
                if(appeal > maxAppeal)
                    maxAppeal = appeal;
            }
        }
    }
    return maxAppeal;
}

This is not a review, but an extended comment.

Consider the question:

  Given two arrays, A and B, find the maximum of A[i] + B[j]

I suppose you can do it in linear time.

Now rearrange the original equation as (A[k] - k) + (A[j] + j) and consider two auxiliary arrays, formed by A[i] - i and A[i] + i.

Finally realize that you don't need these arrays at all.


Grouping positive & negative reviews and counting the words Can somebody help me make the code below smarter and free of redundancy? The code does exactly what I want, but I believe there are many redundant loops and arrays.

from collections import Counter
import csv
import re

with open("/Users/max/train.csv", 'r') as file:
  reviews = list(csv.reader(file))

def get_texts(reviews, score):
  texts = []
  texts.append([r[0].lower() for r in reviews if r[1] == str(score)]) ;
  return texts

def getWordListAndCounts(text):
  words = []
  for t in text:
      for tt in t:
        for ttt in (re.split("\s+", str(tt))):
            words.append(str(ttt))
  return Counter(words)

negative_text_list = get_texts(reviews, -1)
positive_text_list = get_texts(reviews, 1)

negative_wordlist_counts = getWordListAndCounts(negative_text_list)

positive_wordlist_counts = getWordListAndCounts(positive_text_list)

print("Negative text sample: {0}".format(negative_text_list))
print("Negative word counts: {0}".format(negative_wordlist_counts))
print("Positive text sample: {0}".format(positive_text_list))
print("Positive word counts: {0}".format(positive_wordlist_counts))

Sample contents in train.csv are as below.

i like google,1
google is evil,-1
Apple is cute,1
Microsoft booo,-1

Your code is actually really good.
But:

getWordListAndCounts should be get_word_list_and_counts.
This is as Python has a style guide called PEP8.
You use a ; but for no reason. It's best to never use it.
In get_texts you do [].append([...]). This can be: [[...]].
In get_texts you have r, it would be better to use review,
this makes it easier to read. Also all the ts in getWordListAndCounts.
You can make getWordListAndCounts a list/generator comprehension.
You seem to mix spaces and tabs. This leads to highly breakable code.
(Look at your question)
Don't have white space after code. It's really annoying, and violates PEP8.

Overall I would say your code is pretty decent.

I would re-write getWordListAndCounts to this for example:

def get_word_list_and_counts(text):
    # Rename the `t`'s I have no context on what they are.
    return Counter(
        str(ttt)
        for t in text
        for tt in t
        for ttt in (re.split("\s+", str(tt)))
    )

To remove the [].append, you can use the following if it has to be a list.

def get_texts(reviews, score):
    return [r[0].lower() for r in reviews if r[1] == str(score)]

As no-one has said it:

[[...]] in get_texts seems redundent, adds complexity, and makes the algorithm worse in memory, from \$O(1)\$ with generator comprehensions, to \$O(n)\$.

The way you call the code shows us also that there is potential to reduce the complexity.

getWordListAndCounts(get_texts(reviews, 1))

To show the change to get \$O(1)\$:

def get_texts(reviews, score):
    return (
        review[0].lower()
        for review in reviews
        if review[1] == str(score) # use ferada's change
    )

def get_word_list_and_counts(text):
    return Counter(
        word
        for line in text
        for word in (re.split("\s+", line))
    )

The above shows you can clearly see it's capable to be in a single function,
if you wish for it.
And remove one of the fors in the get_word_list_and_counts.

It as one function would look like this:

def get_word_list_and_counts(reviews, score):
    return Counter(
        word
        for review in reviews
        if review[1] == str(score)
        for word in re.split("\s+", review[0].lower())
    )

That with ferada's change is probably the smallest and most concise you'll get it.
Also the second for statement will only run if the if is true.