Sorting efficiently by multiple columns I need to sort a large array (5 columns x ~500,000 rows) of string variables by two columns. I have code that works, but it takes an unacceptably long time, particularly because I have to do this on five sheets with different data.

I have been doing research, but the code I found tend to be for either a single column, or exclusively for integers. I need to be able to change it to work with between 3-8 columns in the array as well, but I'm hopeful I can figure that out myself.

Link 1
Link 2
Link 3

Sorting on two columns:

   ActiveWorkbook.Worksheets("Quantity Available").Sort.SortFields.Clear
    ActiveWorkbook.Worksheets("Quantity Available").Sort.SortFields.Add Key:=Range( _
        "C2:C" & LastRowAvail), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
        xlSortNormal
    ActiveWorkbook.Worksheets("Quantity Available").Sort.SortFields.Add Key:=Range( _
        "E2:E" & LastRowAvail), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
        xlSortNormal
    With ActiveWorkbook.Worksheets("Quantity Available").Sort
        .SetRange Range("A1:F" & LastRowAvail)
        .Header = xlYes
        .MatchCase = False
        .Orientation = xlTopToBottom
        .Apply
    End With

Sorting on one column:

   ActiveWorkbook.Worksheets("Main Tab").Sort.SortFields.Clear
    ActiveWorkbook.Worksheets("Main Tab").Sort.SortFields.Add Key:=Range( _
        "U2:U" & LastRow), SortOn:=xlSortOnValues, Order:=xlAscending, DataOption:= _
        xlSortNormal
    With ActiveWorkbook.Worksheets("Main Tab").Sort
        .SetRange Range("A1:AU" & LastRow)
        .Header = xlYes
        .MatchCase = False
        .Orientation = xlTopToBottom
        .Apply
    End With

My time became limited so here is the first and probably the preferable as it would facilitate migrating the data (lots of data) to a proper database.

Note I have mixed some StackExchange code and Microsoft code with my code (that was never intended for excel) to accomplish a form of data manipulation for you, as an example.

Please note you probably need to look at the connection string in "Public Function RecordSetFromSheet" specifically this part "Extended Properties='Excel 8.0;HDR=Yes;IMEX=1'"

Option Explicit

' How to use as a memory file
'  DataSet.Fields.Append "FileName", adVarChar, 254
'  DataSet.Fields.Append "FileType", adVarChar, 254
'  DataSet.Fields.Append "FileDateTime", adDBTimeStamp
'
'  OpenData
'  DataSet.Sort = "FileDateTime DESC, FileType ASC, FileName ASC"

' Use Save / CreatePersistant / OpenPersistant as required

' Create a Recordset
Public DataSet As Object 'Facilitate Late Binding
Public MyFSO As New FileSystemObject
Private Sub Construct()

  Set DataSet = CreateObject("ADODB.Recordset") ' New ADODB.Recordset

  'See this link for datatypeenum "https://docs.microsoft.com/en-us/sql/ado/reference/ado-api/datatypeenum"

  DataSet.Fields.Append "FileName", adVarChar, 254
  DataSet.Fields.Append "FileType", adVarChar, 254
  DataSet.Fields.Append "FileDateTime", adDBTimeStamp

End Sub
Private Sub Destruct()

  Set DataSet = Nothing

End Sub
Public Sub CloseData()

' Close the Recordset
  DataSet.Close

End Sub
Public Function FileExist(pFileName as string) as boolean

DIM RetValue as Boolean

  RetValue = False

  If MyFSO.FileExists(sFilename) Then
     RetValue = True
  End If

  FileExist = RetValue

End Function
Public Sub CreatePersistData(sFilename As String)

' Create the Recordset

  If FileExist(sFilename) = True Then
     MyFSO.DeleteFile sFilename
  End If

  DataSet.Save sFilename, adPersistADTG

End Sub
Public Sub OpenPersistData(sFilename As String)

' Open the Recordset

  If FileExist(sFilename) = True Then
     DataSet.Open sFilename, , adOpenStatic, adLockOptimistic, adCmdFile
     If DataSet.RecordCount > 0 Then
        DataSet.MoveFirst
     End If
  End If

End Sub
Public Sub Save()

  DataSet.Save

End Sub
Public Sub DeleteAllRecords()

  While DataSet.RecordCount > 0
        DataSet.MoveFirst
        DataSet.Delete
  Wend

End Sub
Public Sub Sort1

  DataSet.Sort = "FileDateTime DESC, FileType ASC, FileName ASC"

End Sub
Public Sub Sort2

  DataSet.Sort = "FileDateTime ASC, FileType ASC, FileName ASC"

End Sub
Public Sub Sort3

  DataSet.Sort = "FileType ASC, FileName ASC, FileDateTime DESC"

End Sub
Public Sub LoadWorkSheet(ws as string)

' See this Microsoft Document "https://msdn.microsoft.com/en-us/library/office/ff839240(v=office.15).aspx"

  For iCols = 0 to DataSet.Fields.Count - 1
      ws.Cells(1, iCols + 1).Value = DataSet.Fields(iCols).Name
  Next

  ws.Range(ws.Cells(1, 1), _

  ws.Cells(1, DataSet.Fields.Count)).Font.Bold = True

  ws.Range("A2").CopyFromRecordset DataSet

End Sub
'
' See here for how to create initial file as in I have adapted it for consistency "http://stackoverflow.com/questions/2484516/vba-create-adodb-recordset-from-the-contents-of-a-spreadsheet"
'
Public Function RecordSetFromSheet(sheetName As String)

'Dim rst As New ADODB.Recordset
Dim cnx As Object ' To facilitate late binding New ADODB.Connection
Dim cmd As Object ' To Facilitate Late binding New ADODB.Command

  Set cnx = CreateObject("ADODB.Connection") ' New ADODB.Connection
  set cmd = CreateObject("ADODB.Command") ' New ADODB.Command

  'setup the connection
  '[HD    R=Yes] means the Field names are in the first row
  With cnx
      .Provider = "Microsoft.Jet.OLEDB.4.0"
      .ConnectionString = "Data Source='" & ThisWorkbook.FullName & "'; " & "Extended Properties='Excel 8.0;HDR=Yes;IMEX=1'"
      .Open
  End With

  'setup the command
  Set cmd.ActiveConnection = cnx
  cmd.CommandType = adCmdText
  cmd.CommandText = "SELECT * FROM [" & sheetName & "$]"

  DataSet.CursorLocation = adUseClient
  DataSet.CursorType = adOpenDynamic
  DataSet.LockType = adLockOptimistic
  'open the connection
  DataSet.Open cmd
  'disconnect the recordset
  Set DataSet.ActiveConnection = Nothing

  'cleanup
  If cmd.State = adStateOpen Then
     Set cmd = Nothing
  End If

  If cnx.State = adStateOpen Then
     cnx.Close
  End if

  Set cnx = Nothing

  '"return" the recordset object
  Set RecordSetFromSheet = DataSet

End Function

' This plus the use of the DataSet.Sort = "String" examples may be all you need or want
' In addition this Answer may be all you need to persist vs the more generic I created
' Easiest would be to use rs.Save "filename" and rs.Open "filename" to serialize client-side recordsets to files.
' DataSet.Save "FileName" .... DataSet.Open "filename"

Public Sub Test()

'Dim rstData As ADODB.Recordset

  Set DataSet = RecordSetFromSheet("Sheet1")

  Sheets("Sheet2").Range("A1").CopyFromRecordset DataSet

End Sub

Public Sub Main

  Construct

  If FileExist("YourFileNameGoesHere") then
     OpenPersistData "YourFileNameGoesHere"
  Else
     CreatePersistData "YourFileNameGoesHere"
  End If
'
' These are for examples only and initial debuging / understanding uncomment one or build your own
'
'  Sort1
'
'  LoadWorkSheet "YourWorkSheetNameGoesHere"
'
'  Sort2
'
'  LoadWorkSheet "YourWorkSheetNameGoesHere"
'
'  Sort3
'
'  LoadWorkSheet "YourWorkSheetNameGoesHere"

  Destruct

End Sub


Matching huge data set of business data I am pretty knew to the VBA world but I am really eager to improve my skills. I have a case where I would like to match cancellation with new business to see what we get back from our dropped business as new orders.

The matching is as follows: a variable called Fastighet (meaning "real estate" in Swedish) will be specified and takes a value from the dropped business and searches in the new orders business. If they match with same real estate then will check the date of new business if it is +/- 90 days then it will bring me back something called "Service-ID". The macros below work and get me the result but it takes 3 hours every time to finish matching as it matches 2200 rows in dropped business with 25000 rows of new orders!

My question is: Is there a way to make it faster?

Sub MatchingNedVSUpp()

Dim LRow, LRow2, i, n, serviceID As Long
Dim Fastighet As String

LRow = Sheet5.Range("A" & Rows.Count).End(xlUp).Row
LRow2 = Sheet6.Range("A" & Rows.Count).End(xlUp).Row

For i = 3 To LRow
    Fastighet = Sheet5.Range("CA" & i).Value
    For n = 3 To LRow2
       serviceID = Sheet6.Range("B" & n).Value

       If Sheet6.Range("BH" & n).Value = Fastighet And Sheet6.Range("AH" & n) <= Sheet5.Range("BM" & i) And Sheet6.Range("AH" & n) >= Sheet5.Range("BL" & i) And Sheet5.Application.WorksheetFunction.CountIf(Range("BU:BU"), serviceID) = 0 Then
           Sheet6.Range("B" & n).Copy
           Sheet5.Range("BU" & i).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
                     :=False, Transpose:=False
           Application.CutCopyMode = False
       End If
   Next n
Next i

End Sub

The line Dim LRow, LRow2, i, n, serviceID As Long isn't doing what you think it is. When you declare multiple variables on the same line, you need to individually assign types to all of them - otherwise the default to Variant.  I'm assuming that line was meant to be Dim LRow As Long, LRow2 As Long, i As Long, n As Long, serviceID As Long. You could probably also give them names that have more meaning than i and n.

Kudos on using the code names to refer to your worksheets instead of indexing the Worksheets collection - this is a much more reliable method to access them. However, you are still repeatedly accessing them.  I'd wrap the majority of this Sub in With Sheet6.  That not only keeps you from repeatedly dereferencing the same thing, it makes it much more clear when you are using Sheet5 by making it stand out more. While you're at it, I'd rename the worksheet code names to something more descriptive. I have no clue what Sheet5 even means beyond that it was the fifth worksheet that was added to the workbook.

This might be a micro-performance thing, but using .Cells() is typically better than using .Range() when you only need one cell. This not only avoids concatenating your cell addresses, but it prevents making Excel convert your alpha columns back to numbers. Finally, it lets you use column constants that are more descriptive than "AH".  You can use something like this...

Const FooColumn As Long = 34
'...
Sheet6.Cells(currentRow, FooColumn)

...which is much more readable.

You are using implicit default member calls on the tests Sheet6.Range("AH" & n) <= Sheet5.Range("BM" & i) and Sheet6.Range("AH" & n) >= Sheet5.Range("BL" & i). This appears to be an oversight, in that you're using explicit calls to Value everywhere else.

Performance

DON'T USE COPY AND PASTE FOR VALUES. This is horrendously inefficient. You can assign these directly like so: Sheet5.Range("BU" & i).Value = Sheet6.Range("B" & n).Value. Even better would be to suck all of your source data into an array and use that instead. That cuts down on the round-trips you're making to the worksheet.  Ever time you touch a cell, Excel has to at minimum recalculate and redraw. If you can do all of your work in memory, do that, then use the Worksheet simply for "output".

The call to Sheet5.Application.WorksheetFunction.CountIf(Range("BU:BU"), serviceID) is absolutely killing you.  First, you're applying it over the entire column (1,048,576 rows) and you're doing it in a nested loop. Finally, If statements are not short-circuited in VBA, so you're always performing this lookup. If the only thing you need to do is find out if the serviceID exists in that column, read all of the values there into a Scripting.Dictionary, and look them up in that instead:

