Oh my goodness: How many selfies have I taken? The title is a joke on how some people take a lot of selfies pictures

Recently, in The 2nd Monitor, there was a brief discussion on selfie answers, starting with this post.

At one point, a user said this:

  I don't know how many selfies I have :s

I immediately thought: this is the perfect opportunity for an SEDE query!

Try it out!

DECLARE @QuestionId INT = (SELECT Id FROM PostTypes WHERE Name = 'Question');
DECLARE @AnswerId INT = (SELECT Id FROM PostTypes WHERE Name = 'Answer');

DECLARE @UserAnswers TABLE (AnswerId INT, ParentId INT);
DECLARE @UserQuestions TABLE (QuestionId INT);

INSERT INTO @UserAnswers
SELECT Id, ParentId
FROM Posts
WHERE OwnerUserId = ##UserId## AND PostTypeId = @AnswerId;

INSERT INTO @UserQuestions
SELECT Id
FROM Posts
WHERE OwnerUserId = ##UserId## And PostTypeId = @QuestionId;

SELECT AnswerId
FROM @UserAnswers
JOIN @UserQuestions
ON ParentId=[@UserQuestions].QuestionId;

The above query takes the user's ID and returns a list of all their selfie answers that they have posted.

This is my first time ever writing any form of SQL and I'm still trying to get used to the overall flow of it, so I'm really open to any recommendations.

However, here are a few things I had in mind:

Is it good that I stored the posts in tables, or is it unnecessary?
Am I following good SQL practices/conventions?
I was really confused on how I should break up the SQL into multiple lines, so, after much trial and error, it seemed to me having one instruction per line was the cleanest. However, if this is not recommended, I am open to change.
Am I missing indentation anywhere?
To me, reading SQL is just like reading English. Therefore, writing documentation seemed redundant. However, I am not familiar with good documentation and SQL, so is there anything documentation-wise that I should add?

One by one, then:

  Is it good that I stored the posts in tables, or is it unnecessary?

It is unnecessary, in this case. What your table variables are doing for you here really is simulating a self-referencing join. You could instead use aliases on the actual table(s), for instance:

FROM Posts AS "Answers"
  JOIN Posts AS "Questions"
    ON "Answers".OwnerUserId = "Questions".OwnerUserId
    AND "Answers".ParentId = "Questions".Id
    AND "Answers".PostTypeId = @AnswerId
    AND "Questions".PostTypeId = @QuestionId
WHERE "Answers".OwnerUserId = ##UserId##
  AND "Questions".OwnerUserId = ##UserId##

(Note that double-quotes are not needed for aliases, but I personally like using them as a visual reference that something is an alias instead of an actual database object)

  Am I following good SQL practices/conventions?

Yes, I think that looks good (besides what I just mentioned).

  I was really confused on how I should break up the SQL into multiple lines, so, after much trial and error, it seemed to me having one instruction per line was the cleanest. However, if this is not recommended, I am open to change.

And...

  Am I missing indentation anywhere?

In SQL there are as many indentation/formatting styles as there are people who write SQL. One way that seems to work good for many is to have all your primary keywords at minimum indent, and the rest indented 2, 4 or more spaces, whatever is called for.

The primary keywords are (for SELECT statements):

WITH
SELECT
FROM
WHERE
GROUP BY
HAVING
ORDER BY

Of course many times you won't use all of those, but this works pretty good as a thumb rule. For the rest, as long as you keep it readable and use some of your common sense you would apply for other languages, you should do fine.

Update: Addressing additional question.

  To me, reading SQL is just like reading English. Therefore, writing documentation seemed redundant. However, I am not familiar with good documentation and SQL, so is there anything documentation-wise that I should add?

The answer as usual is it depends. In simple queries like yours, documentation would probably just be clutter. In SQL as in every other language, if the code is written well (especially good naming, aliasing, etc.) you often don't need documentation.

There is one style of documentation which I have come up with that can be useful for queries where others might have to look at after you. Here's an example (imagine you were querying data over a certain period of time, which you could also do with SEDE by asking the user to input a period):

DECLARE /* The interval in Days how far back we want to query for values. */
    @DaysInterval INT = 90;
IF @DaysInterval > 0 /* meaning we would go into the future instead of the past... */
    SET @DaysInterval = @DaysInterval * -1;
DECLARE /* Variables for which dates to query from and up to, based on @DaysInterval. */
      @FromDate DATETIME = DATEADD(DAY, @DaysInterval, GETDATE())
    , @ToDate DATETIME = GETDATE();
DECLARE /** Filters for which tags to query.  */
    @TagsFilter TABLE (
        Tag VARCHAR(20)
    );
INSERT INTO @TagsFilter
    SELECT 'sql' UNION
    SELECT 'python' UNION
-- etc.

I take advantage of the fact SQL often reads like English to add in documentation that flows with it like that.

There are also cases when you would want to add documentation that explains why you are doing something a certain way, as it is now always obvious... For example:

/**
 * Temp table to hold results from the posts tables from each respective SE site database.
 * This will be used in conjunction with @TagsFilter in order to match records.
 */
IF OBJECT_ID('tempdb..#SESites') IS NOT NULL
    DROP TABLE #SESites;

CREATE TABLE #SESites (
-- etc.

Hope this helps :)


Displaying content based on the current time I'm new to PHP and just wrote a bunch of if statements to display content based on the current time.

Is there a better way of writing the following block of code to easily be maintainable? The only thing that changes in each if statement is the if variables, h4 tag and the $rows variable.

<?php

    date_default_timezone_set("Europe/London");

    $currentTime = date('Hi');
    $breakfastStart = "0200";
    $breakfastEnd = "1129";

    $lunchStart = "1130";
    $lunchEnd = "1559";

    $dinnerStart = "1600";
    $dinnerEnd = "2129";

    $lateStart = "2130";
    $lateEnd = "0159";

?>

<?php if ($currentTime >= $breakfastStart && $currentTime <= $breakfastEnd): ?>
        <?php
            $rows = get_field('breakfast', 41190);
            $row_count = count($rows);
            $i = rand(0, $row_count - 1);
        ?>

Breakfast

            <?php echo $rows[ $i ]['dish_name']; ?>

           <?php echo $rows[ $i ]['dish_description'];  ?>

            Served<?php echo $rows[ $i ]['time_served'];  ?>

<?php endif; ?>

<?php if ($currentTime >= $lunchStart && $currentTime <= $lunchEnd): ?>
        <?php
            $rows = get_field('lunch', 41190);
            $row_count = count($rows);
            $i = rand(0, $row_count - 1);
        ?>

Lunch

            <?php echo $rows[ $i ]['dish_name']; ?>

           <?php echo $rows[ $i ]['dish_description'];  ?>

            Served<?php echo $rows[ $i ]['time_served'];  ?>

<?php endif; ?>

<?php if ($currentTime >= $dinnerStart && $currentTime <= $dinnerEnd): ?>
        <?php
            $rows = get_field('dinner', 41190);
            $row_count = count($rows);
            $i = rand(0, $row_count - 1);
        ?>

Dinner

            <?php echo $rows[ $i ]['dish_name']; ?>

           <?php echo $rows[ $i ]['dish_description'];  ?>

            Served<?php echo $rows[ $i ]['time_served'];  ?>

<?php endif; ?>

<?php if ($currentTime <= $lateStart && $currentTime <= $lateEnd): ?>
        <?php
            $rows = get_field('late_night', 41190);
            $row_count = count($rows);
            $i = rand(0, $row_count - 1);
        ?>

Late Night

            <?php echo $rows[ $i ]['dish_name']; ?>

           <?php echo $rows[ $i ]['dish_description'];  ?>

            Served<?php echo $rows[ $i ]['time_served'];  ?>

<?php endif; ?>

You are repeating a lot of HTML, which I guess you know could be condensed down, you can also make use of elseif this way it will execute the first TRUE statement and skip the rest.

