Hard-to-test 3 Tier Architecture I'm having trouble testing some code. It's laid out in such a way that the business logic relies on the persistence layer.

Some classes require that an object be saved to a database. Some classes re-read the object they were working to get the "original" version of it. All of this tends to happen in what's been labeled the business logic layer.

This a WCF application where the business logic layer manipulates objects from the DAL

Code sample below.

public class WCFService : IWCFService
{

    public OrderHeader GetOrder(int id)
    {
        var om = new OrderManager();
        return om.Read(id);
    }

}

[DataContract]
public class OrderHeader
{
    [DataMember]
    public int id { get; set; }

    public OrderHeader Read(int id)
    {
        // ADO.NET calls
        return new OrderHeader();
    }

    public void CreateUpdate()
    {
        // ADO.NET calls
        this.id = GetSqlOutputParam();
    }

    private int GetSqlOutputParam()
    {
        // get the identity supplied by the executed stored proc
    }
}

public class OrderManager
{

    public OrderHeader Read(int id)
    {
        OrderHeader oh = new OrderHeader();
        return oh.Read(id);
    }

    public bool SaveOrder(OrderHeader order)
    {
        // do lots of complex stuff
        order.CreateUpdate(); // update here because some other class is going to want to do a fresh read
        // instantiate other "manager" classes
        order = order.Read(order.id); // make sure we get the latest record from the database in case other manager classes changed my order
        // do more complex stuff
        order.CreateUpdate();

        return true;

    }
}

Firstly and unfortunately, there isn't an immediate plan to replace the entity specific ADO.NET with an ORM.

I would like to start by making the code a little more testable by removing all persistence calls from the business logic layer, or separating the actual logic away from these calls while keeping them in the same layer. Instead of update() and read() littered throughout those manager classes, the object would be passed around as much as needed.

To give some context, the code suffers from a lot of persistence related bugs. I feel like removing persistence from the equation would make matters easier to deal with.

Edit: I miscommunicated the intent of the GetSqlOutputParam() method. It's not just returning a const, it is pulling it from an executed SqlCommand.

Firstly and unfortunately, there isn't an immediate plan to replace the entity specific ADO.NET with an ORM.

That's fine.

This is more questionable:

private int GetSqlOutputParam() { return 1; }

It would be much clearer to declare this as a descriptively named constant near the top of the class (being class-level scoped):

private const int defaultEntityId = 1;

But that's far from being what's making that code hard to test.

  I would like to start by making the code a little more testable by
  removing all persistence calls from the business logic layer, or
  separating the actual logic away from these calls while keeping them
  in the same layer. Instead of update() and read() littered throughout
  those manager classes, the object would be passed around as much as
  needed.

A class such as an OrderHeader exists to hold data. Nothing less, but also and more importantly nothing more. It's a Plain Old CLR Object, a POCO. You hit the nail on the head, that's exactly what you need to do. Move the ADO.NET code into its own class.

As much as I dislike this pattern when it's used on top of Entity Framework, if I had to refactor some ADO.NET code into its own class, I'd probably write something like a repository:

  This blog post (click the diagram) explains how dependency inversion enables testing.

  (I am not the author of the blog this link points to, it's pure coincidence that this guy's name is Mat. I'm not this Mat's Mug.)


TicTacToe Telnet server in Python The code below works. The timings are as close as I can get to the system not packing out.

Basically telnet to a server to play tic-tac-toe. Automated it. Bottom left is 1, middle left is 2, middle right is 3 etc

Once a game is finished, I say "yes" and go again.

Anyway I can optimise it?

#!/usr/bin/env python
from telnetlib import Telnet
import sys
from pprint import pprint
from random import randint

HOST = '127.0.0.1'
PORT = 6666

tn = Telnet(HOST, PORT)
board=['','','','','','','','','']
winLines = [[0,1,2], [3,4,5], [6,7,8], [0,3,6], [1,4,7], [2,5,8], [0,4,8], [2,4,6]]

def main():
   startup()
   while True:
      readBoard()
      selectMove()

   tn.close()

def startup():

   print tn.read_until("WordsWeNeverSee",0.1)
   tn.write("O\n")
   print tn.read_until("go first.", 0.1)

def foobar():

   tn.write("yes\n")
   print tn.read_until("WordsWeNeverSee",0.1)
   tn.write("O\n")
   readBoard()

def readBoard():
   tIn = tn.read_until("WordsWeNeverSee",0.3)

   tLines = tIn.split('\n')
   sys.stdout.write(tIn)
   tLines = tLines[::-1]
   if len(tLines)==13:
       row = tLines[3].replace(' ', '').split('|')
       #X|X|O
       board[0] = row[0]
       board[1] = row[1]
       board[2] = row[2]

       row = tLines[7].replace(' ', '').split('|')
       #X|X|O
       board[3] = row[0]
       board[4] = row[1]
       board[5] = row[2]

       row = tLines[11].replace(' ', '').split('|')
       #X|X|O
       board[6] = row[0]
       board[7] = row[1]
       board[8] = row[2]
   elif len(tLines)==14:
       row = tLines[3].replace(' ', '').split('|')
       #X|X|O
       board[0] = row[0]
       board[1] = row[1]
       board[2] = row[2]

       row = tLines[7].replace(' ', '').split('|')
       #X|X|O
       board[3] = row[0]
       board[4] = row[1]
       board[5] = row[2]

       row = tLines[11].replace(' ', '').split('|')
       #X|X|O
       board[6] = row[0]
       board[7] = row[1]
       board[8] = row[2]
   else:
      readBoard()

   if tIn.find("play again") != -1:
      foobar()

def countCharInCells(char, tup): # X, [2,4,6]
   count = 0
   for n in tup:
      if board[n] == char:
         count += 1

   return count

def findEmpty(tup): # [2,4,6]
   for n in tup:
      if board[n] == '':
         return n

   return -1

def selectMove():

   xCount = board.count('X')

   # Strong first turn move
   if xCount == 0:
      tn.write('9')
      return

   # Best response to center start
   if board.count('X') == 1:
      if board[4] == 'X':
         tn.write('9')
         return

   # See if we can win
   for wl in winLines: #[[0,1,2], [3,4,5], [6,7,8], [0,3,6], [1,4,7], [2,5,8], [0,4,8], [2,4,6]]
      if countCharInCells('O', wl) == 2:
         choice = findEmpty(wl)
         if choice != -1:
            tn.write(str(choice+1))
            return

   # Try to block opposition winning moves
   for wl in winLines:
      if countCharInCells('X', wl) == 2:
         choice = findEmpty(wl)
         if choice != -1:
            tn.write(str(choice+1))
            return

   # Try to snag a corner
   choice = findEmpty([0,2,6,8])
   if choice != -1:
      tn.write(str(choice+1))
      return

   # Pick a random valid square when stumped
   while True:
      choice = randint(0,8)
      if board[choice] == '':
         tn.write(str(choice+1))
         return

if __name__ == "__main__":
    main()
    main()

I am not particularly a python expert but can offer some feedback from a fairly generic perspective.

There are no useful (plain English) comments until line 105. There is definitely room to improve the commenting of this code.
Unless StackExchange is affecting the formatting, this doesn't appear to adhere to PEP 8 (the definitive Python style guide). For example, you are using three spaces per indentation level, not four.
As @200_success spotted in a comment, you are calling main() twice at the end: is this intentional? I cannot understand why this would be.
foobar() does not sound like a sensible name for a function: it gives me absolutely no clue as to what the function's purpose is.
What is the significance of "WordsWeNeverSee"? This might be well documented in the Telnet library but it might be helpful to offer a brief explanation here, so somebody can follow this code without knowing that library well.  Failing that, at least a reference to the main documentation would be good.
Your main game engine architecture smells funny to me. When main() first calls readBoard(), this can then lead to readBoard() being called recursively, and/or control passing to foobar() which then calls readBoard() again. This feels like it might lead to a deeply-nested call stack. Without following the logic through carefully I have no idea what the rationale is for this, but it seems unlikely to be the best design approach.
Relatedly, your key functions readBoard() and selectMove() take no input and return no output. They are operating on data structures that exist globally, aside from their function scope - i.e. they are having side effects. This also seems unlikely to be the best approach; I'm not really qualified to say what the best or most-pythonic way might be, but a more object-oriented approach could be one way to go.
readBoard() seems to have two blocks of identical code, inside the first if and following elif. In other words, you're saying: if len(tLines)==13, then do some stuff; if len(tLines)==14, then do the same stuff; else do something different.  Can you see the quick way to simplify this code?


Security feedback sought on Perl code running command on unsafe filename in Unix env I'm looking for security feedback on the following fully functional code.

The code is trying to safely use the Unix 'file' command to give details about the file.

A hard link is used to create a safe filename that can be used in a command line.

The paramref is a hash read from config, and considered safe. file_base_dir is a directory with only alphanumerics in the name. The 'file' command is the standard one.

What ways could my code be exploited?

sub file_details {
    my ( $self, %arg ) = @_;
    my $filename = $arg{'filename'};
    my $paramref = $arg{'paramref'};
    my $file_cmd = '/usr/bin/file';

    my $safe_filename = $$paramref{'file_base_dir'} . "/link_to_unsafe_file_file_type_build";

    # Just in case the link exists.
    unlink($safe_filename);
    if ( not link( $filename, $safe_filename ) ) {
        confess "Failed to create link $safe_filename to $filename because $!";
    }

    my $file_type = `$file_cmd '$safe_filename'`;

    # remove the link
    unlink($safe_filename);
    $file_type =~ s/\A[^:]+://x;
    chomp $file_type;

    return $file_type;
} ## end sub file_details

Interesting approach with link.
If the only vulnerable parameter is filename,
this technique protects from malicious input,
because the filename parameter will not be part of the `...` bit executing shell commands.
I don't think it's possible to trick link into command execution.

On the other hand, although you say that "file_base_dir is a directory with only alphanumerics in the name",
this function doesn't ensure that, simply trusts it to be true.
It would be good to add a call in this function to another function that validates the directory parameter.

I tried to think of other ways to validate the filename parameter,
without creating a link.
You could forbid certain characters such as ; and $ to prevent command injection,
but I'm not sure that's an exhaustive list.
So while the link trick seems hackish,
it does give comfort that vulnerable parameters never participate in a `...` shell expansion.


First jQuery Plugin, Would like a code review I created my first jQuery plugin over this weekend. I would like a code review so that I can improve my code in the future and learn from any mistakes I make. Any tips for Github would also be greatly appreciated since I am also new to GitHub :).

Demo: https://rawgithub.com/Shivambh28/ezToggle/master/demo.html

GitHub: https://github.com/Shivambh28/ezToggle

Plugin code:

(function($){
    $.fn.ezToggle = function(options) {
        var defaults = {
            selector: '.selector',
            speed : 200
        }
        defaults.minHeight = $(defaults.selector).outerHeight(true);
        var options =  $.extend(defaults, options);
        return this.each(function() {
            var o    = options,
                yDiv = $(this),
                oHei = $(yDiv).height();
            $(yDiv).not('.opened').addClass('closed');
            $('.closed').height(defaults.minHeight);
            $('.opened').height(oHei);
            $(defaults.selector).click(function() {
                if ($(this).closest(yDiv).hasClass('closed') ) {
                    $('.opened').removeClass('opened').addClass('closed').animate({height:defaults.minHeight}, defaults.speed);
                    $(this).closest(yDiv).removeClass('closed').addClass('opened').animate({height:oHei}, defaults.speed);
                } else if ( $('.opened').length == 1 ) {
                    $(this).closest(yDiv).removeClass('opened').addClass('closed').animate({height:defaults.minHeight}, defaults.speed);
                }
            });
        });
    };
})(jQuery);

Props to you on your first plugin! Well here are a few pointers in no particular order:

- Double Wrapping: When you cache yDiv=$(this);, you're wrapping the element with the jQuery object. And then you do: $(yDiv).not(... What this does is wrap that object twice. You've already wrapped it so you don't need to do so again. Just do this yDiv.not(... after you cache.

So this is how you should do it:

//Don't
yDiv = $(this);
$(yDiv).not('.opened').addClass('closed');

//Instead do:
yDiv = $(this);
yDiv.not('.opened').addClass('closed');

- Variable Names: If you want anyone else to provide you input on your code, they have to be able to understand it. You should use variable names that will make sense to anyone reading your code. Yes, in this simple example one can deduce the what and why you named the variables the way you did and we can sort of follow along. But when you start working on large scale projects, things can get out of hand real quick. Take for example your yDiv variable. The keyword this can refer to any element that goes through the .each() which might not be a div in the first place. What if it was a ul or any other element. In that case, the name yDiv would no longer represent what is actually stored in there. Anyways, try to use meaningful and descriptive variable names and stay away from names that might not exactly represent what is stored in said variable.

- Cache your selectors: As a rule of thumb, if you use a selection more than once, you should cache it. What happens when you use $("someElem") is jQuery has to jump into the DOM and look through all elements that would match that selection. So you should really do a search only once, and save your results for future use. This way you can look and play with them whenever you want without having to try and find them again.

Ex:

var closestElem = $(this).closest(yDiv); //Cache it
if (closestElem.hasClass('closed')){ //Now use it
    //...
}

- Save a function calls: If you read the jQuery source code you'll see that shortcut methods like .click(), .scroll(), and etc. all reference the .on() method. All they do is basically call the .on() method with some parameters. Well why not just go straight to the meat and potatoes?

Check this out:

$("#foo").click(function() {
    //Do your awesomeness
});

//That does the same thing as this:
$("foo").on("click", function() {
    //Do your awesomeness even awesomener.
});

//You can also use that to set up your own custom events
$("foo").on("myEvent", function() {
    //bar
});

That just makes the syntax so easy to remember as well as provide you with enormous control over your events. If you're going to use jQuery on a regular basis, I recommend that you take some time and read through the source code of the methods you're using. To find them quickly just use CTRL+F and type methodName:. That should jump you straight to what you want to know. This way you can understand what and how they are doing things, and even find better ways to do them on your own. Also if you see something you think should be done better or differently, you understand how it works and you can contribute to jQuery.


Django model with methods to register various kinds of events Is there any better way to define the function in the class which shares most of the same parameters like functools.partial?

from django.db import models

class RegisterUserAction(models.Manager):
    def register_adhoc_created(self, **kwargs):
        return self.create(action_type="ADHOC_CREATED", **kwargs)

    def register_adhoc_deleted(self, *args, **kwargs):
        return self.create(action_type="ADHOC_DELETED", **kwargs)

    def register_broadcast_created(self, **kwargs):
        return self.create(action_type="BROADCAST_CREATED", **kwargs)

One way would be keep a map of these method names and related action_type. Now we can override __getattribute__ to check if one of these methods were accessed and return the partial function with related action_type.

from functools import partial

from django.db import models

register_map = {
    'register_adhoc_created': 'ADHOC_CREATED',
    'register_adhoc_deleted': 'ADHOC_DELETED',
    'register_broadcast_created': 'BROADCAST_CREATED',
}

class RegisterUserAction(models.Manager):

    def __getattribute__(self, attr):
        if attr in register_map:
            return partial(self.create, action_type=register_map[attr])
        return super(RegisterUserAction, self).__getattribute__(attr)

Personally I would do something like this only when the number of such methods are huge and are polluting the code base. Because simplicity in this case greater than being DRY and other devs will have to spend some understanding it. Also because of this custom implementation the code editors won't allow developers to jump into respective code because such method(s) actually doesn't exist.


Extract numerics from matrix A with logical matrix B I have a matrix A containing numeric values and a matrix B containing 0/1s:

A <- matrix(1:25, 5, 5)
B <- matrix(c(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0), 5, 5)

A                             B
1    6   11   16   21         0    0    1    0    0
2    7   12   17   22         0    0    0    0    1
3    8   13   18   23         0    0    0    0    1
4    9   14   19   24         0    0    1    0    0
5   10   15   20   25         0    0    1    0    0

I want to extract the elements of A where B==1 in row order. The desired result is:

11 22 23 14 15

I've come up with two possible ways to do this:

#1
rowSums(A*B)

#2
t(A)[as.logical(t(B))]

It seems like there should be a better (more elegant or faster) way to do this...

While rowSums(A*B) looks pretty nice for me, you can also try

A[which(t(B) == 1, arr.ind = T)[,2:1]]


Search suggestion box with jQuery and AJAX var searchUser = function(searchQuery){
    var request = $.ajax({
        url:"search.php",
        type: "POST",
        data: { search:searchQuery},
        success: function(reply){
            $("#search-box").css("display", "block");
            $("#search-box").html(reply);
        }
    });
}

I know this is a terrible way of doing it, right? How else would I best go about outputting the search result to my #search-box? Right now, I send an AJAX request to my search.php file which returns all the HTML of the search. Surely this wouldn't be good from a performance perspective?

You should ensure your spacing is consistent.
Since you're using a function expression rather than a declaration, you should terminate the line with a semicolon.
You aren't making any use of the request variable, so why even declare it?
You aren't taking advantage of method chaining. This is causing you to construct two jQuery objects for #search-box when you only need one.
You're right in that this is not the best way to handle it; consider returning JSON or XML.
You have a success callback but not an error callback. In this particular case I don't imagine it would be too bad (your search box will just remain hidden, presumably), but you may want to add one anyway.

Here's how I think your code could look.