Private Function GetServiceIDLookup() As Scripting.Dictionary
    Dim results As New Scripting.Dictionary
    With Sheet5
        Dim currentRow As Long
        Dim serviceID As Long
        For currentRow = 1 To .Cells(.Rows.Count, LookupTargetColumn).End(xlUp).Row
            serviceID = .Cells(currentRow, LookupTargetColumn).Value
            If Not results.Exists(serviceID) Then results.Add serviceID, serviceID
        Next
    End With
    Set GetServiceIDLookup = results
End Function

Build the lookup, then instead of Sheet5.Application.WorksheetFunction.CountIf(Range("BU:BU"), serviceID) = 0, you would be doing something more like this:

'Outside the loops
Dim serviceIDs As Scripting.Dictionary
Set serviceIDs = GetServiceIDLookup
'...
For i = 3 To LRow
    Fastighet = Sheet5.Cells(i, 79).Value
    For n = 3 To LRow2
        serviceID = Sheet6.Cells(n, 2).Value
        'Dictionary lookup
        If serviceIDs.Exists(serviceID) And WhateverElse Then

Between that change and pulling arrays instead of repeated worksheet accesses, you should be able to reduce the runtime by several orders of magnitude.


MySQL query to find country and state of active users Is there any reason NOT to wrap a long query over multiple line via new lines vs using concatenation?

For example, consider this:

$this->query("SELECT u.*, COALESCE(c2.name,u.state) AS real_state, c.name AS real_country, bc.alpha3 AS country_iso, bs.code AS state_iso
                    FROM " . MAIN_DB . ".users AS u
                    INNER JOIN " . MAIN_DB . ".countries AS c ON u.country=c.id
                    LEFT JOIN " . MAIN_DB . ".countries AS c2 ON u.state=c2.id
                    LEFT JOIN " . BLESTA_DB . ".countries AS bc ON c.country_iso_code=bc.alpha2
                    LEFT JOIN " . BLESTA_DB . ".states AS bs ON bc.alpha2=bs.country_alpha2 AND COALESCE(c2.name,u.state)=bs.name
                    WHERE u.active=1 AND u.mail_activated=1 AND u.blesta_id=0");

VS

$this->query("SELECT u.*, COALESCE(c2.name,u.state) AS real_state, c.name AS real_country, bc.alpha3 AS country_iso, bs.code AS state_iso " .
                    "FROM " . MAIN_DB . ".users AS u " .
                    "INNER JOIN " . MAIN_DB . ".countries AS c ON u.country=c.id " .
                    "LEFT JOIN " . MAIN_DB . ".countries AS c2 ON u.state=c2.id " .
                    "LEFT JOIN " . BLESTA_DB . ".countries AS bc ON c.country_iso_code=bc.alpha2 " .
                    "LEFT JOIN " . BLESTA_DB . ".states AS bs ON bc.alpha2=bs.country_alpha2 AND COALESCE(c2.name,u.state)=bs.name " .
                    "WHERE u.active=1 AND u.mail_activated=1 AND u.blesta_id=0");

Are there any possible problems with the first code or is either fine?

The first way is less error-prone. When you write like this:

"FROM " . MAIN_DB . ".users AS u " .
"INNER JOIN " . MAIN_DB . ".countries AS c ON u.country=c.id " .

... it's easy to "forget" to add a trailing space on the previous line, which is necessary to separate the clauses of the statement.

This might be slightly better, though it might look somewhat awkward:

" FROM " . MAIN_DB . ".users AS u " .
" INNER JOIN " . MAIN_DB . ".countries AS c ON u.country=c.id " .

Here, it's easier to see the space before the expressions, as they are all aligned to the left, your eyes don't have to scan the right ends to see if you put a space there.

With your first version, this problem doesn't exist: the embedded newlines ensure that the clauses are separated, you cannot accidentally forget whitespace in between, it's naturally there. Another advantage of the first version is that it's easier to copy and paste to a query runner program.

Finally, when embedding variables in long strings, I find the interruptions caused by concatenation quite annoying, so I prefer to use formatting with a template, like this:

$query = sprintf(
    "SELECT u.*, COALESCE(c2.name,u.state) AS real_state, c.name AS real_country, bc.alpha3 AS country_iso, bs.code AS state_iso
    FROM %s.users AS u
    INNER JOIN %s.countries AS c ON u.country=c.id
    LEFT JOIN %s.countries AS c2 ON u.state=c2.id
    LEFT JOIN %s.countries AS bc ON c.country_iso_code=bc.alpha2
    LEFT JOIN %s.states AS bs ON bc.alpha2=bs.country_alpha2 AND COALESCE(c2.name,u.state)=bs.name
    WHERE u.active=1 AND u.mail_activated=1 AND u.blesta_id=0",
    MAIN_DB, MAIN_DB, MAIN_DB, BLESTA_DB, BLESTA_DB);

The drawback of this approach is that now the table names are not so easily visible in their contexts, which can invite mistakes in the argument list.

A helper function from this post on Stack Overflow could improve that:

function vsprintf_named($format, $args) {
    $names = preg_match_all('/%\((.*?)\)/', $format, $matches, PREG_SET_ORDER);

    $values = array();
    foreach($matches as $match) {
        $values[] = $args[$match[1]];
    }

    $format = preg_replace('/%\((.*?)\)/', '%', $format);
    return vsprintf($format, $values);
}

Like this:

$query = vsprintf_named(
    "SELECT u.*, COALESCE(c2.name,u.state) AS real_state, c.name AS real_country, bc.alpha3 AS country_iso, bs.code AS state_iso
    FROM %(main).users AS u
    INNER JOIN %(main).countries AS c ON u.country=c.id
    LEFT JOIN %(main).countries AS c2 ON u.state=c2.id
    LEFT JOIN %(blesta).countries AS bc ON c.country_iso_code=bc.alpha2
    LEFT JOIN %(blesta).states AS bs ON bc.alpha2=bs.country_alpha2 AND COALESCE(c2.name,u.state)=bs.name
    WHERE u.active=1 AND u.mail_activated=1 AND u.blesta_id=0",
    array('main' => MAIN_DB, 'blesta' => BLESTA_DB));


Convert human readable string to TimeSpan I want to pass time interval as a string (to program args) and then I want to parse it to TimeSpan. I created simple class:

public static class TimeSpanConverter
    {
        public static TimeSpan Convert(string input)
        {
            var units = new Dictionary<string, int>()
            {
                {@"(\d+)(ms|mili[|s]|milisecon[|s])", 1 },
                {@"(\d+)(s|sec|second[|s])", 1000 },
                {@"(\d+)(m|min[|s])", 60000 },
                {@"(\d+)(h|hour[|s])", 3600000 },
                {@"(\d+)(d|day[|s])", 86400000 },
                {@"(\d+)(w|week[|s])", 604800000 },
            };

            var timespan = new TimeSpan();

            foreach(var x in units)
            {
                var matches = Regex.Matches(input, x.Key);
                foreach(Match match in matches)
                {
                    var amount = System.Convert.ToInt32(match.Groups[1].Value);
                    timespan = timespan.Add(TimeSpan.FromMilliseconds(x.Value * amount));
                }
            }

            return timespan;
        }
    }

Which is working, and I can easly parse like this:

var interval = TimeSpanConverter.Convert("1day13hour2min12s52ms");

What do you think about that? How can I improve that? I have a feeling, that it is not well coded.

public static class TimeSpanConverter
    {
        public static TimeSpan Convert(string input)

To people who use WPF a lot, the name hints at this class being a TypeConverter or an IValueConverter. I would say that it's really a parser. Similarly, I think that the method should be called Parse.

            var units = new Dictionary<string, int>()
            {
                {@"(\d+)(ms|mili[|s]|milisecon[|s])", 1 },
                {@"(\d+)(s|sec|second[|s])", 1000 },
                {@"(\d+)(m|min[|s])", 60000 },
                {@"(\d+)(h|hour[|s])", 3600000 },
                {@"(\d+)(d|day[|s])", 86400000 },
                {@"(\d+)(w|week[|s])", 604800000 },
            };

The prefix milli has two ls. millisecond ends in a d.

[|s] is more idiomatically written as s?.

Given the way the regexes are used (searching for matches without anchoring), most of them could be simplified. E.g. any match for min would also match m, so @"(\d+)m" will find exactly the same multiples of a minute.

The numbers on the right are magic numbers, and at a glance I can't be completely certain that they're correct. I would prefer to use TimeSpan instances and take advantage of TimeSpan.FromMilliseconds(1), TimeSpan.FromSeconds(1), etc.

I think that a real world example of a human-readable string would be "1 day, 13 hours, 2 minutes, 12 seconds, and 52 ms". At the very least, I would add \s* between the digits and the units.

Another quite realistic example would be "2 minutes and 13.5 seconds".

It's also plausible that people will use : as a separator: "2 days, 11:04:20"

Then you get ugly stuff like months and years, which don't have fixed lengths.

If you want to refuse to parse some of these, that's fine. But in that case, I think the method still needs to handle them by throwing an exception. At present they would all return TimeSpan.Zero, which is misleading.

Have you given any thought to localisation? If you only want to use this in an English-language context, wouldn't [0-9] make more sense than \d? If you're matching all digits, you should have test cases for things like "¹day١hourminute①second".

            var timespan = new TimeSpan();

Can you think of a more descriptive name for this variable?

                var matches = Regex.Matches(input, x.Key);
                foreach(Match match in matches)

If the string has more than one match for a given regex, isn't that rather worrying? I wouldn't consider "4hours2minutes1hour" to be a very realistic input.

                    var amount = System.Convert.ToInt32(match.Groups[1].Value);
                    timespan = timespan.Add(TimeSpan.FromMilliseconds(x.Value * amount));

Again, amount isn't a very descriptive name.

TimeSpan supports the operator +, and IMO total += addend; is more readable than total = total.Add(addend);.


Capture Min() And Max() Of Numbers Input I have two text boxes where a user inputs a start and a end, that is how it is supposed to work at least.  Rather than throwing an error if end < start, I was thinking I could use the built in functions Min() & Max() to capture which value is actually which.

Is this the most efficient way of capturing the Min() & Max() value of text box input?

int start = Math.Min(Convert.ToInt32(txtStart.Text), Convert.ToInt32(txtEnd.Text));
int end = Math.Max(Convert.ToInt32(txtStart.Text), Convert.ToInt32(txtEnd.Text));

not going to be much better than what you have
only parse once and only compare once

int min = Convert.ToInt32(txtStart.Text);
int max = Convert.ToInt32(txtEnd.Text);
if(max < min)
{
   int temp = min;
   min = max;
   max = temp;
}


Sessions and Authentication I've a feeling I'm overdoing my Auth class, and that it could be done in a simpler and more understandable way.

Could you give me advice on this, please?

This is for my learning process and I just want to learn. This is why I am 'reinventing the wheel'.

class Auth
{

    protected $pdo;
    protected $session = null;
    protected $error = array();

    /**
    *
    */
    function __construct($PDO)
    {
        $this->pdo = $PDO;
    }

    /**
    *
    */
    public function authenticate_user($password,$email)
    {
        try
        {
            $select_user = $this->pdo->prepare('SELECT * FROM users WHERE user_email = :email LIMIT 1');
            $select_user->execute(array(':email' => $email));
            $user_data = $select_user->fetch(PDO::FETCH_ASSOC);
            if (password_verify($password,$user_data['user_pass']))
            {
                $this->set_session($user_data);
                return true;
            }
            else
            {
                $this->error['authenticate_user'] = 'Incorrect email or password';
            }
        }
        catch (PDOException $e)
        {
            echo $e->getMessage();
        }
    }

    /**
    *
    */
    public function set_session($user_data)
    {
        if ($user_data)
        {
            $this->session = $user_data;
        }
    }

    /**
    *
    */
    public function unset_session()
    {
        if (isset($this->session))
        {
            $this->session =  null;
        }
    }

    /**
    *
    */
    public function get_session()
    {
        if (isset($this->session))
        {
            return $this->session;
        }
        else
        {
            return false;
        }
    }

    /**
    *
    */
    public function is_logged_in()
    {
        if (isset($this->session))
        {
            return true;
        }
        else
        {
            return false;
        }
    }

    /**
    *
    */
    public function get_error($key)
    {
        if (isset($this->error[$key]))
        {
            return $this->error[$key];
        }
        else
        {
            return false;
        }
    }

}

And on the login.php:

  if (!empty($_POST))
  {
      $response = $Auth->authenticate_user($_POST['password'],$_POST['email']);
  }

  session_start();

  if (isset($response))
  {
      $_SESSION = $Auth->get_session();
  }

  print_r($_SESSION);

  echo $Auth->get_error('authenticate_user');

And in the file that loads all the other files (at the top of everything):

if (isset($_SESSION)
{
  $Auth->set_session($_SESSION);
}

The code is understandable, but it can be simplified.

Instead of:

public function unset_session()
{
    if (isset($this->session))
    {
        $this->session =  null;
    }
}

You can simply:

public function unset_session()
{
    $this->session =  null;
}

Instead of:

public function get_session()
{
    if (isset($this->session))
    {
        return $this->session;
    }
    else
    {
        return false;
    }
}

You can simply:

public function get_session()
{
    return $this->session;
}

(if you don't mind that it returns a falsy value (null) instead of a real false)

Instead of:

public function is_logged_in()
{
    if (isset($this->session))
    {
        return true;
    }
    else
    {
        return false;
    }
}

You can simply:

public function is_logged_in()
{
    return isset($this->session);
}

Instead of:

public function get_error($key)
{
    if (isset($this->error[$key]))
    {
        return $this->error[$key];
    }
    else
    {
        return false;
    }
}

As of PHP 5.3, using the short ternary operator, you can simply:

public function get_error($key)
{
    return $this->error[$key] ?: false;
}


Sum Of Two Arrays element by element and arrays can be of unequal length In this question, I need to find the sum of 2 arrays element by element.
Arrays can be of unequal length.
Explanation: Sum of [1, 0, 2, 9] and [3, 4, 5, 6, 7] is [3, 5, 5, 9, 6] and the first digit represents carry over , if any (0 here ).
Here's what I could come up with. Let me know if anything could be done better,or anything i did wrong.
Also earlier someone suggested me that I should split my code into smaller pieces that are easier to read and test individually. Perhaps if anyone could help with the method signatures that I should make(if any).
    private static void sum(int[] arr1, int[] arr2) {
    ArrayList<Integer> arraySum = new ArrayList<>();
    int[] largerArray, smallerArray;
    boolean flag;
    if (arr1.length == arr2.length) {
        largerArray = arr1;
        smallerArray = arr2;
        flag = true;
    } else if (arr1.length > arr2.length) {
        largerArray = arr1;
        smallerArray = arr2;
        flag = false;
    } else {
        largerArray = arr2;
        smallerArray = arr1;
        flag = false;
    }
    int sum = 0, carry = 0;
    int diff = largerArray.length - smallerArray.length;
    for (int i = smallerArray.length - 1; i >= 0; i--) {        //traverse through smaller array
        sum = largerArray[i + diff] + smallerArray[i] + carry;
        carry = 0;
        if (sum > 9) {
            int N = sum;
            while (N != 0) {
                int rem = N % 10;
                if (N < 10) {
                    carry = rem;
                } else {
                    arraySum.add(rem);
                }
                if (i == 0 && carry != 0 && flag) {     //if we are at last element & carry!=0 add carry as first digit
                    arraySum.add(carry);
                }
                N = N / 10;
            }
        } else {
            arraySum.add(sum);
        }
    }
    if (!flag) {
        for (int i = diff - 1; i >= 0; i--) {       // now sum remaining elements of larger array
            sum = arr1[i] + carry;
            carry = 0;
            if (sum > 9) {
                int N = sum;
                while (N != 0) {
                    int rem = N % 10;
                    if (N < 10) {
                        carry = rem;
                    } else {
                        arraySum.add(rem);
                    }
                    if (i == 0 && carry != 0) {     //if we are at last element & carry!=0 add carry as first digit
                        arraySum.add(carry);
                    }
                    N = N / 10;
                }
            } else {
                arraySum.add(sum);
            }
        }
    }
    display(arraySum);
}

Instead of reviewing your code as is, I'll propose an alternative solution that should be a tad simpler than yours:

We don't need to know which array of the two is the larger, we only need our result array to be able to contain it:

int[] result = new int[Math.max(arr1.length, arr2.length) + 1];

Note that we need an extra element to adjust for possible "overflow" (aka. carry in the last added digit).

Now since we need to sum things, let's just copy over one of the two arrays into our result:

System.arraycopy(arr1, 0, result, result.length - arr1.length, arr1.length);

Now the only thing that remains to be done is doing the addition with the other array. We'll need something to store the carry in. Luckily by nature of a carry, it's maximum value is always 1 (it seems you didn't notice that). So we either have a carry-flag or not:

boolean carry = false;

Now let's walk the other array, and perform the addition. Note that I use the conventional iteration indexing \$\{1,\ldots,length-1\}\$ instead of the backwards indexing you used to simplify the reasoning in the addition:

for (int i = 0; i < arr2.length; i++) {
    int digitResult = result[result.length - 1 - i] + arr2[arr2.length - 1 - i];
    // usually you'd write this on properly separate lines
    if (carry) { digitResult += 1; }
    if (digitResult >= 10) {
        carry = true;
        digitResult -= 10;
    } else {
        carry = false;
    }
    result[result.length - 1  - i] = digitResult;
}

This was the "interesting part" already. The only thing remaining to be done is cleaning up a possibly remaining carry:

int i = arr2.length;
while (carry) {
    int r = result[result.length - 1 - i] + 1;
    if (r >= 10) { // could also be == 10
        r -= 10;
    } else {
        carry = false;
    }
    result[result.length - 1 - i] = r;
    i++;
}

Theoretically we might need to check the array access, but it's mathematically provable we don't need to, so I won't do checks. An explanatory comment could help for doing that.

Now as Timothy Truckle already mentioned in his answer your method is responsible for too many things. You should at this point just return the result array to the caller, and that's the extent of the method.

There's some repetition and "uncleanliness" in this code, that I left in on purpose. I'm pretty sure with some thinking you should be able to clean the code up a bit, and it's hard to learn it just from a person telling you what's wrong. Find out :)


Message Object - Best Practice? |  Consider data attribute for pulling text in Is this best practice way to send messages to the user?

Update:  This has been changed to a simple function call.

/**
 *Message - Sends message to the .innerHTML of an element
 */

var Message = ( function ()
{
    var messages =
    {
        name:         'Please enter a valid name',
        email:        'Please enter a valid email',
        email_s:      'Please enter a valid email.',
        pass:         'Please enter password, 6-40 characters',
        url:          'Please enter a valid url',
        title:        'Please enter a valid title',
        tweet:        'Please enter a valid tweet',
        empty:        'Please complete all fields',
        same:         'Please make emails equal',
        taken:        'Sorry, that email is taken',
        validate:     'Please contact support to reset your password',
    };
    var MessageInternal = function (element)
    {
        this.element = element;
    };
    MessageInternal.prototype.display = function( type )
    {
        this.element.innerHTML = messages[ type ];
    };
    return MessageInternal;
} () );

I don't really understand what you are asking here, but I'll give some general pointers:

I find that you have two different variables called Message very confusing.
It's not really a good idea to add proprietary properties to DOM elements. You should consider using the HTML5 dataset API instead.
It seems wrong to hard code the fade effect.
IMHO this seems like a lot of code and unnecessarily over-engineered for displaying a simple message. It reminds me a lot how of the Twitter bootstrap library does things.


Handler for a control to pan a map in four directions I have a pan control that look like this:

Pretty standard Google Mapsish controls, IMO.

When the user clicks in the top 3rd of the control, it should pan up (deltaY = +1); right third means it should pan east (deltaX = -1); Down is deltaY = -1, and West is deltaX = +1. The control is 50px, square. I have working code, but I believe it can be written much more elegantly than this using a single (if discontinuous) mathematical function.

$(control).click(function (evt) {
    var clickPointX = (evt.layerX || evt.offsetX),
        clickPointY = (evt.layerY || evt.offsetY),
        deltaX,
        deltaY;

    // control is 50x50 px.
    // Click in the top third, get a positive 'Y' value.
    // Click in the left third, get a positive 'X' value.
    // Click in the middle, get zero.
    if (clickPointX < 16) {
        deltaX = +1;
    } else if (clickPointX > 34) {
        deltaX = -1;
    } else {
        deltaX = 0;
    }

    if (clickPointY < 16) {
        deltaY = +1;
    } else if (clickPointY > 34) {
        deltaY = -1;
    } else {
        deltaY = 0;
    }
    map.panDirection(deltaX, deltaY);
});

A few improvements:

replace magic numbers
extract repetition into a function
dynamically calculate dimensions

Example code:

function getDelta(clickPoint, controlDimension, threshold){
    var delta;
    if (clickPoint < threshold) {
        delta = +1;
    } else if (clickPoint > controlDimension- threshold) {
        delta = -1;
    } else {
        delta = 0;
    }
    return delta;
}

$(document).ready(function () {
    var control = $('#example');
    var controlHeight = control.height();
    var controlWidth = control.width();
    var widthThreshold = controlHeight/3;
    var heightThreshold = controlWidth/3;

    $(control).click(function (evt) {
        var clickPointX = (evt.layerX || evt.offsetX);
        var clickPointY = (evt.layerY || evt.offsetY);
        var deltaX = getDelta(clickPointX, controlWidth, widthThreshold);
        var deltaY = getDelta(clickPointY, controlHeight, heightThreshold);
        alert('delta x: ' + deltaX + '\n' + 'delta y: ' + deltaY);
        //map.panDirection(deltaX, deltaY);
    });
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.7.1/jquery.min.js"></script>
<html>
    <div id='example' style='background-color: red; height: 50px; width: 50px;' />
</html>


Different approach Union Find I am studying algorithms and I did this "Union Find Like" algorithm.
I have one array of objects with a reference and I make the union pointing to the same reference instead of have two int[] with numbers and weights.

Its not necessary to initialize the array.
You will have a maximum of N/2 extra objects ( if you do a union in pairs ), but in an array with a lot of unions you will have just a few objects (only the R roots) with only references pointing to the same object.
It's a linear time.

Can I have some feedback about this idea?
Thanks.
public class UnionFind {

    public static class Pointer {
        Pointer pointerForJoin;
    }

    // number of elements in array
    private static final int N = 10;
    private static Pointer[] connection = new Pointer[N];

    private static void union(int a, int b) {
        if(connection[a] != null && connection[b] != null) {
            if(connection[a].pointerForJoin != connection[b].pointerForJoin )
                connection[a].pointerForJoin = connection[b].pointerForJoin = connection[a];
        } else if(connection[a] != null) {
            connection[b] = connection[a];
        } else if(connection[b] != null) {
            connection[a] = connection[b];
        } else {
            connection[a] = connection[b] = new Pointer();
            connection[a].pointerForJoin = connection[b].pointerForJoin = connection[a];
        }
    }

    private static boolean isConnected(int a, int b) {
        if (a == b) return true;
        if(connection[a] == null || connection[b] == null) return false;
        return connection[a].pointerForJoin == connection[b].pointerForJoin;
    }

    public static void main(String[] args) {
        union(1,2);
        union(2,3);
        union(5,6);
        union(8,9);
        union(8,2);

        System.out.println(isConnected(8,3)); //true
        System.out.println(isConnected(8,2)); //true
        System.out.println(isConnected(9,1)); //true
        System.out.println(isConnected(1,6)); //false
        System.out.println(isConnected(1,7)); //false
        System.out.println(isConnected(0,0)); //true
    }
}
```

First of all, I like the idea and the implementation.
I have refactored it a bit and came up with that:
isConnected: Extract connection[a] and connection[b] in local variables and simplify the condition.
Eclipse can assist you with the first step, the second one I did manually.
    private static boolean isConnected(int a, int b) {
        if (a == b) {
            return true;
        } else {
            final var pa = connection[a];
            final var pb = connection[b];
            return pa != null && pb != null && pa.pointerForJoin == pb.pointerForJoin;
        }
    }

In union I did the same with the local variables (beware of the assignments in the array.
Then I used nested IFs, which make the program flow easier to read.
This results in:
private static void union(int a, int b) {
    final var pa = connection[a];
    final var pb = connection[b];
    if(pa != null) {
        if (pb != null) {
            if(pa.pointerForJoin != pb.pointerForJoin)
                pa.pointerForJoin = pb.pointerForJoin = pa;
        } else {
            connection[b] = pa;
        }
    } else {
        // pa == null
        if(pb != null) {
            connection[a] = pb;
        } else {
            connection[a] = connection[b] = new Pointer();
            connection[a].pointerForJoin = connection[a];
        }
    }
}

The next step is to use a helper class instead of static variables, like that:
public class UnionFind {

    public static class Pointer {
        Pointer pointerForJoin;
    }

    private final Pointer[] connection;

    public UnionFind(int n) {
        connection = new Pointer[n];
    }

    private void union(int a, int b) {
        final var pa = connection[a];
        final var pb = connection[b];
        if(pa != null) {
            if (pb != null) {
                if(pa.pointerForJoin != pb.pointerForJoin)
                    pa.pointerForJoin = pb.pointerForJoin = pa;
            } else {
                connection[b] = pa;
            }
        } else {
            // pa == null
            if(pb != null) {
                connection[a] = pb;
            } else {
                connection[a] = connection[b] = new Pointer();
                connection[a].pointerForJoin = connection[a];
            }
        }
    }

    private boolean isConnected(int a, int b) {
        if (a == b) {
            return true;
        } else {
            final var pa = connection[a];
            final var pb = connection[b];
            return pa != null && pb != null && pa.pointerForJoin == pb.pointerForJoin;
        }
    }

    public static void main(String[] args) {
        var uf = new UnionFind(10);
        uf.union(1,2);
        uf.union(2,3);
        uf.union(5,6);
        uf.union(8,9);
        uf.union(8,2);

        System.out.println(uf.isConnected(8,3)); //true
        System.out.println(uf.isConnected(8,2)); //true
        System.out.println(uf.isConnected(9,1)); //true
        System.out.println(uf.isConnected(1,6)); //false
        System.out.println(uf.isConnected(1,7)); //false
        System.out.println(uf.isConnected(0,0)); //true
    }
}

You see how you can create multiple instances of UnionFind? You can even set a capacity at runtime.
I will leave the addition of the missing JavaDoc to you.


Retrieving result sets from a MySQL table This retrieves result sets from a MySQL table. The objective of this function is to correctly retrieve any number of rows (with $sql) from a MySQL table, binding the needed values accordingly ($params[]), having the possibility to choose the column/s you want to retrieve ($cols[]).

$db = new PDO('mysql:host=127.0.0.1;dbname=xxx;charset=utf8','root','');

function query($sql, $params=[], $cols=[], $db){
    if($stmt = $db->prepare($sql)){
        $x = 1;
        $vec = [];
        if(count($params)){
            foreach($params as $p){
                $stmt->bindValue($x, $p);
                $x++;
            }
        }
    }
    if($stmt->execute()){
        while($row = $stmt->fetch(PDO::FETCH_OBJ)){
            $array = '';
            foreach ($cols as $col) {
                $array .= $row->$col.' ';
            }
            $vec[] = $array;
        }
        return $vec;
    } else {
        return $db->errorInfo();
    }
}

Call:

foreach (query('SELECT * FROM angajati WHERE nume=?',['admin'],['id','nume','prenume'],$db) as $q){
    echo $q,'<br/>';
}

My questions are the following:

How can I improve this function?
Is there any query that can not be run correctly by this function? joins, subqueries, etc.
Is there anything else you think that isn't right at this function?

The first thing I've noticed is that you return 2 different things:

The data that the user wanted;
The error information.

Now, how will you distinguish between an error-ed query and a successful one? You can't! And that makes angels angry and cry.

Return null in case of error.

Your variable names are really....... conflicting and incomplete...

$x = 1; //Should be $i
$vec = []; //No idea what this is for
foreach($params as $p) //$p should be $param
$array = ''; // ... a variable called $array that receives a string ... WTH!?

A good variable name will give us an idea of it's content without having to read through all the code.
The name for $i (instead of $x) is the standard name used for an increment. Since you have $x++ below, it is a good name.

Now, you have this block:

if($stmt->execute()){
    while($row = $stmt->fetch(PDO::FETCH_OBJ)){
        $array = '';
        foreach ($cols as $col) {
            $array .= $row->$col.' ';
        }
        $vec[] = $array;
    }
    return $vec;
} else {
    return $db->errorInfo();
}

Use early returns, like this:

if(!$stmt->execute()){
    return null;
}
while($row = $stmt->fetch(PDO::FETCH_OBJ)){
    $array = '';
    foreach ($cols as $col) {
        $array .= $row->$col.' ';
        }
    $vec[] = $array;
}
return $vec;

This helps the code to be more readable and reduces the nesting level. Too many chained ifs and loops will cause your code to be hard to read.

You have the following if, just a little bellow:

if($stmt = $db->prepare($sql)){
    $x = 1;
    $vec = [];
    if(count($params)){
        foreach($params as $p){
            $stmt->bindValue($x, $p);
            $x++;
        }
    }
}

Applying the early return:

$stmt = $db->prepare($sql)
if(!$stmt){
    return null;
}

$i = 1;
$vec = [];

foreach($params as $param){
    $stmt->bindValue($i, $param);
    $i++;
}

See? It is so much easier to read!

In fact, the loop can be like that: (this point is subjective!)

foreach( array_keys($params) as $i => $param ) {
    $stmt->bindValue($i + 1, $params[$param]);
}

Bye bye $i++;!

Going deeper on your code, I see that you also have some useless code.

Check this block:

if(count($params)){
    foreach($params as $p){
        $stmt->bindValue($x, $p);
        $x++;
    }
}

Why do you have that count() there? It does nothing there. Just do this:

foreach($params as $param){
    $stmt->bindValue($i, $param);
    $i++;
}

The foreach doesn't care about the length of $params. As long as it is an array.

And now, going to this block:

while($row = $stmt->fetch(PDO::FETCH_OBJ)){
    $array = '';
    foreach ($cols as $col) {
        $array .= $row->$col.' ';
    }
    $vec[] = $array;
}
return $vec;

Question time:

What, in the name of the Lord, is going on here?
Why are you fetching an object instead of an associative array?
What does $array do?
Why is it called $array?
What is $vec for? What's in it?

After testing myself, I concluded this: It's bugged!!!
The expected result is different that the provided result.
I really don't get the reasoning to do $row->$col.' ';.

Here's a test code I've build, based on yours:

//test variable, fakes the database
$items = array(
    (object)array(
        'b'=>5,
        'c'=>'a'
    ),
    (object)array(
        'b'=>7
    )
);

$cols = array('b','c');//only 1 column

//simulates $stmt->fetch(PDO::FETCH_OBJ)

while($row = array_shift($items)){
    $array = '';
    foreach ($cols as $col) {
        $array .= $row->$col.' ';
    }
    $vec[] = $array;
}
var_export($vec);

Result I expected (added extra whitespace):

array (
    0 =>
    array (
        'b' => 5,
        'c' => 'a',
    ),
    1 =>
    array (
        'b' => 7,
    ),
)

Result obtained:

Notice:  Undefined property: stdClass::$c in [...][...] on line 7

array (
    0 => '5 a ',
    1 => '7  ',
)

Check it on http://sandbox.onlinephpfunctions.com/code/1e0527eccba4ada6de7b4cdba9967ab9df4e2ce7

In conclusion: The code is broken! Fixing it is left as an exercise for you.


How to create a validation loop for multiple inputs I wrote some code to get 3 integers from the user. The code then prints the sum of the 3 integers. The code works as expected. I want to validate all input before doing the calculation. However, the code just doesn't feel right. Is there another approach I should be taking? It seems.. redundant. I am looking into nested try/except, but so far no luck.
msg = "Invalid Input"

while True:
    try:
        a = int(input("Enter 1st number: "))
        break
    except:
        print(msg)
while True:
    try:
        b = int(input("Enter 2nd number: "))
        break
    except:
        print(msg)
while True:
    try:
        c = int(input("Enter 3rd number: "))
        break
    except:
        print(msg)

print(a + b + c)

Your revised code in on the right track. You can simplify the function a fair bit:
def get_integer_input(prompt):
    while True:
        try:
            # Just return directly. No need for an invalid_input flag.
            return int(input(prompt))
        except ValueError:
            print('Invalid input')

# Where you see repetition in your code, consider using a data structure to
# address it. There are libraries that will give you the ordinals. Personally,
# I would just rephrase the input() prompt so that the ordinals are not needed.
ORDINALS = ('1st', '2nd', '3rd')

# Get the integers.
fmt = 'Enter {} number: '
a, b, c = [get_integer_input(fmt.format(o)) for o in ORDINALS]
print(a + b + c)

# Or if you care only about the total, just get it directly.
tot = sum(get_integer_input(fmt.format(o)) for o in ORDINALS)
print(tot)


Implementation of Kamasutra cipher This is exercise 3.1.31. from the book Computer Science An Interdisciplinary Approach by Sedgewick & Wayne:

Write a filter KamasutraCipher that takes two strings as command-line
argument (the key strings), then reads strings (separated by
whitespace) from standard input, substitutes for each letter as
specified by the key strings, and prints the result to standard
output. This operation is the basis for one of the earliest known
cryptographic systems. The condition on the key strings is that they
must be of equal length and that any letter in standard input must
appear in exactly one of them. For example, if the two keys are
THEQUICKBROWN and FXJMPSVLAZYDG, then we make the table
T H E Q U I C K B R O W N
F X J M P S V L A Z Y D G
which tells us that we should substitute F for T, T for F, H for X, X
for H, and so forth when filtering standard input to standard output.
The message is encoded by replacing each letter with its pair. For
example, the message MEET AT ELEVEN is encoded as QJJF BF JKJCJG. The
person receiving the message can use the same keys to get the message
back.

Here is my program:
public class KamasutraCipher
{
    public static void encrypt(String s, String t)
    {
        int m = s.length();
        int n = t.length();
        if (m != n)
        {
            throw new RuntimeException("The key lengths must be equal");
        }
        while (!StdIn.isEmpty())
        {
            String word = StdIn.readString();
            int wordLength = word.length();
            for (int i = 0; i < wordLength; i++)
            {
                for (int j = 0; j < m; j++)
                {
                    if (String.valueOf(word.charAt(i)).equals(String.valueOf(s.charAt(j))))
                    {
                        String temp = word;
                        word = temp.substring(0,i) + String.valueOf(t.charAt(j)) + temp.substring(i+1);
                    }
                    else if (String.valueOf(word.charAt(i)).equals(String.valueOf(t.charAt(j))))
                    {
                        String temp = word;
                        word = temp.substring(0,i) + String.valueOf(s.charAt(j)) + temp.substring(i+1);
                    }
                }
            }
            System.out.print(word + " ");
        }
    }
    public static void main(String[] args)
    {
        encrypt(args[0], args[1]);
    }
}

StdIn is a simple API written by the authors of the book. I checked my program and it works.
Is there any way that I can improve my program?
Thanks for your attention.

The implementation looks good, I just have a few suggestions.
Single-responsibility principle
The method encrypt seems to have a lot of responsibility:

Reads the input from the user
Encrypts the input
Outputs the result to the console

One definition of SRP is "A class should have only one reason to change". But there are many reasons for KamasutraCipher to change:

The input may come from System.in, file, database, etc.
The library StdIn changes.
The output needs to go to a file, etc.
The output needs to be formatted nicely for the user
etc..

The only responsibility of KamasutraCipher should be to encrypt (or decrypt) a string and return the result.
The interface can be refactored from this:
public class KamasutraCipher {
    public static void encrypt(String s, String t)
}

To:
public class KamasutraCipher {
    public KamasutraCipher(String key1, String key2)
    public String encrypt(String s)
}

Now the only reason for KamasutraCipher to change is for optimizations or if the Kamasutra algorithm changes, but the latter is not going to happen soon.
All the logic for requesting the input and producing the output is pushed to the main.
Strings are immutable
In Java strings are immutable objects, and any modification to a string creates a new string. Therefore this part:
String temp = word;
word = temp.substring(0,i) + String.valueOf(t.charAt(j)) + temp.substring(i+1);

Can be changed to:
word = word.substring(0,i) + t.charAt(j) + word.substring(i+1);

Optimization
The complexity of the method encrypt is O(m*n) where m is the length of the input string and n is the length of the key. (ignoring the methods of String and the while loop).
A more efficient way would be to use a map to store the string keys. For example, given the string keys ABC and FGH, the map would contain:

A -> F
B -> G
C -> H
F -> A
G -> B
H -> C

The method encrypt then becomes a simple lookup on the map, reducing the complexity  to O(m):
public String encrypt(String s) {
    StringBuilder sb = new StringBuilder(s.length());
    for (int i = 0; i < s.length(); i++) {
        Character c = s.charAt(i);
        sb.append(keyMap.get(c));
    }
    return sb.toString();
}

SpringBuilder lets us save some memory by creating the result string more efficiently. keyMap is created in the constructor because the keys don't change after initialization.
Input validation

any letter in standard input must appear in exactly one of them (keys)

As mentioned by others this is a requirement that needs to be handled, possibly in the method encrypt.
For the exceptions you can use IllegalArgumentException instead of RuntimeException.
Refactored code
public class KamasutraCipher {

    private final Map<Character,Character> keyMap;

    public KamasutraCipher(String key1, String key2) {
        if (key1.length() != key2.length()) {
            throw new IllegalArgumentException("The key lengths must be equal");
        }
        keyMap = new HashMap<>();
        for (int i = 0; i < key1.length(); i++) {
            keyMap.put(key1.charAt(i), key2.charAt(i));
            keyMap.put(key2.charAt(i), key1.charAt(i));
        }
    }

    public String encrypt(String s) {
        StringBuilder sb = new StringBuilder(s.length());
        for (int i = 0; i < s.length(); i++) {
            Character c = s.charAt(i);
            if(!keyMap.containsKey(c)) {
                throw new IllegalArgumentException(String.format("'%c' is not in the keys", c));
            }
            sb.append(keyMap.get(c));
        }
        return sb.toString();
    }

    public static void main(String[] args) {
        KamasutraCipher cipher = new KamasutraCipher(args[0], args[1]);
        while (!StdIn.isEmpty()) {
            String input = StdIn.readString();
            System.out.println(cipher.encrypt(input));
        }
    }
}


Targeting string to wrap with HTML element to italicize Objective
I set up my page to dynamically wrap  tags around a targeted keyword (string) on a title.
Background
When I worked on the static parts of the website, I could manually wrap that word. But Some parts of the page will be generated by the server, so if the word appears in the title, I need it wrapped so that it shows up as italicized.
For the sake of an example, let's say that if the keyword is "cool" then it needs to be converted to cool if it appears in a title, which in this case is class "exec-company"

<?php the_field('company') ?>

Lets say the output was this HTML

Cool company

I followed along to this tutorial. Which advised on using jQuery and showed this sample
var oldString = 'john',
newString = 'John',
newText = $('p').text().replace(RegExp(oldString,"gi"),newString);

$('p').html(newText);

I modified to this so that it is always uppercase, and always italicized
var oldString = 'cool',
newString = 'COOL',
newText = $('.exec-company').text().replace(RegExp(oldString,"gi"),newString);

$('.exec-company').html(newText);

On codepen, I made a demo which produced this.

but wondering if this was the best way to approach this problem. Especially since the solution won't really scale well since the word is used site wide.

First of all, I would highly recommend NOT doing this in JS for the following reasons:

You'd have JS scan through words, which may be slow.
FOUC, where the word is not italic until the JS runs.
jquery.text() captures newlines and whitespace. If the word is split with unexpected newlines in between, the script breaks.
jquery.html replaces HTML using innerHTML. This means:

Existing event handlers, especially those created by frameworks, are not cleaned up properly.
Existing event handlers will not be re-attached to your new HTML.

In addition to that, I suggest not hardcoding the replacement string. Use capture groups to have RegExp capture the match. Then convert the matched string to upper case and wrap it with the element afterwards. That way, you don't have hard-coded replacements, only the searches.

What I highly suggest is to do this on the PHP side instead. You can take the same logic, except just to it there. I believe preg_replace is what you need for this. If you cannot access the server-side code, then request access.

JS might be the quick and easy way to do this, but stuff like this over time leads to fragmented, highly unmaintainable, and hard-to-find code. A normal developer would think "Hey, it's probably italicized in the source." until they find out days later (yes, days) it was never in PHP. Thenthey'll have to hunt it down in JS, except they won't know where to start.


Given an integer, return its binary representation. You cannot use any Integer.toXXXString methods Here is my code:

package com.app;

public class Solution {

    public String toBinaryString(int n) {
        char[] buffer = new char[32];
        for (int i = 0; i < buffer.length; i++) {
            buffer[i] = '0';
        }
        int i = buffer.length - 1;
        while (n != 0) {
            if (n % 2 != 0) {
                buffer[i] = '1';
            }
            i--;
            n >>>= 1;
        }
        return new String(buffer);
    }
}

And I also wrote a few tests:

package com.app;

import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;

class SolutionTest {

    @Test
    void one() {
        Solution solution = new Solution();
        assertEquals("00000000000000000000000000000001", solution.toBinaryString(1));
    }

    @Test
    void minusOne() {
        Solution solution = new Solution();
        assertEquals("11111111111111111111111111111111", solution.toBinaryString(-1));
    }

    @Test
    void intMin() {
        Solution solution = new Solution();
        assertEquals("10000000000000000000000000000000", solution.toBinaryString(Integer.MIN_VALUE));
    }

    @Test
    void intMax() {
        Solution solution = new Solution();
        assertEquals("01111111111111111111111111111111", solution.toBinaryString(Integer.MAX_VALUE));
    }

    @Test
    void even() {
        Solution solution = new Solution();
        assertEquals("00000000000000000000000001111100", solution.toBinaryString(124));
    }

    @Test
    void odd() {
        Solution solution = new Solution();
        assertEquals("00000000000000000000000000100101", solution.toBinaryString(37));
    }
}

The problem seems to be pretty easy. However, it took me some time to make it work with negative numbers and some edge cases. So I'd greatly appreciate if you noticed any bugs in my code.

A single loop would be preferable.

public static String toBinaryString(int n) {
    char[] buffer = new char[32];
    for (int i = buffer.length - 1; i >= 0; i--) {
        buffer[i] = '0' + (n & 1);
        n >>>= 1;
    }
    return new String(buffer);
}


Asynchronous lookup over HTTP and checking the status For some reason declaring ApiResult before checking if the answer is in a correct status doesn't look good. I wonder if there is any better way to check the status and return the result:

public async Task<ApiResult> GetCustomersByOutcode(string outcode)
{
    using (var client = new HttpClient())
    {
        ConfigureHttpClient(client);

        var response = await client.GetAsync(
            $"customer?q={WebUtility.UrlEncode(outcode)}");

        ApiResult result = null;
        if (response.IsSuccessStatusCode)
        {
            result = await response.Content.ReadAsAsync<ApiResult>();
        }
        return result;
    }
}

I suggest to remove the result variable. You can just return the result. You don't have to store it in a temporary variable:

if (response.IsSuccessStatusCode)
{
    return await response.Content.ReadAsAsync<ApiResult>();
}
else {
    return null;
}

In this case I would even use the ternary ? : operator:

return response.IsSuccessStatusCode
    ? await response.Content.ReadAsAsync<ApiResult>()
    : null;

You could also write it like this:

if (!response.IsSuccessStatusCode)
    return null;

return await response.Content.ReadAsAsync<ApiResult>();


BFS to check if path from start node to end node exists I'm trying to solve a problem from leetcode called Jump Game and it seems to be a pretty simple graph problem where we have to find if a path exists from a start node to an end node. I personally always struggle with problems like this, where graphs are represented as arrays (instead of a Node class with connections) so I would appreciate a review of the code I wrote. My current solution looks as follows:

class Solution {

    public boolean canJump(int[] nums) {

        if(nums.length == 1)
            return true;

        HashMap<Integer, ArrayList<Integer>> graph = new HashMap<Integer, ArrayList<Integer>>();

        for(Integer i = 0; i < nums.length; i++) {

            ArrayList<Integer> edges = new ArrayList<Integer>();

            int jumps = nums[i];

            if (jumps != 0) {
                for(Integer j = 1; j <= jumps; j++) {
                    edges.add(i+j);
                }
            } else {
                edges.add(-1);
            }

            graph.put(i, edges);
        }

        //we have our graph ready

        Integer target = nums.length - 1;
        System.out.println("target is : " + target);

        HashSet<Integer> visited = new HashSet<Integer>();

        Queue<Integer> queue = new LinkedList<Integer>();
        queue.add(0);

        while(!queue.isEmpty()) {
            System.out.println("queue " + queue.peek());
            visited.add(queue.peek());
            ArrayList<Integer> edges = graph.get(queue.poll());

            System.out.println("edges is : " + edges);

            for(Integer edge : edges) {

                if(!visited.contains(edge)){

                    if (edge == target) {
                        return true;
                    }

                    if(!queue.contains(edge) && edge > 0){
                        queue.add(edge);
                    }
                }
            }

        }

        return false;

    }

}

So firstly, I know I'm doing some redundant operations. Right now I am creating a HashMap to store my graph in Node->ConnectionsList pairs since its just easier for me to visualize. I know I can just skip doing this and just directly use the input array and a queue to run my search, and I will be optimizing this later.

I would mostly like someone to go over my BFS logic implementation (in the while loop) and let me know if there is anything logically wrong with my code for it.

As it stands I pass 73/75 test cases on leetcode because of a time out, but I'd like to make sure my BFS logic is sound before I continue. There seems to be another easier way to solve this problem without BFS, but for now I'm using this to practice implementing graph search algorithms so I'd appreciate a review of that portion.

Notes:

You should strive to keep the left-hand side type as general as possible. That means HashMap<Integer, ArrayList<Integer>> graph should be
Map<Integer, List<Integer>> graph.
This is preferrable to enable simple substitution of implementations.
For more information why that's useful, look up the "Liskov Substitution Principle"

The same considerations apply for edges and visited, though interestingly not for queue.
It's significantly easier for the compiler and the JVM to iterate with an int counter and not an Integer counter, because the latter requires unboxing and boxing on every single operation you perform.
Your algorithm only performs forward jumps. While the implications of the problem itself, as well as the fact that you're actually checking every single node that's reachable forward mean that you never have to consider backward jumps, that is problematic for general graph searches.
You can save a ton of time by implementing the Queue with a TreeSet. Using a Set prevents duplicate iterations, which you currently do not. You should also be able to get a lot of speed out of "greedily" searching, by jumping as far ahead as you possibly can in each step.

Note that what I said in 4 also should give you a hint as to how you can solve the problem without using a graph-search.


What is the optimal way to retrieve a list of sorted dates in a list of sorted periods in python? Suppose I have a list with N sorted dates and M non-overlapping sorted periods with start date, end date (inclusive), and a tax rate for example. I have to make an efficient algorithm retrieve all tax rates for all dates. If there is not period including this date it should raise an error.

Given the brute-force approach I could have a O(N * M) with two nested loops. It is possible to break the inner loop when one date is found (maintains the code worst-case complexity). Another optimization would be to store the index of last period, since the lists are sorted, then I believe I got O(N + M). Is there a more optimal way of doing that? Maybe using other data structures?

Working code in Python:

import collections
import datetime
import sys
from typing import List

RatePeriod = collections.namedtuple("RatePeriod", ["start_date", "end_date", "rate"])

periods = [
    RatePeriod(datetime.datetime(2019, 1, 3), datetime.datetime(2019, 4, 1), 10.7),
    RatePeriod(datetime.datetime(2019, 4, 2), datetime.datetime(2019, 12, 2), 20.5),
    RatePeriod(datetime.datetime(2019, 12, 3), datetime.datetime(2020, 1, 2), 37.8),
]

def get_rates(dates: List[datetime.datetime]) -> List[float]:
    rates = []
    last_period = 0
    for idx, date in enumerate(dates, 1):
        for idx2 in range(last_period, len(periods)):
            period = periods[idx2]
            last_period = idx2
            if period.start_date <= date <= period.end_date:
                rates.append(period.rate)
                break
        if len(rates) < idx:
            sys.exit("No period found for date: {}".format(date))
    return rates

series = [
    datetime.datetime(2019, 2, 20),
    datetime.datetime(2019, 3, 6),
    datetime.datetime(2019, 12, 14),
]

result = get_rates(series)
expected = [10.7, 10.7, 37.8]
assert result == expected

Your algorithm seems fine, it seems you brought it down in complexity enough. I have thought about it and couldn't think of anything better.

Still, the code can be rewritten to be more Pythonic:

import collections
import sys
import timeit
from datetime import datetime as dt
from typing import List

RatePeriod = collections.namedtuple("RatePeriod", ["start_date", "end_date", "rate"])

periods = [
    RatePeriod(dt(2019, 1, 3), dt(2019, 4, 1), 10.7),
    RatePeriod(dt(2019, 4, 2), dt(2019, 12, 2), 20.5),
    RatePeriod(dt(2019, 12, 3), dt(2020, 1, 2), 37.8),
    RatePeriod(dt(2020, 1, 3), dt(2020, 12, 2), 41.3),
    RatePeriod(dt(2020, 12, 3), dt(2021, 1, 2), 52.7),
]

series = [
    dt(2019, 2, 20),
    dt(2019, 3, 6),
    dt(2020, 1, 5),
    dt(2020, 12, 5),
    # dt(2022, 1, 1),  # error, no period found
]

def get_rates(dates: List[dt]) -> List[float]:
    rates = []
    last_period = 0
    for idx, date in enumerate(dates, 1):
        for idx2 in range(last_period, len(periods)):
            period = periods[idx2]
            last_period = idx2
            if period.start_date <= date <= period.end_date:
                rates.append(period.rate)
                break
        if len(rates) < idx:
            sys.exit("No period found for date: {}".format(date))
    return rates

def get_rates_generator(dates: List[dt]) -> List[float]:
    last_period = 0
    for date in dates:
        for idx_period, period in enumerate(periods[last_period:], start=last_period):
            if period.start_date <= date <= period.end_date:
                last_period = idx_period
                break
        else:
            sys.exit(f"No period found for date: {date}")
        yield period.rate

result = get_rates(series)
result_generator = list(get_rates_generator(series))

setup = "from __main__ import get_rates, get_rates_generator, series"
print("Old:", timeit.timeit("get_rates(series)", setup=setup))
print("New:", timeit.timeit("list(get_rates_generator(series))", setup=setup))

expected = [10.7, 10.7, 41.3, 52.7]

assert result == result_generator == expected

where the printed output will be somewhere in the ballpark of

Old: 2.3620867
New: 2.3765742000000003

aka, the rewritten function is not actually faster.
A couple notes on the new, suggested approach:

imported datetime.datetime as dt for readability; from datetime import datetime is likely confusing and should be avoided
handling the case of not finding an item is nicely handled using for/else. In fact, it is one of the prime uses of for/else. The else block is run if no break was encountered for the entire loop. Since there is already a break in place, it is straightforward to implement the else block. This also gets rid of having to enumerate over dates.
instead of building and returning a list, a generator only yields on request. Having found a match, the loop is broken out of and yield is hit. The function exits and returns to its saved state on the next iteration. As such, list() can be called on the generator object to exhaust it fully and receive a list, as in your code. If no full list is desired, a generator is lighter than a full list.
the enumerate function, together with list slicing, can do what your range code did. This way, the new code is a bit dense on that line; since it is also verbose, I think it's manageable. I find it to be more readable.


Displaying information about people from a JSON file I'm working with Python 2.7.5 and I have written the following code:

def func(x):
  if x == 1:
    list = data.parseJSON(fileName)
    person = [l for l in list if l['worker'] == sys.argv[1]]
    displayPeople(flatten(person))
  elif x == 2:
    list = data.parseJSON(fileName)
    person = [l for l in list if sys.argv[1] in set(l['children'])]
    displayPeople(person)
  elif x == 3:
    list = data.parseJSON(fileName)
    person = [l for l in list if l['age'] == sys.argv[1]]
    displayPeople(flatten(person))

def flatten(lData):
  if len(lData) == 0
    return lData
  return reduce(lambda x,y:x+y,lData)

I realize that I am repeating myself a lot in the if else section. I would make a function that would do this but, as you can see, the way to get the person list is a little different each time. Also, you'll notice that the person list sometimes needs to be flattened before being passed in the displayPeople() method.

I was thinking that I could use a lambda to get the person, but I don't think that will work. I may need to add more cases, so I think it would be best to have a function where I could just change how the person is calculated and flatten it if necessary, and then pass it to displayPeople().

Any suggestions on how to make this code more modular?

The first step is to shift out duplicate code from the if branch.

def func(x):
  list = data.parseJSON(fileName)
  if x == 1:
    person = [l for l in list if l['worker'] == sys.argv[1]]
    displayPeople(flatten(person))
  elif x == 2:
    person = [l for l in list if sys.argv[1] in set(l['children'])]
    displayPeople(person)
  elif x == 3:
    person = [l for l in list if l['age'] == sys.argv[1]]
    displayPeople(flatten(person))

Secondly, we can flatten the list before putting it into the displayPeople() function.

def func(x):
  list = data.parseJSON(fileName)
  if x == 1:
    person = flatten([l for l in list if l['worker'] == sys.argv[1]])
    displayPeople(person)
  elif x == 2:
    person = [l for l in list if sys.argv[1] in set(l['children'])]
    displayPeople(person)
  elif x == 3:
    person = flatten([l for l in list if l['age'] == sys.argv[1]])
    displayPeople(person)

Thanks to Python's variable scope rules, you can access person outside the branches. We shift the duplicate code out once again:

def func(x):
  list = data.parseJSON(fileName)
  if x == 1:
    person = flatten([l for l in list if l['worker'] == sys.argv[1]])
  elif x == 2:
    person = [l for l in list if sys.argv[1] in set(l['children'])]
  elif x == 3:
    person = flatten([l for l in list if l['age'] == sys.argv[1]])
  displayPeople(person)

Since you mention the possibility that there may be more cases, we can maintain a dictionary of cases.

def func(x):
  def case_1(list):
    return flatten([l for l in list if l['worker'] == sys.argv[1]])
  def case_2(list):
    return [l for l in list if sys.argv[1] in set(l['children'])]
  def case_3(list):
    return flatten([l for l in list if l['age'] == sys.argv[1]])

  list = data.parseJSON(fileName)
  cases = {
    1: case_1,
    2: case_2,
    3: case_3
  }
  if x not in cases:
    raise ValueError
  displayPeople(cases[x](list))

Finally, we can clear up the list comprehensions. Since we're just iterating through the list and filtering everything based on a condition, we can use the function fliter().

def func(x):
  def case_1(list):
    return flatten(filter(lambda l: l['worker'] == sys.argv[1], list))
  def case_2(list):
    return filter(lambda l: sys.argv[1] in set(l['children']), list)
  def case_3(list):
    return flatten(filter(lambda l: l['age'] == sys.argv[1]], list))

  cases = {
    1: case_1,
    2: case_2,
    3: case_3
  }

  list = data.parseJSON(fileName)
  if x not in cases:
    raise ValueError
  displayPeople(cases[x](list))

Certainly much longer than Jaime's answer but personally I find that this is more expressive; having a list of values which need to have the flatten function applied before passing to displayPeople seems hackish to me. It has a few DRY violations but should be adequate.

If flatten can be applied to any list without ill effects (which should be the case), it's alright to waste a few CPU cycles if the list isn't large. In that case, we can reduce the code down to this:

def func(x):
  cases = {
    1: lambda l: l['worker'] == sys.argv[1],
    2: lambda l: sys.argv[1] in set(l['children']),
    3: lambda l: l['age'] == sys.argv[1]
  }

  list = data.parseJSON(fileName)
  if x not in cases:
    raise ValueError
  displayPeople(flatten(filter(list, cases[x])))


Perl script to match case law references I am very new to Perl and decided to work on a simple script that could solve a problem I encounter in my day to day work. The purpose of the code that follows is to search over a body of text and extract English case law publication references. There are quite a few flavours of reference style, so I have several expressions looking for different reference structures.

I'm very much at the "hooray, I've managed to make it work"-stage on the Perl learning curve, but know enough to recognise that this code is pretty hideous.

There are two main areas I'm aiming to improve:

Getting the source text from a file, rather than plonking it
directly into the code.
Getting the output to write to a output.txt file.

An examples of obvious poor practice is that I have used strict or warnings. The interpreter didn't like various aspects of the code.

I'd be really grateful for some feedback on this very modest first attempt to use Perl in an applied way. It is pretty hideous and I'm sure there are endless ways in which this could be achieved more elegantly.

The Code

#!/usr/bin/perl

# Paste text to review below

$search_text = <<EOD;

    In Salomon v A Salomon and Co Ltd [1897] AC 22, the House of Lords held that these principles applied as much to a company that was wholly owned and controlled by one man as to any other company. In Macaura v Northern Assurance Co Ltd [1925] AC 619, the House of Lords held that the sole owner and controller of a company did not even have an insurable interest in property of the company, although economically he was liable to suffer by its destruction. Lord Buckmaster, at pp 626-627 said:

EOD

print "\n-----------------------------\n";
print "Case References Found in Text";
print "\n-----------------------------\n";

# Find NCits

print "\n***** NCits...\n\n";

while ($search_text =~ m/((\(|\[)\d{4}(\)|\]))(\s+((EWHC)|(EWHC\s+Admin)|(CAT)|(EWCA)|(EWCA\s+Civ)|(EWCA\s+Crim)|(EWCOP)|(EWFC)|(EWFC\s+B)|(EWPCC)|(UKHL)|(UKIAT)|(UKPC)|(UKSC)|(CSOH)|(CSIH)|(NICA)|(IESC)|(IECCA)|(IECA)|(IEHC)|(UKUT))\s+\d+)/ig) {

    print "$1$4\n";
}

# Find WLR

print "\n***** WLR references...\n\n";

    while ($search_text =~ m/((|\[)\d{4}(\)|\]))(\s+\d+\s+((WLR))\s+\d+)/ig) {

        print "$1$4\n";

}

# Find AC with vol number

print "\n***** Appeal Cases references...\n\n";

    while ($search_text =~ m/((|\[)\d{4}(\)|\]))(\s+\d+\s+((AC))\s+\d+)/ig) {

        print "$1$4\n";

}

# Find AC with no vol number

    while ($search_text =~ m/((|\[)\d{4}(\)|\]))(\s+((AC))\s+\d+)/ig) {

        print "$1$4\n";
}

# Find QB with vol number

print "\n***** Queen's Bench Cases cases...\n\n";

    while ($search_text =~ m/((|\[)\d{4}(\)|\]))(\s+\d+\s+((QB))\s+\d+)/ig) {

        print "$1$4\n";
}

# Find QB with no vol number

while ($search_text =~ m/((|\[)\d{4}(\)|\]))(\s+((QB))\s+\d+)/ig) {

    print "$1$4\n";
}

print "\n***** External references...\n\n";

while ($search_text =~ m/((\(|\[)\d{4}(\)|\])){0,1}(\s*\d+\s+((TLR)|(TLR\s+\(Pt\s+1\))|(TLR\s+\(Pt\s+2\))|(LGR)|(Cr\s*App\s*R)|(Cr\s*App\s*R\s*\(S\))|(Ll\s*L\s*Rep)|(LlLR)|(TC)|(FLR)|(BCLC))\s+\d+)/ig) {

    print "$1$4\n";
}

# Find NIQB

print "\n***** NI Queen's Bench Cases cases...\n\n";

while ($search_text =~ m/((|\[)\d{4}(\)|\]))(\s+((NIQB))\s+\d+)/ig) {

    print "$1$4\n";
}

# Find NZLR

print "\n***** NZLR references...\n\n";

    while ($search_text =~ m/((|\[)\d{4}(\)|\]))(\s+\d+\s+((NZLR))\s+\d+)/ig) {

        print "$1$4\n";

}

# Find All ER

while ($search_text =~ m/((|\[)\d{4}(\)|\]))(\s+\d+\s+((All ER))\s+\d+)/ig) {

    print "$1$4\n";
}

# Find ECR

print "\n***** ECR references...\n\n";

while ($search_text =~ m/((|\[)\d{4}(\)|\]))(\s+((ECR))\s+\d+)/ig) {

    print "$1$4\n";
}

# Old Style ICLR Refs

print "\n***** Older volume references...\n\n";

while ($search_text =~ m/(\d+\s+((App\s*Cas)|(Ch\s*D)|(CPD)|(Ex\s*D)|(P.D.)|(Q.B.D.))\s+\d+)/ig) {

    print "$1$4\n";
}

Adding use warnings; at the top of the script doesn't emit any warnings. Adding use strict; makes the variable $search_text undeclared, but it's easy to fix it by adding my before its first use.

The easiest way to read the input from a file is to specify the filename as a parameter to the script, and read from the file with the diamond operator <> looping over the input lines:

while (my $search_text = <>) {

Also, there's a lot of repetition in your code. To deduplicate it, you can store the regular expressions in a hash keyed by the type of the reference. It shortens your script to

#!/usr/bin/perl
use warnings;
use strict;

my %reference = (
    NCits => [ qr/((\(|\[)\d{4}(\)|\]))(\s+((EWHC)|(EWHC\s+Admin)|(CAT)|(EWCA)|(EWCA\s+Civ)|(EWCA\s+Crim)|(EWCOP)|(EWFC)|(EWFC\s+B)|(EWPCC)|(UKHL)|(UKIAT)|(UKPC)|(UKSC)|(CSOH)|(CSIH)|(NICA)|(IESC)|(IECCA)|(IECA)|(IEHC)|(UKUT))\s+\d+)/ ],
    WLR => [ qr/((|\[)\d{4}(\)|\]))(\s+\d+\s+((WLR))\s+\d+)/ ],
    'Appeal Cases' => [ qr/((|\[)\d{4}(\)|\]))(\s+\d+\s+((AC))\s+\d+)/,
                        qr/((|\[)\d{4}(\)|\]))(\s+((AC))\s+\d+)/ ],
    "Queen's Bench Cases" => [ qr/((|\[)\d{4}(\)|\]))(\s+\d+\s+((QB))\s+\d+)/,
                               qr/((|\[)\d{4}(\)|\]))(\s+((QB))\s+\d+)/ ],
    'External references' => [ qr/((\(|\[)\d{4}(\)|\])){0,1}(\s*\d+\s+((TLR)|(TLR\s+\(Pt\s+1\))|(TLR\s+\(Pt\s+2\))|(LGR)|(Cr\s*App\s*R)|(Cr\s*App\s*R\s*\(S\))|(Ll\s*L\s*Rep)|(LlLR)|(TC)|(FLR)|(BCLC))\s+\d+)/ ],
    "NI Queen's Bench Cases" => [ qr/((|\[)\d{4}(\)|\]))(\s+((NIQB))\s+\d+)/ ],
    NZLR => [ qr/((|\[)\d{4}(\)|\]))(\s+\d+\s+((NZLR))\s+\d+)/,
              qr/((|\[)\d{4}(\)|\]))(\s+\d+\s+((All ER))\s+\d+)/ ],
    ECR => [ qr/((|\[)\d{4}(\)|\]))(\s+((ECR))\s+\d+)/ ],
    'Older volume' => [ qr/(\d+\s+((App\s*Cas)|(Ch\s*D)|(CPD)|(Ex\s*D)|(P.D.)|(Q.B.D.))\s+\d+)/ ],
);

print "\n-----------------------------\n";
print "Case References Found in Text";
print "\n-----------------------------\n";

while (my $search_text = <>) {
    for my $type (keys %reference) {
        print "\n***** $type *****\n";
        for my $regex ( @{ $reference{$type} } ) {
            while ($search_text =~ /$regex/ig) {
                print "$1$4\n";
            }
        }
    }
}

In a regex, (\(|\[) can be more readably written as ([[(]). For grouping without capturing, you can use (?: instead of (. For example, the NCits regex can be written as

/((?:[[(])\d{4}(?:[])]))(\s+(?:EWHC(?:\s+Admin)?|CAT|EWCA(?:\s+(?:Civ|Crim))?|EW(?:COP|PCC)|EWFC(?:\s+B)?|UK(?:HL|IAT|[PS]C|UT)|CS(?:[OI]H)|NICA|IE(?:SC|CCA|CA|HC))\s+\d+)/

which changes the variables to output to $1$2.

To output to a file, just redirect the output:

script.perl input-file > output-file


My approach to sorting algorithm For practicing purposes, I had the idea of making a sorting algorithm in Python.

My approach to it was to iterate through a given unsorted list to find the shortest number in it, add the number to a second list, remove shortest number from unsorted list, do that until the unsorted list is empty and return the sorted list.

How could my code be improved?

from math import inf
from random import randint

def my_sort(my_list):
    """
    Iterate through list and find shortest number, add
    shortest number to list until there are none left,
    return sorted list (shortest to highest).
    """
    unsorted_list = [*my_list]
    sorted_list = []
    for item in my_list:
        shortest_num = inf
        for num in unsorted_list:
            if num < shortest_num:
                shortest_num = num
        unsorted_list.remove(shortest_num)
        sorted_list.append(shortest_num)
    return sorted_list

# Randomly generate a list of 20 numbers ranging from -20 to 20
random_list = [randint(-20, 20) for i in range(20)]

print(random_list)
print(my_sort(random_list))

Since the goal is the best possible implementation of this algorithm, I'd suggest the following. However, faster algorithms do exist.

To conform to PEP8 make sure you have two blank lines after your imports and surrounding function definitions.
Since, you aren't editing each item, but rather adding and removing items until the list is sorted, I'd use the while keyword.
The builtin min function is faster than looping through yourself. If you want to make your own minimum function as an exercise you can always do that and replace min with your own my_min say

My Code:

def my_sort2(my_list):
    unsorted_list = [*my_list]
    sorted_list = []
    while len(unsorted_list) > 0:
        shortest_num = min(unsorted_list)
        sorted_list.append(shortest_num)
        unsorted_list.remove(shortest_num)
    return sorted_list

When timing mysort2 vs your mysort, I get a 12% improvement on my machine.

Timing code: (Not the nicest but it does the job)

t1 = inf
t2 = inf
for _ in range(1000):
    t1 = min(t1, timeit.timeit(
        stmt=f'my_sort(random_list)',
        setup=f'from __main__ import my_sort, random_list',
        number=100))
    t2 = min(t2, timeit.timeit(
        stmt=f'my_sort2(random_list)',
        setup=f'from __main__ import my_sort2, random_list',
        number=100))


Find first repeating Char in String Given a string, find the first repeating character in it.

  Examples:

  firstUnique("Vikrant") → None
  firstUnique("VikrantVikrant") → Some(V)

Scala implementation:

object FirstUniqueChar extends App {
  def firstUnique(s: String): Option[Char] = {
   val countMap =  (s groupBy (c=>c)) mapValues(_.length)
    def checkOccurence(s1: String ): Option[Char] = {
      if (countMap(s1.head) > 1) Some(s1.head)
      else if (s1.length == 1) None
      else checkOccurence(s1.tail)
    }
    checkOccurence(s)
  }
  println(firstUnique("abcdebC"))
  println(firstUnique("abcdef"))
}

I also have a followup question. What is the recommended way if I do not want to solve this problem with recursion? Instead of using the checkOccurence method I can traverse through the string and break when I find the first element with a count more than 1. But that will require a break, which is discouraged in Scala.

Your checkOccurrence(s) is just a clumsy way to write s.find(countMap(_) > 1).

You can significantly simplify the solution by taking advantage of .distinct.

def firstUnique(s: String): Option[Char] =
  s.zipAll(s.distinct, '\u0000', '\u0000')
   .collectFirst({ case ab if ab._1 != ab._2 => ab._1 })


Appending data to a web page from a form using a JavaScript class Recently I've been reading about classes in JavaScript and decided to code this simple form to test them out.
Even if the code works I am still pretty sure I lack understanding of classes and other js utilities, for example:

did I need to use constructor in this class? if not when should I use it?
is there a more efficient way to append multiple elements to the page?
is the try and catch used properly in this case?
generally speaking - is it correct example of where i should be using classes at all?

I'm not concerned with the looks of it but with functionality. Thanks for your help.

const outp = document.getElementById('err');

class data{
  constructor(email, url, number){
    this.email = email;
    this.url = url;
    this.number = number;
  }
  static append(){
    var par = document.createElement('P');
    var em = document.createTextNode(email.value);
    var ur = document.createTextNode(url.value);
    var num = document.createTextNode(number.value);
    par.appendChild(em);
    par.appendChild(ur);
    par.appendChild(num);
    document.body.appendChild(par);
  }
}

document.getElementById('submit').addEventListener('click', () => {
  outp.innerHTML = "";
  let email = document.getElementById('email').value;
  let url = document.getElementById('url').value;
  let number = document.getElementById('number').value;
  try{
    if(email == '' || url == '' || number == '' ) throw "Empty";
    data.append();
  }
  catch(err){
    outp.innerHTML = err;
  }
})
<!DOCTYPE html>

    <meta charset="utf-8">
    <link type="text/css" rel="stylesheet" href="style.css" />
    <title></title>

        Email
        <input type="email" id="email" required />

        Url
        <input type="url" id="url" required />

        Number
        <input type="number" id="number" required />

      <button type="button" id="submit">Submit</button>

The convention with class names is that they are usually written in pascal case, for example, PascalCase. So you should use Data for your class name.

While it's great that you're experimenting with javascript classes, I feel that they're not the right tool to use for this. The main idea of a class is that they can be used to create multiple objects based off the class.

Here's a pseudo-code example of when using a class is ideal.

class Vehicle {
    constructor(model, regNumber) {
        this.model = model;
        this.regNumber = regNumber;
    }

    start() {}
    accelerate(acc) {}
    brake() {}
}

class Car extends Vehicle {
    // additional methods
}

const ford = new Car('Ford', 'HR28AP0000');
const auto = new Vehicle('ExampleDX', 'TN09XX0000');

The vehicle class has some attributes and methods. This example lends itself very well to the idea of classes.

For your code, it's best to use an append function instead.

I'd suggest looking up Object Oriented Programming. Some well written books and blogs will do a much better job of explaining when and where to use classes and objects and other OOP principles.

Edit: About try-catch, it's common to throw an Error object or one of it's descendants.

if(email == '' || url == '' || number == '' ) {
    throw new Error("Empty");
}


Testing if the binary representation of an integer has an odd number of ones The fn odd_ones(x: u32) -> bool function should return true when x contains a odd number of 1s.

Assumption:

x is 32 bit unsigned.

Restriction:

The code should contain a total of at most 12 arithmetic, bitwise and logical operations.

Forbidden:

Conditionals, loops, function calls and macros.
Division, modulus and multiplication.
Relative comparison operators (<, >, <= and >=).

Allowed operations:

All bit level and logic operations.
Left and right shifts, but only with shift amounts between 0 and w - 1
Addition and subtraction.
Equality (==) and inequality (!=) tests.
Casting between u32 and i32.

My code: (Rust playground)

fn odd_ones(x: u32) -> bool {
   let mid = (x >> 16) ^ x;
   let mid2 = (mid >> 8) ^ mid;
   let mid3 = (mid2 >> 4) ^ mid2;
   let mid4 = (mid3 >> 2) ^ mid3;

   (((mid4 >> 1) ^ mid4) & 1) == 1
}

fn odd_ones_test(x: u32) -> bool {
    let sum = (0..32).map(|y| x >> y )
                     .fold(0, |sum ,y| sum + (y & 1));
    sum % 2 != 0
}

fn test (from: u32, to: u32) -> bool {
    (from..to).all(|x| odd_ones_test(x) == odd_ones(x))
}

fn main() {
    println!("{}", test(!0 - 45345, !0));
}

Any better way of doing this?

Leveraging code that other people wrote is always a great idea. In this case, there is count_ones:

fn odd_ones(x: u32) -> bool {
    x.count_ones() % 2 == 1
}

This method is a shim for the LLVM intrinsic @llvm.ctpop.i32, which seems likely to become the very optimized SSE4.2 popcnt instruction when available.

Beyond that, you have some poor indentation habits. Rust uses 4-space indents, not the 3 (?!?) spaces you have in your odd_ones function.

There should not be a space between a function name and the arguments, whether in the function definition or the function call. Your test method should be fixed.

Be consistent with your spacing on commas. No space before, one space after. |sum ,y| is just wrong.

You should give names to magic constants. A rogue 32 laying around doesn't mean anything. Give it a name like BITS_IN_U32. There's an unstable constant that you might be able to use someday.

When mapping and folding over an iterator, you might as well put all the mapping into the map call. There's no reason to do the bitwise-and in the fold.

I have no idea what !0 - 45345 is supposed to mean or why those particular values are useful. That's bad news when you try to understand this code in the future.

const BITS_IN_U32: usize = 32;

fn odd_ones(x: u32) -> bool {
    x.count_ones() % 2 == 1
}

fn odd_ones_test(x: u32) -> bool {
    let sum =
        (0..BITS_IN_U32)
        .map(|y| (x >> y) & 1)
        .fold(0, |sum, y| sum + y);
    sum % 2 != 0
}

fn test(from: u32, to: u32) -> bool {
    (from..to).all(|x| odd_ones_test(x) == odd_ones(x))
}

fn main() {
    println!("{}", test(!0 - 45345, !0));
}


Return the amount of Quarters, Dimes, Nickels, and Pennies that you would get from the total amount of change I made windows form in C# that takes the amount of change given, 1-99 and tells you the amount of quarters, dimes, nickels, and pennies you would get back. Can I improve the math in any way? It takes the total change and subtracts the value of the largest coin value that applies till the change becomes equal to zero.

        //set the variables
        int quarters = 0;
        int dimes = 0;
        int nickels = 0;
        int pennies = 0;
        int change = Convert.ToInt32(txtChange.Text);

        //logic
        do
        {
            if (change >= 25 && change <= 99)
            {
                change = change - 25;
                quarters++;
            }
            else if (change <= 24 && change > 9)
            {
                change = change - 10;
                dimes++;
            }
            else if (change == 5)
            {
                change = change - 5;
                nickels++;
            }
            else if (change <=4)
            {
                change = change - 1;
                pennies++;
            }

            txtQuarters.Text = quarters.ToString("n");
            txtDimes.Text = dimes.ToString("n");
            txtNickels.Text = nickels.ToString("n");
            txtPennies.Text = pennies.ToString("n");

        } while (change > 0);

Your loop only adds one coin to the change every repetition, which can be done faster: you don;t need the else if.

You don't want to set your Text properties inside your loop, but only when you're finished.

Also, you want to check if change > 99 before starting, otherwise your loop never ends.

Instead of

something = something + x;

you can use

something += x;

Once you're down to pennies, the number of pennies is your remaining change.

Instead of adding one coin every time, simply add all the quarters possible, then all the dimes, etcetera:

if (change > 99) throw new ArgumentOutOfRangeException("Too much change!");

while (change > 24)
{
    change -= 25;
    quarters++;
}
while (change > 9)
{
    change -= 10;
    dimes++;
}
while (change > 4)
{
    change -= 5;
    nickels++;
}

pennies = change;

Now, you can actually do away with all the loops very simply, by dividing the remaining change by the size of your next coin. The resulting integer is the amount of coins.

Then you get:

int quarters = change/25;
change -= quarters*25;
int dimes = change/10;
change -= dimes*10;
int nickels = change/5;
int pennies = change - nickels * 5;

Even shorter, using the % (modulo) operator:

int quarters = change/25;
change %= 25;
int dimes = change/10;
change %= 10;
int nickels = change/5;
int pennies = change%5;


Shorten code to perform search based on four slash-delimited parameters How can I make this code more compact?

It is a ASP.net WebForms project

It is a SinglePageApplication for searching in list mode and map mode.

I have many paths for a page.

/Denmark
/Denmark/Copenhagen
/Denmark/Copenhagen/Christiansborg
Map/Denmark
Map/Denmark/Copenhagen
Map/Denmark/Copenhagen/Christiansborg

in which that control the UI and parameteres passed to a JavaScript function

string parameters = (string)Page.RouteData.Values["params"],
    param1 = string.Empty,
        param2 = string.Empty,
            param3 = string.Empty,
                param4 = string.Empty;

if (!string.IsNullOrWhiteSpace(parameters))
{
    var parameterArray = parameters.Split('/');
    var i = 0;
    while (i < parameterArray.Length)
    {
        if (i == 3)
            param4 = parameterArray[3];

        if (i == 2)
            param3 = parameterArray[2];

        if (i == 1)
            param2 = parameterArray[1];

        if (i == 0)
            param1 = parameterArray[0];

        i++;
    }
}

SearchState = new RoomSearchState(param1, param2, param3, param4);

And the RoomStateSearch

public RoomSearchState(string param1, string param2, string param3, string param4)
{
    if (!string.IsNullOrWhiteSpace(param1))
    {
        if (param1.Equals("Kort", StringComparison.InvariantCultureIgnoreCase))
        {
            ViewType = "Map";

            if (!string.IsNullOrWhiteSpace(param2) && !string.IsNullOrWhiteSpace(param3) && !string.IsNullOrWhiteSpace(param4))
            {
                Country = param2.ToUnfriendlyUrl();
                Area = param3.ToUnfriendlyUrl();
                SubArea = param4.ToUnfriendlyUrl();
                SearchType = SearchStateType.Address;
            }
            else if (!string.IsNullOrWhiteSpace(param2) && !string.IsNullOrWhiteSpace(param3))
            {
                SearchType = param2.StartsWith("@") ? SearchStateType.Coordinates : SearchStateType.Address;
                Area = param3.ToUnfriendlyUrl();
                Country = param2.ToUnfriendlyUrl();
            }
            else if (!string.IsNullOrWhiteSpace(param2) && param2.StartsWith("@"))
            {
                SearchType = SearchStateType.Coordinates;
                Area = param2;
            }
            else if (!string.IsNullOrWhiteSpace(param2))
            {
                SearchType = SearchStateType.Address;
                Area = param2.ToUnfriendlyUrl();
            }
            else
            {
                SearchType = SearchStateType.Empty;
            }
        }
        else
        {
            ViewType = "Default";

            if (!string.IsNullOrWhiteSpace(param1) && !string.IsNullOrWhiteSpace(param2) && !string.IsNullOrWhiteSpace(param3))
            {
                Country = param1.ToUnfriendlyUrl();
                Area = param3.ToUnfriendlyUrl();
                SearchType = SearchStateType.Address;
                SubArea = param2.ToUnfriendlyUrl();
            }
            else if (!string.IsNullOrWhiteSpace(param1) && !string.IsNullOrWhiteSpace(param2))
            {
                Area = param2.ToUnfriendlyUrl();
                SearchType = SearchStateType.Address;
                Country = param1.ToUnfriendlyUrl();
            }
            else if (!string.IsNullOrWhiteSpace(param1) && param1.StartsWith("@"))
            {
                SearchType = SearchStateType.Coordinates;
                Area = param1;
            }
            else if (!string.IsNullOrWhiteSpace(param1))
            {
                SearchType = SearchStateType.Address;
                Area = param1.ToUnfriendlyUrl();
            }
            else
            {
                SearchType = SearchStateType.Empty;
            }
        }
    }
    else
    {
        SearchType = SearchStateType.Empty;
    }
}

Code Structure

It is quite tedious what you trying to do in the first piece of code. You might as well just pass the string array directly. As accessing an indexed element is not much different than variable named like paramx.
The second piece of code is mirrored from the middle. The lower section and upper section are essentially doing the same job with only 1 offset in parameter, and ViewType being different.
Never repeatedly make the same test, if possibly. Try to process by reduction and elimination, or change the flow of code.

Logic Flaws

When only one argument or two arguments (that begins with "Map" or  "Kort") are passed, then Area is being assigned instead of Country.

else if (!string.IsNullOrWhiteSpace(param1))
{
    SearchType = SearchStateType.Address;
    Area = param1.ToUnfriendlyUrl();
}

When 3 or 4(with Map/Kort) arguments are passed, SubArea is assigned before Area.

if (!string.IsNullOrWhiteSpace(param1) && !string.IsNullOrWhiteSpace(param2) && !string.IsNullOrWhiteSpace(param3))
{
    Country = param1.ToUnfriendlyUrl();
    Area = param3.ToUnfriendlyUrl();
    SearchType = SearchStateType.Address;
    SubArea = param2.ToUnfriendlyUrl();
}

Polished Code

SearchState = new RoomSearchState(parameters.Split('/'))

public RoomSearchState()
{
    SearchType = SearchStateType.Empty;
    ViewType = "Default";
}

public RoomSearchState(string[] args) : this()
{
    if (!args.Any()) return;

    if (args[0] == "Kort" || args[0] == "Map")
    {
        ViewType = "Map";
        args = args.Skip(1).ToArray();
    }

    switch(args.Length)
    {
        case 1:
            if (args[0].StartsWith("@"))
            {
                Country = args[0];
                SearchType = SearchStateType.Coordinates;
            }
            else
            {
                Country = args[0].ToUnfriendlyUrl();
                SearchType = SearchStateType.Address;
            }
            break;
        case 2:
            Country = args[0].ToUnfriendlyUrl();
            Area = args[1].ToUnfriendlyUrl();
            SearchType = SearchStateType.Address;
            break;
        case 3:
            Country = args[0].ToUnfriendlyUrl();
            Area = args[1].ToUnfriendlyUrl();
            SubArea = args[2].ToUnfriendlyUrl();
            SearchType = SearchStateType.Address;
            break;
        default: throw new ArgumentException("args");
    }
}