ByteLandian challenge from CodeChef I am trying to go through the problems on CodeChef and came across this problem:

  In Byteland they have a very strange monetary system. Each Bytelandian
  gold coin has an integer number written on it. A coin n can be
  exchanged in a bank into three coins: n/2, n/3 and n/4. But these
  numbers are all rounded down (the banks have to make a profit). You
  can also sell Bytelandian coins for American dollars. The exchange
  rate is 1:1. But you can not buy Bytelandian coins. You have one gold
  coin. What is the maximum amount of American dollars you can get for
  it?

I approached it using the following algorithm:

Check if the total amount on gold coin is greater than (Total/2)+(Total/3)+(Total/4), return total amount on gold coin.
Otherwise, recursively evaluate the maximum total amounts for (Total/2),(Total/3) and (Total/4) and return the sum total of these maximums.

My attempt:

public class ByteLandGold{

public static void main(String[] args){
        System.out.println("Printing the bytelandian equivalent for:");
        int byteLandCoin = Integer.parseInt(args[0]);
        System.out.println(ByteLandianConversion(byteLandCoin));
}

private static int ByteLandianConversion(int goldCoinAmount){
        int halfValue = (goldCoinAmount/2);
        int thirdValue = (goldCoinAmount/3);
        int fourthValue = (goldCoinAmount/4);

        int totalIntermediate = halfValue + thirdValue + fourthValue;
        if(goldCoinAmount > totalIntermediate)
                return goldCoinAmount;
        else{
                int maxHalfPortion = ByteLandianConversion(halfValue);
                int maxThirdPortion = ByteLandianConversion(thirdValue);
                int maxFourthPortion = ByteLandianConversion(fourthValue);
                return maxHalfPortion+maxThirdPortion+maxFourthPortion;
        }
}
}

One thing that I see in my code is that the recursive steps are calculating the overlapping sub-problems repeatedly. What would be one way to ensure that doesn't happen? Can somebody go through my code and give me some pointers on how to optimize it further and what can I change this snippet to solve it in a better way?

If you don't want to recompute the overlapping sub-problems, you could use a cache.

Create the cache in your class:

private HashMap<int, int> resultCache = new HashMap<int, int>();

Then check it at the top of the function:

if (resultCache.contains(goldCoinAmount)) return resultCache.get(goldCoinAmount);

And before returning a result, cache it:

if(goldCoinAmount > totalIntermediate)
    resultCache.put(goldCoinAmount, goldCoinAmount);
    return goldCoinAmount;
else{
    int maxHalfPortion = ByteLandianConversion(halfValue);
    int maxThirdPortion = ByteLandianConversion(thirdValue);
    int maxFourthPortion = ByteLandianConversion(fourthValue);

    int result = maxHalfPortion+maxThirdPortion+maxFourthPortion;
    resultCache.put(goldCoinAmount, result);
    return result;
}

Otherwise, your code is really fairly straightforward and thus fairly readable/maintainable. Readability is far more important than raw performance most of the time.


NQueens Puzzle in Java Place N amount of queens in such a way that none of them can attack each other.

import java.util.*;

public class NQueensRewrite {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);
        System.out.println("Enter amount of queens");
        int n = input.nextInt();
        int[] board = new int[n];
        fillBoard(board);
        printBoard(board);
    }

    public static void fillBoard(int[] board) {
        int queens = 1;
        while (queens < board.length) {
            boolean validColumnFound = false;
            //Start from previous confirmed column or check all if valid column found in row
            for (int column = board[queens] + 1; column < board.length; column++) {
                if (isValid(column, queens, board)) {
                    board[queens] = column;
                    queens++;
                    validColumnFound = true;
                    break;
                }
            }
            //If no other valid column is found, then clear the row and go back to try another column
            if (!validColumnFound) {
                board[queens] = 0;
                queens--;
            }
        }
    }

    public static boolean isValid(int column, int row, int[] board) {
        //Check if any other row has the specified column
        for (int i = 0; i < row; i++) {
            if (board[i] == column) {
                return false;
            }

            //Check the differences bewteen the spaces if they are uniform (even)
            //If the difference between the column of ith row and column being checked is
            //the same as the difference between the current row and row being checked,
            //then some previous queen is diagnal to the current spot being checked
            if (Math.abs(board[i] - column) == Math.abs(i - row)) {
                return false;
            }
        }
        return true;
    }

    public static void printBoard(int[] board) {
        for (int i = 0; i < board.length; i++) {
            for (int j = 0; j < board.length; j++) {
                if (board[i] == j) {
                    System.out.print("|Q");
                }
                else {
                    System.out.print("| ");
                }
            }
            System.out.println("|");
        }
    }
}

It took me a while to figure out your code. I have solved this problem a couple of times before, but found your code hard to read. In large part, because your variable names are misleading....

board is not a board, it is an array of int, with the value of the queen's column. I see the board as being a 2D matrix, you have it as a 1D array.
queens is not the number of queens, but the number of rows you have currently populated.

Then, your code is not as general-purpose as I was expecting.... Your code only ever finds solutions where the position (0,0) is a queen. Now, that's not to say that solutions with a queen there are wrong, but, are you sure? What if there is no solution with a queen in the corner....

If you know for sure there will be one, then you should at least document that..... if not, then it's a bug.

All in all, your code could use some more help to get it readable.


Any way to make this recursive function better/faster? Is there anything that can be done differently for this function?  Any way to make it faster?

public List<Channel> ChildrenOf(Channel startingChannel)
{
    List<Channel> result = new List<Channel>();

    foreach (Channel child in startingChannel.Children)
    {
        result.Add(child);
        result.AddRange(ChildrenOf(child));
    }

    return result;
}

Just to round out the other answers: I would be inclined to write your solution like this:

static IEnumerable<T> DepthFirstTreeTraversal<T>(T root, Func<T, IEnumerable<T>> children)
{
    var stack = new Stack<T>();
    stack.Push(root);
    while(stack.Count != 0)
    {
        var current = stack.Pop();
        // If you don't care about maintaining child order then remove the Reverse.
        foreach(var child in children(current).Reverse())
            stack.Push(child);
        yield return current;
    }
}

And now to achieve your aim, you just say:

    static List<Channel> AllChildren(Channel start)
    {
        return DepthFirstTreeTraversal(start, c=>c.Children).ToList();
    }

Now you have a more general-purpose tool that you can use to get a depth-first traversal of any tree structure, not just your particular structure.

Another nice feature of my solution is that it uses a fixed amount of call stack space. Even if your hierarchy is twenty thousand deep, you never run out of stack space because the method is not recursive to begin with. All the information that would be needed for recursion is stored on the "stack" data structure instead of in activation records on the real call stack.


How to break up long lines of code. (Example Line: Results of method call added to list.) Often I generate long lines of code such as the following...

shippedItems.AddRange(OrderItem.Fetch(market: this.MARKET, shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE, orderPlacedAfter: serverTime.AddDays(-7), orderPlacedBefore: serverTime.AddHours(-85)));

... which adds the results of a method call to an existing list.

Adding white space to this line could improve readability. At one point in time or another have rationalized almost every possible behavior between:

Leave everything on a single line and let the editor wrap where it feels is best.
Put even shippedItems.AddRange( on a line by itself.

While over time I feel that the clarity and readability of the code that I write has improved -- and lets hope for reasons other then white space -- I have never come to peace with how to break long lines.

I will up vote any answer that does NOT include as the solution:

shorter variable names.
disregarding named parameters (at least for this example).
creation of variables only used once.

I would break it up something like this:

shippedItems.AddRange(
    OrderItem.Fetch(market: this.MARKET,
                    shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,
                    orderPlacedAfter: serverTime.AddDays(-7),
                    orderPlacedBefore: serverTime.AddHours(-85)));

Depending on previous indentation, some lines might flow over the "max line length", but I think that characters per line is more of a suggestion and there are good times to break that rule because breaking it leads to code that is more readable than code that doesn't.

Rules that I find helpful:

New line after an open paren.
Line breaks after commas.
Indent the inner method calls.
Line up parameters to a method that are on new lines.
Break "max line length" rules if it means the code is more readable.


PHP function to extract numbers from the list whose squares are exceed some threshold This is my naive approach:

$sample1 = [3, -20, 1, -19, -4, 6, 77, -1, 0, 6];
$sample3 = [4, 5, -7, 0, 2, 44, -3, -5, 66, -7];
$sample4 = [4, 25, 1, 6, 9, 5, 999, 4, 43, 2];

$sample2 = [];
for ($i = 0; $i < mt_rand(10, 30); $i++) {
  $sample2[] = mt_rand(-1000, 1000);
}

extractNumbersThatSquaresAreGreaterThan($sample1);
extractNumbersThatSquaresAreGreaterThan($sample2);
extractNumbersThatSquaresAreGreaterThan($sample3);
extractNumbersThatSquaresAreGreaterThan($sample4);

var_dump($sample1);
//var_dump($sample2);
var_dump($sample3);
var_dump($sample4);

function extractNumbersThatSquaresAreGreaterThan(&$list, $boundary = 26) {
  foreach ($list as $key => $value) {
    if (pow($value, 2) <= $boundary) {
      unset($list[$key]);
    }
  }
}

Any hints for optimization? Is it possible, I've tried few, but in the end it was more complicated (and slower) solution

Hints?  Ok.

Don’t square the values, O(n), instead square-root the boundary.  You’ll need an or in your test, though.
Don’t unset values in the list.  That requires shifting values in memory.  Make a new list of same size, fill in values, without gaps, and then resize result.


Simple folder/file lock handler I have two python scripts (or one bash script + one python script) which are working independently from another, one script puts data into a folder, the other one reads from it.
Since I don't want to accidentally read from the folder at the same time it's updated, I created this simple folder lock using a dummy file.
In the bash script a similar thing is done, by checking whether that lock.file exists, and if not create it, put the current PID into it, and after it's done delete it.
import os
import sys
import time
import logging

LOGGER = logging.getLogger()

class SimpleFolderLock():
    def __init__(self, folder_path, time_out=5):
        self._folder_to_lock = folder_path
        self._lock_file_path = os.path.join(self._folder_to_lock, "folder.lock")
        self._lock_time_out = time_out

    def __enter__(self):
        start_time = time.time()
        while os.path.exists(self._lock_file_path):
            LOGGER.debug(f"Waiting for {self._lock_file_path} to clear")
            time.sleep(5)
            if (time.time() - start_time) > self._lock_time_out:
                LOGGER.error(
                    f"Time-out for waiting for {self._lock_file_path} to clear")
                raise TimeoutError(f"Can't lock {self._folder_to_lock} after {self._lock_time_out} seconds")

        LOGGER.debug(f"Locking with {self._lock_file_path}")
        with open(self._lock_file_path, "w") as fn:
            fn.write(str(os.getpid()))

        return self._lock_file_path

    def __exit__(self, type, value, traceback):
        try:
            LOGGER.debug(f"Clearing {self._lock_file_path}")
            os.remove(self._lock_file_path)
        except FileNotFoundError:
            LOGGER.error(f"File {self._lock_file_path} already got deleted")

And for a simple test:
# Just some dummy logging configuration
logging.basicConfig(stream=sys.stdout, level=logging.DEBUG)
test_folder = "/tmp/test_dir/"
os.makedirs(test_folder, exist_ok=True)
with SimpleFolderLock(test_folder) as lock:
    lock_path = lock
    LOGGER.info(f"Locked with {lock_path}")
    assert os.path.exists(lock_path)
    fiddle_around_in_folder(test_folder)

assert not os.path.exists(lock_path)

There is a race condition here:
while os.path.exists(self._lock_file_path):
...
with open(self._lock_file_path, "w") as fn:

The file could have been created by the other process in between. You need to check file existence and create the file in one operation.
Since python 3.3 there is a special open mode x for this use case:

open for exclusive creation, failing if the file already exists

For example:
while (time.time() - start_time) < self._lock_time_out:
    try:
        with open(self._lock_file_path, "x") as fn:
            fn.write(str(os.getpid()))
    except FileExistsError:
        time.sleep(5)


Executing a command using Net::SSH According to the local cop (rubocop), my method has too many lines.

lib/awesomelibrary/tunnelable.rb:4:5: C: Method has too many lines. [17/10]
    def tunnel_run(cmd) ...
    ^^^^^^^^^^^^^^^^^^^

Below is the method. How do I refactor it?

module AwesomeLibrary
  # Gives object the ability to execute bash commands on itself via ssh
  module Tunnelable
    def tunnel_run(cmd)
      retries = 0
      code = nil
      Net::SSH.start(@tunnel_ip, @tunnel_username, keys: @tunnel_key_name, verify_host_key: false) do |ssh|
        the_channel = ssh.open_channel do |channel|
          channel.exec cmd do |ch, success|
            raise "could not execute command" unless success
            ch.on_data { |_c, data| print data }
            ch.on_extended_data { |_c, _type, data| print data }
            ch.on_request("exit-status") { |_ch, data| code = data.read_long }
          end
        end
        the_channel.wait
      end
      abort "#{cmd} returned #{code} !!" if code != 0
    rescue Net::SSH::ConnectionTimeout
      puts "Net::SSH::ConnectionTimeout"
      retry if (retries += 1) < 3
    end
  end
end

I'm using this is the library: Net::SSH 4.x.

In my opinion, your method does not have too many lines, but the line calling Net::SSH.start is too long.

My terminals and text editors are 80 characters wide, so I would split the long line into 2 lines

      Net::SSH.start(@tunnel_ip, @tunnel_username, keys: @tunnel_key_name,
                     verify_host_key: false) do |ssh|

If you must reduce the number of lines, you can move some lines into new methods. The extra method calls will make spaghetti of the control flow, so the code will be difficult to read and understand, but it might satisfy your cop.

    def tunnel_run(cmd)
        retries = 0
        code = tunnel_run_command(cmd)
        abort ... if code != 0
    rescue Net::SSH::ConnectionTimeout
        ...
    end

    private
    def tunnel_run_command(cmd)
        code = nil
        Net::SSH.start(...) do |ssh|
            the_channel = ssh.open_channel do |channel|
                channel.exec do |ch, success|
                    code = tunnel_run_channel(ch, success)
                end
            end
            the_channel.wait
        end
        code
    end

    def tunnel_run_channel(ch, success)
        code = nil
        raise ... unless success
        ch.on_data { ... }
        ch.on_extended_data { ... }
        ch.on_request("exit-status") { ... }
        code
    end

Beware that the private methods tunnel_run_command and tunnel_run_channel will pollute the method namespace in all objects that extend Tunnelable.  Privacy in Ruby is by object, so the object inherits the private methods from Tunnelable, and Ruby allows the object to call those methods.

The names tunnel_run_command and tunnel_run_channel must not conflict with other methods of the object.


Percentage of questions that are unanswered or answered - Part 2 I've refactored my previous query and added better checks when selecting the data. The query link can be found here. The formulas used to calculate the two percentages are the same, and can be seen below:

$$
\text{Answered Percentage} = 100 - \left(\frac{\text{Unanswered}}{\text{Answered}} \times 100\right) \\
\text{Unanswered Percentage} = \frac{\text{Unanswered}}{\text{Answered}} \times 100
$$

Finally, here's the source.

DECLARE @TotalQuestions FLOAT = (
  SELECT COUNT(*) FROM Posts WHERE
        PostTypeId = 1
    AND ClosedDate IS NULL
);

DECLARE @UnansweredQuestions FLOAT = (
  SELECT COUNT(*)
    FROM Posts AS TopPostsQuery WHERE (
            ClosedDate IS NULL
        AND PostTypeId = 1
        AND AnswerCount = 0
      )
      OR (
            ClosedDate IS NULL
        AND PostTypeId = 1
        AND AnswerCount <= 1
        AND AcceptedAnswerId IS NULL
        AND (
          SELECT Score FROM Posts AS SubPostsQuery WHERE
                PostTypeId <> 1
            AND ParentId = TopPostsQuery.Id
        ) <= 0
      )
);

DECLARE @AnsweredPercentage FLOAT =
  100.0 - ((@UnansweredQuestions / @TotalQuestions) * 100.0);

DECLARE @UnansweredPercentage FLOAT =
  (@UnansweredQuestions / @TotalQuestions) * 100.0;

SELECT
    [Percentage Of Answered Questions]   = @AnsweredPercentage
  , [Percentage Of Unanswered Questions] = @UnansweredPercentage;

Your code has certainly improved from last time, however there's a few things that could be improved:

AND AnswerCount <= 1: that could be AND AnswerCount < 0.
You don't need to recreate the amount of unanswered questions twice:

DECLARE @AnsweredPercentage FLOAT =
  100.0 - ((@UnansweredQuestions / @TotalQuestions) * 100.0);

DECLARE @UnansweredPercentage FLOAT =
  (@UnansweredQuestions / @TotalQuestions) * 100.0;

into:

DECLARE @UnansweredPercentage FLOAT =
  (@UnansweredQuestions / @TotalQuestions) * 100.0;

DECLARE @AnsweredPercentage FLOAT =
  100.0 - @UnansweredPercentage;


Controller method to update properties of a request I have a view that displays details of a work order request submitted by a user. As far as security and database querying is concerned, which one of the following options would be better if I'm going to update a table named "Requests" with some changes to the properties? Is the extra query to the dbcontext in option 2 worth the security benefit?

Note: some properties omitted to reduce code.

Option 1:

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Details(DetailsViewModel request)
{

    var model = new Request
    {
        Id = request.Id,
        RequestDate = request.RequestDate,
        Status = request.Status,
        FirstName = request.FirstName,
        LastName = request.LastName,
        LastModificationDate = DateTime.Now,
        AffiliationId = request.AffiliationId,
        IssueId = request.IssueId,
        RequestTypeId = request.RequestTypeId,

    };

    _context.Requests.Attach(model);
    var entry = _context.Entry(model);
    entry.Property(e => e.Status).IsModified = true;
    entry.Property(e => e.AffiliationId).IsModified = true;
    entry.Property(e => e.IssueId).IsModified = true;
    entry.Property(e => e.RequestTypeId).IsModified = true;
    entry.Property(e => e.LastModificationDate).IsModified = true;

    _context.SaveChanges();

    return RedirectToAction("Requests");

Option 2:

[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult Details(DetailsViewModel request)
{
    var model = _context.Requests.Find(request.Id);

    model.Status = request.Status;
    model.AffiliationId = request.AffiliationId;
    model.IssueId = request.IssueId;
    model.RequestTypeId = request.RequestTypeId;
    model.LastModificationDate = DateTime.Now;

    _context.Requests.Attach(model);
    var entry = _context.Entry(model);
    entry.Property(e => e.Status).IsModified = true;
    entry.Property(e => e.AffiliationId).IsModified = true;
    entry.Property(e => e.IssueId).IsModified = true;
    entry.Property(e => e.RequestTypeId).IsModified = true;
    entry.Property(e => e.LastModificationDate).IsModified = true;

    _context.SaveChanges();

    return RedirectToAction("Requests");

I find the second aproach if better for these reasons:

You dont't have to specify all properties but only those that change
You immediately know if the specified Id is correct because

  If an entity with the given primary key values exists in the context, then it is returned immediately [..]. Otherwise, a request is made to the store for an entity with the given primary key values and this entity, if found, is [..] returned. If no entity is found in the context or the store, then null is returned.

You don't have to manually set the modified properties as modified (unless you've disabled tracking) becasue

  this entity, if found, is attached to the context

so EF will figure out the properties that changed. All you need is:

var model = _context.Requests.Find(request.Id);

// check if model == null and handle it apropriately

// update the properties
model.Status = request.Status;
model.AffiliationId = request.AffiliationId;
model.IssueId = request.IssueId;
model.RequestTypeId = request.RequestTypeId;
model.LastModificationDate = DateTime.Now;

// no need to attach it or set properties as modified
// EF already knows this model and can track changes

// save changes
_context.SaveChanges();

I'd say this is more secure as you work with an entity that really exists, in case someone tried to post an invalid Id. Less work becasue EF handles tracking and ataching the entity thus less error prone.

source: DbSet.Find Method


LeetCode: House Robber I C# https://leetcode.com/problems/house-robber/

  You are a professional robber planning to rob houses along a street.
  Each house has a certain amount of money stashed, the only constraint
  stopping you from robbing each of them is that adjacent houses have
  security system connected and it will automatically contact the police
  if two adjacent houses were broken into on the same night.

  Given a list of non-negative integers representing the amount of money
  of each house, determine the maximum amount of money you can rob
  tonight without alerting the police.

  Example 1:

 Input: [1,2,3,1] Output: 4 Explanation: Rob house 1 (money = 1) and
 then rob house 3 (money = 3).
 Total amount you can rob = 1 + 3 = 4. Example 2:

  Example 2:
      Input: [2,7,9,3,1] Output: 12 Explanation: Rob house 1 (money = 2),
      rob house 3 (money = 9) and rob house 5 (money = 1).
      Total amount you can rob = 2 + 9 + 1 = 12.

using System;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{
    /// <summary>
    ///  https://leetcode.com/explore/interview/card/top-interview-questions-easy/97/dynamic-programming/576/
    /// </summary>
    [TestClass]
    public class HouseRobber
    {
        [TestMethod]
        public void HouseRobberTest()
        {
            int[] nums = {1, 2, 3, 1};
            HouseRobberClass robber = new HouseRobberClass();
            Assert.AreEqual(4, robber.Rob(nums));
        }

        [TestMethod]
        public void HouseRobber2Test()
        {
            int[] nums = { 1, 2, 3, 1 };
            HouseRobberClass robber = new HouseRobberClass();
            Assert.AreEqual(4, robber.Rob2(nums));
        }
    }

    public class HouseRobberClass
    {
        private int[] _mem;
        //option 1 for a solution - this is easier for me to think this way
        public int Rob(int[] nums)
        {
            _mem = Enumerable.Repeat(-1, nums.Length).ToArray();

            return Helper(nums,  0);
        }

        private int Helper(int[] nums, int index)
        {
            if (index >= nums.Length)
            {
                return 0;
            }

            if (_mem[index] >= 0)
            {
                return _mem[index];
            }

            _mem[index] = Math.Max(Helper(nums, index + 2) + nums[index], Helper(nums, index + 1));
            return _mem[index];
        }

       // option 2 for a solution
        public int Rob2(int[] nums)
        {
            if (nums.Length == 0)
            {
                return 0;
            }
            int[] memo = new int[nums.Length + 1];
            memo[0] = 0;
            memo[1] = nums[0];
            for (int i = 1; i < nums.Length; i++)
            {
                int val = nums[i];
                memo[i + 1] = Math.Max(memo[i], memo[i - 1] + val);
            }

            return memo[nums.Length];
        }
    }
}

Your first solution's time complexity is non-trivial to determine, and it won't work for long lists because it'll run out of stack, while your second solution obviously needs linear time (optimal) and is unlikely to run out of memory, so I would scrap the first one right away.

The first solution is also inferior because of the _mem member. There is no reason to create this in Rob, and there is no reason to make it a member. Making it a member means:
 - this code can't be called concurrently
 - it leaves memory hanging around once the method exists
 - it is susceptible to interference by other members
 - it is hard to reason about than a variable because it is removed from the method that use it

One nice solution would be to make _mem a local variable, and Helper a local function in Rob: that way you don't have to pass nums and _mem around, and there is no way to mis-use the code. Then you can make all these methods static.

Naming

The naming could be better:

_mem and memo are both cryptic (_mem is providing memoisation, and memo is arguably not)
Rob and Rob2 convey almost nothing
Helper says nothing at all (what is it helping? what does it do?)

Documentation

Please take the time to add documentation. It doesn't matter if you are practising for an interview: real code needs documentation (so that it can be consumed and maintained), writing it is a skill worth practising, and doing so helps you to better reason about your code and solidify edge-case behaviour.

Tests

The tests are limited and not very good. They do not test the case where you need to 'skip' a pair (e.g. 9, 1, 1, 9) and do not cover any edge cases.

Both methods will throw on a NullReferenceException on null input, when they should probably throw a nice ArgumentException so that the caller knows immediately what they did wrong: ideally this would be tested.


SortedList implementation in Java I've implemented a SortedList which takes in streams of floats and returns a median at any point.

public class SortedList {

        float[] list;
        int pos;
        int capacity;

        public SortedList(){
            this.pos = 0;
            this.capacity = 2;
            list = new float[capacity];
        }

        public void offer(int item){
            if(pos == 0){
                list[pos] = item;
                pos++;
                return;
            }
            if(pos == 1){
                if(item > list[0]){
                    list[1] = item;
                }else{
                    float temp = list[0];
                    list[0] = item;
                    list[1] = temp;

                }
                pos++;
                return;
            }
            if(pos - list.length <= 1) resize();
            int index = -1;
            for(int i=0; i< pos -1; i++){
                if(item < list[i]){
                    index = i;
                    break;

                }else if((item > list[i] && item < list[i+1])){
                    index = i+1;
                    break;
                }
            }
            if(index > -1){
                for(int i= pos; i > index; i--){
                    list[i] = list[i -1];
                }
                list[index] = item;

            }else{
                list[pos] = item;
            }
            pos++;
        }
        public float median(){
            float median = -1;
            if(pos%2 == 0){
                median = (list[pos/2 -1] + list[(pos/2)])/2;
            }else{
                median = list[(pos/2)];
            }
            return median;
        }
        private void resize(){

            capacity = capacity*2;
            float[] arr = new float[capacity];
            for(int i=0; i< pos; i++){
                arr[i] = list[i];
            }
            list = arr;
        }

        public void trace(){
            for(int i=0; i< pos; i++){
                System.out.print(list[i] + " ");
            }
            System.out.println();
        }
}

Invite comments on efficiency and accuracy. In the test case that I've used it works fine.

Please protect your internals by preceding them with private:

private float[] list;
private int pos;
private int capacity;

pos is a very poor name for a field caching the size of the list: size is more clear.

You don't need capacity, just ask list.length.

Instead of trace(), I would override the String toString() method.

All in all, I had this in mind:

import java.util.Random;

public class SortedList {

    private float[] list;
    private int size;

    public SortedList() {
        this.list = new float[2];
    }

    public void offer(float item) {
        if (size == list.length) {
            expandArray();
        }

        int i = 0;

        for (; i < size; ++i) {
            if (list[i] > item) {
                break;
            }
        }

        for (int j = size - 1; j >= i; --j) {
            list[j + 1] = list[j];
        }

        list[i] = item;
        size++;
    }

    public float median() {
        if (size == 0) {
            return Float.NaN;
        }

        if (size % 2 == 0) {
            return (list[size / 2 - 1] + list[(size / 2)]) / 2;
        } else {
            return list[(size / 2)];
        }
    }

    private void expandArray() {
        float[] arr = new float[2 * list.length];
        System.arraycopy(list, 0, arr, 0, list.length);
        list = arr;
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder().append("[");

        for (int i = 0; i < size; ++i) {
            sb.append(list[i]);

            if (i < size - 1) {
                sb.append(", ");
            }
        }

        return sb.append(']').toString();
    }

    public static void main(String[] args) {
        long seed = System.nanoTime();
        SortedList l = new SortedList();
        Random random = new Random(seed);

        System.out.println("Seed = " + seed);

        for (int i = 0; i < 10; ++i) {
            l.offer(random.nextInt(100));
            System.out.println(l);
        }
    }
}

Hope this helps.


Proper implementation of generic repository What can be done better in this code?  I am sure it's not missing much.

You can copy and paste the whole thing in LinqPad; it's all there.

public interface IRep<T>
{
    List<T> GetAll();
}

public interface IEntity
{
    int Id{get;set;}
}

public class Customer: IEntity
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int Age { get; set; }
}

public class Product: IEntity
{
    public int Id { get; set; }
    public string Code { get; set; }
    public decimal Price {get;set;}
}

public class RepProduct: IRep<Product>
{
    public List<Product> GetAll()
    {
        List<Product> list = new List<Product>();
        list.Add(new Product() {Id=1, Code = "burger", Price = 2.99M});
        list.Add(new Product() {Id=1, Code = "fries", Price = 1.99M});
        list.Add(new Product() {Id=1, Code = "pepsi", Price = 1.99M});
        return list;
    }
}

public class RepCustomer: IRep<Customer>
{
    public List<Customer> GetAll()
    {
        List<Customer> list = new List<Customer>();
        list.Add(new Customer() {Id=1,Name="Fred", Age=44   });
        list.Add(new Customer() {Id=2,Name="Victoria", Age=13   });
        list.Add(new Customer() {Id=3,Name="Kiefer", Age=10 });
        return list;}
}

void Main()
{

    IRep<IEntity> rep = null;

}

To get it to work, use IEnumerable<T> instead of List<T> as it's covariant:

public interface IRep<out T>
{
    IEnumerable<T> GetAll();
}

public interface IEntity
{
    int Id { get; set; }
}

public class Customer: IEntity
{
    public int Id { get; set; }
    public string Name { get; set; }
    public int Age { get; set; }
}

public class Product: IEntity
{
    public int Id { get; set; }
    public string Code { get; set; }
    public decimal Price {get;set;}
}

public class RepProduct: IRep<Product>
{
    public IEnumerable<Product> GetAll()
    {
        List<Product> list = new List<Product>();
        list.Add(new Product() {Id=1, Code = "burger", Price = 2.99M});
        list.Add(new Product() {Id=1, Code = "fries", Price = 1.99M});
        list.Add(new Product() {Id=1, Code = "pepsi", Price = 1.99M});
        return list;
    }
}

public class RepCustomer: IRep<Customer>
{
    public IEnumerable<Customer> GetAll()
    {
        List<Customer> list = new List<Customer>();
        list.Add(new Customer() {Id=1,Name="Fred", Age=44   });
        list.Add(new Customer() {Id=2,Name="Victoria", Age=13   });
        list.Add(new Customer() {Id=3,Name="Kiefer", Age=10 });
        return list;
    }
}

void Main()
{
    IRep<IEntity> rep = null;

    rep = new RepCustomer();
    Console.WriteLine(rep.GetAll());

    rep = new RepProduct();
    Console.WriteLine(rep.GetAll());
}


Dependency Injected StackExchange.Redis Client I am currently building cache for my application, and I would like to have my redis cache client work the same way you would dependency inject a DbContext.

I am using StackExchange.Redis to manage my redis cache:
https://github.com/StackExchange/StackExchange.Redis

I have made it work, but I am not very confident in the way I implemented it.

In my services I have added:

        services.AddSingleton<IConnectionMultiplexer(ConnectionMultiplexer
            .Connect("localhost:6379,allowAdmin=true"));

        services.AddScoped<ICacheClient, CacheClient>();

I could not figure out how to add IDatabase to services.

My CacheClient looks like this:

    public class CacheClient : ICacheClient
    {
        private readonly IConnectionMultiplexer _connectionMultiplexer;
        private readonly IDatabase _redisCache;
        private readonly ISerializer _serializer;

        public CacheClient(IConnectionMultiplexer connectionMultiplexer, ISerializer serializer)
        {
            _connectionMultiplexer = connectionMultiplexer;
            _serializer = serializer;
            _redisCache = connectionMultiplexer.GetDatabase();
        }

        //add/set cache methods removed for the sake of brevity.

    }

From here I can use _redisCache to access the database and do operations such as _redisCache.StringSetAsync etc...

Now ICacheClient can be injected into my application and I can use my cache logic in any class. Which is very nice.

Is there anything wrong with this way of implementing my cache client?

I can share with you an example version of Redis service such as getting subscribers or accessing the other databases and also for catching connection issues.
ConfigurationOptions allows us to initialize service with more Redis servers(Redis pool), if needed you can easily change the constructor parameters.
Note that ConnectionMultiplexer.Connect(); is already static method, so you can initialize service in class when needed as I did below.
In Addition, I assume that you take ISerializer for the object serialization, if it is, I added an example of serialize/deserialize for you.
I hope it helps you.
    public class CacheClient : ICacheClient
    {
        private readonly ConfigurationOptions configuration = null;
        private Lazy<IConnectionMultiplexer> _Connection = null;

        public CacheClient(string host = "localhost", int port = 6379, bool allowAdmin = false)
        {
            configuration = new ConfigurationOptions()
            {
                //for the redis pool so you can extent later if needed
                EndPoints = { { host, port }, },
                AllowAdmin = allowAdmin,
                //Password = "", //to the security for the production
                ClientName = "My Redis Client",
                ReconnectRetryPolicy = new LinearRetry(5000),
                AbortOnConnectFail = false,
            };
            _Connection = new Lazy<IConnectionMultiplexer>(() =>
            {
                ConnectionMultiplexer redis = ConnectionMultiplexer.Connect(configuration);
                //redis.ErrorMessage += _Connection_ErrorMessage;
                //redis.InternalError += _Connection_InternalError;
                //redis.ConnectionFailed += _Connection_ConnectionFailed;
                //redis.ConnectionRestored += _Connection_ConnectionRestored;
                return redis;
            });
        }

        //for the 'GetSubscriber()' and another Databases
        public IConnectionMultiplexer Connection { get { return _Connection.Value; } }

        //for the default database
        public IDatabase Database => Connection.GetDatabase();

        public T JsonGet<T>(RedisKey key, CommandFlags flags = CommandFlags.None)
        {
            RedisValue rv = Database.StringGet(key, flags);
            if (!rv.HasValue)
                return default;
            T rgv = JsonConvert.DeserializeObject<T>(rv);
            return rgv;
        }

        public bool JsonSet(RedisKey key, object value, TimeSpan? expiry = null, When when = When.Always, CommandFlags flags = CommandFlags.None)
        {
            if (value == null) return false;
            return Database.StringSet(key, JsonConvert.SerializeObject(value), expiry, when, flags);
        }
        private void _Connection_ErrorMessage(object sender, RedisErrorEventArgs e)
        {
            throw new NotImplementedException();
        }

        //add/set cache methods removed for the sake of brevity.
    }

Adding Service
 services.AddSingleton<ICacheClient>(new CacheClient(allowAdmin:true));


Decorator to take away repetitive code I'm using django and I wrote this decorator to take away some of the repetitive code I found for ajax views and I want to know your opinion (too basic, bad design, try this instead, etc).

def ajax_only(func):
    def _ajax_only(request,*args,**kwargs):
        if not request.is_ajax():
            return HttpResponse('
Ajax not supported.
')
        else:
            return func(request,*args,**kwargs)
    return _ajax_only

You should use the functools.wraps function, it makes sure the docstring/name/etc gets passed through
Instead of "ajax not supported" shouldn't it be: "only ajax supported"?
Shouldn't you respond with a Status Code 400 or something if not using ajax when you should be
Why do you want to check this? What's the point of enforcing ajax calls, won't that just make testing harder?


Haskell Hspec/Quickcheck test for numeric function I'm learning to use HSpec and QuickCheck.
As example I was implementing the Pseudocode from Wikipedia:Extended Euclidean Algorithm. You can  find the project at github for the implementation of the tested code.
In particular I wonder about two practices:

selection of test cases - I took two trivial samples, examples from the wikipedia page and took three property tests.
Generation of cases - the a>0 && b>0 seems inefficient to me.

I'm most interested what would be a good practice to confirm two algorithms produce the same results.
module EuclidSpec  ( spec )
where

import Test.Hspec
import Test.Hspec.Core.QuickCheck
import Test.QuickCheck
import Lib

spec :: Spec
spec = do
  describe "Trivial" $ do
    it "trivial example 99 1" $
       let trivial = extendedEuclid 99 1
       in  trivial `shouldBe` (EuclidRes 1 (0) 1)
    it "trivial example 99 99" $
       let trivial = extendedEuclid 99 99
       in  trivial `shouldBe` (EuclidRes 99 (0) 1)
  describe "Examples" $ do
    it "explanation example 99 78" $
       let wikiExample = extendedEuclid 99 78
       in  wikiExample `shouldBe` (EuclidRes 3 (-11) 14)
    it "explanation example flipped 78 99" $
       let wikiExample = extendedEuclid 78 99
       in  wikiExample `shouldBe` (EuclidRes 3 14 (-11) )
    it "explanation example 99 78" $
       let wikiExample = extendedEuclid 240 46
       in  wikiExample `shouldBe` (EuclidRes 2 (-9) 47)
  describe "properties" $ do
      it "both numbers divisible a%gcd == 0, b%gcd ==0" $ property $
            prop_divisible
      it "bezout a*s+b*t = gcd" $ property $
            prop_bezout
      it "recursive and iterative algorithm have same result" $ property $
            prop_same_as_recursive

prop_divisible a b = a>0 && b>0 ==> a `mod` d ==0 && b `mod`d == 0
  where EuclidRes d s t = extendedEuclid a b

prop_bezout a b = a>0 && b>0 ==> a*s + b*t == d
  where EuclidRes d s t = extendedEuclid a b

prop_same_as_recursive a b = a>0 && b>0 ==> extendedEuclid a b == extendedEuclid' a b

Ah, a fine Spec. Has been a while since I've used Hspec, but your tests seem reasonable. So, first of all: well done!
There is one bit we should fix though, and you have identified it yourself: the property tests.
QuickCheck's newtypes
Creating any kind of number and then checking whether it's positive is a hassle, as half the numbers will get discarded per candidate. However, since Hspec uses QuickCheck, we can use Positive to only generate positive numbers:
prop_divisible (Positive a) (Positive b) =  a `mod` d == 0 && b `mod`d == 0
  where EuclidRes d s t = extendedEuclid a b

Other than that there are no more objective improvements.
However, there are some personal I would use in my own specs.
Reduce let … in … bindings in specs
Consider the following spec
  describe "Trivial" $ do
    it "trivial example 99 1" $
       let trivial = extendedEuclid 99 1
       in  trivial `shouldBe` (EuclidRes 1 (0) 1)

If I want to understand the spec, I have to read the first line, remember the value of trivial (and that it hasn't been changed after calling extendedEuclid), and supply it in the next one.
If I instead write
  describe "Trivial" $ do
    it "trivial example 99 1" $
       extendedEuclid 99  1 `shouldBe` (EuclidRes 1 (0) 1)
-- or
    it "trivial example 99 99" $
       extendedEuclid 99 99
           `shouldBe` (EuclidRes 99 (0) 1)

I immediately see that extendedEucild is getting tested. This also fits the official style, where let … in … bindings aren't used at all.
Other small pieces
You can use prop from Test.Hspec.QuickCheck instead of it "..." $ property $ ...:
import Test.Hspec.QuickCheck

...

  describe "properties" $ do
      prop "both numbers divisible a%gcd == 0, b%gcd ==0" $
            prop_divisible
      ...


Efficient way to calculate game time It's a common occurrence nowadays to have different refresh rates across different devices. That's why it's very important to keep track of the device's frame rate to keep the game time consistent across different refresh rates.

Is this an efficient way to go about this?

var timeBetween;
var timeLast;

function gameLoop() {
  var timeNow = window.performance.now();
  timeBetween = timeNow - timeLast;
  timeLast = timeNow;

  gameLogic(timeBetween); // Send time to game logic to calculate e.g. game speed etc.
  gameDraw();

  requestAnimationFrame(gameLoop);
}

One thing I recommend is to use a "scale factor" (I'm making names, there might be an official name for this approach) - a unit-less value that you multiply to time-sensitive values to compensate for frame rate differences. One perk when doing it this way is you don't have to write time sorcery in your equations (which is everywhere usually). You write them as is and just multiply this value to the result.

The formula is scale = delta / 16.6 where 16.6 is the frame time for 60fps. But this may differ per device, so you might need to find out the device refresh rate first and adjust this accordingly. According to MDN:

  The number of callbacks is usually 60 times per second, but will generally match the display refresh rate in most web browsers as per W3C recommendation.
  https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame

Here's an example calculation with a box that should move 60 units at 60fps. At 60fps, it moves an intended 60 units. But if the frame rate drops to 40fps, it should not move 40 units. Scale factor compensates for that by scaling up the calculated value.

// At 60fps, a frame takes 16.6ms
const delta = 16.6;
const scale = delta / 16.6; // 1
const boxMoved = 60; // your logic at 60fps calculates 60 units
const officialBoxMoved = boxMoved * scale; // 60

// At 40fps, a frame takes 25ms
const delta = 25;
const scale = delta / 16.6; // 1.5
const boxMoved = 40; // your logic at 40fps calculates 40 units
const officialBoxMoved = boxMoved * scale; // 60

Your code would look like:

let lastTime = performance.now();

(function gameLoop(currentTime){
  requestAnimationFrame(gameLoop);

  const scale = (currentTime - lastTime) / (100 / 6);
  lastTime = currentTime;

  gameLogic(scale);
  gameDraw();

}());

In terms of the code you just posted, there isn't much of a difference. But where the difference will be is in how you write your game logic using this value.


Hangman in stateless JavaScript I just want some feedback to improve readability and structure for this implementation of Hangman. The goal was to write it in a purely functional way. However, coming from an imperative background, I found it difficult to organize functional code.

function Hangman(word, revealed = word.replace(/\w/g, '_'), errors = 0) {
  const hasLost = errors > 5;
  const hasWon = !hasLost && word === revealed;

  const tryGuess = (letter, character, guess) =>
    new RegExp(letter, "i").test(character) ? character : '_';

  const revealCharacter = (letter, character, guess) => /_/.test(guess) ?
    tryGuess(letter, character, guess) : guess;

  const revealLetters = (letter, [head, ...rest], [guess, ...guessed]) => {
    const character = / /.test(head) ? ' ' : revealCharacter(letter, head, guess);
    return rest.length ?
      character + revealLetters(letter, rest, guessed) : character;
  }

  const getErrorCount = (guess, word, count) =>
    new RegExp(guess, "i").test(word) ? count : count + 1;

  const getResult = () => hasLost ? 'You lose' : 'You win';

  if (!hasWon && !hasLost) {
    this.guess = letter => new Hangman(word,
      revealLetters(letter, word, revealed),
      getErrorCount(letter, word, errors));
    console.log(revealed, errors);
  } else {
    console.log(getResult());
  }
}

const game = new Hangman('Agile Development');
game.guess('e').guess('a').guess('t').guess('z');

UPDATE: Below is the revised edition of the code with the advised changes. The code base looks much cleaner and I decided to try to stick to only using primitives and functions. Having each function accomplish a single task increased the code size, but definitely improved the readability and maintainability of the code.

function Hangman(text, revealed = null, errors = 0) {
  { // constructor
    if (revealed === null) {
      return Hangman(text, revealMatch(/\s/g, text), 0);
    }

    if (isDone()) {
      console.info(getGameStatus());
    } else {
     console.info(arguments);
      return makeGuess;
    }
  }

  function createMatchedCharacterList(matcher) {
    return (revealed, actual, index) => {
      if (matcher.test(actual)) {
        revealed[index] = actual;
      }
      return revealed;
    }
  }

  function revealMatch(matcher, text) {
    return text.split('').reduce(createMatchedCharacterList(matcher), [])
  }

  function revealCharacter(guess) {
    return Object.assign([], revealed, revealMatch(new RegExp(guess, 'i'), text));
  }

  function makeGuess(guess) {
    return (hasCharacter(guess) && !hasGuessed(guess)) ?
      Hangman(text, revealCharacter(guess), errors) :
      Hangman(text, revealed, errors + 1);
  }

  function hasCharacter(character) {
    return text.search(character) > -1;
  }

  function hasGuessed(guess) {
    return revealed.join('').search(guess) > -1;
  }

  function hasLost() {
    return errors > 5;
  }

  function hasWon() {
    return text === revealed.join('');
  }

  function isDone() {
    return hasLost() || hasWon();
  }

  function getGameStatus() {
    return isDone() ? hasLost() ? 'You lose' : 'You win' : 'Incomplete';
  }
}

console.log(Hangman('eatz')('e')('a')('t')('z'));

Such a great question,

hasLost and hasWon should be functions
You should write functions with the keyword function, otherwise you get a mess of anonymous functions in stack-traces. The fat arrow syntax was only meant for inline functions.
head is not a great variable name, I don't know what it is supposed to be from the name
On the whole, the challenge in pure fp is to never assign, which you do a few times, read this for good measure: https://stackoverflow.com/a/33114654/7602
I am not sure I like that the guess function creates a new Hangman object every single time, there has to be a cleaner approach.

I found an interesting guide: https://drboolean.gitbooks.io/mostly-adequate-guide/ which states that this should be avoided.. Meanwhile, this is my this littered approach. It follows all my recommendations, and shows where I feel fat arrows have a place. I will try to rewrite again once I finish the guide ;)

function Hangman(word, copy, mistakes){

  //Initialize again, no assignments here ;)
  if(!copy){
    return Hangman(word, '_'.repeat(word.length), 0 );
  }

  //Non Exposed functions requiring context
  const won = (() => {
    return function won(){
      return word == copy;
    };
  })();

  const lost = (() => {
    return function lost(){
      return mistakes > 5;
    }
  })();

  const updateMistakes = (() => {
    return function updateMistakes(oldCopy , newCopy){
      return mistakes += oldCopy == newCopy ? 1 : 0;
    }
  })();

  const applyGuess = (() => {
    return function applyGuess(letter){
      return copy  = word.split('').map( (c,i) => letter == c ?  c : copy[i] ).join('');
    }
  })();

  //Exposed Functions
  function guess(){
    return function guess(letter){
      if(!done()){
        updateMistakes( copy , applyGuess(letter) );
      }
      this.display();
      return this;
    };
  }

  function display(){
    return function display(){
      console.log( won()? 'You won' : lost() ? 'You lost' : copy + ' ' + mistakes );
    };
  }

  //Return object
  return {
    guess : guess(),
    display: display(),
  }

  //Pure function?
  function done(){
    return won() || lost();
  }

}

const game = Hangman('Agile Development');
game.guess('e').guess('a').guess('t').guess('z');

Okay, 2nd version with far less this, many more clean functions:

function Hangman(word, copy, mistakes){

  //Initialize again, no assignments here ;)
  if(!copy){
    return Hangman(word, '_'.repeat(word.length), 0 );
  }

  //Exposed Functions
  function guess(){
    return function guess(letter){
      if( !done( won(word,copy), lost(mistakes) ) ){
        //An assignment, it was unavoidable I think
        mistakes = updateMistakes( mistakes, copy , copy = applyGuess(word,copy,letter) );
      }
      display(word, copy, mistakes, letter );
      return this;
    };
  }

  //Return object, kind pointless now that there is only 1 function :/
  return {
    guess : guess()
  }

  //Not entirely pure function, not cacheable
  function display(word, copy, mistakes, letter){
    console.log( won(word,copy) ? 'You won' :
                 lost(mistakes) ? 'You lost' :
                 copy + ' ' + mistakes + ' You tried (' + letter + ')' );
  }

  //Pure functions!!
  function applyGuess(word, copy, letter){
    return word.split('').map( (c,i) => letter == c ?  c : copy[i] ).join('');
  }

  function updateMistakes(mistakes, oldCopy , newCopy){
    return mistakes + (oldCopy == newCopy ? 1 : 0 );
  }

  function done(won, lost){
    return won || lost;
  }

  function won(word, copy){
    return word == copy;
  }

  function lost(mistakes){
      return mistakes > 5;
  }

}

const game = Hangman('Agile Development');
game.guess('e').guess('a').guess('t').guess('z');


Evaluating a hashing function used to create secure API key What are your thoughts on the following code? is it secure enough?

Note: $password is used to represent the secret, which would essentially be a SHA512 hash of

username . password . signupdate(unixtime)

Do you think I'm covered?

Here's a demo for the testing purposes.

error_reporting(E_ALL & ~E_NOTICE | E_STRICT);
ini_set("display_errors", "1");
echo '
';
$break = '
';
$password = trim("password");
echo 'step1 the password is posted
';
echo $password;
echo $break;
echo $break;
$time = date("F j, Y, g:i a");
echo 'step2 get the time in human readable format
';
echo $time;
echo $break;
echo $break;
$salt1 = $time . hash('sha512', (sha1 .$time));
echo 'step3 create salt1
';
echo $salt1;
echo $break;
echo $break;
$salt2 = substr(md5(uniqid(rand(), true)), 0, 25);
echo 'step4 create salt2
';
echo $salt2;
echo $break;
echo $break;
$hash = str_split($password);
echo 'step5 split the password into an array
';
print_r($hash, false);
echo $break;
echo $break;
echo 'step6 hash each charachter of the password in the array
';
foreach($hash as $key => $value) {
$hashed[] = $salt2 . hash('sha512', ($salt . $value)) . $salt . hash('sha256', (salt2 . $key));
}
print_r($hashed, false);
echo $break;
echo $break;
$hashed_2 = implode($hashed);
echo 'step7 implode the array into a single hash
';
echo $hashed_2;
echo $break;
echo $break;
$hashfinal = str_shuffle($hashed_2);
echo 'step8 shuffle the hash to decrease entropy
';
print_r($hashfinal, false);

No, it isn't secure.

Inventing your own hashing algorithms is a horrible idea. When doing password hashing, use something invented by cryptographers that was made for password hashing, e.g. bcrypt. Because I'm not a cryptographer either, I can't give you a full explanation why your code is insecure. In short, the mistake you made was hashing each character of the password on its own. A few things I spotted:

The final output has a length proportional to the length of the password. This can be abused for denial-of-service attacks or to limit the search space when cracking via brute force.
In your final hash, the letters of $salt2 will occur more often. Therefore the resulting hash isn't evenly distributed over the output domain.
Same letters in the password obviously have the same hash. But as that is directly reflected in the output, this can be used as an attack vector (even if I don't know which letter is repeated).
It is not possible to extract the salt(s) from a given hash because you shuffle the string – therefore it is impossible to say whether a given password is the same as the hashed password (we can only check whether the lengths are the same). This makes your hash function unsuitable for authentication – but for what are you using it then? For an API key, an unique random number that does not depend on user input would be better.
…

Your code also uses, but does not define the $salt variable – it assigns $salt1 and $salt2. The $salt1 variable isn't used anywhere.

Secure password hashing is only difficult if you try to use a home-brewed solution. I outlined suggested authentication workflows in another answer which includes a list of recommended password hashing solutions (neither your code, SHA-512, nor MD5 is on that list, don't use them for passwords).


Calculate Kth Row of Pascal's Triangle Purpose

Implement a solution that returns the values in the Nth row of Pascal's Triangle where N >= 0.

Math

First three rows of Pascal's Triangle:

    1
  1   1
1   2   1
...
...

These row values can be calculated by the following methodology:

For a given non-negative row index, the first row value will be the binomial coefficient where n is the row index value and k is 0).
The next row value would be the binomial coefficient with the same n-value (the row index value) but incrementing the k-value by 1, until the k-value is equal to the row index value.

Implementation

There are two things the calculator would need in order to operate correctly:

A Binomial Coefficient Calculator, which it would depend on directly
A Factorial Calculator, which the Binomial Coefficient Calculator would depend on directly

public class FactorialCalculator {

  public static int calculateFactorial(int n) {
    if (n < 0) {
      throw new IllegalArgumentException("n value should be non-negative");
    }

    int factorialValue = 1;

    if (n == 0) {
      return factorialValue;
    }

    for (int multiplier = n; multiplier > 0; multiplier--) {
      factorialValue *= multiplier;
    }

    return factorialValue;
  }
}

public class BinomialCoefficientCalculator {

  public static int calculateBinomialCoefficient(int n, int k) {
    if (n < 0) {
      throw new IllegalArgumentException("n value cannot be negative");
    }

    if (k < 0) {
      throw new IllegalArgumentException("k value cannot be negative");
    }

    if (n < k) {
      throw new IllegalArgumentException("n value cannot be less than k value");
    }

    return FactorialCalculator.calculateFactorial(n) / (FactorialCalculator.calculateFactorial(k) * FactorialCalculator.calculateFactorial(n - k));
  }
}

public class PascalsTriangleRowValuesCalculator {

  public static List<Integer> calculateRowValues(int rowIndex) {
    if (rowIndex < 0) {
      throw new IllegalArgumentException("row index must be non-negative");
    }

    List<Integer> rowValues = new ArrayList<>();
    for (int k = 0; k <= rowIndex; k++ ) {
      rowValues.add(BinomialCoefficientCalculator.calculateBinomialCoefficient(rowIndex, k));
    }
    return rowValues;
  }
}

In case n == 0 the loop body of calculateFactorial wouldn't be executed at all, and factorialValue would remain 1. You may safely remove the special case test.
The calculations of the binomial wastes time by recomputing factorials of the same number over and over again. The way the main loop in calculateRowValues is set up exaggerates the problem. Consider precomputing the list of required factorials once.
The code begins to fail at fairly small n: \$14!\$ is already beyond the 32-bit range. I recommend drop the factorial approach whatsoever, and compute the next row directly from the previous, using an \$\binom{n+1}{k} = \binom{n}{k-1} + \binom{n}{k}\$ identity.


Getting paginated data from ajax call and putting the data on the page At the moment, it gets the first page worth of data and then each subsequent page when the user scrolls down to the bottom of the page.

My JavaScript/jQuery level is fairly weak, however I have managed to cobble this together and would like to know how I can improve my code.

 var page = 1;

 $(function() {
   getUsers(page);
   page++;
 });

 $(window).scroll(function() {
   if ($(window).scrollTop() + $(window).height() == $(document).height()) {
     getUsers(page);
     page++;
   }
 });

 function getUsers(page) {

   $.ajax({
     url: '/ajax/users?page=' + page
   }).done(function(data) {
     var users = data;
     var user;
     var url = "<?php echo url('profile/') ?>";

     if (page > users.last_page) {
       return true;
     } else {
       $.each(users.data, function(key, value) {
         var image;
         if (value.image[0]) {
           image = value.image[0].name;
         } else {
           if (value.profile.gender == 'male') {
             image = 'maleDefault.png';
           } else {
             image = 'femaleDefault.jpg';
           }
         }
         user = "

";
         $('.panel-body').append(user);
       });
     }
   });
 }

When you have an if/else statement inside an else statement and that is the only thing inside that statement, you should make the else statement into and else if instead.  So you would change this:

     if (value.image[0]) {
       image = value.image[0].name;
     } else {
       if (value.profile.gender == 'male') {
         image = 'maleDefault.png';
       } else {
         image = 'femaleDefault.jpg';
       }
     }

to this

if (value.image[0]) {
   image = value.image[0].name;
} else if (value.profile.gender == 'male') {
     image = 'maleDefault.png';
} else {
     image = 'femaleDefault.jpg';
}

All that this does is remove a level of indentation to make the code easier to read.


Rust Echo Command Implementation I'm just starting out in Rust and I find the concept of ownership confusing so I wrote an implementation of the echo command. I would like to know if I could have set the initial value on the echo variable any better or just any general improvements.

use std::env;

fn main() {
    let args: Vec<String> = env::args().collect();

    let mut echo: String;
    if let Some(string) = args.get(1) {
        echo = string.to_string();
    } else {
        return;
    }

    for arg in &args[2..] {
        echo.push(' ');
        echo.push_str(arg.as_str());
    }
    echo.push('\n');

    println!("{}", echo);
}

I would not say I am any more proficient but I would make these changes if I wrote it.

skip(1) first arg.
join(" ") instead of iterate though args.

You do not need a mutable value.

use std::env;

fn main() {
    let args: Vec<String> = env::args().skip(1).collect();

    if args.len() == 0 {
        println!("");
    } else {
        println!("{}", args.join(" "));
    }
}

I would do one more thing and send to stdout since your mimicking echo, but maybe much for this answer.


Slider with JavaScript I built this simple slider with JS, but I am not so experienced with programming and I wonder what can be improved in this code, how to dry it, or how to apply some OOP paradigms to it, what is not good in it, what is bad practice... If you are going to build this simple slider how would you approach it?

JSFIDDLE

HTML

    <button id="prev">Prev</button>

    <button id="next">Next</button>

CSS

* {
    margin: 0;
    padding: 0;
    border: 0;
    list-style-type: none;
    transition: all .5s;
}
body {
    font: bold 100%/1.5 monospace;
}
.container {
    width: 300px;
    height: 200px;
    margin: auto;
    position: relative;
}
.slider-wrapper {
    overflow: hidden;
}
.slider-wrapper ul {
    width: calc(8 * 300px);
    overflow: hidden;
}
.slider-wrapper li {
    float: left;
}
img {
    display: block;
}
button {
    font: bold 80%/1.5 monospace;
    cursor: pointer;
    padding: 1em;
    background-color: purple;
    color: whitesmoke;
    border-radius: 50%;
    position: absolute;
    top: 35%;
    z-index: 5;
}
#prev {
    left: -50px;
}
#next {
    right: -50px;
}

JS

    //window.addEventListener('load',function(){
var imgs = document.getElementsByTagName('img');
var numOfImgs = imgs.length; // 4
var widthOfImg = 300; //imgs[0].getBoundingClientRect().width; // 300
var prevBtn = document.getElementById('prev');
var nextBtn = document.getElementById('next');
var currentImg = 0;
var sliderWrapper = document.querySelector('.slider-wrapper>ul');
var translatedPixels = 0;

prevBtn.addEventListener('click', function () {
    if (currentImg === 0) {
        return;
    }
    currentImg--;
    sliderWrapper.style.transform = 'translateX(' + (-translatedPixels + widthOfImg) + 'px)';
    translatedPixels -= 300;
});
nextBtn.addEventListener('click', function () {
    if (currentImg === numOfImgs - 1) {
        return;
    }
    currentImg++;
    sliderWrapper.style.transform = 'translateX(' + -widthOfImg * currentImg + 'px)';
    translatedPixels += 300;
});
//});

* {
    margin: 0;
    padding: 0;
    border: 0;
    list-style-type: none;
    transition: all .5s;
}

First, I wouldn't do this. This resets styles on all elements, including other elements on the page. One rule I follow in CSS is not to unnecessarily override styles, especially the defaults. Unless you have a good reason to do so or you have custom base style for everything (like bootstrap).

.gallery{...}
.gallery .button{...}
.gallery .items{...}

//or

.gallery{...}
.gallery-button{...}
.gallery-items{...}

Another good practice in CSS is to namespace your styles. Selectors after another selector only take effect to descendants of the former selector. This makes it possible to limit the coverage of the style down to a certain element's descendants. Another way to do it is to use a naming scheme that's similar to namespacing. This makes your styles mean something, appear like it has heirarchy, and does not accidentally override other elements' styles.

<div class="gallery"

The markup can also be simplified. A slider is simply a list of things. The markup should be built in a way that it does resemble a list semantically. Then you improve using JS from there. You can add the buttons dynamically in JS, as they mean nothing in the markup.

;(function(){

 // Your code here

}());

One thing I find annoying in JS is that everything you write is exposed to the global namespace by default. Your code will be prone to conflicting names, or mutating values. One thing you can do to prevent being a victim, or being the cause of conflicts is to enclose your code inside a closure. This prevents your code from being overwritten. To prevent your code overwriting stuff, you must declare stuff in this scope (use var).

var imageWidth = 300;

function generateXTranslate(index, width){
  return 'translateX(-' + (index * width) + 'px)';
}

nextBtn.addEventListener('click', function () {
  if (index === numOfImgs - 1) return;
  sliderWrapper.style.transform = generateXTranslate(++index, imageWidth);
});

prevBtn.addEventListener('click', function () {
  if (!index) return;
  sliderWrapper.style.transform = generateXTranslate(--index, imageWidth);
});

A lot of the code is redundant. You can simplify by taking out common code and moving them out to their function. In here, you can move out the setting of the transform into a function. Also, you can simply track down the current image as an index instead of the pixels, since the translated pixels is just a function of the index times the width of the images.


Should I use atexit to close db connection? I just realized that I could turn:

import MySQLdb

def main():
    db = MySQLdb.connect( .. )
    try:
        while True:
           do_thingy()
    finally:
        db.close()

into:

import MySQLdb, atexit

def main():
    db = MySQLdb.connect( .. )
    atexit.register( db.close )
    while True:
        do_thingy()

Which lets me keep the code for closing the database next to the opening, and saves me a line of code and an indentation level, which I like. Is there a reason why I shouldn't do this?

Firstly, an alternative:

import MySQLdb, contextlib

def main():
    db = MySQLdb.connect( .. )
    with contextlib.closing(db):
         do_thingy()

The database connection will be closed after the with block even in the case of exceptions.

The primary problem created by atexit is in any attempt to reuse that code in question. It will only release the database connection when the entire program exits. That's fine in the case where this function is your entire program. But at some later point that my cease to be the case.

If you really want the database connection to be closed at the end of the function, its clearest if you write code that does that rather then assuming that this is the only function called in the program.

There really is not much of saving to use the atexit route. I think the clarity in the "with" method is worth the extra level of indentation. Additionally, you can use this same technique anywhere in code not just in a main function.


Prompt user for input then create an acronym from it My program is complete and working, but I would like second opinions on it. The program prompts the user to enter something like a phrase or some words. Then it will create an acronym of what was entered. What are some adjustments I can make to my program to make it more efficient?

#include <iostream>
#include <string>
#include <cctype>
using namespace std;

string acronym(string str);

int main()
{
    string str;

    while (true)
    {
        cout << "\nPlease enter a string: ";
        getline(cin, str);

        if (str == "")
        {
            break;
        }

        cout << "\n\nThe acronym is \"" << acronym(str) << "\"" << "\n";
    }

    system("PAUSE");

    return 0;
}

string acronym(string str)
{
    string phrase = "";

    phrase = str[0];

    for (int i = 0; i < str.length(); i++)
    {
        if (str[i] == ' ')
        {
            phrase += str[i+1];
        }
    }

    return phrase;
}

The procedure name acronym does not tell you what this procedure is doing. Naming it create_acronym would make the code more obvious.
Error checking: You assume that the user only enters a single space between words. This will cause problems if the user enters more than one space by accident, introducing spaces in your acronym
Most times acronyms are capitalized. You might want to capitalize each letter before adding it to the phrase.
With renaming your procedure create_acronym, you can rename the phrase variable to acronym, which is more appropriate since an acronym is not a phrase. A phrase is one or more words.
Pausing execution: Some say using system("PAUSE") is bad. Instead, just read from cin to a variable and discard the value. That way the application will hang waiting for input until the user presses the ENTER key.


Read string of any length in C I've written code in C for reading a string of any length, and printing the string. It's one of my first C programs. I posted the program as part of a Stack Overflow question, and received a comment:

... there are many things wrong with your C version.

I'd really like to know what is wrong with the function I have written. Suspects: EOF handling, calling dynamic memory allocation functions too many times.
#include <stdlib.h>
#include <stdio.h>

/* Asks the user for string input.
 * Returns a pointer to the string entered by the user.
 * The pointer must be freed.
 */
char* input()
{
    char *s = malloc(sizeof(char));  /* For the null character */
    if (s == NULL) {
        fprintf(stderr, "Error: malloc\n");
        exit(1);
    }
    s[0] = '\0';

    /* Read characters one by one */
    char ch;
    size_t s_len = 0;
    while((ch = (char) getchar()) != '\n') {
        if (ch == EOF) {
            exit(0);
        }
        s_len++;
        s = realloc(s, (s_len * sizeof(char)) + sizeof(char));
        if (s == NULL) {
            fprintf(stderr, "Error: realloc\n");
            exit(1);
        }
        s[s_len - 1] = ch;
        s[s_len] = '\0';
    }
    return s;
}

int main()
{
    printf("Name: ");
    char *s = input();
    printf("%s\n", s);
    free(s);
    return 0;
}

/* Asks the user for string input.
 * Returns a pointer to the string entered by the user.
 * The pointer must be freed.
 */

Slightly misleading in that this function doesn't ask for input. (As written it is not responsible for printing the prompt.)

Perhaps also should clarify the intended behavior:

If there is input that is not terminated by a newline, is that string returned?
When input is terminated with a newline, is the newline preserved in the returned string?

char* input()
{
    char *s = malloc(sizeof(char));  /* For the null character */

In C, char by definition is 1 byte. sizeof (char) is unnecessary and adds visual noise.

    if (s == NULL) {
        fprintf(stderr, "Error: malloc\n");
        exit(1);

It's usually considered poor behavior if calling a library function causes the entire program to terminate.  You should return an error value (e.g. NULL) and leave that decision up to the caller.

    }
    s[0] = '\0';

    /* Read characters one by one */
    char ch;

ch must be an int.  getchar returns an int precisely so that it can return a value outside the range of char (technically unsigned char) to indicate failure (i.e., the EOF value).  Otherwise you would not be able to distinguish EOF from a legitimate byte value.

    size_t s_len = 0;
    while((ch = (char) getchar()) != '\n') {
        if (ch == EOF) {

It's more idiomatic to check for EOF in the loop condition and then check for character values inside the loop body:

int ch;
while ((ch = getchar()) != EOF) {
    if (ch == '\n') {
        ...

Checking for EOF in the loop condition allows you to safely truncate ch to char throughout the entire loop body, which makes it a bit easier to reason about.

            exit(0);

Same thing here about terminating the program.  It's especially weird here since this exits with a success code and doesn't return the string to the caller.

        }
        s_len++;
        s = realloc(s, (s_len * sizeof(char)) + sizeof(char));

Never do x = realloc(x, ...).  If realloc fails and returns NULL, you will have lost the old value of x and will be unable to free it, resulting in a memory leak.  You instead should use a temporary variable:

char* newBuffer = realloc(s, ...);
if (newBuffer == NULL) {
  ...
}
s = newBuffer;

Again, sizeof (char) is noise.  You additionally should generally avoid sizeof (Type); it's not robust if the types change since you would need to edit more places (and neglecting to do so could lead to silent buffer overflows and introduce security vulnerabilities).  It's better to use sizeof expression where expression is based on the corresponding variable.  For example, if you want the code to be robust if the type of s changes (e.g. if you wanted to adapt the code to support wchar_t), you should do:

char* newBuffer = realloc(s, (s_len + 1 /* NUL */) * sizeof *s);
...

Finally, calling realloc for every byte read is grossly inefficient.  A better approach is to maintain a buffer where you keep track of its allocated size and to grow it exponentially (e.g. doubling in size) whenever you need more space.

        if (s == NULL) {
            fprintf(stderr, "Error: realloc\n");
            exit(1);

Same thing about terminating the program.  Also should do free(s); along this path.

int main()
{
    printf("Name: ");

I/O is normally buffered.  You should call fflush(stdout) afterward to ensure that the prompt is printed to the screen before waiting for input.


Highlighting partitions in distinct color Given:

G is an arbitrary graph
partitions is a list of its edges divided into (in this case) 3 sets.

Color generation:

Its purpose is to generate one distinct color for each partition.
The function turns a number between 0 and 1 into an RGB color.
For the given example, with 3 partitions, it would be called with 1/3, 2/3, and 1, and return a list of codes for green, blue, and red.
For other numbers it would be used to spread the different colors as widely as possible.
Visually, think of a circle with R, G, and B equally spaced around it.
The number is the fraction of the way around the circle from R to pick the color, so if the number is .4, the color would be between G and B, closer to the G.

Color assignment:

For each set in the partition, a different color is generated and assigned to a list of colors.
In this case edge_color_list would be 9 greens, 4 blues, and 2 reds.
The actual list is:
[(0, 1.0, 0.0), (0.0, 0, 1.0), (0.0, 0, 1.0), (0, 1.0, 0.0), (0, 1.0, 0.0), (0, 1.0, 0.0), (0, 1.0, 0.0), (0, 1.0, 0.0), (0, 1.0, 0.0), (0, 1.0, 0.0), (0.0, 0, 1.0), (0, 1.0, 0.0), (1.0, 0, 0.0), (1.0, 0, 0.0), (0.0, 0, 1.0)]

Problem:
The top and bottom sections can't really be changed, but the def_color() function and edge_color_list sections each look like they were written in C, and I think they could be done more elegantly.
It's obvious how I was thinking while writing it (i.e. how I would write it in C), but I'd like to know how python coders think.
I'm having trouble writing python without a heavy C accent (I wrote C for decades).
I understand the python language fairly well (or know how to recognize what I don't know and how to look it up).
And I've reached the stage where what I've written feels wrong.
But I still don't seem to think in native python:
#!/usr/bin/env python3
import matplotlib.pyplot as plot
import networkx as nx

G = nx.petersen_graph()
partitions = [{0, 3, 4, 5, 6, 7, 8, 9, 11}, {1, 2, 10, 14}, {12, 13}]

def get_color(fraction):
    if fraction < 1/3:
        color = (1-3*fraction, 3*fraction, 0)
    elif fraction < 2/3:
        fraction -= 1/3
        color = (0, 1-3*fraction, 3*fraction)
    else:
        fraction -= 2/3
        color = (3*fraction, 0, 1-3*fraction)
    return color

edge_color_list = [None] * len(G.edges())
for i in range(len(partitions)):
    items = list(partitions[i])   # convert from set
    for j in range(len(items)):
        edge_color_list[items[j]] = get_color((i+1)/len(partitions))

nx.draw(G, with_labels=True, pos=nx.circular_layout(G), width=2,
        node_color='pink', edge_color=edge_color_list)
plot.show()

Any suggestions about how I could have thought differently while writing the above would be appreciated.

I would change this
for i in range(len(partitions)):
    items = list(partitions[i])   # convert from set
    for j in range(len(items)):
        edge_color_list[items[j]] = get_color((i+1)/len(partitions)

to:
for idx, partition in enumerate(partitions): # I personally do it your way
    items = list(partition)   # convert from set
    for item in items:
        edge_color_list[items] = get_color((idx+1)/len(partitions)
        # idx is the `i` from before

I referenced this.
There might be a linting plugin to help align your code towards being pythonic


Radix sort is deceiving I wanted to compare radix_sort to quick_sort for values limited to 0..127 so I implemented this :

void bin_radix_sort(int *a, const size_t size, int digits) {
    assert(digits % 2 == 0);

    int *b = malloc(size * sizeof(int));

    for (int exp = 0; exp < digits; exp++) {
        // Count elements
        size_t count[2] = {0};
        for (size_t i = 0; i < size; i++)
            count[(a[i] >> exp) & 1]++;

        // Cumulative sum
        count[1] += count[0];

        // Build output array
        for (int i = size - 1; i >= 0; i--)
            b[--count[(a[i] >> exp) & 1]] = a[i];

        int *p = a; a = b; b = p;
    };

    free(b);
}

I was able to compare it to qsort with :

struct timespec start;

void tic() {
    timespec_get(&start, TIME_UTC);
}

double toc() {
    struct timespec stop;
    timespec_get(&stop, TIME_UTC);
    return stop.tv_sec - start.tv_sec + (
        stop.tv_nsec - start.tv_nsec
    ) * 1e-9;
}

int cmpfunc (const void * a, const void * b) {
   return ( *(int*)a - *(int*)b );
}

int main(void) {
    const size_t n = 1024 * 1024 * 50;
    printf("Init memory (%ld MB)...\n", n / 1024 / 1024 * sizeof(int));
    int *data = calloc(n, sizeof(int));
    printf("Sorting n = %ld data elements...\n", n);
    size_t O;

    tic();
    O = n * log(n);
    qsort(data, n, sizeof(data[0]), cmpfunc);
    printf("%ld %lf s\n", O, toc());
    int d = 6;

    tic();
    O = d * (n + 2);
    bin_radix_sort(data, n, d);
    printf("%ld %lf s\n", O, toc());
}

This gives me this output:

$ gcc -Os bench.c -lm
$ ./a.out
Init memory (200 MB)...
Sorting n = 52428800 data elements...
931920169 1.600909 s
314572812 0.963436 s

I guess my code needs code-review since I was expecting to be six times better than quick sort.

Well, I guarantee that qsort doesn't start with a heap allocation. Benchmark several different sizes of array and graph the results, to see where the lines hit the Y-axis: how much of your benchmark is just measuring the speed of malloc?

size_t count[2] = {0};

Depending on the smartness of your compiler, an array that doesn't have to be an array might be a big performance hit. Arrays are often stored in memory, on the stack, as opposed to scalar variables which can be stored in registers without any real cleverness on the compiler's part. Plus, in this case, your code seems to be needlessly convoluted by the use of an array instead of two different variables count0 and count1. Compare:

for (int exp = 0; exp < digits; ++exp) {
    // Count elements
    size_t count0 = 0;
    size_t count1 = 0;
    for (size_t i = 0; i < size; ++i) {
        if ((a[i] >> exp) & 1) {
            count1 += 1;
        } else {
            count0 += 1;
        }
    }

    // Cumulative sum
    count1 += count0;

    // Build output array
    for (int i = size - 1; i >= 0; --i) {
        if ((a[i] >> exp) & 1) {
            b[--count1] = a[i];
        } else {
            b[--count0] = a[i];
        }
    }
    int *p = a; a = b; b = p;
}

After rewriting like this, it becomes apparent that after the first loop, count0 + count1 == size; and after the "Cumulative sum" step, count1 == size. So we can eliminate half of the code.

    size_t count0 = 0;
    size_t count1 = size;
    for (size_t i = 0; i < size; ++i) {
        if (((a[i] >> exp) & 1) == 0) {
            count0 += 1;
        }
    }

    // Build output array
    for (int i = size - 1; i >= 0; --i) {
        if ((a[i] >> exp) & 1) {
            b[--count1] = a[i];
        } else {
            b[--count0] = a[i];
        }
    }

Then, the "Build output array" step is doing the exact same workload (a[i] >> exp) & 1 a second time! That seems like a fruitful source of optimization. What if you folded the second loop into the first loop, something like this?

for (int exp = 0; exp < digits; ++exp) {
    size_t up = 0;
    size_t down = size;
    for (size_t i = 0; i < size; ++i) {
        int x = a[i];
        if ((x >> exp) & 1) {
            b[--down] = x;
        } else {
            b[up++] = x;
        }
    }
    assert(up == down);
    // Now elements [up..size) are in reversed order,
    // so we need to flip them back around.
    reverse_array(b + up, b + size);
    int *temp = a; a = b; b = temp;
}

Writing reverse_array is left as an exercise for the reader.

I'd be interested to see your benchmark results for this "improved" algorithm.


HTMLAudioElement sound delay on keyboard clicks I am using Vue.js to play mp3 sounds on keyboard clicks. Even though each mp3 is less than 40 KB, there is a 95ms delay on heroku, on each request.

Is there a way to download sounds when dom is ready and not when clicking the buttons?

 let a = 'sound1.mp3',
        b = 'sound2.mp3';
    new Vue({
        created() {
            this.onKeyDown = this.onKeyDown.bind(this);
            document.getElementById('btn-sound').addEventListener('keydown', this.onKeyDown);
        },
        destroyed() {
            document.getElementById('btn-sound').removeEventListener('keydown', this.onKeyDown);
        },
        methods: {
            playSound(sound) {
                if (sound) {
                    var audio = new Audio(sound);
                    audio.play();
                }
            },

            onKeyDown(e) {
                switch (e.keyCode) {
                    case 12:
                        this.playSound(a);
                        break;
                    case 13:
                        this.playSound(b);
                        break;

                }

            }

        }
    });

SFC

Consider using Single File Components instead of the declarative syntax you are using right now. It removes a lot of the headache of dealing with the this keyword, and removes the necessity of having to deal with handlers. See the documentation for more information.

Life cycle hooks

Vue has a couple of life cycle hooks. You are currently using two of them. However, your code currently only works, because your html is not part of the component you just wrote. The destroyed life cycle hook is called after everything has been torn down, so there would be no html element to remove the handler from. You should be using the beforeDestroy hook instead. See this lifecycle diagram for more information.

Internal data

Your sound filenames should be internal data of your component, not some random variables outside it. They should also have descriptive names.

Sound playback delay

You are experiencing playback delay, because you load the sounds when you want to play them, instead of pre-loading them. You have two ways of dealing with this problem. If your sound file names are hardcoded, you can simply use the data initialisation function to create HTMLAudioElement s you can then later start or stop. See mdn for more information.

data () {
  return {
    sounds: {
      enter: new Audio('sound1.wav'),
      whatevercode12is: new Audio('sound2.mp3')
    }
  }
}

If you pass these sounds as properties, you need to be a bit more clever about it. You can use the created lifecycle hook to fill the sounds object like we created above, but would instead be initialised as an empty object, or you could put a watcher on the property with the immediate flag on true, and fill the sounds object as needed.

In either case you must play and immediately pause them once so they are downloaded. Then you can look them up and play them whenever you like:

this.playSound('enter')

and

playSound (soundName) {
  const playableSound = this.sounds[soundName];

  if (!playableSound) {
    throw new Error(`Called 'playSound' with unknown identifier [${soundName}]`);
  }

  playableSound.play();
}

Implementing it all

When you implement all that, you will end up with something like this:

<template>
  <div id="app">
    <input v-on:keydown.prevent="onKeyDown" value="Just focus me and press keys" />
  </div>
</template>

<script>
export default {
  name: "App",
  data() {
    return {
      sounds: {
        enter: new Audio(
          "http://www.pacdv.com/sounds/miscellaneous_sounds/air_blower_1.wav"
        ),
        shift: new Audio(
          "http://www.pacdv.com/sounds/miscellaneous_sounds/bottle_pop_1.wav"
        )
      }
    };
  },

  created() {
    for (const sound of Object.values(this.sounds)) {
      // This makes sure the sounds are loaded, but not played
      sound.play().then(() => {
        sound.pause();
      });
    }
  },

  methods: {
    playSound(soundName) {
      const playableSound = this.sounds[soundName];

      if (!playableSound) {
        throw new Error(
          `Called 'playSound' with unknown identifier [${soundName}]`
        );
      }

      if (!playableSound.paused) {
        // The previous sound is playing. We could reset the current sound with
        // playableSound.currentTime = 0
        // but we probably don't want to cut off that sound, so lets make a copy of the
        // sound and dereference the previous one
        this.sounds[soundName] = new Audio(playableSound.src);
      }

      this.sounds[soundName].play();
    },

    onKeyDown(e) {
      switch (e.keyCode) {
        case 13:
          return this.playSound("enter");
        case 16:
          return this.playSound("shift");
        default:
          // Do nothing
          break;
      }
    }
  }
};
</script>


Proper way of handling exceptions while reading user input from file I am parsing a text file (UTF-8) and checking for several conditions that need to be satisfied for the code to be happy. If not, then there is no point to continue and the program should die.

At the moment I am catching exceptions and if caught, throw RuntimeException to signal the user that something went wrong.

What would be a proper way of doing this or is it OK the way it is?

/**
 * Reads the maze from plain text and converts it to char[][]
 * Marks the start (B) and end (F) coordinates if found, if not throws exception
 * Expects each line to have the same length as the the first line, throws exception if not
 *
 * @return maze as char[][]
 */
private char[][] readMazeFromFile() {
    List<String> stringList;
    try {
        stringList = Files.readAllLines(Paths.get("h40.in"), Charset.forName("UTF-8"));
    } catch (MalformedInputException e){
        throw new RuntimeException(String.format("Did not understand file,
        check the charset, expecting UTF-8\n%s", e.getMessage()));
    } catch (IOException e) {
        throw new RuntimeException(String.format("Did not understand file,
        sorry\n%s", e.getMessage()));
    } catch (Exception e){
        throw new RuntimeException(String.format
        ("Generic error while reading the input file, sorry\n%s", e));
    }

    char[][] charMaze = new char[stringList.size()][];
    for (int i = 0; i < stringList.size(); i++) {
        if (stringList.get(i).contains("B")) {
            startX = i;
            startY = stringList.get(i).indexOf("B");
        }
        if (stringList.get(i).contains("F")) {
            endX = i;
            endY = stringList.get(i).indexOf("F");
        }
        try {
            charMaze[i] = stringList.get(i).toCharArray();
        } catch (ArrayIndexOutOfBoundsException e) {
            throw new RuntimeException(String.format("Did not understand the maze,
            expecting square.\n%s", e.getMessage()));
        }

    }
    if (startX == -1 || startY == -1)
        throw new RuntimeException("Could not find starting point (B), aborting.");
    if (endX == -1 || endY == -1)
        throw new RuntimeException("Could not find ending point (F), aborting.");
    return charMaze;
}

I agree with @skiwi on the File input value. hardcoding the file name in this method is not helpful. I disagree with some of the Exception suggestions though...

IOExceptions are a special breed. If you cannot read the file, then you have to report it as an IOException. It irritates me that FileNotFoundException is an IOException though... so, I handle that one specially..... Otherwise, I tend to let IOExceptions 'escape' method calls. and the calling code should handle them... it's a bigger problem than the function.

Additionally, there is no reason to catch the broader Exception, and wrap it as a RuntimeException. There are no other thrown exceptions from the readLines method that are not RuntimeExceptions anyway....

Also, MalformedInputException is an IOException anyway... may as well handle them together.

Finally, your check to make sure the maze is square, does not. It should be checking the line-length to make sure it matches the expected size.... but it does not.

(Edit: Use StandardCharsets.UTF_8)

Here's how I would write your method:

/**
 * Reads the maze from plain text and converts it to char[][]
 * Marks the start (B) and end (F) coordinates if found, if not throws exception
 * Expects each line to have the same length as the the first line, throws exception if not
 *
 * @return maze as char[][]
 */
private char[][] readMazeFromFile(Path mazeFile) throws IOException {
    if (!Files.isRegularFile(mazeFile) || !Files.isReadable(mazeFile)) {
        throw new IllegalArgumentException("Cannot locate readable file " + mazeFile);
    }
    List<String> stringList = Files.readAllLines(mazeFile, StandardCharsets.UTF_8));

    char[][] charMaze = new char[stringList.size()][];
    for (String line : stringList) {
        if (line.length()  != charMaze.length) {
            throw new IllegalArgumentException("Expect the maze to be square, but line " + i + " is not " + charMaze.length + " characters long";
        }
        if (line.contains("B")) {
            startX = i;
            startY = line.indexOf("B");
        }
        if (line.contains("F")) {
            endX = i;
            endY = line.indexOf("F");
        }
        charMaze[i] = line.toCharArray();
    }
    if (startX == -1 || startY == -1)
        throw new IllegalArgumentException("Could not find starting point (B), aborting.");
    if (endX == -1 || endY == -1)
        throw new IllegalArgumentException("Could not find ending point (F), aborting.");
    return charMaze;
}


Modelling employees in Go as a basic OOP exercise I am learning Go and have the following working code:

$GOPATH/src/mvctest/main.go

package main

import (
    "fmt"
    "mvctest/model"
)

func main() {
    // Initialise the data model
    employees := model.NewPeople("Employee")

    // Populate the model
    employees.Add(model.NewPerson("Joe", 99))
    employees.Add(model.NewPerson("Sue", 45))
    employees.Add(model.Person{Name: "Tom", Age: 22}) // break encaps a bit

    // Test retrieval from model
    fmt.Printf("You have %d employees.\n", employees.Count())

    for i := 0; i < employees.Count(); i++ {
        f, err := employees.Person(i)
        if err != nil {
            fmt.Printf("Unable to get employee %d because %v", i, err)
            return
        }
        fmt.Printf("Employee %d is %s\n", i+1, f)
    }

    // Test error handling for impossible values
    test(employees, -1)
    test(employees, 42)

}

// Tests whether a person can be retrieved using an index value
func test(ppl *model.People, i int) *model.Person {
    p, err := ppl.Person(i)
    if err != nil {
        fmt.Printf("Unable to get %s %d because %v\n", ppl.Group(), i, err)
        return nil
    }
    return p
}

$GOPATH/src/mvctest/model/model.go

package model

import (
    "errors"
    "fmt"
)

// ------------------------------------------------------

type Person struct {
    Name    string
    hatsize int
    Age     int
}

func (p *Person) String() string {
    return fmt.Sprintf("%s aged %d.", p.Name, p.Age)
}

func NewPerson(name string, age int) Person {
    return Person{name, 0, age}
}

// ------------------------------------------------------

type People struct {
    group string
    list  []Person
}

func (ppl *People) SetGroup(g string) {
    ppl.group = g
}

func (ppl *People) Group() string {
    return ppl.group
}

func (ppl *People) Add(p Person) {
    ppl.list = append(ppl.list, p)
}

func (ppl *People) Count() int {
    return len(ppl.list)
}

func (ppl *People) Person(i int) (*Person, error) {
    n := len(ppl.list)
    if i < 0 || i >= n {
        return nil,
            errors.New(fmt.Sprintf("%s index %d is out of range %d-%d.",
                ppl.group, i, 0, len(ppl.list)-1))
    }
    return &ppl.list[i], nil
}

func NewPeople(group string) *People {
    ppl := new(People)
    ppl.group = group
    return ppl
}

Output

You have 3 employees.
Employee 1 is Joe aged 99.
Employee 2 is Sue aged 45.
Employee 3 is Tom aged 22.
Unable to get Employee -1 because Employee index -1 is out of range 0-2.
Unable to get Employee 42 because Employee index 42 is out of range 0-2.

I'd appreciate any comments but a few things that I am uncertain about or which bother me.

I'm not sure whether defining constructing NewPerson and NewPeople is the right thing to do. I can't have two functions called New in the same file / package and I can't make these into methods of different structs (i.e. different object types) since the caller doesn't yet have an instance to use.
I'm wondering if there is a simple way to allow iterating using range rather than the for loop in main.go?

Using a NewType() constructor-like function is the idiomatic way in Go. See Effective Go: Constructors and composite literals. Also relevant: Effective Go: Package names:

  The importer of a package will use the name to refer to its contents, so exported names in the package can use that fact to avoid stutter. (Don't use the import . notation, which can simplify tests that must run outside the package they are testing, but should otherwise be avoided.) For instance, the buffered reader type in the bufio package is called Reader, not BufReader, because users see it as bufio.Reader, which is a clear, concise name. Moreover, because imported entities are always addressed with their package name, bufio.Reader does not conflict with io.Reader. Similarly, the function to make new instances of ring.Ring—which is the definition of a constructor in Go—would normally be called NewRing, but since Ring is the only type exported by the package, and since the package is called ring, it's called just New, which clients of the package see as ring.New. Use the package structure to help you choose good names.

And if you want to iterate over a whole "collection" (e.g. slice, array, map, channel), the For statements with range clause is the clearest way. "Unfortunately" your People type does not expose such values, so it's not possible to use for range here.

It can still be improved to call People.Count() once:

for i, count := 0, employees.Count(); i < count; i++ {
    f, err := employees.Person(i)
    // ...
}

Other improvements

Your NewPeople() could be simply written as:

func NewPeople(group string) *People {
    return &People{group: group}
}

In People.Person() it's redundant to call errors.New() and fmt.Sprintf(), as the fmt package also has an fmt.Errorf() function doing just that:

func (ppl *People) Person(i int) (*Person, error) {
    if n := len(ppl.list); i < 0 || i >= n {
        return nil, fmt.Errorf("%s index %d is out of range %d-%d.",
            ppl.group, i, 0, n-1)
    }
    return &ppl.list[i], nil
}

Also it's just a matter of taste, but I find it easier to read if you use the max index instead of the length (which is max+1):

if max := len(ppl.list) - 1; i < 0 || i > max {
    return nil, fmt.Errorf("%s index %d is out of range %d-%d.",
        ppl.group, i, 0, max)
}

Also it's a good habit for a specific type to be consistent. Either use pointers in all places, or non-pointers. In case of Person, you mix both: People.Add() expects Person, but People.Person() returns *Person.