var searchUser = function(searchQuery) {
    $.ajax({
        url: "search.php",
        type: "POST",
        data: { search: searchQuery },
        dataType: "json",
        success: function(response) {
            var resultList = response.reduce(function(accumulator, value) {
                return accumulator + "
" + value + "
";
            }, "
");
/* alternative for older browsers:
            var resultList = "
";
            $.each(response, function(index, element) {
                resultList += "
" + element + "
";
            });
*/
            resultList += "
";
            $("#search-box").html(resultList).show();
        },
        error: function() {
            $("#search-box").html("An error occurred. Search suggestions could not be loaded.");
        }
    });
};

search.php would then have to do something like:

$results = array("suggest", "suggestion", "suggested", "suggesting");
die(json_encode($results));


Calling tries to lazy load in every properties getter I've inherited a class which has a bunch of properties defined as:

public int ID
{
    get { return m_nOID; }
    set { m_nOID = value; }
}

public int ConxnDetail
{
    get { LoadDetails(); return m_nConxnDetail; }
    set { m_nConxnDetail = value; }
}

public bool ConxnConstraints
{
    get { LoadDetails(); return m_bConxnConstraints; }
    set { m_bConxnConstraints = value; }
}

public bool IsShowCables
{
    get { LoadDetails(); return m_bIsShowCables; }
    set { m_bIsShowCables = value; }
}

public int DefaultQueue
{
    get { LoadDetails(); return m_nDefaultQueue; }
    set { m_nDefaultQueue = value; }
}

private User LoadDetails()
{
    if (!IsLoaded && !IsNew)
    {
        Logger.InfoFormat("Loading user details. UserID: [{0}]", m_nOID);
        User user = RemoteActivator.Create<RemoteUser>().GetUserById(m_nOID);
        if (user != null)
        {
            CopyDetails(user);
            MarkLoaded();
        }
        else
            ClearDetails();
    }
    return this;
}

I would absolutely love to clean up the conventions here a bit and use only automatic getters/setters. Is this possible to do? If not, any other suggestions on reducing the boilerplate needed?

There is the INotifyPropertyChanged event handler, but I'm not sure why you would need that in this case?  Instead why not just load the details when the ID changes as that seems to be the only value that will change between LoadDetails calls.

Perhaps something like:

public int ID
{
    get { return m_nOID; }
    set {

       if(m_NOID != value) {
          InitialiseUserDetails(m_nOID);
       }

       m_nOID = value;
    }
}

If this was ok, you could remove the private backing fields as I believe you wish to do.


Add elements to an array inside a foreach loop I have the code below:

<?php

$i = array(1, 2, 3);

$w = array(1 => '11', 2 => '22', 3 => '33');

foreach ($i as $f) {
    $ws = $w[$f];

    $file_names[] = array(
        'fn' => 'fname',
        'sn' => 'sname',
        'is' => 'is',
        'rv' => 'rv'
    );

    foreach ($file_names as $k => $v) {
        if (!isset($file_names[$k]['mod'])) {
            $file_names[$k]['mod'] = array(
                'ct',
                'pt'
            );
        }

        if (!isset($file_names[$k]['pw'])) {
            $file_names[$k]['pw'] = array(
                'task'  => '3',
                'min'   => 'min',
                'max'   => 'max',
                'value' => "" . $ws . ""
            );
        }
    }
}

print_r($file_names);

?>

What I’m trying to do is add new elements to an array in some particular conditions. The only thing that I don’t like is the second foreach loop part. I.e.:

    foreach ($file_names as $k => $v) {
        if (!isset($file_names[$k]['mod'])) {
            $file_names[$k]['mod'] = array(
                'ct',
                'pt'
            );
        }

        if (!isset($file_names[$k]['pw'])) {
            $file_names[$k]['pw'] = array(
                'task'  => '3',
                'min'   => 'min',
                'max'   => 'max',
                'value' => "" . $ws . ""
            );
        }
    }

Is there any way to write this in a more elegant manner?

maybe I am missing something, but why do you even need to have two different arrays $i and $w? second thing, $file_names array should go outside of the foreach loop since this array doesn't depend on the iteration of the first foreach loop.

I am not sure about the exact output which you expect but it seems that you can only iterate through the file names array and when you need to update this line

'value' => "" . $ws . ""  , you could just look up the value of $ws in the particular array rather than iterating the entire array every time.


Transforming arrays into the correct formats I am still attempting to learn how to best handle arrays and iterate their values transforming them into the correct formats that are needed for my database/API.

So I have this array:

$arr = [
    0 => [
        'field' => [
            0 => [
                '@name' => 'first_name',
                '@value' => 'Jack'
            ],
            1 => [
                '@name' => 'last_name',
                '@value' => 'Lew'
            ],
            2 => [
                '@name' => 'email',
                '@value' => 'jack@gmail.com'
            ]
        ]
    ]
];

And I need it to look like this:

[0] => Array
    (
        [first_name] => Jack
        [last_name] => Lew
        [email] => jack@gmail.com
    )

To get to this I have taken the same approach I usually take:

for( $i = 0; $i < count($arr); $i++ ) {
    foreach($arr[$i] as $k => $v) {
        print_r($k . ': \r\n');
        if( is_array($v) ) {
            foreach($v as $key => $val) {
                $new[$i][$val['@name']] = $val['@value'];
            }
        }
    }
}

The problem I having here is, I believe there has to be a better way to achieve the same results without being three levels deep of loops, since they are slower and just harder to debug altogether.

I would appreciate any insights on how I can make this code better in both readability and performance.

If you know that the key field exists, you could use array_map() to apply a function on each row. This function could use array_column() to transform a column as keys, and another as values:

$arr = [
    [
        'field' => [
            ['@name' => 'first_name', '@value' => 'Jack'],
            ['@name' => 'last_name', '@value' => 'Lew'],
            ['@name' => 'email', '@value' => 'jack@gmail.com']
        ]
    ],
    [
        'field' => [
            ['@name' => 'first_name', '@value' => 'John'],
            ['@name' => 'last_name', '@value' => 'Doe'],
            ['@name' => 'email', '@value' => 'John@gmail.com']
        ]
    ]
];

$data = array_map(function($item) {
    return array_column($item['field'], '@value', '@name');
}, $arr) ;

print_r($data);

Output:

Array
(
    [0] => Array
        (
            [first_name] => Jack
            [last_name] => Lew
            [email] => jack@gmail.com
        )

    [1] => Array
        (
            [first_name] => John
            [last_name] => Dow
            [email] => John@gmail.com
        )

)


Saving preferences in localStorage I am working on a small app to help my kids study their multiplication and addition.  And I have various preferences I want to be persisted between visits.  What do you think of the below to get the settings out of local storage?  I know I could do it even more simply by simply stringifying the whole object at once, but I want the preferences to contain the answers and I don't want to save all of the answers every time the preferences change...although as I write this that seems like less of an issue...this is for my kids so I don't care about supporting older browsers.

function getPrefs() {
        "use strict";

        getPrefs.prefs = getPrefs.prefs || new Object();
        var prefs = getPrefs.prefs;

        if (prefs.populated) return prefs;

        var localStorage = window['localStorage'];

        prefs.addAnswer = function (answer, correct) {
            this.answers.unshift(answer);
            if (correct) {
                this.correctAnswers++;
                localStorage.setItem("correctAnswers", this.correctAnswers.toString());
                if (this.correctAnswers % 12 === 0) {
                    this.level++;
                    localStorage.setItem("level", this.level.toString());
                }
            } else {
                this.wrongAnswers++;
                localStorage.setItem("wrongAnswers", this.wrongAnswers.toString());
            }
            localStorage.setItem("answers", JSON.stringify(this.answers.slice(0, 100)));
            this.answeredAdded();
        }

        prefs.answeredAdded = function () { };

        prefs.answers = JSON.parse(localStorage.getItem("answers") || '[]');

        prefs.topValue = Number(localStorage.getItem("topValue"));
        prefs.bottomValue = Number(localStorage.getItem("bottomValue"));

        prefs.showHistory = localStorage.getItem("showHistory") === "false";
        prefs.operator = localStorage.getItem("operator") || "x";

        prefs.correctAnswers = Number(localStorage.getItem("correctAnswers") || 0);
        prefs.wrongAnswers = Number(localStorage.getItem("wrongAnswers") || 0);
        prefs.level = Number(localStorage.getItem("level") || "1");

        prefs.populated = true;

        return prefs;
    }

Design

Since you want only one instance of preferences, you can make use of the singleton pattern:

var prefs = (function () {
    var instance;

    return {
        getInstance: function () {
            if (!instance) instance = new Prefs();
            return instance;
        }
    };
})();

Then you can call the method wherever needed: prefs.getInstance().

You can also encapsulate initialisation logic into a constructor, it seems like you wanted to use OOP anyway:

function Prefs() {
    // init your properties here, use 'this', e.g:
    this.answers = [];
}

Prefs.prototype.addAnswer = function (answer, correct) {
    // use 'this' just as you did in your code
};

localStorage

The way you deal  with localStorage seems ok to me. If you want, you can turn correctAnswers and wrongAnwsers into setters, tough syntax for them is a bit baroque:

Object.defineProperty(Prefs.prototype, 'correctAnswers', {
    set: function (val) {
        localStorage.setItem('correctAnswers', val);
        return val;
    }
});

Use it as you previously did: this.correctAnswers++.

Minor issues

This line is useless:

var localStorage = window['localStorage'];

You can skip toString here:

localStorage.setItem("correctAnswers", this.correctAnswers.toString());

topValue and bottomValue don't have default values. They will be NaN if there is no such item in localStorage.


Help reducing loops with filter code The following code is very slow. I was wondering if you can help me speed it up.

private void filter(List<CheckBox> boxList)
        {
            // refLanguageTabs is a TabControl the Tabcount is 9
            for (int i = 0; i < refLanguageTabs.TabCount; i++)
            {
                Control[] ctrls = refLanguageTabs.TabPages[i].Controls.Find(refLanguageTabs.TabPages[i].Name + "Grid", true);
                DataGridView dgv = ctrls[0] as DataGridView;

                // average row count is 3000
                for (int j = 0; j < dgv.RowCount; j++)
                {
                    for (int k = 0; k < boxList.Count; k++)
                    {
                        if (dgv.Rows[j].Cells[1].Value.ToString() != boxList[k].Name.ToString())
                        {
                            dgv.Rows[j].Visible = false;
                        }
                    }
                }
                dgv.Refresh();
            }
        }

Some few thoughts without testing or anything else:

Cache the value of the cell in a local variable.
Replace for-loops if possible with foreach-loops.
The call of boxList[x].Name.ToString() is unnecessary, it is already a String.
break after setting the visibility to avoid unnecessary checks.

foreach(DataGridViewRow row in dgv.Rows)
{
    String cellValue = row.Cells[1].Value.ToString();
    foreach(CheckBox boxItem in boxList)
    {
        if(cellValue == boxItem.Name)
        {
            row.Visible = false;
            break;
        }
    }
}

On another sidenote, please use meaningful variable names, even in such stupid iterating loops.


Get most recent modified time of a folder recursively with PHP I'm relatively new to php and any advice is appreciated.

I have written a code to the most recently updated time for a folder in php.
Can you please correct me if I am doing anything wrong.

And here the code I use:

function get_m_time_dir ($directory)
{
$last_modified_time = 0;
$handler = opendir ($directory);
while ($file = readdir ($handler))
{
    if ($file == '.' || $file == '..')
    {
        continue;
    }
    if (is_file ($directory . DIRECTORY_SEPARATOR . $file))
    {
        $filemtime = filemtime ($directory . DIRECTORY_SEPARATOR . $file);
        if ($filemtime > $last_modified_time)
        {
            $last_modified_time = $filemtime;
        }
    } else
    {
        $filemtime = get_m_time_dir ($directory . DIRECTORY_SEPARATOR . $file);
        if ($filemtime > $last_modified_time)
        {
            $last_modified_time = $filemtime;
        }
    }
}
return $last_modified_time;
}

Can you please correct me if I am doing anything wrong.

It depends on whether you care for the deleted files or not. If you do, you should also check the modification time for the directory itself, not only files in it.

The code itself could be also improved:

you can remove the duplicated condition
which in turn, can be substituted with max() function.
also you can utilize the glob() function that can save you that boring $file == '..' condition, as well as other unnecessary code.

So in the end the code could be

function get_m_time_dir ($directory)
{
    $last_modified_time = 0;
    $dirmtime = filemtime ($directory);
    foreach (glob("$directory/*") as $file)
    {
        if (is_file ($file))
        {
            $filemtime = filemtime ($file);
        } else {
            $filemtime = get_m_time_dir ($file);
        }
        $last_modified_time = max($filemtime, $dirmtime, $last_modified_time);
    }
    return $last_modified_time;
}


Node.JS HTTP server displaying Google I'm reading Professional Node.js: Building Javascript Based Scalable Software by Pedro Teixeira and created a web page responding with www.google.de. Could you take a look at my code and let me know if the implementation is not good?

var request = require('request');
var url = 'http://www.google.de'; // input your url here

// use a timeout value of 10 seconds
var timeoutInMilliseconds = 10 * 10000;
var opts = {
    url : url,
    timeout : timeoutInMilliseconds
};

require('http').createServer(function(req, res) {

    res.writeHead(200, {
        'Content-Type' : 'text/html'
    });

    request(opts, function(err, webRes, body) {
        if (err) {
            console.dir(err);
            return;
        }
        var statusCode = webRes.statusCode;
        console.log('status code: ' + statusCode);
        res.write(body);
    });
}).listen(4000);

Here are my suggestions:

Get rid of the anonymous functions. They are notoriously difficult to test. I am hoping that you have written the test cases. If not, now would be a good time to try writing these tests.
There is either a documentation or implementation bug where you want the timeout to be 10s (10000ms) but are actually using a value of 10*10000ms which is 100s.
Rather than calling a method directly on require('http'), I'd like it if you import it the way you have imported request.
You are always sending back a response code of 200. You should wait till after the request has finished fetching the page and set the status code accordingly unless you have some way of serving the right content (using a cahed copy, for example).

Other than that, I think that you have the actual functionality pretty much nailed down.


Functions that converts day of year to month and day and reverse with error checking There is no error checking in day_of_year or month_day. remedy this defect.

Here is the solution:

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

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

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

        return day;
    }

    return -1;

}

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

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

        for(i = 1; yearday > daytab[leap][i]; i++) {
          yearday -= daytab[leap][i];
        }
        *pday = yearday;
        *pmonth = i;
    }
    else {
        printf("error: the yearday is invalid");
    }
}

In the day_of_year's case I have to check if the yearday is a valide one. 1 <= yearday <= (365 || 366). I changed the parameters type to unsigned, because a day can't be negative nor a year.

In the month_day's case I check if the month is a valid one, it should be 1 <= month <= 12. After this, I check if the day belongs to a valid interval.

This exercise can be found in K&R2 at page 126.

Incorrect yearday limit

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

month_day() and day_of_year() should use consistent types for month.  Suggest int for both.