If there are no matches, for breakfast, lunch, dinner we can assume it must be late night, as there are no more hours left in the day. As this is the case we can just } else { for the last statement.

I have added a check for $row_count as we can never guarantee there will be results returned

<?php

date_default_timezone_set("Europe/London");

$currentTime = date('Hi');
$breakfastStart = "0200";
$breakfastEnd = "1129";

$lunchStart = "1130";
$lunchEnd = "1559";

$dinnerStart = "1600";
$dinnerEnd = "2129";

$lateStart = "2130";
$lateEnd = "0159";

// set up your variables
$rows = $field_key = $field_title = false;

// is it breakfast time?
if( $currentTime >= $breakfastStart && $currentTime <= $breakfastEnd )
{

    // if it is, set the field key and title
    $field_key = 'breakfast';
    $field_title = 'Breakfast';

    // if not, is it lunch time?
} elseif( $currentTime >= $lunchStart && $currentTime <= $lunchEnd )
{

    $field_key = 'lunch';
    $field_title = 'Lunch';

    // if not, is it dinner time?
} elseif( $currentTime >= $dinnerStart && $currentTime <= $dinnerEnd )
{

    $field_key = 'dinner';
    $field_title = 'Dinner';

    // if not, lets assume its late night
} else {

    $field_key = 'late_night';
    $field_title = 'Late Night';

}

$rows = get_field( $field_key , 41190);
$row_count = count($rows);
$i = rand(0, $row_count - 1);

// if we have some rows..
if( $row_count )
{
?>

<?=$field_title?>

        <?php echo $rows[ $i ]['dish_name']; ?>

       <?php echo $rows[ $i ]['dish_description'];  ?>

        Served<?php echo $rows[ $i ]['time_served'];  ?>

<?php
}


Define a range based on passed arguments TL;DR I'm self teaching VBA, learning about functions and passing arguments from a subroutine into the function and want to know if my very basic function to define a range is optimal and follows correct referencing/standards/practices.

I'm teaching myself VBA (mostly in MS Excel) as a starting point into programming in my spare time (what little of it I have) with the goal of a career change and am now learning how to utilise functions.

Below is a function I have written to define a range based on the variables passed as arguments. As you will see the worksheet, first row and first column are required arguments and the second row and second column are optional.

The function will assign the value of the first row (or column) to the second row (or column) if the optional argument is omitted.

Function defineRange(mySheet As Object, startRow As Long, firstColumn As Long, Optional endRow As Long, Optional secondColumn As Long) As Object

'will create a single row range
If endRow = 0 Then
    endRow = startRow
End If
'will create a single column range
If secondColumn = 0 Then
    secondColumn = firstColumn
End If

'Define the A1 reference for the range with cells.address parameter using arguments as the row and column index.
Set defineRange = mySheet.Range(Cells(startRow, firstColumn).Address, Cells(endRow, secondColumn).Address)

End Function

As an example I'd call it from a subroutine as follows:

Sub test()

Dim myRange As Object

Set myRange = defineRange(ThisWorkbook.Sheets(1), 5, 2, 10, 4)

myRange.Select

End Sub

The result is Range("B5:D10") is selected on sheet1 of the workbook.

Is the function code optimal?
Have best practices been followed?

First, kudos for teaching yourself VBA - I was once in your shoes and how I wish I could have had Code Review to help me back then!

Let's start with the function's signature:

Function defineRange(mySheet As Object, startRow As Long, firstColumn As Long, Optional endRow As Long, Optional secondColumn As Long) As Object

It's not clear whether the function's implicit public accessibility is intended or not. If it means to be accessible from outside the module it's declared in, then it should be explicitly Public. Otherwise, it should be Private - a private procedure/function/method can only be invoked from within the module it belongs to. I'm thinking the intent is for it to be Public here.

You probably noticed pretty much everything in VBA type libraries is PascalCase. From Range to Worksheet.Name - best practice is to "blend in" and write code that adheres to the naming standard. Hence, DefineRange would be better. I like how the name begins with a verb (as it should!), however "define" doesn't strike me as the best choice in this case. The function is getting, or rather acquiring a Range object. The Excel object model already provides an API for this, so reinventing-the-wheel is appropriate here :)

The parameters are all implicitly passed by reference (ByRef), which is an unfortunate default. Would this be expected/desired behavior?

Dim firstRow As Long
firstRow = 12

Dim lastRow as Long
lastRow = 0

Dim target As Range
Set target = defineRange(Sheet1, firstRow, 1, lastRow)

Debug.Print lastRow ' prints 12, not 0. is that expected?

By passing the parameters ByVal, instead of passing a pointer ("reference") to the value, you essentially pass a copy of that value, and the calling code's variables can't get modified by the function. In VB.NET and many other languages, the implicit default is to pass arguments by value: it's somewhat rare that a parameter needs to be passed by reference.

mySheet is declared as an Object, which means the function will happily take a Collection, some UserForm1, any Range, or even MyClass for this mySheet parameter... and then the mySheet.Range call will fail at run-time with error 438 "can't find property or method" (unless there's a method named Range on that UserForm1 or MyClass object, of course). By declaring it As Object, you made every member calls against that object late-bound, meaning the compiler can't help you, and will happily let you try to invoke mySheet.Rnge.

It's usually best to avoid deferring failures to run-time, and fail at compile-time instead, whenever possible. We do this with early binding, by declaring objects using a specialized interface we know we can work with - in this case, Excel.Worksheet, or just Worksheet. We can do this, because when VBA is hosted in Excel, the VBA project is guaranteed to have a reference to the Excel type library. If we were hosted in Word or PowerPoint, we would have to explicitly add that reference (through tools/references), or work late-bound and keep it As Object.

The parameter names are meant to be pairs, but they're inconsistent:

startRow, endRow
firstColumn, secondColumn

"Start/End" was a better, clearer idea than "First/Second" - I'd go and rename the parameters to have startColumn and endColumn, matching startRow and endRow.

Kudos for declaring an explicit return type - all Function procedures return something, whether you declare a return type or not. Then again, it would be better to return a Range rather than an Object.

So, that covers the function's signature :)

There are a number of possible bugs and edge cases that should be handled. The first thing any function should do, is validate its inputs.

VBA doesn't have unsigned integer types, so a Long could very well be a negative number, or it could be zero -- but Excel's object model isn't going to like you trying to get the cell at row 0 and column -728.

There are several ways to deal with this. The simplest is to use Debug.Assert at the very top of the function, i.e. halt program execution if assumptions aren't validated:

'Debug.Assert TypeOf mySheet Is Excel.Worksheet
Debug.Assert startRow > 0
Debug.Assert firstColumn > 0

Another way is to have a guard clause, again at the very top of the function, that explicitly throws an error given invalid arguments:

If startRow > 0 Then Err.Raise 5, "defineRange", "Argument 'startRow' must be greater than zero."
If firstColumn > 0 Then Err.Raise 5, "defineRange", "Argument 'firstColumn' must be greater than zero."

One advantage of using guard clauses, is that if a function is given arguments it cannot possibly work with, then we fail early. This makes it easier to debug if something goes wrong later: instead of dealing with a cryptic and rather useless "Method 'Range' of class 'Worksheet' failed" error, the code that tried to invoke our function now knows exactly what went wrong, and how to fix it.

The return value assignment will fail if mySheet isn't the ActiveSheet:

mySheet.Range(Cells(startRow, firstColumn).Address, Cells(endRow, secondColumn).Address)

That's because the unqualified Cells calls are context-dependent.

If the function is written in a worksheet module's code-behind, then it's implicitly Me.Cells.

If the function is written anywhere else, then it's implicitly [_Global].Cells, which ultimately resolves to ActiveSheet.Cells. That isn't a problem if mySheet is active, but then if it isn't...

Sheet1.Range(Sheet2.Cells(...), Sheet2.Cells(...))

That's guaranteed to throw the dreaded run-time error 1004 "application-defined error", because only Schrödinger's Range can belong to two worksheets at the same time.

You can fix this with a With block:

With mySheet
    Set defineRange = .Range(.Cells(startRow, firstColumn).Address, .Cells(endRow, secondColumn).Address)
End With

Note the . dereferencing operators qualifying the Cells member calls with the object reference held by the With block. It's equivalent to this:

Set defineRange = mySheet.Range(mySheet.Cells(startRow, firstColumn).Address, mySheet.Cells(endRow, secondColumn).Address)

That said, you don't need to work off the .Address string: Range is more than happy to work with the Range references returned by Cells(...):

Set defineRange = mySheet.Range(mySheet.Cells(startRow, firstColumn), mySheet.Cells(endRow, secondColumn))

While that works, I find it quite a mouthful, and as shown above, it has too many reasons to fail to my taste - I'd probably split it up:

Dim startCell As Range
Set startCell = mySheet.Cells(startRow, firstColumn)

Dim endCell As Range
Set endCell = mySheet.Cells(endRow, secondColumn)

Making the return assignment rather trivial:

Set defineRange = mySheet.Range(startCell, endCell)


Swing Temperature Converter I made a simple temperature converter to serve as an introduction to swing.  I used Eclipse WindowBuilder and did not attempt to make the components more modular.  I'm curious as to how you guys would organize this code to improve readability.

Also, if you have any other criticism I'd love to hear it.

import java.awt.BorderLayout;
import java.awt.EventQueue;

import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.border.EmptyBorder;
import javax.swing.JTextField;
import java.awt.TextField;
import java.awt.FlowLayout;
import javax.swing.JTextPane;
import javax.swing.JButton;
import javax.swing.JRadioButton;
import javax.swing.JLabel;
import javax.swing.UIManager;
import java.awt.event.ActionListener;
import java.awt.event.ActionEvent;
import javax.swing.ButtonGroup;

public class TempConvert extends JFrame {

private JPanel contentPane;
private final ButtonGroup buttonGroup = new ButtonGroup();

/**
 * Launch the application.
 */
public static void main(String[] args) {
    EventQueue.invokeLater(new Runnable() {
        public void run() {
            try {
                TempConvert frame = new TempConvert();
                frame.setVisible(true);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    });
}

/**
 * Create the frame.
 */
public TempConvert() {

    setTitle("Temperature Converter");
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    setBounds(0, -33, 385, 272);
    contentPane = new JPanel();
    contentPane.setBorder(new EmptyBorder(5, 5, 5, 5));
    setContentPane(contentPane);
    contentPane.setLayout(null);

    JTextPane txtTempIn = new JTextPane();
    txtTempIn.setBounds(12, 46, 124, 26);
    contentPane.add(txtTempIn);

    JRadioButton celRadio = new JRadioButton("Celcius");
    buttonGroup.add(celRadio);
    celRadio.setBounds(212, 34, 67, 24);
    contentPane.add(celRadio);

    JRadioButton fahrenRadio = new JRadioButton("Fahrenheit");
    buttonGroup.add(fahrenRadio);
    fahrenRadio.setBounds(212, 62, 85, 24);
    contentPane.add(fahrenRadio);

    JLabel lblTem = new JLabel("Degrees");
    lblTem.setBounds(154, 49, 78, 16);
    contentPane.add(lblTem);

    JTextPane tempOutput = new JTextPane();
    tempOutput.setBackground(UIManager.getColor("Button.background"));
    tempOutput.setEditable(false);
    tempOutput.setBounds(12, 138, 267, 22);
    contentPane.add(tempOutput);

    JButton btnNewButton = new JButton("Convert");
    btnNewButton.setBounds(12, 84, 98, 26);
    contentPane.add(btnNewButton);
    btnNewButton.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent arg0) {

            if(celRadio.isSelected()){
                try{
                    double temp=Double.parseDouble(txtTempIn.getText());
                    temp = temp * 1.8 + 32;
                    tempOutput.setText(Double.toString(temp));
                }catch(Exception e){
                    tempOutput.setText("Please enter a valid temperature.");
                }
            }else{
                try{
                    double temp=Double.parseDouble(txtTempIn.getText());
                    temp = (temp - 32) * (5.0/9.0);
                    tempOutput.setText(Double.toString(temp));
                }catch(Exception e){
                    tempOutput.setText("Please enter a valid temperature.");
                }
            }
        }
    });
}
}

Inheritance

Do not extend any class unless you you have an "is a" relationship and add new behavior to the extended class.

You extend JFrame without adding Frame related behavior, you just configure it.

  I don't understand the objection to extending JFrame here. The program needs a top level container.

Right, but you program does not need to inherit form JFrame to get one:

public class TempConvert {

private JPanel contentPane;
private final ButtonGroup buttonGroup = new ButtonGroup();

/**
 * Launch the application.
 */
public static void main(String[] args) {
    EventQueue.invokeLater(new Runnable() {
        public void run() {
            try {
                TempConvert frame = new JFrame();
                new TempConvert(frame);
                frame.setVisible(true);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    });
}

/**
 * Create the frame.
 */
public TempConvert(JFrame frame) {

    frame.setTitle("Temperature Converter");
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.setBounds(0, -33, 385, 272);
    frame.contentPane = new JPanel();
    frame.contentPane.setBorder(new EmptyBorder(5, 5, 5, 5));
    frame.setContentPane(contentPane);
    frame.contentPane.setLayout(null);

Layouting

When using NULL layout you throw away any convenience Layoutmanager could give you. Your UI will be bound to the "ideal" Frame size. The problem is that the definition of "ideal" changes with every user and every device your program runs on.

Magic Numbers

Because of your wrong interpretation of "Flexibility" that results in using the NULL Layout you need to specify bound of your buttons. And you do this by using random literal numbers (from the readers perspective). You should at least define constants for them.

Code duplication

Your code has lots of duplicated code.

You create a bunch of buttons. you could extract the creation of a single button to a parameterized method.

Also in your anonymous Listener class the try/catch block is duplicated. Here you could simply move one of then around the whole if and delete the other one.


Classify the urgency by comparing the current time to two timestamps I'm currently working on a typescript-class with this method:

  /**
   * Gets the state of a package. The state is determined based on two timestamps stored as fields in the {@link
    * Package}. As the current time equals or passes a timestamp, the state changes.
   *
   * @param currentTime The time to compare the timestamps with
   * @param sourcePackage The package containing the two timestamps to compare the currentTime with
   * @returns {CardState} The calculated state of the package
   */
  static determineStateOfPackage(currentTime: Date, sourcePackage: Package): CardState {
    if (sourcePackage.targetTime.getTime() <= currentTime.getTime()) {
      return CardState.URGENT;
    }
    if (sourcePackage.warningTime.getTime() <= currentTime.getTime()) {
      return CardState.WARNING;
    }
    return CardState.FRIENDLY;
  }

It bugs me that the two ifs are repetitive but I can't really figure out how to improve this. How can I improve this code especially to avoid calling the two ifs with nearly the same content twice?

A couple notes:

Is CardState an enum?  If not, this function doesn't return a CardState object, but rather what appears to be values described within CardState class constants.  Your signature should reflect this.
I honestly don't think it is that problematic to have multiple if conditions as you are doing, as your usage is not increasing cyclomatic complexity of the method.  Now, if you started introducing new states, then it may get unwieldy and be good reason for refactoring.  As your code is now, it is very straightforward and easy to understand.  Why over-complicate?
Is currentTime a good variable name?  This method does nothing to ensure the passed Date object represents a time that is "current". Why name this variable *Time when it holds a Date object?
Consider less verbose determinePackageState vs. determineStateOfPackage package vs. sourcePackage.
You could possibly set getTime() value to constant in order to remove redundant calls (though this is probably splitting hairs).

This might result in (slightly) easier to read code such as:

  static determinePackageState(date: Date, package: Package): CardState {
    const timestamp: number = date.getTime();
    if (package.targetTime.getTime() <= timestamp) {
      return CardState.URGENT;
    }
    if (package.warningTime.getTime() <= timestamp) {
      return CardState.WARNING;
    }
    return CardState.FRIENDLY;
  }


Zip code reduce function My task is to write a function that would take an array of zip codes and spit out only the zip codes that do not qualify. A non-qualifying zip code will not exist in the database and does NOT have other input zip codes within 140 miles.

For example, if I input 90210, 90211, 77389 into the function, it should only spit out 77389 since it is not close to 90210 and 90211. Both 90210 and 90211 are disqualifying zip codes because they are close to one another.

Here are the two functions I am using:

//Accepts an input array of zips and outputs an array of zips that are not in proximity of any other input zip by a predefined distance

    protected function qualifyZips($zips, $radius=70){
        $validInputZips = array();
        //looping through input zips to validate them against the zip master table
        foreach ($zips as $key => $zip) {
            $inputZip = ZipCode::where('zip', $zip)->first(); //some zip data from DB
            //While we are at it, let's grab the lat/lon
            if ($inputZip) {
                $validInputZips[$zip]['latitude'] = $inputZip->latitude;
                $validInputZips[$zip]['longitude'] = $inputZip->longitude;
                $validInputZips[$zip]['hits'] = 0;
            }
        }

       $maskedInputZips = $validInputZips;
       $outputArray = array();

        foreach ($validInputZips as $zip => $latlon) {

            foreach ($maskedInputZips as $maskedZip => $maskedLatlon) {

                if ($zip != $maskedZip) {
                    $miles = $this->haversineGreatCircleDistance($latlon['latitude'], $latlon['longitude'], $maskedLatlon['latitude'], $maskedLatlon['longitude'], 3959);

                    if ($miles < ($radius * 2)) {
                        $validInputZips[$zip]['hits'] += 1;
                    }
                }
            }
            if ($validInputZips[$zip]['hits'] == 0){
                $outputArray[] = $zip;
            }
        }

        return $outputArray;
    }

   protected function haversineGreatCircleDistance($latitudeFrom, $longitudeFrom, $latitudeTo, $longitudeTo, $earthRadius = 3959)
    {
        // convert from degrees to radians
        $latFrom = deg2rad($latitudeFrom);
        $lonFrom = deg2rad($longitudeFrom);
        $latTo = deg2rad($latitudeTo);
        $lonTo = deg2rad($longitudeTo);
        $latDelta = $latTo - $latFrom;
        $lonDelta = $lonTo - $lonFrom;
        $angle = 2 * asin(sqrt(pow(sin($latDelta / 2), 2) +
                cos($latFrom) * cos($latTo) * pow(sin($lonDelta / 2), 2)));
        return $angle * $earthRadius;
    }

Some data:

array:3 [
  90210 => array:3 [
    "latitude" => "34.0900"
    "longitude" => "-118.4100"
    "hits" => 0
  ]
  90211 => array:3 [
    "latitude" => "34.0600"
    "longitude" => "-118.3800"
    "hits" => 0
  ]
  77389 => array:3 [
    "latitude" => "30.0600"
    "longitude" => "-95.3800"
    "hits" => 0
  ]
]

An alternative

   protected function haversineGreatCircleDistance($latitudeFrom, $longitudeFrom, $latitudeTo, $longitudeTo, $earthRadius = 3959)
    {
        // convert from degrees to radians
        $latFrom = deg2rad($latitudeFrom);
        $lonFrom = deg2rad($longitudeFrom);
        $latTo = deg2rad($latitudeTo);
        $lonTo = deg2rad($longitudeTo);

You can simplify this if you only call it from the one place.

    protected function haversineGreatCircleDistance($from, $to, $earthRadius = 3959)
    {
        // convert from degrees to radians
        $latFrom = deg2rad($from['latitude']);
        $lonFrom = deg2rad($from['longitude']);
        $latTo = deg2rad($to['latitude']);
        $lonTo = deg2rad($to['longitude']);

Not a big deal, but this is shorter.

Avoid unnecessary variables

                $validInputZips[$zip]['hits'] = 0;

You create this purely to use for a single iteration in a later loop.  Why bother?  Create a variable in the loop and let it die after.  Or in this case, just take action in the loop and skip the variable altogether.  More on that later.

Don't do many times what you can do once

If you do

          $diameter = 2 * $radius;

before the nested loops, you only have to do the multiplication once.

Keep it simple

            foreach ($maskedInputZips as $maskedZip => $maskedLatlon) {

                if ($zip != $maskedZip) {
                    $miles = $this->haversineGreatCircleDistance($latlon['latitude'], $latlon['longitude'], $maskedLatlon['latitude'], $maskedLatlon['longitude'], 3959);

                    if ($miles < ($radius * 2)) {
                        $validInputZips[$zip]['hits'] += 1;
                    }
                }
            }
            if ($validInputZips[$zip]['hits'] == 0){
                $outputArray[] = $zip;
            }

You can simplify this as

            foreach ($maskedInputZips as $maskedZip => $maskedLatlon) {

                if ($zip != $maskedZip) {
                    $miles = $this->haversineGreatCircleDistance($latlon, $maskedLatlon);

                    if ($miles < $diameter) {
                        continue 2;
                    }
                }
            }

            $outputArray[] = $zip;

You don't need to increment the variable, as you never use it except to check for 0.  A simple Boolean is plenty.  This skips to the next iteration of the outer loop as soon as it finds the zip that makes it unqualified.  If that never happens, we add the zip to the output.

Careful about calling a function and explicitly entering the default value of a parameter.  When you do that, it can make it confusing as to which value matters.  The same value appears in two places, but only one matters.  What if someone changes the wrong one?


Tree traversal into List The interview question was to traverse a tree into a list.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication2
{
    public class TreeToList
    {
        public TreeToList()
        {

            //            0
            //        2         4
            //     6    8          10
            TreeNode root = new TreeNode(0);
            root.Left = new TreeNode(2);
            root.Right = new TreeNode(4);
            root.Left.Left = new TreeNode(6);
            root.Left.Right = new TreeNode(8);
            root.Right.Right = new TreeNode(10);

            List<int> res = ConvertTreeToList(root);

        }

        //pre-order 0 2 4
        //in-order 2 0 4
        //post-order 2 4 0

        private List<int> ConvertTreeToList(TreeNode root)
        {
            List<int> res = new List<int>();
            if (root != null)
            {
                res.Add(root.Index);
            }
            else
            {
                return null;
            }
            if(root.Left != null)
            {
                res.AddRange(ConvertTreeToList(root.Left));
            }
            if(root.Right != null)
            {
                res.AddRange(ConvertTreeToList(root.Right));
            }
            return res;
        }
    }
}

My output is:

0,2, 6, 8, 4, 10

You're creating a new List<int> for every node in the tree, which is a lot of garbage for the garbage collector. Each value will also end up getting appended to many temporary lists.

Let's kick the can down the road and assume that the work of creating the list has been taken care of for us. We take the result list as a parameter and append to that

private static void ConvertTreeToList(TreeNode root, List<int> result)
{
    if (root == null)
    {
        return;
    }

    result.Add(root.Index);
    ConvertTreeToList(root.Left, result);
    ConvertTreeToList(root.Right, result);
}

Now we just add a helper function

private static List<int> ConvertTreeToList(TreeNode root)
{
    var result = new List<int>();
    ConvertTreeToList(root, result);
    return result;
}


Putting array name in variable and shortening code I'm a beginner in C# and am doing a simple programming exercise. I would like to put the Item.price and Item.Name into Listbox2.

Is it possible to put the array name into a variable and iterate with a foreach loop?  This is to avoid a very long if, switch, or while loop.  It could also be useful in the future if I have 40 lists.

Example

Array variable = Drinks;
foreach(Product item in variable)
{
    listBox2.Items.Add(item.ProductName + item.Price);
}

PS: I've already tried using a temporary List, where I put the Drinks into the list and call it by Product.Name and/or Product.price.

Could it also be more efficient? I'm very open to new ideas or approaches on how I should make my code shorter and more efficient.

    public partial class Form1 : Form
    {
        List<Product> Drinks = new List<Product>() { new Product("Coca Cola", 1.2F), new Product("Fanta", 2.0F), new Product("Sprite", 1.5F) };
        List<Product> Bread = new List<Product>() { new Product("Brown Bread", 1.2F), new Product("White Bread", 2.0F), new Product("Some otherBread", 1.5F) };
        public Form1()
        {
            InitializeComponent();
        }

        private void comboBox1_SelectedIndexChanged(object sender, EventArgs e)
        {
            listBox1.Items.Clear();
// instead of 40 if loops or a while loop is it possible to change the List Name with a variable and use only 1 foreach loop as the example i showed above?
            if (comboBox1.Items.IndexOf(comboBox1.SelectedItem) == 0)
            {
                foreach (Product item in Drinks)
                {
                    listBox1.Items.Add(item.ProductName);
                }
            }
            else
            {

                foreach (Product item in Bread)
                {
                    listBox1.Items.Add(item.ProductName);
                }
            }
        }

        private void listBox1_MouseDoubleClick(object sender, MouseEventArgs e)
        {
            // do something here
        }
    }

    public class Product
    {
        private string productName;
        private float price;

        public Product(string productName, float price)
        {
            this.ProductName = productName;
            this.Price = price;
        }

        public string ProductName
        {
            get { return productName; }
            set { productName = value; }
        }

        public float Price
        {
            get { return price; }
            set { price = value; }
        }

    }

Is it possible to put the array name into a variable and iterate by a foreach loop?

YES.

Array ProductItems = Drinks.ToArray();
foreach(Product item in ProductItems)
                    {
                        Console.WriteLine(item.ProductName);
                    }

Now about your second question, (it's unclear what's the significance of the first question)...

If there are many products and user selects one from the combobox1 how to update the product details in listbox1? (is this your question?)

Answer : Make a Dictionary of combobox1 index and ProductList. Iterate over the combobox1 list and map the indexes with the ProductList. This may help.

Dictionary ProductDict = new Dictionary<int, List<Product>>();

Then after SelectedIndexChanged event is fired get the ProductList corresponding with combobox1 index. Then iterate over the list and add the details it in listbox.

General Review

Since productName and price is trivial make them auto-implemented properties

public string ProductName { get; set; }


Regex for finding After Effects files in scene code format I haven't used regex a lot and I needed to set up a script that can gather a list of file paths that should adhere to a strict formatting convention, so I thought that sounded like a good opportunity to use them.

To explain a bit, there's a set of sequence folders inside a root folder, and in those sequences is a set of scene folders. In those is a set of constant named folders, the relevant one being "AEP Files". Then in there is a set of after effects files, of which I want to get the highest numbered version which is denoted but _v##.aep at the end of the file. A sample path might look like this:

P:\ProjectName\Scenes\e10_q04\e10_q04_s12\AEP Files\proj_e10_q04_s12_v04.aep

I'm particularly interested to know

If I'm using regex correctly,
Whether I should use something other than if re.match(...)?
Whether I could make it more efficient (particularly the list comprehensions); and
Given the complexity, how is the current readability?

I had ideas on more efficiency but they involved collapsing list comprehensions down and I thought that would make an unreadable mess, so I opted not to. Any other feedback you want to give is also welcome!

import os
import re

root = r'P:\ProjectName\Scenes'
IGNORE = re.IGNORECASE

folders = [os.path.join(root, f) for f in os.listdir(root)
            if re.match(r'e\d*q\d*', f, IGNORE)]
folders.sort()

scene_folders = [os.path.join(folder, f) for folder in folders
                for f in os.listdir(folder)
                if re.match(r'e\d*_q\d*_s\d*', f, IGNORE)]
scene_folders.sort()

scenes = []
missing_scenes = []
for folder in scene_folders:
    matches = [re.match(r'proj_q\d*_s\d*_v(\d*)\.aep', f, IGNORE)
                for f in os.listdir(os.path.join(folder, "AEP Files"))
                if re.match(r'proj_q\d*_s\d*_v(\d*)\.aep', f, IGNORE)]
    matches = [(match.group(), match.groups()[0]) for match in matches]
    if matches:
        scenes.append(os.path.join(folder, sorted(matches)[-1][0]))
    else:
        missing_scenes.append(folder)

root should be capitalized as it is a constant.
You can shorten the code by using sorted that returns a new list instead of working in place.
IGNORE in my opinion reduces readibility as it is less obvious than IGNORECASE.
Your regexes are pretty hard, I would extract them into constants and give them a name.
You make good use of list comprehensions, they shorten and simplify code, good job!
Performance may not be a primary concern here, but it is a good habit to use lazy generators: putting round parenthesis instead of square brackets outside the match comprehension will not build the list, saving memory and time. The same goes for scene_floders.


Calling a function from a dynamically loaded DLL I need to implement some functionality of the otherwise monolithic application via DLL that is loaded at runtime. (Think about a customized DLL -- different for each customer.)

The Visual C++ from VS 2013 is used.

The application gets the full name name of the DLL. The function to be called has a fixed name (convert in my case) and fixed number and type of the arguments (a reference to a constant vector of strings -- see the cref_vs below). This means the function is implemented in C++.

I have modified the example from the MSDN doc, and it seems to work. However, I must be sure I did not overlooked something important. Here is the code (simplified) to load the DLL and call the function:

typedef const std::vector<std::string> & cref_vs;
typedef int(*CONVERTPROC)(cref_vs vs);

Notice that unlike in the official example that mixes calling a C code in DLL from a C++ function, I do not use __cdecl. I guess that this way I can reliably pass a C++ object (like a vector of strings). Is it correct?

int call_convert_from_dll(const std::string & dllname,
                          const std::vector<std::string> & vs)
{
    HINSTANCE hinstLib;
    CONVERTPROC convert;
    BOOL fFreeResult, fRunTimeLinkSuccess = FALSE;

    // Get a handle to the DLL module.
    hinstLib = LoadLibrary(dllname.c_str());

    // If the handle is valid, try to get the function address.
    if (hinstLib != nullptr) {
        convert = (CONVERTPROC)GetProcAddress(hinstLib, "convert");

        // If the function address is valid, call the function.
        if (convert != nullptr) {
            (convert)(vs);
        }

        // Free the DLL module.
        fFreeResult = FreeLibrary(hinstLib);
    }

    if (!fRunTimeLinkSuccess)
        return 1;

    return 0;
}

Now, because I want the unmangled name of the function inside the DLL, I need to use extern "C", and it must be exported via __declspec(dllexport). However, I do not want to use __cdecl calling convention:

#include <windows.h>
#include <iostream>

using namespace std;

extern "C" {

__declspec(dllexport) int convert(const std::vector<std::string> & vs)
{
    cout << "my.dll -- convert called with arguments:\n";
    for (int i = 0; i < argc; ++i)
         cout << argv[i] << endl;
    return 0;
}

} // extern "C"

Can you confirm that I am doing it correctly?

Short answer: No, but I won't hold it against you. ;)

I'd suggest reading up on using C++ with DLLs in a compatible fashion, and would suggest this excellent article as a good place to start. This article also touches on exporting STL types, which your code is doing.

So, the question is, how portable/robust do you need/expect your approach to be? If you need developers from anywhere to be able to plug into your system, stick to a C interface if possible (avoiding STL types -- pass a char** across the boundary, and convert that on the C++ side to an STL vector<string>). Otherwise, upgrade to a virtual class based system -- and make sure that you don't directly use STL, but instead use virtual class wrappers for those types as well. Note that this carries on -- e.g., if you want to use an iterator class used by the STL type you're wrapping, you'll need another abstract (iterator) class that your abstract (iterable) class references, so it can get a little ugly.

Minor code notes...

fFreeResult is assigned, but never used.
fRunTimeLinkSuccess is not set (after initialization).
The "convert" string might better be contained in a macro (or a small set of macros), so that your plugin clients can declare a convert function, and you can refer to that function name without the possibility of typo.
Your example plugin refers to argc and argv that don't exist.


Leetcode 38: The “count-and-say” sequence 38. Count and Say

  The count-and-say sequence is the sequence of integers with the first five terms as following:

1.     1
2.     11
3.     21
4.     1211
5.     111221

  1 is read off as "one 1" or 11.

  11 is read off as "two 1s" or 21.

  21 is read off as "one 2, then one 1" or 1211.

  Given an integer n where 1 ≤ n ≤ 30, generate the nth term of the count-and-say sequence.

  Note: Each term of the sequence of integers will be represented as a string.

With iteration:

How to reduce 3 kinds of situation to one solution?

class Solution {
    func countAndSay(_ n: Int) -> String {
        var ans = ["1"]
        var temp: [String], j: Int
        for _ in 1..<n{
            //  from the beginning 1 to what we what
            j = 1
            temp = []
            for i in 1..<ans.count{
                if ans[i - 1] == ans[i]{
                    j += 1
                    //  Situation one: the current is equal to the previous ( util the last )
                }
                else{
                    temp.append(String(j))
                    temp.append(ans[i-1])
                    //  Situation two: the current is not equal to the previous ( util the last )
                    j = 1
                }
            }
            //  Situation three: the last value
            temp.append(String(j))
            temp.append(ans.last!)
            ans = temp
            print(ans.reduce("", +))
        }
        return ans.reduce("", +)
    }
}

With Recursion, which is a little leaner.

class Solution {
    func countAndSay(_ n: Int) -> String {
        if n == 1 {
            return "1"
        }
        let str = countAndSay(n-1)
        let arr = Array(str).map{Int(String($0))}
        var last = arr[0]!
        var temp : [(count: Int, val: Int)] = [(1, last)]
        for num in arr[1...]{
            if last == num{
                var final = temp.last!
                final.count += 1
                temp.removeLast()
                temp.append(final)
            }
            else{
                temp += [(1, num!)]
            }
            last = num!
        }
        var result = ""
        for key in temp{
            result += "\(key.0)\(key.1)"
        }
        return result
    }
}

How to go on doing some improvement?

Your iterative solution

This is generally fine, with the only exception that it operates on an array of strings –  I'll come back to that later.

Variables should be defined in the narrowest scope where they are used. This applies to

var temp: [String], j: Int

which are used only in the inner loop. Both variable names are non-descriptive and can be improved (e.g.: j actually counts the number of consecutive equal numbers).

Your recursive solution

Here

let arr = Array(str).map{Int(String($0))}

the conversion from str to an array is not needed: A Swift string is a collection of its characters, and you can call map() directly on the string. You also know that the string contains only decimal digits. Therefore you can force-unwrap here, replacing several force-unwraps in the following code:

let arr = str.map{ Int(String($0))! }

This

var final = temp.last!
final.count += 1
temp.removeLast()
temp.append(final)

is more simply and more efficiently done as

temp[temp.count - 1].count += 1

And this loop

var result = ""
for key in temp{
    result += "\(key.0)\(key.1)"
}
return result

can be simplified using map():

return temp.map { "\($0.count)\($0.val)" }.joined()

Generally, this looks less efficient to me than the first approach, because a string is converted to an integer array in each recursion step.

Redesign

In order to avoid unnecessary conversions between strings, characters, and integers, I would suggest to operate on an integer array generally, and create the string answer only as the final step.

Instead of an inner loop or recursion I would put the code to compute the next iteration into a separate function. That makes the code easier to read and to test:

class Solution {
    func countAndSay(_ n: Int) -> String {
        var seq = [1]
        for _ in 1..<n {
            seq = transform(seq)
        }
        return seq.map(String.init).joined()
    }

    func transform(_ seq: [Int]) -> [Int] {
        // ... TODO ...
    }
}

This works with both your solutions. For your first solution the transform function would be something like this:

func transform(_ seq: [Int]) -> [Int] {
    var result = [Int]() // The transformed sequence
    var current = seq[0] // The current number
    var count = 1 // Counts # of consecutive equal numbers
    for num in seq.dropFirst() {
        if num == current {
            count += 1
        } else {
            result.append(count)
            result.append(current)
            current = num
            count = 1
        }
    }
    result.append(count)
    result.append(current)
    return result
}

Instead of comparing against arr[i-1] the current number is “remembered” in a local variable, so that we can iterate over the sequence (without its first element) instead of iterating over the array indices.

Your second solution creates an array of (count, value) tuples first, and therefore does not need to handle the case of the last run separately. If you start with an empty array of tuples (and use optional chaining to compare against the last value) then you even don't have to handle the first value separately:

func transform(_ seq: [Int]) -> [Int] {

    var pairs: [(count: Int, val: Int)] = []
    for num in seq {
        if num == pairs.last?.val {
            pairs[pairs.count - 1].count += 1
        } else {
            pairs.append((count: 1, val: num))
        }
    }
    return Array(pairs.map { [$0.count, $0.val] }.joined())
}

In my tests this was a bit faster than your original solution, but slower than the first approach, probably because of the intermediate array.

A third solution

Here is another possible solution, which combines the advantages of both approaches. Instead of traversing the array elements, we directly search for the next index of a number different from the current. Neither the first nor the last run has be be treated specially:

func transform(_ seq: [Int]) -> [Int] {
    var result = [Int]()
    var fromIndex = seq.startIndex // Start index of current run
    while fromIndex != seq.endIndex {
        let current = seq[fromIndex]
        // Start index of next run:
        let toIndex = seq[(fromIndex+1)...].firstIndex(where: { $0 != current }) ?? seq.endIndex
        result.append(toIndex - fromIndex)
        result.append(current)
        fromIndex = toIndex
    }
    return result
}


Yet another Fibonacci number generator First attempt at dynamic programming. I want to make this run faster and better.

fibonacci.cpp

#include "fibonacci.h"

std::map<int, unsigned long long int> initializeMap() {
    std::map<int, unsigned long long int> m;
    m[0] = 0;
    m[1] = 1;
    return m;
}

std::map<int, unsigned long long int> __fib_result_map = initializeMap();

unsigned long long int fibonacci(int seq) {
    using namespace std;
    map<int, unsigned long long int>& m = __fib_result_map;
    if(m[seq] == 0 && seq != 0) {
        m[seq] = fibonacci(seq - 2) + fibonacci(seq - 1);
    }
    return m[seq];
}

fibonacci.h

#ifndef __fibonacci
#define __fibonacci

#include <iostream>
#include <map>
#include <vector>
#include <string>
#include <algorithm>

unsigned long long int fibonacci(int seq);

#endif

fib_cpp.cpp

#include "fibonacci.h"
#include <cstdlib>
using namespace std;

int main(int argc, char* argv[]) {

    if(argc <= 1) {
        cerr << "No arguments specified. Usage: " << argv[0] << " <values>\n";
        return 1;
    }
    for(int i = 1; i < argc; i++) {
        cout << fibonacci(atoi(argv[i])) << endl;
    }
    return 0;
}

Here is the repository on Github.

Assuming you're using the current c++ standard, since you don't specify in your question.

Prefer stoi to atoi. atoi has issues.
Prefer standard int types: std::uint128_t instead of unsigned long long int
Create a type alias for your map type since you use it a lot.
Don't include unnecessary headers in the fibbonacci.h file. Include them in the implementation.
It would make more sense to implement your memoizing map statically in the function.
You should use the map count method rather than relying on [] returning zero for a missing key

Putting these together

using fibmap = std::map<int, std::uint128_t>;

std::uint128_t fibonacci(int seq) {

    static fibmap m = {{0,0}, {0,1}};
    if (!m.count(seq)){
        m[seq] = fibonacci(seq - 2) + fibonacci(seq - 1);
    }
    return m[seq];
}


Mongoose query using conditional formatting and regex I this the function createQuery is fully functionally, but I would like a review on it. I'm pretty sure this can be shorten and most important (improve performance). Any ideas?

const express = require("express");
const router = express.Router();
const CompanyPart = require("../models/parts");

router.post("/search", (req, res) => {
  function createQuery(bodyFieldName, mongodbFieldName) {
    if (bodyFieldName !== undefined && bodyFieldName !== "") {
      const trimmedString = bodyFieldName.trim();
      if (
        trimmedString.charAt(0) !== "*" &&
        trimmedString.charAt(trimmedString.length - 1) !== "*"
      ) {
        // screw
        return (query[mongodbFieldName] = new RegExp(
          "^" + trimmedString + "$",
          "i"
        ));
      } else if (
        trimmedString.charAt(0) === "*" &&
        trimmedString.charAt(trimmedString.length - 1) === "*"
      ) {
        // *screw*
        return (query[mongodbFieldName] = new RegExp(
          trimmedString.replace(/[*]/g, ""),
          "i"
        ));
      } else if (
        trimmedString.charAt(0) !== "*" &&
        trimmedString.charAt(trimmedString.length - 1) === "*"
      ) {
        // screw*
        return (query[mongodbFieldName] = new RegExp(
          "^" + trimmedString.replace(/[*]/g, ""),
          "i"
        ));
      } else if (
        trimmedString.charAt(0) === "*" &&
        trimmedString.charAt(trimmedString.length - 1) !== "*"
      ) {
        // *screw
        return (query[mongodbFieldName] = new RegExp(
          trimmedString.replace(/[*]/g, "") + "$",
          "i"
        ));
      }
    }
  }

  console.log("req.body", req.body);
  let query = {};

  createQuery(req.body.partNumber, "itemNumber");
  createQuery(req.body.partName, "partName");
  createQuery(req.body.customsTariff, "customsTariff");
  console.log("query", query);

  CompanyPart.find(query)
    .sort({
      itemNumber: 1
    })
    .exec((err, parts) => {
      if (err) {
        res.status(500).json(err);
        return;
      }
      res.status(200).json({ parts });
    });
});

module.exports = router;

First, using new RegExp on user input is a bit like running eval on user input (though not nearly as bad - usually). If it can be avoided, it should be. In this case, if I pass .*? as the partName, the regex will be /^.*?$/i which will match every entry in the database, or I could pass ( which would result in the function throwing an error. This can be mitigated by escaping all special characters, but still isn't a good idea if there is another way to do this. (I haven't used MongoDB, but it appears the $search key might be useful - MongoDB Docs.

If the built in $search parameter doesn't work for you, then the function should escape all special characters in the search string before creating a RegExp with it.

One way to do this is the following:

function createRegex(userInput) {
  return new RegExp(
    // Escape all special characters except *
    "^" + userInput.replace(/([.+?^=!:${}()|\[\]\/\\])/g, "\\$1")
      // Allow the use of * as a wildcard like % in SQL.
      .replace(/\*/g, ".*") + "$",
    'i'
  );
}

['test', '*test*', '*test', 'test*', '[tes{2}t.'].forEach(test => {
  console.log(test, '=>', createRegex(test))
});

While this may be slightly slower than the current function, it is much safer and can greatly simplify the createQuery function. It also allows createQuery to cover the edge case where * appears in the middle of a string.

createQuery can now be implemented in only a few lines.

function createQuery(userInput, dbField) {
  if (userInput === undefined) {
    return;
  }
  // Trim the input before checking as otherwise searching for a
  // single space could cause problems.
  userInput = userInput.trim();
  if (userInput !== "") {
    query[dbField] = createRegex(userInput);
  }
}

Alternatively, this could be shortened (at the cost of some readability) with

function createQuery(userInput, dbField) {
  if (userInput && userInput = userInput.trim()) {
    query[dbField] = createRegex(userInput);
  }
}


List of students with their classname in current year I have 3 tables: students and classes and years. In a fourth table called StudentsInClass, I'm saving students classes in each year. Now I want to show a list of students with their className in Current Year.

This is the query I use but I'm not feeling good with it:

   @StartRow int,
   @EndRow int
As
SET NOCOUNT ON;
  begin
    declare @YearID int;
    set @YearID= (select top 1 ID from Years where IsCurrent =1 );
         select * from(
    select top 10000
              S.*,T2.Title as ClassName,Row_Number() over (order by S.ID)  as
        ResultSetRowNumber from Students S
         left outer join (
    select SC.StudentCode,T1.Title from
             (select * from StudentsInClass where YearID=@YearID) SC
        left outer join Classes T1 on SC.ClassID = T1.ID
    ) T2 on S.Code= T2.StudentCode order by S.Family asc
     ) as PagedResults where ResultSetRowNumber > @StartRow and ResultSetRowNumber <=
     @EndRow;
  end

Not entirely sure I got your schema right, but something like this should be possible.
(omitted a bit of ordering, and I assume you only have one current year)

select top 10000
    s.*,
    c.Title classname,
    row_number() over (order by s.id) rownumber,
from
    students s
    inner join years
        on years.iscurrent = 1
    inner join studentsinclass sc
        on s.code = sc.studentcode and sc.yearid = years.id
    inner join class c
        on c.id = sc.classid
where
    row_number() over (order by s.id) >= @StartRow
    and
    row_number() over (order by s.id) <= @EndRow

You could possibly swap on years.iscurrent = 1 with on years.id = year(getdate()).

If you can, you should generally try to avoid too many subqueries. At least attempt to indent them properly.

You could also benefit from having a look at common table expressions. These can help you group up the subqueries in predefined results for your query. Makes it a lot easier to read.
http://msdn.microsoft.com/en-us/library/ms190766.aspx


Variable declaration closer to usage Vs Declaring at the top of Method As part of our sprints, we do peer review each others code.

Here is the code that I am reviewing.

public void SendEmail()
{

    string emailAddress = string.Empty;
    string managerEmailAddress = string.Empty;

    if (//condition)
    {
        //do something
        //Retrieve emailAddress and managerEmailAddress
        EmailMessage.To = emailAddress;
        EmailMessage.CC = managerEmailAddress;
    }
}

The suggestion from Resharper was to have the string declarations closer to the usage. However, my colleague suggest that it is not a good idea to have them closer to the usage and have them declared right at the beginning. And, I sort of agree with him because for methods where the variables are used in multiple places with different values being assigned depending upon conditions...it would make sense to have them all grouped right at the top but for smaller methods, where there is just one if condition, stick them along with the usage (for instance the above mentioned code).

What do you think?

I agree with Guffa and dreza, variables should only be declared at an outer scope if it is really really necessary as doing so is confusing and error prone. Also, if a method is long enough that having the variable declaration at the top means it isn't close to it's use then it could probably benefit from refactoring into separate methods. Doing this will also help readability because you'll have method names explaining what's happening.

Some examples (quotes as this is only my opinion):

1: "Bad" example

As an outside coder, if I saw this I would wonder why the variables are declared at the outer scope and would waste time worrying there was a special reason for it.

string emailAddress = string.Empty;
string managerEmailAddress = string.Empty;

if (//condition)
{
  emailAddress = Foo();
  EmailMessage.To = emailAddress;
}
else
{
  emailAddress = Bar();
  // Do something different with emailAddress;
}

// No more usages of emailAddress

2: "Improved" example:

This is better because the variables are defined where they are used so there is no need to go looking around for other usages.

if (//condition)
{
  var emailAddress = Foo();
  EmailMessage.To = emailAddress;
}
else
{
  var emailAddress = Bar();
  // Do something different with emailAddress;
}

3: "Even better" example:

I think this is the best it can get because the method names will help with understanding the code.

if (//condition)
{
  ExplanatoryName1(//pass anything in that is required);
}
else
{
  ExplanatoryName2(//pass anything in that is required);
}

4: If you need a return from the methods:

var methodReturn = condition ? ExplanatoryName1(args) : ExplanatoryName2(args);


UPDATE SQL with prepared stmt using only 1 variable HTML form field names must be equal to SQL table field names.

Changing only table name and allowed fields can be used in many other update pages.

How can I improve this?

$allowed = array("name","surname","email","rank");
$items = '';
foreach($_POST as $key => $value) {
    if (in_array($key , $allowed)) {
        $items.="`".str_replace("`","``",$key)."`". "='$value', ";
    }
}
$items = substr($items, 0, -2);

$table = "users" ;
$userId = $_POST['userId'];

$sql = "UPDATE $table SET $items WHERE ID = ?";
if ($stmt = $mysqli->prepare($sql)) {
    $stmt->bind_param('i', $userId);
    $stmt->execute();
}

CODE REVIEW with PDO prepare statement

In accord with the answer, my first code was open to SQL injection and i change it
with PDO prepared statement like a suggestion.

$dbh = new PDO('mysql:host=localhost;port=0000;dbname=xxx','yyy','zzz',
array(PDO::ATTR_PERSISTENT => false,PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION, PDO::ATTR_EMULATE_PREPARES => false));

$allowed = array("name","surname","email","rank");
$params = array();
$items = null;

foreach($_POST as $key => $value) {
    if (in_array($key , $allowed)) {
        $items .= "$key=:$key ,"; // create parametrized string
        $params[$key] = $value; // populate the array with allowed $_POST values
    }
}

$items = substr($items, 0, -2); // escaping the last coma
$table = "users" ;

$sql = "UPDATE $table SET $items WHERE ID = :userId ";
$params['userId'] = $_POST['userId']; // add the WHERE param value to array
$stmt = $dbh->prepare($sql);
$stmt->execute($params);

In this way, the variable $items doing all job :)

As it stands right now, you are wide open to sql injection injection attacks because you are copying posted data directly into your sql update statement.  All kinds of nasty things could be injected.

I started to rewrite it but it became a pain since mysqli requires you to bind each parameter individually.  Way too much work.  I'd suggest shifting to PDO which supports binding by arrays as well as named parameters.  Not to mention exceptions.

Something like:

// Some test data
$_POST   = array('name' => 'New Name', 'email' => 'New Email', 'something' => 'nothing', 'userId' => 1);

$allowed = array("name","surname","email","rank");

$params = array();
$items = null;

foreach($_POST as $key => $value) {
    if (in_array($key , $allowed)) {
        if ($items) $items .= ', ';
        $items .= "$key=:$key";
        $params[$key] = $value;
    }
}
if (!$items) die("Nothing to update");
echo $items . "\n"; // name=:name, email=:email

$sql = sprintf('UPDATE %s SET %s WHERE ID = :userId','users',$items);

echo $sql . "\n"; // UPDATE users SET name=:name, email=:email WHERE ID = :userId

$params['userId'] = $_POST['userId'];

$stmt = $pdo->prepare($sql);
$stmt->execute($params); // This is reason enough to shift to PDO


Should I include a view for each message? For now, I have two views using Twig:

messages.html.twig
item.html.twig

The first one is the view which displays all of the users' posts in my forum. When a user post a new message, it's done via Ajax so the server returns a view with the posts information. That view is item.html.twig

So I have something like:

messages.html.twig

{% for item in posts %}

         Posted by {{ item.author }}

         {{ item.body }}

{% endfor %}

(well much more information in each posts of course)

item.html.twig

         Posted by {{ item.author }}

         {{ item.body }}

But the code is now duplicated and if I have to add a functionality or if I change my class names, I'll have to edit both of the views.

Would it be okay, in terms of performances, if I just included the item.html.twig in messages.html.twig like:

{% for item in posts %}
    {% include 'item.html.twig' with {item: item} %}
{% endfor %}

I don't know if there's a better solution so I would like to hear about ideas to improve my code.

I would strongly recommend you to go for the include solution. You will probably take a small performance hit, but that should not outweigh the benefits of having code that is much easier to maintain. Whenever you have to duplicate code, you are doing something wrong.

Also, if you find performance really important, may I suggest not sending the rendered (sub)view trough ajax. It would be much better to send just the data as JSON over the wire. It will be much smaller, and the server does not need to take time to render anything. You can then parse the JSON in your js to render the view there. There are templating languages for js that you could use, there is even a js version of twig (I haven't used it yet, so I don't know if it is any good, but I like the idea...)


Printing contents of list with indices from other list I have two lists: L stores some values and P holds values that are indices in the first list that should be printed. Lists are sorted in ascending order. Error-checking can be ignored, and both lists are valid and values are valid.

Do I understand correctly, that since I always continue from the place I ended at, and since the values in both lists only increase, the cost of this operation is \$O(n)\$, since I only iterate over the list once?

I'd like to clean up the code. For example, is there a way to get the current index from the iterator, that would remove two additional int variables?

#include <stdio.h>
#include <iostream>
#include <list>

using std::list;

void printLots(const list<int> &l, const list<int> &p);

int main(int argc, const char * argv[]) {

    list<int> listL;
    listL.push_back(2);
    listL.push_back(4);
    listL.push_back(6);
    listL.push_back(8);
    listL.push_back(10);
    listL.push_back(12);
    listL.push_back(14);
    listL.push_back(16);
    listL.push_back(18);
    listL.push_back(20);

    list<int> listP;
    listP.push_back(0);
    listP.push_back(9);

    printLots(listL, listP);
}

void printLots(const list<int> &l, const list<int> &p) {

    auto lIterator = l.begin();
    auto pIterator = p.begin();

    int lIndex = 0;
    int pIndex = 0;

    bool finished = false;

    while (!finished) {
        int indexToPrint = *pIterator;

        while (lIndex != indexToPrint) {
            lIndex++;
            lIterator++;
        }

        std::cout << *lIterator << std::endl;
        pIndex++;
        pIterator++;

        if (pIterator == p.end()) {
            finished = true;
        }
    }
}

It doesn't look like you're using <stdio.h>, so you can just remove it.
If you won't ever need to provide command line arguments, then you can remove main()'s parameters.
If you'll just be iterating though these lists forward, then it may be more beneficial to use an std::forward_list instead.  Or maybe you can just use an std::vector if there's no special need for a linked list.
Your list assignments is still pre-C++11.  Instead, use its initializer list:

list<int> listL { 2, 4, 6, 8, 10, 12, 14, 16, 18, 20 };

list<int> listP { 0, 9 };

This will also allow you to make them const right away.


TicTacToe in Python 3 I have just started learning Python. This is code for a TicTacToe game. Can you please suggest any improvements?

pos_matrix=[[' ',' ',' '],[' ',' ',' '],[' ',' ',' ']]
def my_tictactoe(pos,symbol):
    if pos_matrix[pos[0]-1][pos[1]-1]!=' ':
        print('Invalid input')
        exit
    else:
        pos_matrix[pos[0]-1][pos[1]-1]=symbol
    for i in range(0,3):
        print(pos_matrix[i][0]+' | '+pos_matrix[i][1]+' | '+pos_matrix[i][2])
        print('--|---|--')

    if is_win():
        print('GAME OVER. \n Player with symbol {x} wins!'.format(x=pos_matrix[pos[0]-1][pos[1]-1]))
        for i in [0,1,2]:
            pos_matrix[i][0]=pos_matrix[i][1]=pos_matrix[i][2]=' '

def is_win():
    for i in [0,1,2]:
        if pos_matrix[i][0]==pos_matrix[i][1]==pos_matrix[i][2]!=' ':
            return True
        elif pos_matrix[0][i]==pos_matrix[1][i]==pos_matrix[2][i]!=' ':
            return True
    if pos_matrix[0][0]==pos_matrix[1][1]==pos_matrix[2][2]!=' ':
        return True
    elif pos_matrix[0][2]==pos_matrix[1][1]==pos_matrix[2][0]!=' ':
        return True
    else:
        return False

my_tictactoe((1,1),'o')
my_tictactoe((2,2),'x')
my_tictactoe((3,2),'o')
my_tictactoe((1,3),'x')
my_tictactoe((2,1),'o')
my_tictactoe((3,3),'x')
my_tictactoe((3,1),'o')
my_tictactoe((1,2),'x')

PyLint is a useful tool for checking your code, ensuring good coding habits.  For example, PyLint doesn’t like a lack of spaces around operators or after commas.  Neither do my old weak eyes.  Instead of this:

pos_matrix=[[' ',' ',' '],[' ',' ',' '],[' ',' ',' ']]

... code like this:

pos_matrix = [[' ', ' ', ' '], [' ', ' ', ' '], [' ', ' ', ' ']]

An if statement that does a return or exit doesn’t need to be followed by an else or elif; if the if statement is true, the execution path doesn’t need to skip around the else clause, because execution has already left the function.

For clarity:

if pos_matrix[pos[0]-1][pos[1]-1]!=' ':
    print('Invalid input')
    exit
else:
    pos_matrix[pos[0]-1][pos[1]-1]=symbol

for i in range(0,3):
    # print rows of game grid

After the if-else statement is the code to print out the game grid.  It appears this code belongs to both paths through the if-else, where as it will only execute if the False path is taken.  As such, it should be indented to belong as part of the else: clause:

if pos_matrix[pos[0] - 1][pos[1] - 1] != ' ':
    print('Invalid input')
    exit
else:
    pos_matrix[pos[0] - 1][pos[1] - 1] = symbol

    for i in range(0,3):
        # print rows of game grid

Alternately, the else: can be omitted entirely, and the statement in the else: clause outdented to the same level as the if statement:

if pos_matrix[pos[0]-1][pos[1]-1]!=' ':
    print('Invalid input')
    exit

pos_matrix[pos[0]-1][pos[1]-1]=symbol

for i in range(0,3):
    # print rows of game grid

Invalid input should not unconditionally terminate the program.

if pos_matrix[pos[0]-1][pos[1]-1]!=' ':
    print('Invalid input')
    exit

This code makes it harder to use in a larger program, since an invalid input will terminate the entire program.

Instead, a ValueError exception should be raised:

if pos_matrix[pos[0] - 1][pos[1] - 1] != ' ':
    raise ValueError('Invalid input')

This allows the caller to use a try ... except: structure.  The user could enter an invalid input, the function detects this and raises the error.  The caller catches the error, and request the user re-enter a valid move.

Without the appropriate try ... except: statement, raising the exception will also terminate the program.

You decode pos using [pos[0]-1][pos[1]-1] multiple times.  This is ugly and hard to follow.  Use variable names to avoid repeating the same calculations:

row, col = pos[0] - 1, pos[1] - 1

Then you can write:

if pos_matrix[row][col] != ' ':

Instead of looping over range(0, 3) or [0, 1, 2], just use range(3):

for i in range(3):
    ....

def my_tictactoe(pos,symbol):
    ...
    pos_matrix[pos[0]-1][pos[1]-1]=symbol
    ...
    print('GAME OVER. \n Player with symbol {x} wins!'.format(x=pos_matrix[pos[0]-1][pos[1]-1]))

Why the complex expression to set xin the format statement?  You’ve just assigned symbolto that pos_matrix entry; just use that!

print('GAME OVER. \n Player with symbol {x} wins!'.format(x=symbol))

Or better, if you are using python 3.6 or later, use an f-string:

print(f'GAME OVER. \n Player with symbol {symbol} wins!')

Your is_win code is more complex than needed because you are checking if anyone has won, which means a row, column or diagonal has 3 of the same symbol, which is not a space.  If you passed the symbol to the function, you can check if that player has won, which means checking for 3 of that symbol only, which is slightly shorter & simpler.

def is_win(symbol):
    for i in range(3):
        if all( pos_matrix[i][col] == symbol for col in range(3)):
            return True
        if all( pos_matrix[row][i] == symbol for row in range(3)):
            return True
    if all( pos_matrix[i][i] == symbol for i in range(3)):
        return True
    if all( pos_matrix[i][2 - i] == symbol for i in range(3)):
        return True
    return False

Who's turn is it, anyway?

You call my_tictactoe(...) and pass in the character to place in the grid.  As Hack Saw pointed out, this could be a 'p'!

Instead, it would be better for the game to keep track of who's turn it is, and automatically use the appropriate symbol.

For example, you could set a global symbol variable initially to 'x', and then have it flip between 'x' and 'o' after processing a valid move.

symbol = 'x'

def my_tictactoe(pos):

    # ... existing validation, printing, and win detection code ...

    # Alternate players: 'x' <--> 'o'
    symbol = 'o' if symbol == 'x' else 'x'

As a bonus, after one player wins, the other player will automatically become the first player.

To test the program, with the automatic player tracking, (expanding on Hack Saw's suggestion a bit) the test code could become:

def main():

    for pos in ((1,1), (2,2), (3,2), (1,3), (2,1), (3,3), (3,1), (1,2)):
        my_tictactoe(pos)

if __name__ == "__main__":
    main()


Multicolumn iterations with cucumber data tables in Ruby Cucumber is not designed to support multi-column iterations, but it is possible to make it work. Here, I want to try each combination of path and role.

This is the Cucumber Gherkin:

  Scenario: cannot access paths
    When I access "path" as "role" then I should see an error
      | /path1 | user1 |
      | /path2 | user2 |
      | /path3 | user3 |
      | /path4 |       |

And this is the Cucumber step_definition:

When(/^I access "path" as "role" then I should see an error$/) do |table|
  paths, roles = table.raw.transpose.map { |e| e.reject(&:blank?) }
  roles.each do |role|
    step "I am logged in as #{role}"
    paths.each do |path|
      p "#{role} user visiting #{path}"
      visit path
      step 'I should see the privileges error'
    end
  end
end

table.raw looks like:

[
    [0] [
        [0] "/path1",
        [1] "user1"
    ],
    [1] [
        [0] "/path2",
        [1] "user2"
    ],
    [2] [
        [0] "/path3",
        [1] "user3"
    ],
    [3] [
        [0] "/path4",
        [1] ""
    ]
]

and table.raw.transpose produces:

[
    [0] [
        [0] "/path1",
        [1] "/path2",
        [2] "/path3",
        [3] "/path4"
    ],
    [1] [
        [0] "user1",
        [1] "user2",
        [2] "user3",
        [3] ""
    ]
]

I like the line with transpose, but is it possible to make it any more succinct?

See Reference Cucumber for documentation related to Cucumber

For this you could use a Scenario Outline, and I personally, would split that function into two separate functions.

Try doing something like this:

Scenario Outline: cannot access certain paths as roles
  When I access "<path>" as "<role>"
  Then I should see an error

Examples:
 | path   | role  |
 | /path1 | user1 |
 | /path2 | user2 |
 | /path3 | user3 |
 | /path4 |       |

Scenario Outlines are used when you want to test the same fundamental thing, using different parameters, without clogging up your feature file with many Scenarios.

They always use an examples table, and will run through the examples table replacing the placeholders (which will always match the heading rows), with the content of each row (as with a usual table).

In my opinion, this is the best way to test what you are attempting to do.

EDIT

In light of the comments below, maybe this would be the best course of action:

Scenario Outline: cannot access various paths as role 1
  When I access "<path>" as "user1"
  Then I should see an error

Examples:
| path   |
| /path1 |
| /path2 |
| /path3 |
| /path4 |

Scenario Outline: cannot access various paths as role 2
  When I access "<path>" as "user2"
  Then I should see an error

Examples:
| path   |
| /path1 |
| /path2 |
| /path3 |
| /path4 |

Scenario Outline: cannot access various paths as role 3
  When I access "<path>" as "user3"
  Then I should see an error

Examples:
| path   |
| /path1 |
| /path2 |
| /path3 |
| /path4 |

It may be longer, but it is more human readable than perhaps the table would.


Validator for mathematical expressions in infix notation The function belows validates if a mathematical expression, say 12 + 2 * (9 - 3) / 4, does not have operators that are next to each other. It returns false if an operator is next to another operator. Otherwise, it returns true. For example, if the input expression is 2++1, it will return false. The function also recursively evaluates expressions that are inside pairs of parentheses.

private boolean validateOperators(String expression)
{
    if (!validateEndCharacters(expression)) {
        return false;
    }

    Stack s = new Stack(15); // Stack is a user-defined class.
    char[] operators = { '+', '-', '*', '/' };
    char prevOperator = '\0';
    String subexpression = "";
    boolean addSubexpression = false;
    boolean isValid = false;

    for (char c : expression.toCharArray()) {
        if (addSubexpression) {
            if (c == '(') {
                s.push("(");
            } else if (c == ')') {
                s.pop();
                if (s.isEmpty()) {
                    // We found the end of the expression inside the parentheses.
                    isValid &= this.validateOperators(subexpression);
                    subexpression = "";
                    prevOperator = '\0';
                    addSubexpression = false;
                }
            }

            subexpression += Character.toString(c);
        } else {
            if (c == '(') {
                addSubexpression = true;
                s.push("(");
            } else if (this.isIn(c, operators) && prevOperator == '\0') {
                prevOperator = c;
            } else if (this.isIn(c, operators) && prevOperator == '\0') {
                isValid &= false;
                break;
            } else if (Character.isDigit(c)) {
                prevOperator = '\0';
            }
        }
    }

    if (!s.isEmpty()) {
        return false;
    }

    return isValid;
}

private boolean validateEndCharacters(String expression)
{
    char[] operators = { '+', '-', '*', '/' };
    if (this.isIn(expression.charAt(0), operators) ||
       this.isIn(expression.charAt(expression.length() - 1), operators)) {
        return false;
    }

    return true;
}

How else can I improve the functions?

You can already reuse operators in each methods by making it a constant. You can create a Set and use the containsmethod instead of this.isIn.

You can simplify validateEndCharacters, if ( condition ) { return true } else { return false } can always be simplified by return condition.

The two else if looks like the second block will never be executed :

...
} else if (this.isIn(c, operators) && prevOperator == '\0') {
    prevOperator = c;
} else if (this.isIn(c, operators) && prevOperator == '\0') {
    isValid &= false;
    break;
}


Get all combination of a nested object with arbitrary levels I have a JSON data which can be an object/array of recursively nested object or array. The value of an array can not be null, but an value in a object can be null. I would like to return all combination of these keys and values through depth first search.

For example:

var data = {
  "title": {
     "original": "Hitchhiker",
     "more": ["HGTTG"],
     "link": null
  },
  "date": ["20150101", "20160101"]
}

The data's length and depth is arbitrary, and the combination result I want is something like this:

["title-original-Hitchhiker", "title-more-HTTG", "title-link", "date-20150101", "date-20160101"]

What I came up with is recursive:

function nestedObjectToArray(obj) {
  if (typeof(obj) != "object"){
    return [obj];
  }
  var result = [];
  if (obj.constructor == Array){
    for (var i = 0; i <obj.length; i++){
      if (obj[i]){
        var temp = nestedObjectToArray(obj[i]);
        for (var j = 0; j < temp.length; j++){
          result.push(temp[j]);
        }
      }
    }
  } else {
    for (var i in obj){
      if (obj.hasOwnProperty(i)) {
        if (obj[i] == null){
          result.push(i);
        } else {
          var temp = nestedObjectToArray(obj[i]);
          for (var j = 0; j < temp.length; j++){
            result.push(i+"-"+temp[j]);
          }
        }
      }
    }
  }
  return result;
}

Would you have a better/more elegant solution then this chunk of conditions, loops and recursion? I can use any lib if it's more convenient.

Your code is clean and works fine.
The only little point I've noticed is that you didn't follow this rule:

instead of (for instance) for (var i = 0; i <obj.length; i++)
you should write for (var i = 0, n = obj.length; i < n; i++), so the obj.length is evaluated only once rather than for each iteration

That's said, it may be possible that it doesn't matter any longer in modern browsers, due to optimizers.

Beyond that, I was interested by your challenge to find "a better/more elegant solution".

Then I found one that uses reduced code, and probably works faster:

var data = {
  "title": {
     "original": "Hitchhiker",
     "more": ["HGTTG"],
     "link": null
  },
  "date": ["20150101", "20160101"]
}

function nestedObjectToArray(obj, prefix) {
  prefix = prefix ? prefix : '';
  var result = [], isArray = Array.isArray(obj), value, nextPrefix;
  obj = typeof obj == 'object' ? obj : JSON.parse('{"' + obj + '": null}');
  for (var key in obj) {
    if (obj.hasOwnProperty(key)) {
      value = obj[key];
      nextPrefix = prefix + (isArray ? '' : (key + '-'));
      result = result.concat(
        value === null ? (prefix + key) : nestedObjectToArray(value, nextPrefix)
      );
    }
  }
  return result;
}

console.log(nestedObjectToArray(data));

The main points of interest are:

The function gets a second argument prefix which is recursively transmitted, growing as depth increases: this allows to always add the complete prefix at each level, instead of having to re-process the complete result returned to prepend each item with the key of the ancestor level.
There is a unique loop to process arrays and objects: since the only difference is the absence/presence of a key, the test (is array ?) is made at this precise point, inside the generation for the current item:
nextPrefix = prefix + (isArray ? '' : (key + '-'));.
More over, also a final "leaf" is not processed separately, like:
if (typeof(obj) != "object") {return [obj];} in your version;
instead, the leaf is transformed into an object:
obj = typeof obj == 'object' ? obj : JSON.parse('{"' + obj + '": null}');
so it can be processed in the loop, where it falls into the (already planned by design, following your initial needs) case of null value: the trick is that the value became a key.


Logging requests on a separate thread I have an ASP.NET MVC 5 application that needs to log to the database every request for a specific controller action and every link click that happens on the view of that action. Since the user shouldn't be affected of this operation.

I came up with the following pattern:

For the controller:

//get the repository instance
private RequestsRepository _repo = new RequestsRepository();
public ActionResult Index()
{
    //log the request in a new Task
    Task.Factory.StartNew(() =>
    {
        _repo.RegisterRequest(Request.UserHostAddress, Request.UserAgent);
    });

    return View();
}

For the view (client side):

This is a link

Server side method for link click logging:

public void LinkLogger()
{
    Task.Factory.StartNew(() =>
    {
        _repo.RegisterRequest(Request.UserHostAddress, Request.UserAgent);
    });
}

Repository method:

public void RegisterRequest(string ip, string userAgent)
{
    string sql = @"INSERT INTO RequestsLog (Ip, UserAgent) VALUES (@Ip, @UserAgent)";
    using (var conn = ConnectionStringProvider.GetOpenConnection())
    {
        conn.Execute(sql, new { Ip = ip, UserAgent = userAgent);
    }
}

Is this a right way to implement the requests?

I would say the short answer to your questions is no.
But since this is code review a deeper explanation is required.
Natual vs. Supernatural Async
Logging every request to a database is a common practice but can be done many different ways (IIS does this for you by default BTW).  Generally speaking the desire is usually something like this:

"We need to track user metrics or gather data to help identify potentially malicious activity on our web-server.  Let's grab all user information (with time stamp) so we can get better visibility into what is happening.  In order to pull this off though, we need to not impact normal site operations."

Ajax (to me) is the ideal candidate since it will be made on a separate request and any DB connectivity issues, latency, errors etc will not affect the actual application receiving the request.
The question is what level of detail is needed for logging.  Your code seems to indicate that all that is needed is the client IP and User-Agent.  That being the case, I would opt for a pure JS solution that makes a naturally asynchronous call and logs that information.
By using Ajax you're working with the expectations of the web server (at least in an HTTP/1 world) which expects to generally use 1 thread per request to retrieve 1 resource.
By using the TaskFactory or any form of the TPL for asynchronous processing, you're potentially breaking the model.
On the surface, this is a technicality, but if we dig deeper, we find that there are some scalability issues here.
Stephen Cleary (and others) write extensively on this subject so I will include some links to additional reading but allow me to summarize here:
Task.Run is simply a wrapper introduced in .NET 4.5 on top of TaskFactory.StartNew, which had a more complicated signature.  For more on this, see here: https://blogs.msdn.microsoft.com/pfxteam/2011/10/24/task-run-vs-task-factory-startnew/
The behavior of this method is to create a new thread if no thread is available.
Below is a quote from a Stephen Cleary article that succinctly summarizes what happens when Task.Run fires (particularly in an ASP.NET Environment).

Taken from: http://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html

The request starts processing on an ASP.NET thread.
Task.Run starts a task on the thread pool to do the calculations. The ASP.NET thread pool has to deal with (unexpectedly) losing one of its threads for the duration of this request.
The original request thread is returned to the ASP.NET thread pool.
When the calculation is complete, that thread completes the request and is returned to the ASP.NET thread pool. The ASP.NET thread pool has to deal with (unexpectedly) getting another thread.

If you have multiple calls to Task.Run, then the performance problems are compounded. On a busy server, this kind of implementation can kill scalability.
That’s why one of the principles of ASP.NET is to avoid using thread pool threads (except for the request thread that ASP.NET gives you, of course). More to the point, this means that ASP.NET applications should avoid Task.Run.

Code
So, in your JS, I would recommend the following approach:

This is a link

Then in your server-side code, you can remove those task runs completely (as from JS they don't serve a purpose).
//get the repository instance
private RequestsRepository _repo = new RequestsRepository();

public ActionResult Index()
{
    return View();
}

public ActionResult HomeLog() {
    _repo.RegisterRequest(Request.UserHostAddress, Request.UserAgent);
}

public void LinkLogger()
{
    _repo.RegisterRequest(Request.UserHostAddress, Request.UserAgent);
}

And if you really wanted to make those ajax calls more "parallel" you can decorate the actions using async / await.
Here are some articles on the subject for further reading:

http://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html
http://blog.stephencleary.com/2013/10/taskrun-etiquette-and-proper-usage.html
http://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-using.html
https://stackoverflow.com/questions/13489065/best-practice-to-call-configureawait-for-all-server-side-code
http://bradwilson.typepad.com/blog/2012/04/tpl-and-servers-pt1.html


Inserting Elements Into a Sorted Array Very new to JS (wrote my "Hello World" around 3 days ago), but not to programming in general. My documentation is inspired by the approach I followed for Python (since I haven't learned JS documentation conventions yet).

My Code

"use strict";

/*
   Inserts elements from a given source array in a specified target array in sorted order. Modifies the target array in place.

   Args:
      * `source` (Array): Source array. The array is assumed to be already sorted.
      Sorting is not done, as a custom sort function may be desired, and checking for sorting is not done as doing so would
      greatly diminish the performance benefits of working with already sorted data.
      * `target` (Array): Target array. The array is assumed to be already sorted (for similar reasons as above).
      * `cmp` (function): Comparison function (predicate).
         - Defaults to `<` operator.
         - Args:
            `a`.
            `b`.
         - Returns (boolean): Indicates if `a < b`.
            It is important that the predicate indiates `a < b` and not `a <= b` so that the sort is stable.
*/
const sortedInsert = function(source, target, cmp = (a, b) => a < b)
{
   let insertPosition = 0;
   source.forEach(itm => {
      let insertAtEnd = true;
      for(let i = insertPosition; i < target.length; i++)
      {
         if(cmp(itm, target[i]))
         {
            target.splice(i, 0, itm);
            insertPosition = i;
            insertAtEnd = false;
            break;
         }
      }
      if (insertAtEnd)
      {
         target.splice(target.length, 0, itm);
         insertPosition = target.length;
      }
   })
}

Good things:

There is a very descriptive docblock above the function that documents the arguments.
const is used for things that don't change, whereas let is used for values that can be re-assigned.

Suggestions

When inserting at the end, just use Array.push() instead of Array.splice(). This not only simplifies the syntax but also should be faster (see this jsperf).
For performance reasons it would be wise to use a for...of loop instead of a forEach() to iterate over the items in source - especially for large arrays. Functional programming is nice but it has drawbacks - especially when performance is concerned. Consider that each iteration has a call to cmp so calling an extra function for each element in source could lead to a lot of extra overhead. With such a change the variable insertAtEnd could likely be removed if a label outside the outer for loop was added and that label was used with a continue statement instead of the break.


Performance tuning on a text file to object conversion I'm using an API which returns text in the following format:

#start
#p 09060 20131010
#p 09180 AK
#p 01001 19110212982
#end
#start
#p 09060 20131110
#p 09180 AB
#p 01001 12110212982
#end

I'm converting this to a list of objects:

var result = data.match(/#start[\s\S]+?#end/ig).map(function(v){

    var lines = v.split('\n'),
        ret = {};

    $.each(lines, function(_, v2){
        var split = v2.split(' ');
        if(split[1] && split[2])
            ret[split[1]] = split[2];
    });

    return ret;
});

My concern is that the API returns quite a lot of data, therefore I would like some feedback regarding on how to improve the performance.

For instance, is there any way to reduce the mapping complexity from O(N2) to O(N)?

Also, please suggest regex improvements :)

If you use regular expressions for parsing, then I would recommend using them for everything.  Here's a solution that proceeds line by line, using capturing parentheses to see what the line contained.

function parse(data) {
    var re = /(#start)|(#end)|#p\s+(\S+)\s+(\S+)/ig;
    var results = [], match, obj;
    while (match = re.exec(data)) {
        if (match[1]) {           // #start
            obj = {};

        } else if (match[2]) {    // #end
            results.push(obj);
            obj = null;           // ← Prevent accidental reuse if input is malformed

        } else {                  // #p something something
            obj[match[3]] = match[4];
        }
    }
    return results;
}


Factory pattern for getting xml data i am  trying to implement factory pattern for getting XML Document from server

(using javax.xml.parsers.DocumentBuilder)

I have the classes below for now, could you give your opinion ? Does the structure  make sense for this pattern?

DocumentGeneratorFactory (abstract factory)

public interface DocumentGeneratorFactory {

    public Document createDocument(String scheme, String authority,
            String path, HashMap<String, String> parameters)
            throws ParserConfigurationException, SAXException, IOException;

    public Document createDocument(String scheme, String authority,String path)
            throws ParserConfigurationException, SAXException, IOException;
}

ProductDocumentGeneratorFactory (Concreate factory)

public class ProductDocumentGeneratorFactory implements
        DocumentGeneratorFactory {

    public Document createDocument(String scheme, String authority,
            String path, HashMap<String, String> parameters)
            throws ParserConfigurationException, SAXException, IOException {

        Uri.Builder uri = new Uri.Builder();
        uri.scheme(scheme);
        uri.authority(authority);
        uri.path(path);

        Set<Map.Entry<String, String>> set = parameters.entrySet();

        for (Map.Entry<String, String> params : set) {
            uri.appendQueryParameter(params.getKey(), params.getValue());
        }

        URL url = new URL(uri.toString());

        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
        DocumentBuilder db = dbf.newDocumentBuilder();
        Document doc = db.parse(new InputSource(url.openStream()));
        doc.getDocumentElement().normalize();

        return doc;
    }

    public Document createDocument(String scheme, String authority, String path)
            throws ParserConfigurationException, SAXException, IOException {

        Uri.Builder uri = new Uri.Builder();
        uri.scheme(scheme);
        uri.authority(authority);
        uri.path(path);

        URL url = new URL(uri.toString());

        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
        DocumentBuilder db = dbf.newDocumentBuilder();
        Document doc = db.parse(new InputSource(url.openStream()));
        doc.getDocumentElement().normalize();

        return doc;

    }

}

Request (Abstract Product)

public abstract class Request {
    Document doc;
    HashMap<String, String> queryStrings;

    abstract void prepareRequest() throws ParserConfigurationException, SAXException, IOException;

}

ProductRequest (Product)

public class ProductRequest extends Request{
    ProductDocumentGeneratorFactory DocumentGeneratorFactory;
    HashMap<String, String> queryStrings;

    public ProductRequest(ProductDocumentGeneratorFactory DocumentGeneratorFactory,HashMap<String, String> queryStrings){
        this.DocumentGeneratorFactory = DocumentGeneratorFactory;
        this.queryStrings = queryStrings;
    }

    @Override
    void prepareRequest() throws ParserConfigurationException, SAXException, IOException {
        doc = this.DocumentGeneratorFactory.createDocument("http", "ip-address", "default.aspx",this.queryStrings);
    }

}

Makes sense, but you should DRY up your concrete factory at least. Forgive me my rusty Java, but something like this:

public class ProductDocumentGeneratorFactory implements
        DocumentGeneratorFactory {

    public Document createDocument(String scheme, String authority, String path)
            throws ParserConfigurationException, SAXException, IOException {

        Uri.Builder uri = createUri(scheme, authority, path);
        Document doc = createDocument(uri);
        return doc;
    }

    public Document createDocument(String scheme, String authority,
            String path, HashMap<String, String> parameters)
            throws ParserConfigurationException, SAXException, IOException {

        Uri.Builder uri = createUri(scheme, authority, path);
        appendParameters(uri, parameters);
        Document doc = createDocument(Uri.Builder uri);
        return doc;
    }

    private Uri createUri(string scheme, string authority, string path)
    {
        Uri.Builder uri = new Uri.Builder();
        uri.scheme(scheme);
        uri.authority(authority);
        uri.path(path);
        return uri;
    }

    private Document createDocument(Uri.Builder uri)
    {
        URL url = new URL(uri.toString());

        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
        DocumentBuilder db = dbf.newDocumentBuilder();
        Document doc = db.parse(new InputSource(url.openStream()));
        doc.getDocumentElement().normalize();

        return doc
    }

    private void appendParameters(Uri.Builder uri, HashMap<string,string> parameters)
    {
        Set<Map.Entry<String, String>> set = parameters.entrySet();
        for (Map.Entry<String, String> params : set) {
            uri.appendQueryParameter(params.getKey(), params.getValue());
        }
    }
}

Of course you can expand on that by employing the template pattern for the extra parameter settings.

I would also look into extracting the parameters to a "parameter object" instead of several parameters.