// int day_of_year(unsigned int year, unsigned int month, int day) {
// void month_day(unsigned int year, unsigned int yearday, int *pmonth, int *pday) {
int day_of_year(unsigned int year,         int month, int day) {

month_day() and day_of_year() should use consistent types for yearday.  Suggest int for both.

// int day_of_year(unsigned int year, unsigned int month, int day) {
// void month_day(unsigned int year, unsigned int yearday, int *pmonth, int *pday) {
int day_of_year(unsigned int year, int month, int day) {
void month_day(unsigned int year, int yearday, int *pmonth, int *pday) {

Leap year calculation leap = ((year... is good back to 1583.  For years 4 to 1582 it is  leap = (year % 4 == 0);  1582 has other complications.  Before 4 has complications.
Suggest month_day() return int to indicate success or failure.


Inserting data into SQL-Server table dynamically I know I can do all this via SSMA quite quickly, but I wanted to see if I could do it efficiently in PowerShell myself as a little project.

Basically, the entire script takes tables from an Access database and creates a database in SQL Server with the same data. Everything from creating the table structure to cleansing the data is running quick. The slow part is inserting the data, which was inevitable but I want to see if I can make it a bit faster.

Here is the code:

<# Insert data into tables #>
function Insert_Data {
    param ( $data, $tableName )
    $columns = ($data.PSObject.Properties | where {$_.name -eq "Columns"}).value.columnName | Sort-Object

    $Insert = "INSERT INTO $tableName
    VALUES "

    $i = 0
    $x = 0

    foreach ($item in $data) {
        $Insert += "
        ("
        foreach ($item in $columns) {
            $Insert += "'" + $data.rows[$x].$item + "',"
        }
        $Insert = $Insert.Substring(0,$Insert.Length-1)
        $Insert += "),"
        if ($i -eq 900) {
            $Insert = $Insert.Substring(0,$Insert.Length-1)
            $Insert += ";"
            Invoke-SQLCMD -Query $Insert -ServerInstance "." -database tmpAccessData -erroraction "Stop"
            $Insert = "INSERT INTO $tableName
                              VALUES "

            $i = 0
        }
        $i++
        $x++
    }

    $Insert = $Insert.Substring(0,$Insert.Length-1)
    $Insert += ";"

    Invoke-SQLCMD -Query $Insert -ServerInstance "." -database tmpAccessData -erroraction "Stop"

    Remove-Variable -Name data
}

It generates a large SQL query and inserts that into the specified table once it hits 900 values or the end of the Access table.

The data variable is the full Access table pulled into an Object:

$data_Clients       = Get-AccessData -Query "SELECT * FROM Client"    -Source $($settings.FastrackFiles.access_Fastrack);

And tablename is just the name of the table in the destination SQL database.

The main problem with this code is that it's not screening column values, unfortuantely buit in mechanisms in Invoke-SQLCMD aren't viable. It allows to use -Variable but it also accepst stings, like -Variable "p0='var_value'" which don't help at all with escaping, even making it worse.

but here are opesource alternative to Invoke-SQLCMD: https://github.com/RamblingCookieMonster/PowerShell/blob/master/Invoke-Sqlcmd2.ps1

It can be installed by executing

Install-Module WFTools -Force -AllowClobber

Example of unescaped query with Invoke-SQLCMD

> Invoke-Sqlcmd -Database tb-sql-db -Server XXXXX -Username YYYY -Password ZZZZ -Query "SELECT id, um.text FROM archive_srv_db.tbUniMessage um WHERE id = '3161e665-a30e-48c4-87f2-0008d62da8a6'"

id                                   text
--                                   ----
3161e665-a30e-48c4-87f2-0008d62da8a6 Hello!

Same query using parameters and Invoke-Sqlcmd2:

$secpasswd = ConvertTo-SecureString "ZZZZ" -AsPlainText -Force
$mycreds = New-Object System.Management.Automation.PSCredential ("YYYYY", $secpasswd)
Invoke-Sqlcmd2 -Database tb-sql-db -Server XXXXX -Credential $mycreds -Query "SELECT id, um.text FROM archive_srv_db.tbUniMessage um WHERE id = @p0" -SqlParameters @{ p0="3161e665-a30e-48c4-87f2-0008d62da8a6"}

id                                   text
--                                   ----
3161e665-a30e-48c4-87f2-0008d62da8a6 Hello!

If you are concerned with escaping issues, if any had occur, you should migrate to Invoke-Sqlcmd2.
But please take a note, that parametrized query in ADO.NET can hold up to roughly 2000 parameters.
So your loop condition should be  ($i * $columns.Length) -gt 1900  (just assumption that you won't have more than 100 columns. It's to late here to write exact code :-).

And in your loop you should append to query @p$counter, where counter is incrementing variable defined outside of a loop and incremetring inside loop on each value. and build dictionary, populating it with named parameters.

Example:

#define dictionary before outer loop
$dict = @{}
....

#add value inside inner loop
$Insert += "?,"
$dict.Add("@p$counter",$data.rows[$x].$item)


Given an index return item from opposite end of list Given the following code:

namespace AtBash
{
  class Program
 {
    static List<string> letters = new List<string> {
        "Aleph", "Beis", "Gimmel", "Daled", "Hei", "Vav", "Zayin", "Chies", "Teis",
        "Yud", "Chof", "Lamed", "Mem", "Nun", "Samech", "Ayin", "Pei", "Tzadik",
        "Kuf", "Reish", "Shin", "Taf"
    };
    static string AtBash(int index) => letters[Math.Abs((index + 1) - letters.Count)];

    static void Main(string[] args)
    {
        foreach (var item in letters)
        {
            Console.WriteLine(letters.IndexOf(item));
            Console.WriteLine($"{item} -> {AtBash(letters.IndexOf(item))}");
        }
    }
  }
}

Where the purpose is:

  Given an index (say 0) return the corresponding item from the other end of the array (for our example 22).

This is the part of the code that actually does anything:

static string AtBash(int index) => letters[Math.Abs((index + 1) - letters.Count)];

The code works but I am interested in knowing if it can be made faster/more efficient. I would also like to know if there is an official term for this kind of algorithm.

If the entire goal is absolute performance of the AtBash() request, I would simply reverse the letters at startup. I'm speaking outside the context of this particular program and more towards your library where I am making the assumption that the AtBash() method calls vs. startup ratio is much higher to infinite.

If that's the case and we can eat some startup time, and you really want that per-access performance on AtBash(), simply eat a tiny bit of memory to get there:

static List<string> letters = new List<string> {
    "Aleph", "Beis", "Gimmel", "Daled", "Hei", "Vav", "Zayin", "Chies", "Teis",
    "Yud", "Chof", "Lamed", "Mem", "Nun", "Samech", "Ayin", "Pei", "Tzadik",
    "Kuf", "Reish", "Shin", "Taf"
};
static List<string> lettersReversed = letters.AsEnumerable().Reverse().ToList();
static string AtBash(int index) => lettersReversed[index];

Compare the IL in the initial accessor, letters[Math.Abs((index + 1) - letters.Count)]:

IL_0000:  ldsfld      UserQuery+Program.letters
IL_0005:  ldarg.0
IL_0006:  ldc.i4.1
IL_0007:  add
IL_0008:  ldsfld      UserQuery+Program.letters
IL_000D:  callvirt    System.Collections.Generic.List<System.String>.get_Count
IL_0012:  sub
IL_0013:  call        System.Math.Abs
IL_0018:  callvirt    System.Collections.Generic.List<System.String>.get_Item
IL_001D:  ret

To the improved accessor, letters[letters.Count - (index + 1)]:

IL_0000:  ldsfld      UserQuery+Program.letters
IL_0005:  ldsfld      UserQuery+Program.letters
IL_000A:  callvirt    System.Collections.Generic.List<System.String>.get_Count
IL_000F:  ldarg.0
IL_0010:  ldc.i4.1
IL_0011:  add
IL_0012:  sub
IL_0013:  callvirt    System.Collections.Generic.List<System.String>.get_Item
IL_0018:  ret

To the fastest accessor, lettersReversed[index]:

IL_0000:  ldsfld      UserQuery+Program.lettersReversed
IL_0005:  ldarg.0
IL_0006:  callvirt    System.Collections.Generic.List<System.String>.get_Item
IL_000B:  ret

Given your very minimal amount of actual memory in the example (which I don't think is likely to increase), I think this is a perfectly reasonable approach for any high-throughput library.

Feel free to benchmark this. The more accesses per startup, the better the payoff. You should already see payoff pretty quickly because sorting an array of a few dozen items is very fast and that's all the time we're adding to get the long-term benefits here. I wouldn't be surprised if your program even with trivial levels of access already performs better with this approach.


Create an array with the highest values from each array I am working my way through FreeCodeCamp's Javascript challenges and Return Largest Numbers in Arrays took me a minute. In this one the goal was to create an array with the highest values from each array as its elements.

I am currently using for loops to iterate through the various arrays and arrays of arrays. I wanted to see if there was a better way to do this challenge.

function largestOfFour(arr) {
  // You can do this!
  var largestNum = 0;
  var newArray = [];
  for(var i = 0; i < arr.length; i++){
    for(var k = 0; k < arr.length; k++){
      console.log(arr[i][k]);
      //create comparison
      a = arr[i][k];
      if(a>largestNum){
        largestNum = a;
        newArray[i] = largestNum;
      }
    }
    largestNum=0;
  }
  console.log('complete Test' + newArray);
  return newArray;
}

largestOfFour([[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]]);

The challenge says that you will be given exactly four subarrays, but it doesn't specify the length of each subarray.  You've assumed a square matrix, which is not stated in the problem.

To find the maximum of 4, 5, 1, and 3, you could write Math.max(4, 5, 1, 3).  If those numbers are given to you as an array, you could use Math.max.apply(null, array).

To transform each element of the outer array, use arr.map().

function largestOfFour(arr) {
  return arr.map(function(subArr) {
    return Math.max.apply(null, subArr);
  });
}

console.log(largestOfFour([[4, 5, 1, 3], [13, 27, 18, 26], [32, 35, 37, 39], [1000, 1001, 857, 1]]));


Using nested if statements to select textfield for update of associated label So I have written a very simple program using a cocoa pods library to calculate the distance between different cities. Here is what the UI looks like:

So I have a textfield on top for entry of starting location, and then four other textfields for destination city input.

I went with nested if statements which I am not fully happy with, but at least it does the job in terms of calculating distances for each. Is there a better way to do this than nesting if statements as I have done? There are only four cases for the four textfields but if there is a cleaner way to do it I would love to know about that.

Here is the code itself for the ViewController.m file:

#import "ViewController.h"
#import "DistanceGetter/DGDistanceRequest.h"

@interface ViewController ()

@property (nonatomic) DGDistanceRequest *req;

@property (weak, nonatomic) IBOutlet UITextField *startLocation;

@property (weak, nonatomic) IBOutlet UITextField *endLocationA;

@property (weak, nonatomic) IBOutlet UITextField *endLocationB;
@property (weak, nonatomic) IBOutlet UITextField *endLocationC;

@property (weak, nonatomic) IBOutlet UITextField *endLocationD;

@property (weak, nonatomic) IBOutlet UILabel *distanceA;
@property (weak, nonatomic) IBOutlet UILabel *distanceB;

@property (weak, nonatomic) IBOutlet UILabel *distanceC;
@property (weak, nonatomic) IBOutlet UILabel *distanceD;

@property (weak, nonatomic) IBOutlet UIButton *calculateButton;

@end

@implementation ViewController

- (IBAction)calculateButtonTapped:(id)sender {

    self.calculateButton.enabled = NO;

    self.req = [DGDistanceRequest alloc];
    NSString *start = self.startLocation.text;
    NSString *destA = self.endLocationA.text;
    NSString *destB = self.endLocationB.text;
    NSString *destC = self.endLocationC.text;
    NSString *destD = self.endLocationD.text;

    NSArray *dests = @[destA, destB, destC, destD];

    self.req = [self.req initWithLocationDescriptions:dests sourceDescription:start];
    __weak ViewController *weakSelf = self;

    self.req.callback = ^void(NSArray *responses){
        ViewController *strongSelf = weakSelf;
        if (!strongSelf) return;

        NSNull *badResult = [NSNull null];

          if (responses[0] != badResult) {

            double num = ([responses[0] floatValue] / 1000.0);
            NSString *ans1 = [NSString stringWithFormat:@"%.2f km", num];

            strongSelf.distanceA.text = ans1;

            if (responses[1] != badResult) {
                double num = ([responses[1] floatValue] / 1000.0);
                NSString *ans2 = [NSString stringWithFormat:@"%.2f km", num];

                strongSelf.distanceB.text = ans2;

                if (responses[2] != badResult) {
                    double num = ([responses[2] floatValue] / 1000.0);
                    NSString *ans3 = [NSString stringWithFormat:@"%.2f km", num];

                    strongSelf.distanceC.text = ans3;

                    if (responses[3] != badResult) {
                        double num = ([responses[3] floatValue] / 1000.0);
                        NSString *ans4 = [NSString stringWithFormat:@"%.2f km", num];

                        strongSelf.distanceD.text = ans4;

                    }

                }

            }

        }

        strongSelf.req = nil;
        strongSelf.calculateButton.enabled = YES;

    };

    [self.req start];

   }

@end

Please be patient with me as I am pretty much completely new to writing my own code from scratch.

First thing:

When you have a structure like that:

if (responses[1] != badResult) {
...
   if (responses[2] != badResult) {
   ...
      if (responses[3] != badResult) {
      ...

You probably have to use a loop, as the number of if statement will increase in function of the number of results in your array.

Also, you try to access to an array value without checking if this index is inside the size of your array:

i.e. : if (index >= 0 AND index < responses.cout )

What if your array has less than 4 values?

So to prevent this, it will look like that:

for (int i = 0; i < responses.count; i++) {
    if (responses[i] != badResult) {
        double num = ([responses[i] floatValue] / 1000.0);
        NSString *currentAnswer = [NSString stringWithFormat:@"%.2f km", num];
        // Do something with 'currentAnswer' like putting it inside an array (datasource)
    }
}

Second Thing

When you have that kind of UI (repetitive stuff, block/pattern of component with common function)

=> it's always depending on context,

But, it's better to use a component with reusable view, datasource: UITableView, UICollectionView etc...

You can check this tutorial if you need help to build a table view.

/------------------------------------/
          For testing only
/------------------------------------/

Also, there is another way, which is quick and dirty, but it works:

You can use view tag to access them in an ordinate way.

From your storyboard, set the tag value of the first distance label (distanceA) to 100
for distanceB 101
for distanceC 102

and so on...

Now you can access to the label from an integer thanks to

UILabel *label = (UILabel*)[self.view viewWithTag:100];

So if you put it in the previous loop:

 for (int i = 0; i < responses.count; i++) {
    if (responses[i] != badResult) {
        double num = ([responses[i] floatValue] / 1000.0);
        NSString *currentAnswer = [NSString stringWithFormat:@"%.2f km", num];
        // Getting the label with tag, THIS IS WEAK CODE
        UILabel *label = (UILabel*)[self.view viewWithTag:100+i];
        // set the currentAnswer string
        label.text = currentAnswer;
    }
}

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

Remember, using UITableView or UICollectionView is much better!


Simple parser with regular expression If I draw something in Illustrator and extract it with AI2Canvas I want it to be customizable so I wrote this simple program. It adds two variables: one to resize and one to shift the path coordinate.

#include <fstream>
#include <regex>
#include <string>

int main(int argc, char *argv[]) {
  std::string path = argv[1];
  std::ifstream input(path);

  std::size_t found_name = path.find_last_of("/\\");
  std::size_t found_extension = path.find_last_of(".");
  const int steps = found_extension - found_name - 1;
  std::ofstream output(path.substr(found_name + 1, steps) + ".txt");

  std::regex pattern("(\\d+(\\.\\d+)?), (\\d+(\\.\\d+)?)");
  std::string in;
  while (!input.eof()) {
    std::getline(input, in);
    output << std::regex_replace(in, pattern, "($1 * resize.x) + pos.x, ($3 * resize.y) + pos.y")
           << '\n';
  }
  return 0;
}

Test file Raw: https://pastebin.com/fDh9qbd8

Test file Resolved: https://pastebin.com/R2gpXD8Q

Overall, good code. There is a trap code fell into though.

while (!input.eof())

One of the frequently encountered problems with iostreams. Check the output if it has one extra newline. I'm sure you'll find it. intput.eof() is true when EOF is read, not before it.

Instead, use stream >> variable as a condition or the function that behaves like that. The expression returns a reference to stream itself, but the stream is convertible to bool to see if some failure occured. Basically, the expression above is all these steps:

Try to read.
Set failure flags if error encountered
Return the status

Put in a condition, it is the exact combination one needs to perform more or less controlled input. One needs to check if the read succeeded before doing anything. Some other functions in standard library behave like operator>>, std::getline() included.

As a result of above, the first line of the loop would just become a condition, and everything else intact.

while (std::getline(input, in)) {
    output << std::regex_replace(in, pattern, "($1 * resize.x) + pos.x, ($3 * resize.y) + pos.y")
       << '\n';
}

Smaller things follow.

Usage message would be nice. Usually frequently used utilities are put away from the code. Though in case of single argument programs it might not be much of a problem.

std::ofstream output(path.substr(found_name + 1, steps) + ".txt");

Is a little bit alerting. Since this is end user program (e.g. for a human), they might get something wrong.

Some missing consts here and there.

return 0 is redundant.


Performance testing functions for wrapping an index After asking this question, I decided to write a test and determine the fastest way to wrap an index (where my maxSize is always a power of 2).

There are 3 functions that I'm comparing:

// plain wrap
public static int WrapIndex(int index, int endIndex, int maxSize)
{
    return (endIndex + index) > maxSize ? (endIndex + index) - maxSize : endIndex + index;
}

// wrap using mod
public static int WrapIndexMod(int index, int endIndex, int maxSize)
{
    return (endIndex + index) % maxSize;
}

// wrap my masking out the top bits
public static int WrapIndexMask(int index, int endIndex, int maxSize)
{
    return (endIndex + index) & (maxSize - 1);
}

Here is my test:

public static void WrapTest(int numRuns = 10000)
{
    int index = 256;
    int endIndex = 0;
    int maxSize = 4096;

    long wrapPlain = 0;
    long wrapMod = 0;
    long wrapMask = 0;

    Stopwatch sw = new Stopwatch();

    for (int i = 0; i < numRuns; i++)
    {

        // plain
        sw.Start();
        for (int j = 0; j < numRuns; j++)
        {
            WrapIndex(index, endIndex, maxSize);
        }
        sw.Stop();
        wrapPlain += sw.ElapsedTicks;
        sw.Reset();

        // mod
        sw.Start();
        for (int j = 0; j < numRuns; j++)
        {
            WrapIndexMod(index, endIndex, maxSize);
        }
        sw.Stop();
        wrapMod += sw.ElapsedTicks;
        sw.Reset();

        // mask
        sw.Start();
        for (int j = 0; j < numRuns; j++)
        {
            WrapIndexMask(index, endIndex, maxSize);
        }
        sw.Stop();
        wrapMask += sw.ElapsedTicks;
        sw.Reset();

        // change indexes
        endIndex++;
        endIndex = endIndex % maxSize;

        index++;
        index = index % maxSize;
    }
    Console.WriteLine(String.Format("Plain: {0} Mod: {1} Mask: {2}", wrapPlain / numRuns, wrapMod / numRuns, wrapMask / numRuns));
}

I ran the test and I'm consistently getting the following results (in ticks):

Plain: 25 Mod: 16 Mask: 16 (maxSize = 512)
Plain: 25 Mod: 17 Mask: 17 (maxSize = 1024)
Plain: 25 Mod: 17 Mask: 17 (maxSize = 4096)

I was expecting that the mask will be faster than all of them, but it seems to be as fast as using the modulo operator. I've also tried increasing the numRuns, but the results are consistent.

Is my test valid? Is there a better way to test the performance?

Your results do seem odd, so I tried to rewrite your test and see if I can get different results.  I suspected that you might be starting and stopping the stopwatch too frequently, which can skew your results due to the loss of precision every time you start and stop.  In my code, I only start and stop the stopwatch once per type of calculation.

Using the code below, I get results more in line with what you would expect, namely that masking is the fastest, followed by plain subtraction, followed by modulo division.

Here is a typical output from one of my test runs:

Plain: 59.7033
Mod: 64.6872
Mask: 58.1923

In various runs, Plain and Mask tend to vary with respect to each other, and sometimes show very similar numbers.  Mod always tends to be slower.

And here is the code:

static void Main(string[] args)
{
    TestPerf(WrapIndex, "Plain");
    TestPerf(WrapIndexMod, "Mod");
    TestPerf(WrapIndexMask, "Mask");
}

public static void TestPerf(Func<int, int, int, int> getIndex, string label, int numRuns = 10000)
{
    int maxSize = 4096;

    Stopwatch sw = new Stopwatch();
    sw.Start();

    for (int i = 0; i < numRuns; i++)
    {
        for (int index = 0; index < maxSize; index++)
        {
            getIndex(index, 1234, maxSize);
        }
    }

    sw.Stop();
    Console.WriteLine(string.Format("{0}: {1}", label, ((double)sw.ElapsedTicks) / numRuns));
}

EDIT: Also notice that I cast ElapsedTicks to double before dividing to avoid rounding.  You should be careful to avoid rounding in your calculations because that just adds noise to your final result.

EDIT 2:

I played around with the code snippet in your pastie link and I was originally getting the same results as you, but I think I've finally gotten to the bottom of it.

What's happening is that without that Func wrapper, the jitter is able to do some serious optimizing.  I know it's the jitter and not the C# compiler, because all of the code is intact in the ildasm output.  In fact, for 2 out of your 3 methods (mod and mask), it's able to deduce that the methods aren't accomplishing anything at all (because they're doing a simple calculation, and the return value is just discarded), so it doesn't even bother calling them.

To validate that statement, simply comment out the line in WrapIndex() and return a plain 0, which is sure to get optimized out.  Run your program again and you'll see that all three methods now report the same times.  There's no way that doing a mod or a mask has the same cost as returning a constant 0, so that tells you that the code just isn't being executed.

This is another issue you have to be aware of when doing perf tests.  If your test is too simple, the optimizer will just discard all of your code.

In my test, the Func wrapper was preventing the jitter from optimizing as much, because it doesn't know which code is going to be executed, so it can't inline and then discard the code, which is why you get more reasonable numbers.

So I think the results from my code, using the Func delegate, give a more accurate reflection of the relative costs of your three index methods.


Demonstration of pthread calls Please review for any unnecessary casting, memory leaks, wrong use of pthread call, or validation problems in the given code.

/**********************************************************************
*       FILENAME :thread_1.c
*       DESCRIPTION:Contains Code for a program that demonstrates the
*       use of pthread calls.The created threads will accept an
*   argument and return the argument after adding a number.
****************************************************************************/
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
void *thread_entry(void *arg)
{

 int *p;
 p=(int*)arg;
 *p = (*p) + 10;
 pthread_exit(p);
}

int main(int argc, char* argv[])
{
 pthread_t* threads;
 int t,ret,numthreads = 0,thread_success = 0,*iptr;
 void* thread_result;
 if ( argc < 2 )
 {
    printf("Less number of Args -Pl. enter a valid number of threads to be created\n");
    exit(1);
}
numthreads = atoi(argv[1]);
if ( 0 > numthreads )
{
    printf("Pl. enter a valid number of threads to be created\n");
    exit(1);
}
threads = calloc(numthreads,  sizeof(pthread_t));
if (NULL == threads)
{
    printf("Failed to alocate memory in malloc\n");
    exit(1);
}
/*Creating the Threads*/
for(t=0; t < numthreads; t++)
{
    printf("Creating Thread\n");
    iptr = (int*)malloc(sizeof(int));
    *iptr = t;
    ret  = pthread_create(&threads[t],NULL,thread_entry,(void*)iptr);
    if(ret )
    {
        printf("Printf Error Creating Thread");
        free(iptr);
    }
    else
    {
        thread_success++;
    }
}
 //Now waiting for all the threads
   //thread_success is the number of successfully created threads
for(t=0; t < thread_success; t++)
{
    ret  = pthread_join(threads[t],&thread_result);
    if (ret)
    {
        printf("Error joining a thread");
    }
    else
    {
        printf("Joined thread returned %d\n", *((int*)thread_result));
        free(thread_result);
    }
}
  free(threads);
  return 0;
}

Align your code better.
It probably has tabs (replace them with space before pasting) in it that make reading it hard on a website.

Please declare one variable per line:

int t,ret,numthreads = 0,thread_success = 0,*iptr;

You are not saving anything be doing this. But you are making it harder to read fot the next person. Anyway when you get a job (sorry I assume you are in college (forgive me if I am wrong)) no company coding standard will let you get away with it. So best not to get in the habit to start with.

Your detection of bad input is spread over two print statements. Do all the tests that make the input bad in one place then use one print statements. Whose normal content is a description of usage. If you want to be fancy describe what they did wrong. But most people just do it wrong in the first place to see the useage comment.

if ( argc < 2 )
 {
    printf("Less number of Args -Pl. enter a valid number of threads to be created\n");
    exit(1);
}
numthreads = atoi(argv[1]);
if ( 0 > numthreads )
{
    printf("Pl. enter a valid number of threads to be created\n");
    exit(1);
}

// I would have done:

if (( argc < 2 ) || ((numthreads = atoi(argv[1])) == 0))
{
    printf("Usage: %s <number>\nThe number represents the number of threads to start\n", argv[0]);
    exit(1);
}

Your only bug (that I can see) next:

ret  = pthread_create(&threads[t],NULL,thread_entry,(void*)iptr);

You can't see it until you read further and note you are joining on threads[t] even if not all the threads worked. So you need some method to track which threads actually failed and pass that information forward to the join section (what I did below) or you need to re-use the t slot in the thread array (If you loop over the array using thread_success as the limit then you can not leave any blank slots in the array).

typedef struct ThreadAndData
{
    pthread_t   thread;       // Keep all thread info in one place
    int         value;        // That way the master can potentially look at the value
    int         status;       // while the thread is running.
} ThreadAndData;
threads = calloc(numthreads,  sizeof(ThreadAndData));

threads[t].status  = pthread_create(&threads[t].thread,NULL,thread_entry,(void*)&threads[t].value);
if(threads[t].status)
{
    printf("Printf Error Creating Thread");
}

// Now in the joining phase.
// You only need to to join on threads that have a 0 status.
if (thread[t].status == 0)
{
    ret  = pthread_join(threads[t].thread,&thread_result);
}


VBA Excel Looping through recordset and storing the data to use later I have a loop to add Recordset items to a Dictionary so I can do comparisons and retrieve specific data later in the code (not shown below).

The first loop I tried takes around 17 seconds, the second takes 16 seconds, the third takes 15 seconds. It seems like a long wait to add 500-700 records.

The connection is to a SQL Server database.

'add all apps to dictionary
Dim cn As ADODB.Connection
Dim rs As ADODB.Recordset
Set dict = New Scripting.Dictionary
Set rs = New ADODB.Recordset
rs.Open SQLStr2, cn, adOpenStatic

Dim StartTime As Double
Dim SecondsElapsed As Double
StartTime = Timer

'This takes 16 seconds - class module separate
'Source: https://codereview.stackexchange.com/questions/25956/storing-recordset-data
'Dim entity As MyEntity
'Dim entities As New Collection
'While Not rs.EOF And Not rs.BOF

    'Set entity = New MyEntity
    'With entity
        '.Property1 = rs.Fields("app_number").value
        '.Property2 = rs.Fields("FlexFstDispAmt").value
        '.Property3 = rs.Fields("FlexSecDispAmt").value
        '.Property4 = rs.Fields("NonFlexAmt").value
    'End With

    'entities.Add entity
    'rs.MoveNext
'Wend

'this takes 15 seconds
Dim Key1 As String
Dim Key2 As String
Dim Key3 As String
Dim Key4 As String
With dict
    For i = 1 To rs.RecordCount
        Key1 = rs.Fields("app_number").value
        Key2 = rs.Fields("FlexFstDispAmt").value
        Key3 = rs.Fields("FlexSecDispAmt").value
        Key4 = rs.Fields("NonFlexAmt").value
        .Add Key1, Array(Key2, Key3, Key4)
        '.Add rs.Fields("app_number").Value, Array(rs.Fields("FlexFstDispAmt").Value, rs.Fields("FlexSecDispAmt").Value, rs.Fields("NonFlexAmt").Value)
        rs.MoveNext
    Next
End With

'this takes 17 seconds
'With dict
    'For i = 1 To rs.RecordCount
        '.Add rs.Fields("app_number").value, Array(rs.Fields("FlexFstDispAmt").value, rs.Fields("FlexSecDispAmt").value, rs.Fields("NonFlexAmt").value)
        'rs.MoveNext
    'Next
'End With

Debug.Print Round(Timer - StartTime, 2)

rs.Close
cn.Close
Set cn = Nothing
Set rs = Nothing

I'd really like some help speeding this up. Anything is appreciated.

It is much faster to iterate over an array then a recordset. You should also pass the recordset to a function to return the dictionary.  The fewer tasks a subroutine performs the better.

Function RecordsetMap(ByRef rs As ADODB.Recordset, ByVal KeyColumn As Long) As Scripting.Dictionary
    Dim Map As New Scripting.Dictionary
    Dim Key, Item, Values
    Rem 1000000 is used to ensure all rows are returned
    Values = rs.GetRows(1000000)

    Dim r As Long, c As Long

    For r = 0 To UBound(Values, 2)
        ReDim Item(0 To UBound(Values))

        For c = 0 To UBound(Values)
            Item(c) = Values(c, r)
        Next
        Key = Item(KeyColumn)
        Map.Add Key:=Key, Item:=Item
    Next

    Set RecordsetMap = Map

End Function

Usage

Dim KeyColumn As Long, n As Long

' Get Field Name Index if not know
For n = 0 To rs.Fields.Count - 1
    If rs.Fields(n).Name = "app_number" Then
        KeyColumn = n
        Exit For
    End If
Next

Set dict = RecordsetMap(rs, KeyColumn)
Dim Item, Key

Debug.Print "Iterating over Keys"
For Each Key In dict.Keys
    Item = dict(Key)
    Debug.Print Join(Item, ",")
Next

Debug.Print
Debug.Print "Iterating over Items"
For Each Item In dict.Items
    Debug.Print Join(Item, ",")
Next


jQuery UI button and some calculations I have a list of checkboxes styled using JQuery UI as buttons. Each have a data-price attributem containing a price in this format: data-price="40.00", data-price="25.00" etc.

When the user "checks" a box, I am adding it's data-price to the totalPrice var. Every time another box is clicked, it's own data-price is added to the total. If the user unchecks a boxm, that value is taken away. I have tried to prevent the value from going under 0.00 as well.

I then output the totalPrice into a div - totalBox.

I imagine there are some optimisations here - any thoughts?

When working with prices, then in my opinion it's usually a good idea to avoid floating point numbers. Binary rounding errors can easily strike unexpectedly any time.

Instead I'd suggest to work with integers (thus pennies) internally, and just add the decimal point for output.

An other unrelated point: I would move the hard-coded HTML in the script to the HTML document and only write the price.


Codeforces: Queens, Again code review friends/teachers! This is a trivial(no dfs/permutations/backtrace required) n queen problem from codeforces: B. 8 Queens, Again!!

input

2
A1 B5 C8 D6 E3 F7 G2 H4
C3 E4 C4 E1 C4 F4 A8 G6

output

Valid
Invalid

Here is my solution:
queen.h
#ifndef QUEEN
#define QUEEN
#include <array>
constexpr int N = 8;

void paint(std::array<std::array<int, N>, N>& m, int x, int y);

bool check(const std::array<std::array<int, N>, N>& m, int x, int y);

#endif

queen.cpp
#include "queen.h"

#include <cstdlib>
#include <iostream>
#include <string>

void paint(std::array<std::array<int, N>, N>& m, int x, int y)
{
    for(int i = 0; i < N; i++)
    {
        for(int j = 0; j < N; j++)
        {
            if(i == x || j == y || abs(i - x) == abs(j - y))
            {
                m[i][j] = 1;
            }
        }
    }
}

bool check(const std::array<std::array<int, N>, N>& m, int x, int y)
{
    if(m[x][y] == 1)
    {
        return false;
    }
    return true;
}

testqueen.h
#define CATCH_CONFIG_MAIN // This tells Catch to provide a main() - only do this
                          // in one cpp file
#include "catch.hpp"
#include "queen.h"

#include <iostream>
#include <sstream>

TEST_CASE("eight queens test")
{
    std::ostringstream out;
    std::streambuf* coutbuf = std::cout.rdbuf();

    std::cout.rdbuf(out.rdbuf()); // redirect cout to out
    int n;
    std::cin >> n;

    for(int i = 0; i < n; i++)
    {
        std::array<std::array<int, 8>, 8> arr{};
        std::string s;
        for(int j = 0; j < N; j++)
        {
            std::cin >> s;
            auto x = s[0] - 'A', y = s[1] - '1';
            if(check(arr, x, y) == true)
            {
                paint(arr, x, y);
            }
            else
            {
                std::cout << "Invalid" << '\n';
                for(int k = j + 1; k < N; k++)
                {
                    std::cin >> s;
                }
                break;
            }
            if(j == N - 1)
            {
                std::cout << "Valid" << '\n';
            }
        }
    }
    std::cout.rdbuf(coutbuf);
    REQUIRE(out.str() == "Valid\nInvalid\n");
}

Test:
~/.../codeforces/100947-B(eight-queen) >>> ./build/test/tests                       ±[●●][master]
2
A1 B5 C8 D6 E3 F7 G2 H4
C3 E4 C4 E1 C4 F4 A8 G6
===========================================================
All tests passed (1 assertion in 1 test case)

Please help me to point out any bad habits, better algorithms or better c++ paradigms
Thanks in advance!

check features a well-known anti-idiom:

if (condition) {
    return false;
}
return true;

is a long way to say

return !condition;

Testing for j == N - 1 in a loop looks unappealing. The code would be much cleaner if the inner loop (which actually decides on the problem) were factored out into a function:

bool is_position_valid(....)
{
    ....
    for (int j = 0; j < N; j++) {
        if (check(....)) {
            paint(....);
        } else {
            return false;
        }
    }
    return true;
}

and the caller produced the required output.
The complexity is quadratic in both time and space. There is a way to drive the down to linear: have 4 arrays of sizes N, N, 2*N - 1, 2*N - 1 (for files, ranks, and left and right diagonals respectively), and for each queen mark a corresponding entry as taken; if the entry is already taken the position is invalid.


Hackerrank "Bricks Game" Problem URL

This is the solution I came up with the minimax problem stated above. I did a top-down recursive approach with memoization, and I think this should be \$O(n)\$, since we are filling out a memoization table with \$n\$ elements, with constant number of operations per each recursive call. Nevertheless, I am getting timeout errors for something like n = 1000, which probably indicates this is not an \$O(n)\$ implementation after all.

Can I get some advice on what my misunderstanding is?

#include <cmath>
#include <cstdio>
#include <vector>
#include <iostream>
#include <algorithm>
using namespace std;

int pick(vector<int> stack, int idx, int end, vector<int> memo){
    // Check if memoized.
    if (memo[idx] != -1) return memo[idx];

    int last = end - 1;
    // Base Case
    // Nothing to Pick.
    if(idx > last) return 0;
    // Only one brick.
    else if(idx > last - 1) return stack[last];
    // Two bricks.
    else if(idx > last - 2) return stack[last] + stack[last - 1];
    // Three bricks.
    else if(idx > last - 3) return stack[last] + stack[last - 1] + stack[last - 2];

    int case1,case2,case3;
    int p2,p3,p4,p5,p6;

    p2 = pick(stack,idx+2,end,memo);
    p3 = pick(stack,idx+3,end,memo);
    p4 = pick(stack,idx+4,end,memo);
    p5 = pick(stack,idx+5,end,memo);
    p6 = pick(stack,idx+6,end,memo);

    case1 = stack[idx] +
            min({p2,p3,p4});

    case2 = stack[idx] +
            stack[idx+1] +
            min({p3,p4,p5});

    case3 = stack[idx] +
            stack[idx+1] +
            stack[idx+2] +
            min({p4,p5,p6});

    memo[idx] = max({case1,case2,case3});

    return memo[idx];
}

int main() {
    /* Enter your code here. Read input from STDIN. Print output to STDOUT */
    int tests;
    int stackSize;
    vector<int> stack(100000,0),memo(100000,-1);
    cin >> tests;
    for(int test = 0 ;test < tests; test++){
        cin >> stackSize;
        for(int i=0;i<stackSize;i++) cin >> stack[i];
        cout << pick(stack,0,stackSize,memo) << endl;
        fill(memo.begin(),memo.begin() + stackSize ,-1);
    }
    return 0;
}

You are passing the stack vector by value, which means that on each call to pick, you will incur O(n) time to copy the vectors. You need to pass by reference or const reference.

See my answer here to another question with the same problem:
https://codereview.stackexchange.com/a/144084/36120


How to decrease memory usage in codeeval Road Trip challenge I'm solving easy CodeEval challenges in Python 2.7. Usually my solutions use around 256 bytes of memory according to CodeEval. With Road Trip however while time used is still low (30-40ms), memory is >5881866 bytes.  As I'm somewhat of a beginner, is there an easy way to decrease memory usage?

For each test case, which consists of one line of input that lists cities and their locations on a line, the task is to find the incremental distances when visiting them all.  Input follows the template ("city_name", distance;"city_name", distance;):

Rkbs,5453; Wdqiz,1245; Rwds,3890; Ujma,5589; Tbzmo,1303;
Vgdfz,70; Mgknxpi,3958; Nsptghk,2626; Wuzp,2559; Jcdwi,3761;
Yvnzjwk,5363; Pkabj,5999; Xznvb,3584; Jfksvx,1240; Inwm,5720;
Ramytdb,2683; Voclqmb,5236;

The data set is pretty big for this challenge:

The route length is an integer in range [10000, 30000]
The number of cities is in range [500, 600]

My solution is as follows:

test_cases = open(file)
for line in test_cases:
    line = line.strip()
    if line == "": continue
    line = line.replace(';', ' ').replace(',', ' ').split()
    distances = sorted(map(int, [line[i] for i in range(1, len(line), 2)]))
    result = [distances[i] - distances[i-1] for i in range(1, len(distances))]
    result.insert(0, distances[0])
    print ','.join(map(str, result))

I don't think it's a good idea to preprocess the line so much.  line = line.strip() duplicates the line, for no benefit.  I don't know why if line == "": continue should be needed.  line = line.replace(';', ' ').replace(',', ' ').split() degrades the data, and takes two passes to do it.

Here's how I would read the input:

for line in fileinput.input():
    route = itertools.chain([0], sorted(
        int(city.group(1)) for city in re.finditer(r'[A-Za-z]+,(\d+);', line)
    ))

Using fileinput.input() will let you take input from either a file specified on the command line, or sys.stdin if no file is specified.  re.finditer() will look for the pattern and capture the numbers that you need.

In Python 2, you should use xrange() instead of range().  However, if you capture just the numbers, as I've done above, then you wouldn't need to use range() or xrange() to extract every other item.

Each of your list comprehensions results in another temporary copy of the data.  result.insert(0, distances[0]) is also bad for performance, since every entry must be copied to make room for one entry at the front of the list.

To finish off the solution, I would do this:

print(','.join(str(b - a) for a, b in pairwise(route)))

… using the pairwise() recipe from itertools.


HTML and CSS markup for flexible form layout I'm trying to have a HTML / CSS structure for flexible layouts for forms. I tried to look at Bootstrap codes for help with this (without actually using Bootstrap itself). I've gone with something along these lines:

http://cssdeck.com/labs/stidyuic

* {padding: 0; margin: 0; box-sizing: border-box;}
body {width: 80%; margin: 0 auto; margin-top: 30px;}
.form-row {
  display: block;
}
.form-group:first-child {
  padding-left: 0px;
}
.form-group {
  margin-bottom: 20px;
  float: left;
  padding-left: 20px;
}
.col-6 {
  width: 50%;
}
.col-3 {
  width: 25%;
}
label {
  display: block;
  margin-bottom: 10px;
  font-weight: bold;
}
.form-control {
  display: block;
  width: 100%;
  height: 34px;
  padding: 6px 12px;
  font-size: 14px;
  line-height: 1.42857;
  color: #555;
  vertical-align: middle;
  background-color: #FFF;
  background-image: none;
  border: 1px solid #CCC;
  border-radius: 4px;
  box-shadow: 0px 1px 1px rgba(0, 0, 0, 0.075) inset;
  transition: border-color 0.15s ease-in-out 0s, box-shadow 0.15s ease-in-out 0s;
}
input, button, select, textarea {
  font-family: inherit;
  font-size: inherit;
}
form {
  display: block;
  margin-top: 0em;
}

                  Title
                  <input class="form-control" placeholder="Test" size="4" type="text">

                  Category
                  <select class="form-control">
                     <option value="A">
                        A
                     </option>
                     <option value="B">
                        B
                     </option>
                  </select>

                  Type
                  <select class="form-control">
                     <option>
                        A
                     </option>
                     <option>
                        B
                     </option>
                     <option>
                        C
                     </option>
                  </select>

I want to be able to do the following:

Have anywhere from 1 to four inputs on each row.
Have the label above the form
Modify the form layout with CSS when the resolution changes (media query)

Questions:

Is it a sensible approach to use this code structure in order to
achieve this?
Is taking Bootstrap’s form structure a logical approach to this problem?

Your code looks perfectly fine.  In fact, it all validates by the validators at W3C perfectly fine, with one exception - your HTML file should be structured like this:

<!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8" />
        <title>Your Page Title Here</title>
        <!-- other code here -->
    </head>
    <body>
        <!-- your display code here -->
    </body>
</html>

The validators can be found here:

  HTML validator
  CSS validator

Your code seems perfectly fine the way it is, and I would say the only thing wrong with it is from the user's perspective.  You have the Category menu overlapping the Title menu, which is not good; you should adjust your columns to prevent this.


Which way of writing a conditional test is the most readable? I have three conditions like the following:

if (IsAllTriggersRequired && meterFlag && workFlag && timeFlag)
{
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });
}
else if (!IsAllTriggersRequired && (meterFlag || workFlag || timeFlag))
{
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });
}

if ((IsAllTriggersRequired && meterFlag && workFlag && timeFlag) ||(!IsAllTriggersRequired && (meterFlag || workFlag || timeFlag)))
{
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });
}

bool isAdd = false;

if (IsAllTriggersRequired && meterFlag && workFlag && timeFlag)
    isAdd = true;
else if (!IsAllTriggersRequired && (meterFlag || workFlag || timeFlag))
    isAdd = true;

if (isAdd)
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });

In the second one, can I avoid repeating the same code?

I would suggest to add two more intermediate variables:

var allTriggersAvailable = meterFlag && workFlag && timeFlag;
var anyTriggerAvailable = meterFlag || workFlag || timeFlag;

if (allTriggersAvailable || (!IsAllTriggersRequired && anyTriggerAvailable))
{
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });
}


JavaScript to fire event every second until 10s then gradually increase I have a piece of JavaScript that's going to fire event every second after 10s, it's going to gradually increase the interval time. I'm new to JavaScript so it would be nice to have a codereview.

var _lastHeartbeat = window.performance.now();
var _config = {
  heartbeat: {
    start: 1000,
    max: 10000000,
    cap: 10000,
    degrade: function(interval) {
      var int = Math.pow(interval, 1.15);
      return Math.round(int / 10000) * 10000;
    }
  }
}

var heartbeat = function() {
  var end = window.performance.now();
  var timespent = Math.round((end - _lastHeartbeat) / 1000) * 1000
  console.log('test: ' + timespent); // fire event
}

var _startHeartbeat = function() {
  var beat = function(interval) {
    setTimeout(function() {
      heartbeat();
      var end = window.performance.now();
      var timespent = Math.round((end - _lastHeartbeat) / 1000) * 1000
      if (timespent < _config.heartbeat.cap) {
        beat(interval);
      } else if (interval < _config.heartbeat.max) {
        beat(_config.heartbeat.degrade(timespent));
      }
    }, interval);
  };

  beat(_config.heartbeat.start);
}

_startHeartbeat();

Here's JsFiddle. https://jsfiddle.net/noppanit/5cpbyyrg/1/

Your code looks decent, but I think you might have overthought the problem and the need for so many variables to be globally declared. What you want is a nice function that does everything you want in one single call, and I think it looks something like this:

function heartbeat(delay, interval, duration, callback){
 var now = Date.now();
 var end = now + delay + duration;
 var inc = function(v){ return v; };
 function beat(){
  var now = Date.now();
  if(now < end){
   callback(now, interval);
   interval = inc(interval);
   timeout = setTimeout(beat, interval);
  }
 }
 var timeout = setTimeout(beat, interval);
 var delayTimeout = setTimeout(function(){
  inc = function(v){ return Math.pow(v, 1.15); }
 }, delay);
 return function(){
  clearTimeout(timeout);
  clearTimeout(delayTimeout);
 }
}

You can call it like this:

heartbeat(10000, 1000, 10000000, function(now, interval){
    console.log('beat', interval / 1000);
});

Now for a bit of reasoning why I did this, starting with the variable declarations you have made. The problem with it is that you need about four variables to do this, but setting them as globals means you can have exactly one timer, and calling a new one requires a lot of setup. On top of that, you code is very attached to its execution - you want to separate the implementation from the actual code that uses it as much as possible.

First improvement here is using closures. Since you pass a couple of variables and define the actual bulk of the code inside the function, everything has access to these variables inside the function, but not outside. So your _config and _lastheartbeat variables are not shared by anyone else, and they reference themselves internally so they don't clash. Want to start another heartbeat? No problem, it's variables are contained.

As a last touch, your function needed a return value, something useful, so I decided to return a function that will cancel the timeouts, again thanks to the magic of closures.


Is there a better name for this class? I need a version of the mvc RouteValueDictionary that I can chain Add calls to, ie:

new RouteValueDictionaryExtended()
        .AddValue("controller", "Home")
        .AddValue("action", "Index")
        .AddValue("id", 3)

Is there a more description name I can use for this instead of just "Extended"?

   public class RouteValueDictionaryExtended : System.Web.Routing.RouteValueDictionary
    {
        public RouteValueDictionaryExtended() : base() { }
        public RouteValueDictionaryExtended(object values) : base(values) { }
        public RouteValueDictionaryExtended(System.Collections.Generic.IDictionary<string,object> dict) : base(dict) { }
        public RouteValueDictionaryExtended AddValue(string key, object obj)
        {
            var newDict = new RouteValueDictionaryExtended(this);
            newDict.Add(key, obj);
            return newDict;
        }
    }

I would actually recommend you to use the (Constructor) Builder pattern if you are only doing this for easier object initialization:

public class RouteValueDictionaryBuilder
{
    private List<Tuple<string, object>> values_ = new List<Tuple<string, object>>();

    public RouteValueDictionaryBuilder WithValue(string key, object obj)
    {
        values_.Add(Tuple.Create(key, obj));
        return this;
    }

    public RouteValueDictionary Build()
    {
        var dict = new RouteValueDictionary();
        foreach (var value in values_)
        {
            dict.Add(value.Item1, value.Item2);
        }
        return dict;
    }
}

To use:

var dict = new RouteValueDictionaryBuilder()
    .WithValue("controller", "Home")
    .WithValue("action", "Index")
    .WithValue("id", 3)
    .Build();

This avoids the higher coupling (inheritance) between the dictionary and its subclass, which is there only to help with initializing the dictionary.


Speeding up the generation all bit strings with Hamming distance one I'm playing around with hypercubes. For this, I need to create its edge set. Specifically, the edge set is the set of all bit strings with Hamming distance 1. An important property is that I want to generate every such pair exactly once. I do this with the following piece of code (I'm also providing a small benchmark for testing its performance):

#include <iostream>
#include <vector>
#include <chrono>
#include <cmath>

int main()
{
    // With 2^15, takes about 0.8 seconds on my machine.
    const int n = std::pow(2, 15);

    // Storing edges not strictly necessary, but let's do it so that the compiler
    // doesn't optimize something away.
    std::vector<int> edges;
    auto start = std::chrono::high_resolution_clock::now();

    for (int i = 0; i < n; ++i)
    {
        for (int j = 0; j < i; ++j)
        {
            const int u = i ^ j;

            // https://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2
            int k = u - 1;
            k |= k >> 1;
            k |= k >> 2;
            k |= k >> 4;
            k |= k >> 8;
            k |= k >> 16;

            if (k + 1 == u)
            {
                edges.emplace_back(j);
                edges.emplace_back(i);
            }
        }
    }

    auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::high_resolution_clock::now() - start);
    std::cout << (duration.count() / 1000.0) << "\n";

    // 2^(n-1)*n
    std::cout << (edges.size() / 2) << "\n";
}

This works fine, but can we make the above even more efficient?

To start with, here is a trivial optimization you can perform: in the most inner loop, you store u, then have k = u - 1, then compare k + 1 to u, which corresponds to comparing k to u - 1. Basically your inner loop performs unneeded work; just set directly u to (i ^ j) - 1 and you will save some work. Here is the most inner loop once changed:

const int u = (i ^ j) - 1;

// https://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2
int k = u;
k |= k >> 1;
k |= k >> 2;
k |= k >> 4;
k |= k >> 8;
k |= k >> 16;

if (k == u)
{
    edges.emplace_back(j);
    edges.emplace_back(i);
}

It timed it on my computer and it was consistently a bit faster, which means that the compiler wasn't able to make this simple transformation.

On the other hand, we can notice an interesting pattern with your original code: k + 1 can only be equal to u when u is a power of \$2\$. Checking whether a number is a power of \$2\$ or (or \$0\$, but the formula you use already has a special case of \$0\$) is easily done: a power of \$2\$ has exactly one bit set, so you only have to check whether removing a set bit from an integer returns \$0\$ to check whether an integer is \$0\$ or a power of \$2\$. This comparison is so cheap that you can perform it first and perform your current check only when it is true:

const int u = (i ^ j);

if (not (u & (u - 1))) // check whether u is 0 or a power of 2
{
    int k = u - 1;
    k |= k >> 1;
    k |= k >> 2;
    k |= k >> 4;
    k |= k >> 8;
    k |= k >> 16;

    if (k + 1 == u)
    {
        edges.emplace_back(j);
        edges.emplace_back(i);
    }
}

Now, correct me if I'm wrong, but if u is a power of two, you don't actually need to check anything else after (you don't need to round up to a power of \$2\$ since you know it's already one). You can simply remove all the k stuff. I mean, it was a wild guess, but the result were exactly the same on my computer. You can just check that u is a power of \$2\$ then emplace_back your values directly.


Shutdown method in queue implementations I am trying to implement a shutdown method in queue implementation. I took the code from BlockingQueue source from Java and trying to check the shutdown method. Will the following code be thread safe and an acceptable implementation?

    import java.util.concurrent.atomic.*;
    import java.util.concurrent.locks.Condition;
    import java.util.concurrent.locks.ReentrantLock;

public class BlockingQueue<E> {

    /* Node encapsulating the object that needs to be stored */
    static class Node<E>{

        /* Make sure all the threads read / write are done on updated variable */
        volatile E element;
        Node<E> next;
        Node(E value) { element = value;}
    }

    private final int capacity;
    private final AtomicInteger count = new AtomicInteger(0);
    private final AtomicInteger shutdown = new AtomicInteger(0);

    /* Need lock to make the thread re-entrant */
    private final ReentrantLock putLock = new ReentrantLock(true);
    private final ReentrantLock getLock = new ReentrantLock();

    private final Condition queueNotEmpty = getLock.newCondition();
    private final Condition queueNotFull = putLock.newCondition();

    /* Make sure updates to head and tail variables are atomic
     *  May be redundant as I have already enclosed the updated to head and tail
     *  using locks.
     */
    private AtomicReference<Node<E>> head, tail;

    public BlockingQueue()
    {
        this.capacity = Integer.MAX_VALUE;
    }

    public BlockingQueue(int capacity){
        if (capacity <= 0)
            throw new IllegalArgumentException();

        this.capacity = capacity;
        head = tail = new AtomicReference<BlockingQueue.Node<E>>(null);
    }

    public void add(E e) throws InterruptedException, Exception{

        if (e == null) throw new NullPointerException();

        /* We need to wait if there is not enough space on the queue
         *
         */
        final ReentrantLock lock = this.putLock;
        final AtomicInteger count = this.count;
        int c =-1;

        lock.lockInterruptibly();
        try {

            try {
                while(count.get() == capacity)
                    queueNotFull.await();

                if(shutdown.get() == 1)
                    throw new Exception();

            } catch (InterruptedException exception) {
                queueNotFull.signal();
                throw exception;
            }

            // Add the element to the queue
            Node<E> elem = new Node<E>(e);
            Node<E> prevTail = tail.getAndSet(elem);
            prevTail.next = elem;

            c = count.incrementAndGet();
            if (c + 1 < capacity)
                queueNotFull.signal();
        } finally {
            lock.unlock();
        }

        if(c == 0)
            signalNotEmpty();
    }

     /**
     * Signals a waiting get. Called only from put/offer (which do not
     * otherwise ordinarily lock getLock.)
     */
    private void signalNotEmpty() {
        final ReentrantLock takeLock = this.getLock;
        takeLock.lock();
        try {
            queueNotEmpty.signal();
        } finally {
            takeLock.unlock();
        }
    }

     public void shutDown()
     {
         /* Get the locks */
         putLock.lock();
         getLock.lock();

         try{
             head.getAndSet(null);
             tail.getAndSet(null);

             shutdown.getAndSet(1);

             //Signal all put and get threads
             queueNotFull.signalAll();
             queueNotEmpty.signalAll();

         }finally {
             putLock.unlock();
             getLock.unlock();
         }
     }
}

Is there a better way to implement this method?

My immediate thought is that this is an unacceptable implementation.  In particular, you seem to be trying to shutdown the threads accessing the queue by signally conditions, which I think confuses things badly.

See Concurrent Programming in Java, Second Edition by Doug Lea -- especially Chapter 3 ("State Dependence").

Consider this use case: you have a service, with multiple publishers (Runnables) and consumers (also Runnables); publishers offer items to the queue, consumers take items from it.  There's a UI, with a big red SHUTDOWN button on it.  When the user hits that button, what does the user want?

You seem to be implementing the idea that the user wants to shut down the queue.  But I put it to you that it's more accurate that the user wants to shut down the service itself.  The service itself maintains its own shutdown state, which the runnables can access.  Invoking service shutdown then does two things - (1) it sets the shutdown flag, so that the producer and consumer flags can see the change, and (2) it interrupts the producers and consumers, so that they look at the shutdown flag.

So the usual idiom would be to run the publishers and consumers in an ExecutorService, and after the shutdown flag has been set, ExecutorService.shutdownNow() would be invoked to signal the interruption to the running threads.  The threads that are parked on your conditions (queueNotFull.await()) throw InterruptedExceptions, and the handlers for these exceptions can then check the shutdown flag and do the right thing.

Condition.signalAll doesn't interrupt() the threads, it just gets them contending on the lock again.  When they get the lock, they just return from await; which means they check the while condition, and then put themselves back to sleep (Condition.await) because it is still true.

In short, Locks and Conditions are for co-ordination; if you are trying to implement cancellation, you should be using interrupts.