Kotlin shared preferences So I've written a class for storing user/device specific settings, but I've written the Kotlin pretty much like I would in Java:

import android.content.Context
import android.content.SharedPreferences
import java.util.*

class UserSettings {

    private lateinit var _prefs :SharedPreferences

    fun UserSettings(ctx : Context) {
        _prefs = ctx.getSharedPreferences("UserSettings", Context.MODE_PRIVATE)
    }

    private val KEY_DEVICEID = "deviceId"
    var deviceId :String
        get() {
            var value = _prefs.getString(KEY_DEVICEID, null)
            if (null == value) {
                value = UUID.randomUUID().toString()
                this.deviceId = value
            }
            return value
        }
        set(it) {
            _prefs.edit()
                .putString(KEY_DEVICEID, it)
                .apply()
        }

}

This feels really verbose. I've considered writing a helper library to simply store all values from a class in SharedPreferences via reflection, but I'm not sure if that's overkill. The above code obviously only has one value at the moment, but I'll be adding a couple more that will look very similar to the above.

Welcome to Kotlin! Consider this.

// Define constructor this way
class UserSettings(context: Context) {
    // Prefixes for fields are rather uncommon in the Kotlin world.
    // You could remove the type declaration here - Kotlin will infer it for you
    private val prefs: SharedPreferences = context
        .getSharedPreferences("UserSettings", Context.MODE_PRIVATE)

    var deviceId: String
        get() = prefs.getString(KEY_DEVICE_ID, null) ?: UUID.randomUUID().toString().also {
            // `also` block will execute only in case we return a new UUID.
            deviceId = it
        }
        set(it) {
            prefs
                .edit()
                .putString(KEY_DEVICE_ID, it)
                .apply()
        }

    companion object {
        // Depends on a taste / convention your team uses.
        // I like keeping my key constants within the companion object.
        private const val KEY_DEVICE_ID = "deviceId"
    }
}

For simplifying it even more, consider using Android KTX - it has some handy SharedPreferences extensions, described here: https://developer.android.com/kotlin/ktx


Multithreaded password cracker I am learning infosec with DVWA (Damn Vulnerable Web Application). At first I've decided to write something to brute force admin login screen. I've downlaoded a list of most commonly used passwords and created a script which takes them and attempts to log in. What do you think of it, what could be improved?

#!/usr/bin/env python3

import threading
import requests

URL = 'http://localhost/login.php'
PASSWORD_FILE_NAME = 'common-passwords.txt'
entry_found = False

def create_threads(passwords):
    password_list_split_points = [
        (0, len(passwords) // 4),
        (len(passwords) // 4 + 1, len(passwords) // 2),
        (len(passwords) // 2 + 1, 3 * (len(passwords) // 4)),
        (3 * (len(passwords) // 4) + 1, len(passwords) - 1),
    ]
    thread_list = [threading.Thread(
        target=run_cracker,
        args=(
            passwords[split_point[0] : split_point[1]]
        )
    ) for split_point in password_list_split_points]
    return thread_list

def run_cracker(*passwords):
    global entry_found
    for password in passwords:
        if entry_found:
            break
        # Passwords still contain last \n char which has to be stripped.
        if crack_password(password.rstrip()):
            # This is set to True only once. No need for sync mechanisms.
            entry_found = True

def crack_password(password):
    print('[*] Trying password: "{}" ...'.format(password))
    response = requests.post(
        URL,
        data={'username': 'admin', 'Login': 'Login', 'password': password}
    )

    if bytes('Login failed', encoding='utf-8') not in response.content:
        print('[*] Login successful for username: {} password: {}'.format(
            'admin', password
        ))
        return True
    else:
        return False

if __name__ == '__main__':
    with open(PASSWORD_FILE_NAME) as password_file:
        passwords = password_file.readlines()

    thread_list = create_threads(passwords)

    for thread in thread_list:
        print('[*] Running thread: {}.'.format(thread.getName()))
        thread.start()

    for thread in thread_list:
        print('[*] Wating for {} to join.'.format(thread.getName()))
        thread.join()

This seems to me like your create_threads and run_cracker functions are trying to reinvent something that look like multiprocessing.Pools. Using them, you just need to implement crack_password, managing the workload and spawning processes will be done by the pool.

This will however not short-circuit once a working password has been found.

Example implementation:

from multiprocessing import Pool

import requests

def crack_password(password, url='http://localhost/login.php'):
    response = requests.post(
        url,
        data={'username': 'admin', 'Login': 'Login', 'password': password}
    )

    if bytes('Login failed', encoding='utf-8') not in response.content:
        return password

def main(passwords_filename='common-passwords.txt'):
    with open(passwords_filename) as passwords_file:
        passwords = passwords_file.readlines()

    with Pool(4) as pool:
        results = pool.map(crack_password, passwords)
        success = list(filter(None, results))
    print(success)

if __name__ == '__main__':
    main()


Get line from string function I have asked a previous question but at the last moment I realized that the code does not work as intended as it has some small bugs.

I got some ideas for improvement though, but they didn't seem to work as well, so I decided to re-write it. This time, I was following the logic all the time.

char *tbl_getline (char *source, size_t line)
{
    size_t current_line = 1, columns;
    size_t iter;

    for(iter = 0; source[iter] != '\0' && current_line != line; iter++)
        if(source[iter] == '\n') current_line++;
    for(columns = 0; source[iter + columns] != '\n' && source[iter + columns] != '\0'; columns++);

    char *buff = malloc(columns + 1);
    memcpy(buff, source + iter * sizeof(char), columns);
    buff[columns] = '\0';
    return(buff);
}

And that was the code able to return a line from a string in a relatively good speed.

size_t current_line = 1, columns;

It's generally better to either initialize every variable or none in a declaration.  This is especially so since you declare another variable without initialization on the next line.

size_t iter;

This use of iter is ... uncommon.  The name iter has a very specific meaning.  I would have expected this to be an iterator.  Instead, it's just a normal index variable.  You seem to be using it as a generic name for a variable used to iterate in a for loop, but there already is a standard variable for that:  i.

for(iter = 0; source[iter] != '\0' && current_line != line; iter++)

So as I read that, if current_line is greater than line, you should keep on incrementing until the program crashes or we loop around?  Admittedly that should never happen, but this code is not robust in the face of a change that would make it do so.

memcpy(buff, source + iter * sizeof(char), columns);

The iter * sizeof(char) is incorrect.  It will multiply iter by sizeof(char) twice:  once, explicitly; a second time, implicitly, for the pointer arithmetic.  The correct way to write this is just source + iter.  This probably works here, because sizeof(char) is usually 1.  It's highly vulnerable to someone using a multibyte char though.  And of course, it would give the wrong result for almost any other data type.


Entity Framework 7 In Memory Database - Unit Testing using xUnit I'm using EF7 with In-Memory database and xUnit for repository testing. I'd like to know if it's a good practice initializing the database in this manner for reuse it in every test method, specifically through the Initialize() method:

private Context _context;
private BrandsRepository _repository;
private void Initialize()
{
    var db = new DbContextOptionsBuilder();
    db.UseInMemoryDatabase();
    _context = new Context(db.Options);
    _repository = new BrandsRepository(_context);

    _context.Database.EnsureDeleted();
    _context.Database.EnsureCreated();
}

[Fact]
public void Retrieve_Brands()
{
    // Arrange
    Initialize();
    var brands = new List<Brand>()
    {
        new Brand() { Name = "First" },
        new Brand() { Name = "Second" },
        new Brand() { Name = "Third" }
    };
    _context.Brands.AddRange(brands);
    _context.SaveChanges();

    // Act
    var retrievedBrands = _repository.GetAll();

    // Assert
    Assert.Equal(retrievedBrands, brands);
}

[Fact]
public void Add_a_Brand()
{
    // Arrange
    Initialize();
    var brand = new Brand() { Name = "First" };

    // Act
    _repository.Add(brand);

    // Assert
    Assert.Equal(_context.Brands.First(), brand );
}

I don't know xUnit, but in other testing frameworks you can reuse a single in-memory database in multiple test cases, by annotating the text cases (or rather, the entire test class) transactional. That is, the database manipulations within the test cases will be automatically rolled back after leaving the scope of the methods. That way tests can run faster, thanks to not recreating a full text database repeatedly. I would hope a similar concept exists in xUnit too, and research that.

If no such feature exists, your approach seems fine, but Initialize is a bit too generic of a name, I would call it RecreateDatabase instead to make it more clear what it does. Or, actually, other frameworks use annotation like @Before for this kind of purpose, instead of an explicit method call.

Lastly, the test in the second example is a bit weak. You only get that the first item is what you inserted. I suggest to either add a test to verify that there is only 1 item, or require it using GetAll like you did in the other test case.


Building XML elements from a list using LINQ and foreach I wrote the following code:

var xmlArray = from m in (from row in data select row.Mitarbeiter).Distinct()
               select "<Value Type='Text'>" + m + "</Value>";
var xml = string.Join("",xmlArray);

Then I noticed there are two iteration (two from) and also the Distinct() and I rewrote it using a foreach:

var mitarbeiter = new List<string>();
var xmlArray = new List<string>();
foreach(var row in data)
{
    if (!mitarbeiter.Contains(row.Mitarbeiter)){
        mitarbeiter.Add(row.Mitarbeiter);
        xmlArray.Add("<Value Type='Text'>" + row.Mitarbeiter + "</Value>");
    }
}
var xml = string.Join("",xmlArray);

Ma opinion is that the first example is cleaner...but what about efficiency/performance?

The best way to discuss performance is by measuring it. I suggest you use BenchmarkDotNet and never roll your own solution.

Onto your question: it is safe to assume that LINQ will in most cases be slower than the iterative approach. Not only does it allocate more (for example through lambdas) but it's also more expensive in runtime because of interface dispatching and other aspects.

There is obviously a balance to be struck between readability and performance -- if you have no performance issues then LINQ might very well be the best option although I would argue that in this case the iterative approach is very readable as well (if not more).

Something you can consider: use a HashSet<T> instead of a List<T> if your usecase allows it. Performing Contains() is O(n) for a list but O(1) for a Set.


Cythonized Sutherland-Hogman algorithm I want to cythonise the python implementation of the Sutherland-Hogman algorithm. This algorithm updates a list of vertices according to pretty simple rules (being inside or outside an edge, etc.) but the details are not important. Here is the python version it accepts lists of vertices of polygons oriented clockwise. For instance those:

sP=[(50 150),  (200 50),  (350 150), (350 300), (250 300), (200 250), (150 350)(100 250) (100 200)]
cP=[(100, 100), (300, 100), (300, 300), (100, 300)]

and calculate their intersection:

inter=clip(sP, cP)

Here is the code found on rosettacode and slightly modified to return an empty list if there is no intersection.

def clip(subjectPolygon, clipPolygon):
   def inside(p):
      return(cp2[0]-cp1[0])*(p[1]-cp1[1]) > (cp2[1]-cp1[1])*(p[0]-cp1[0])

   def computeIntersection():
      dc = [ cp1[0] - cp2[0], cp1[1] - cp2[1] ]
      dp = [ s[0] - e[0], s[1] - e[1] ]
      n1 = cp1[0] * cp2[1] - cp1[1] * cp2[0]
      n2 = s[0] * e[1] - s[1] * e[0]
      n3 = 1.0 / (dc[0] * dp[1] - dc[1] * dp[0])
      return [(n1*dp[0] - n2*dc[0]) * n3, (n1*dp[1] - n2*dc[1]) * n3]

   outputList = subjectPolygon
   cp1 = clipPolygon[-1]

   for clipVertex in clipPolygon:
      cp2 = clipVertex
      inputList = outputList
      outputList = []
      s = inputList[-1]

      for subjectVertex in inputList:
         e = subjectVertex
         if inside(e):
            if not inside(s):
               outputList.append(computeIntersection())
            outputList.append(e)
         elif inside(s):
            outputList.append(computeIntersection())
         s = e
      if len(outputList)<1:
          return []
      cp1 = cp2
   return(outputList)

This function is excruciatingly slow for my applications so I tried to cythonize it using numpy. Here is my cython version. I had to define the two functions outside clip because I had error messages about buffer inputs.

cimport cython
import numpy as np
cimport numpy as np

def clip(np.ndarray[np.float32_t, ndim=2] subjectPolygon,np.ndarray[np.float32_t, ndim=2] clipPolygon):

    outputList = list(subjectPolygon)
    cdef np.ndarray[np.float32_t, ndim=1] cp1 = clipPolygon[-1,:]
    cdef np.ndarray[np.float32_t, ndim=1] cp2

    for i in xrange(clipPolygon.shape[0]):
       cp2 = clipPolygon[i]
       inputList = outputList
       outputList = []
       s = inputList[-1]

       for subjectVertex in inputList:
          e = subjectVertex
          if inside(e, cp1, cp2):
             if not inside(s, cp1, cp2):
                outputList.append(computeIntersection(cp1, cp2, e, s))
             outputList.append(e)
          elif inside(s, cp1, cp2):
             outputList.append(computeIntersection(cp1, cp2, e, s))
          s = e
       if len(outputList)<1:
         return []
       cp1 = cp2

    return(outputList)

def computeIntersection(np.ndarray[np.float32_t, ndim=1] cp1, np.ndarray[np.float32_t, ndim=1] cp2, np.ndarray[np.float32_t, ndim=1] e, np.ndarray[np.float32_t, ndim=1] s):
    cdef np.ndarray[np.float32_t, ndim=1] dc = cp1-cp2
    cdef np.ndarray[np.float32_t, ndim=1] dp = s-e
    cdef np.float32_t n1 = cp1[0] * cp2[1] - cp1[1] * cp2[0]
    cdef np.float32_t n2 = s[0] * e[1] - s[1] * e[0]
    cdef np.float32_t n3 = 1.0 / (dc[0] * dp[1] - dc[1] * dp[0])
    cdef np.ndarray[np.float32_t, ndim=1] res=np.array([(n1*dp[0] - n2*dc[0]) * n3, (n1*dp[1] - n2*dc[1]) * n3], dtype=np.float32)
    return res

def inside(np.ndarray[np.float32_t, ndim=1] p, np.ndarray[np.float32_t, ndim=1] cp1, np.ndarray[np.float32_t, ndim=1] cp2):

    cdef bint b=(cp2[0]-cp1[0])*(p[1]-cp1[1]) > (cp2[1]-cp1[1])*(p[0]-cp1[0])
    return b

When I time the two versions I gained only a factor of two in speed-up I need at least 10 times that (or 100x !). Is there something to do ?
How does one deal with variable sized list with Cython ?
I do not know if this could be useful but my inputs are of fixed length.

One important thing to mention - if you're cythoning a for loop through a range, you need to cdef the type of the index.

def pointless_add_slow(j):
     for i in range(120000):
           i = i+j
     return i

%timeit pointless_add_slow(1)
100 loops, best of 3: 3.71 ms per loop

New block in jupyter:

%%cython
def pointless_add(j):
     cdef int i
     for i in range(120000):
           i = i+j
     return i

%timeit pointless_add(1)
100 loops, best of 3: 1.98 ms per loop


Prepared statements in PHP I just heard of prepared statement in PHP and decided to prevent SQL injection with it and wrote a script testing it.

I would like to know what security threats this script can prevent, what security threats this script is vulnerable to, and how these threats are blocked.

<?php
require_once ("db.php");
$db = new MyDB();
session_start();

if (isset($_POST['userMsgField']) && !empty($_POST['userMsgField']) || isset($_POST['hash']) && !empty($_POST['hash']))
{
$my_id = $_SESSION['log_id'];
$rep_msg = $_POST['userMsgField'];
$hash = $_SESSION['hash'];
$flag = 0;

$sql =<<<EOF
SELECT * FROM connect WHERE (user_one = '$my_id' AND hash = '$hash') OR (user_two = '$my_id' AND hash = '$hash');
EOF;

$ret = $db->query($sql);

while ($row = $ret->fetchArray(SQLITE3_ASSOC))
{
    $user_one = $row['user_one'];
    $user_two = $row['user_two'];

    if ($user_one == $my_id)
    {
        $to_id = $user_two;
    }
    else
    {
        $to_id = $user_one;
    }

    $isql =<<<EOF
    INSERT INTO messager (message, group_hash, from_id, flag, to_id) VALUES (:message, :group_hash, :from_id, :flag, :to_id);
EOF;
    $bsql =<<<EOF
    INSERT INTO chatportal (message, group_hash, from_id, flag, to_id)
VALUES (:message, :group_hash, :from_id, :flag, :to_id);
EOF;

    $stmt = $db->prepare($isql);
    $bstmt = $db->prepare($bsql);

    $stmt->bindValue(':message', $rep_msg, SQLITE3_TEXT);
    $stmt->bindValue(':group_hash', $hash, SQLITE3_INTEGER);
    $stmt->bindValue(':from_id', $my_id, SQLITE3_INTEGER);
    $stmt->bindValue(':flag', $flag, SQLITE3_INTEGER);
    $stmt->bindValue(':to_id', $to_id, SQLITE3_TEXT);

    $bstmt->bindValue(':message', $rep_msg, SQLITE3_TEXT);
    $bstmt->bindValue(':group_hash', $hash, SQLITE3_INTEGER);
    $bstmt->bindValue(':from_id', $my_id, SQLITE3_INTEGER);
    $bstmt->bindValue(':flag', $flag, SQLITE3_INTEGER);
    $bstmt->bindValue(':to_id', $to_id, SQLITE3_TEXT);

    $result = $stmt->execute();
    $bresult = $bstmt->execute();

    if ($reuslt && $bresult)
    {

    }
}
}

Non-review answers

what security threats this script can prevent - sql injection
what security threats this script is vulnerable to - sql injection
how these threats are blocked - https://stackoverflow.com/questions/8263371/how-can-prepared-statements-protect-from-sql-injection-attacks

Review answer.

Your code is bloated with unnecessary operators, such as heredoc.
It is also recommended to use PDO instead of sqlite, as it will let you to use array binding, which is especially useful in this case as you are inserting equal sets of data.

Besides, you should use prepared statements for your database interactions.

There are also other improvements and fixes.

if (!empty($_POST['userMsgField']) && !empty($_POST['hash']))
{
    $my_id = $_SESSION['log_id'];
    $rep_msg = $_POST['userMsgField'];
    $hash = $_POST['hash'];
    $flag = 0;

    $sql = "SELECT * FROM connect WHERE (user_one = ? AND hash = ?)
                                     OR (user_two = ? AND hash = ?)";
    $stmt = $db->prepare($sql);
    $stmt->execute([$my_id,$hash,$my_id,$hash]);

    while ($row = $stmt->fetch(PDO::FETCH_ASSOC))
    {
        $user_one = $row['user_one'];
        $user_two = $row['user_two'];

        $to_id = ($user_one == $my_id) ? $user_two : $user_one;

        $data = [
            'message' => $rep_msg,
            'group_hash' => $hash,
            'from_id' => $my_id,
            'flag' => $flag,
            'to_id' => $to_id,
        ];

        $sql ="INSERT INTO messager (message, group_hash, from_id, flag, to_id)
                            VALUES (:message,:group_hash,:from_id,:flag,:to_id)";
        $db->prepare($sql)->execute($data);

        $sql ="INSERT INTO chatportal (message, group_hash, from_id, flag, to_id)
                              VALUES (:message,:group_hash,:from_id,:flag,:to_id)";
        $db->prepare($sql)->execute($data);
    }
}

Now this code is much cleaner and easier to read. And safe, of course.


How to copy and paste a filtered range more efficiently I have this bit of code that works for copy and pasting an autofiltered range. I just wanted to see if anyone knew a more efficient way to do so as it does take quite a while to complete it the way I have it now. I looked into trying to do so with a copying into an array but did not seem to work so I'm happy to hear some of your suggestions! Also I have to be sure that the range I am copying and pasting is not the full data range and only the range after the filter was applied.
Sub FullWork()

Dim shWorkBook As Worksheet
Dim shPivotTable As Worksheet
Dim shCarrierRates As Worksheet
Dim shWholesaleLocation As Worksheet
Dim shPWLocation As Worksheet
Dim wkb As Workbook
Dim shSegmentation As Worksheet
Dim MilageMatrix As Worksheet
Dim FullYearData As Worksheet
Dim StartForm As Worksheet
Set wkb = ThisWorkbook
With wkb

Set shWorkBook = .Sheets("Workbook")
Set shPivotTable = .Sheets("PivotTable")
Set shCarrierRates = .Sheets("CarrierRates")
Set shWholesaleLocation = .Sheets("WholesalerLocation")
Set shPWLocation = .Sheets("PWLocation")
Set shSegmentation = .Sheets("Segmentation")
Set shMilageMatrix = .Sheets("MilageMatrix")
Set shFullYearData = .Sheets("FullYearData")
Set shStartForm = .Sheets("StartForm")

End With

Dim DataRange As Range
Dim PWSelection As Variant
Dim LastRow As Long
Dim LastColumn As Long
Dim LocationZip As Variant
Dim dTime As Double

dTime = Timer

Call TurnOffCalc

Set StartCell = shFullYearData.Range("A1")

'Find Last Row and Column

LastRow = shFullYearData.Cells(shFullYearData.Rows.Count, StartCell.Column).End(xlUp).Row
LastColumn = shFullYearData.Cells(StartCell.Row, shFullYearData.Columns.Count).End(xlToLeft).Column

shFullYearData.Activate
shFullYearData.Range(StartCell, shFullYearData.Cells(LastRow, LastColumn)).Select

PWSelection = shStartForm.Cells(1, 1).Value

shFullYearData.Range(shFullYearData.Cells(1, 1), shFullYearData.Cells(1, LastColumn)).AutoFilter
If PWSelection <> "All" Then
shFullYearData.Range(StartCell, shFullYearData.Cells(LastRow, LastColumn)).AutoFilter Field:=12,         Criteria1:=PWSelection
End If
shFullYearData.Range(StartCell, shFullYearData.Cells(LastRow, LastColumn)).AutoFilter Field:=26, Criteria1:=">=1"
shFullYearData.Range(StartCell, shFullYearData.Cells(LastRow, LastColumn)).AutoFilter Field:=3, Operator:= _
xlFilterValues, Criteria2:=Array(0, "12/31/2019")
shFullYearData.Range(StartCell, shFullYearData.Cells(LastRow, LastColumn)).Sort Key1:=Range("G1"), Order1:=xlAscending, Header:=xlYes, Key2:=Range("B1"), Header:=xlYes

''''''Line I need looked at
shFullYearData.Range(StartCell, shFullYearData.Cells(LastRow, LastColumn)).Copy Destination:=shWorkBook.Range("A1")

call TurnOnCalc
end sub

The code as you have it is not able to be executed but once I made some revisions it seems to run just fine. On my test dataset I had no problem with performance, but if you continue to have performance issues after making edits, I would look into something called "Advanced Filter" for Excel, as it has the ability to set a destination at the onset for the filtered data to appear. You can simply look up "Advanced Filter" on your favorite search engine and find links like these, there are hundreds to pick from:

https://www.thespreadsheetguru.com/blog/2015/2/16/advanced-filters-with-vba-to-automate-filtering-on-and-out-specific-values
https://www.youtube.com/watch?v=0YNhxVu2a5s

As for my revisions to your code, see below. I've tried to comment every line I changed and change the minimum amount of lines possible.
Option Explicit

Sub FullWork()

Dim rngFullYearData  ' added to be able to reference easier later
'Dim DataRange As Range ' moved to keep all Dim statements together  ' commented out because it was not being used
Dim StartCell As Range  ' added to be able to Set later
Dim PWSelection As Variant
Dim LastRow As Long
Dim LastColumn As Long
'Dim LocationZip As Variant  ' commented out because it was not being used
'Dim dTime As Double  ' commented out because it was not being used

Dim shWorkBook As Worksheet
'Dim shPivotTable As Worksheet  ' commented out because it was not being used
'Dim shCarrierRates As Worksheet  ' commented out because it was not being used
'Dim shWholesaleLocation As Worksheet  ' commented out because it was not being used
'Dim shPWLocation As Worksheet  ' commented out because it was not being used
'Dim shSegmentation As Worksheet  ' commented out because it was not being used
Dim wkb As Workbook
'Dim shMilageMatrix As Worksheet  ' added sh to beginning to stay consistent  ' commented out because it was not being used
Dim shFullYearData As Worksheet  ' added sh to beginning to stay consistent
Dim shStartForm As Worksheet  ' added sh to beginning to stay consistent

Set wkb = ThisWorkbook
With wkb  ' indenting with block for ease of readability
    Set shWorkBook = .Sheets("Workbook")
    'Set shPivotTable = .Sheets("PivotTable")  ' commented out because it was not being used
    'Set shCarrierRates = .Sheets("CarrierRates")  ' commented out because it was not being used
    'Set shWholesaleLocation = .Sheets("WholesalerLocation")  ' commented out because it was not being used
    'Set shPWLocation = .Sheets("PWLocation")  ' commented out because it was not being used
    'Set shSegmentation = .Sheets("Segmentation")  ' commented out because it was not being used
    'Set shMilageMatrix = .Sheets("MilageMatrix")  ' commented out because it was not being used
    Set shFullYearData = .Sheets("FullYearData")
    Set shStartForm = .Sheets("StartForm")
End With

'dTime = Timer  ' commented out because it was not being used

'Call TurnOffCalc  ' commenting out because sub was not included

Set StartCell = shFullYearData.Range("A1")

'Find Last Row and Column

LastRow = shFullYearData.Cells(shFullYearData.Rows.Count, StartCell.Column).End(xlUp).Row
LastColumn = shFullYearData.Cells(StartCell.Row, shFullYearData.Columns.Count).End(xlToLeft).Column

'shFullYearData.Activate  ' no need to activate if not selecting below
Set rngFullYearData = shFullYearData.Range(StartCell, shFullYearData.Cells(LastRow, LastColumn))  ' remove select, add set instead

PWSelection = shStartForm.Cells(1, 1).Value

With rngFullYearData ' simplify with new variable, add with block for easier referencing
    .AutoFilter
    If PWSelection <> "All" Then
        .AutoFilter Field:=12, Criteria1:=PWSelection  ' using new variable
    End If
    .AutoFilter Field:=26, Criteria1:=">=1"  ' using new variable
    .AutoFilter Field:=3, Operator:= _
    xlFilterValues, Criteria2:=Array(0, "12/31/2019")  ' using new variable
    .Sort Key1:=Range("G1"), Order1:=xlAscending, Header:=xlYes, Key2:=Range("B1"), Header:=xlYes

''''''Line I need looked at
    .Copy Destination:=shWorkBook.Range("A1")  ' using new variable
End With

'Call TurnOnCalc  ' commenting out because sub was not included

' release variables when finished with them
Set wkb = Nothing
Set rngFullYearData = Nothing
Set shWorkBook = Nothing
'Set shPivotTable = Nothing  ' only including as a reminder in case you use this worksheet later
'Set shCarrierRates = Nothing
'Set shWholesaleLocation = Nothing
'Set shPWLocation = Nothing
'Set shSegmentation = Nothing
'Set shMilageMatrix = Nothing
Set shFullYearData = Nothing
Set shStartForm = Nothing
Set StartCell = Nothing
End Sub
```


Executing query until there is nothing more left I often query a database to get a batch of items to procees. I do this as long as the query returns some items. I use this pattern quite a lot so I thought I create a small helper so that I don't have to implement this logic again and again.
It's a small class that executes the query until there is nothing more left:
public static class Unfold
{
    public static async Task ForEachAsync<T>
    (
        Func<CancellationToken, Task<IList<T>>> query,
        Func<IList<T>, CancellationToken, Task> body,
        CancellationToken cancellationToken
    )
    {
        while (true)
        {
            var result = await query(cancellationToken);
            if (result.Any())
            {
                await body(result, cancellationToken);
            }
            else
            {
                break;
            }
        }
    }
}

The reason why I implemented it exactly this way is:

all my queries are async
they must always return IList<T> (if they return a collection of course)
I always process a batch at a time that I then mark as processed

Example
The typical use-case is like this:

get a batch of items from a repository
process this batch
repeat until the batch is empty

async Task Main()
{
    var numbers = new NumberRepository();
    await Unfold.ForEachAsync
    (
        query: async token => await numbers.GetNumbersAsync(token),
        body: ProcessBatch,
        CancellationToken.None
    );
}

A test repository:
public class NumberRepository
{
    private readonly IList<IList<int>> _numbers = new[] { new[] { 1, 2 }, new[] { 3, 4 }, new[] { 5 }, new int[0] };
    private int _batchIndex;

    public Task<IList<int>> GetNumbersAsync(CancellationToken cancellationToken) => Task.FromResult(_numbers[_batchIndex++]);
}

and the processing method:
private Task ProcessBatch<T>(T item, CancellationToken cancellationToken)
{
    item.Dump();
    return Task.CompletedTask;
}

What do you say? Is this a good or a bad solution? Is there anything missing (but null-checks)?

Sorry, but you definitely need a null check here:

  if (result.Any())

Else there is not much to comment.

About the usage:

I don't understand, why you create a lambda for the query argument:

 async Task Main()
{
    var numbers = new NumberRepository();
    await Unfold.ForEachAsync
    (
        query: async token => await numbers.GetNumbersAsync(token),
        body: ProcessBatch,
        CancellationToken.None
    );
}

Why not just:

async Task Main()
{
  var numbers = new NumberRepository();
  await Unfold.ForEachAsync
  (
      query: numbers.GetNumbersAsync,
      body: ProcessBatch,
      CancellationToken.None
  );
}

numbers.GetNumbersAsync is awaitable already?


Traversing a JSON-like data structure in Go In my code I need a function to traverse an arbitrary JSON-like data structure with a path definition. E.g., given the structure

{"root": {"inner": [{"name": "obj1"}, {"name": "obj2"}]}}

and the path ("root", "inner", "0", "name"} I would obtain "obj1". I just started learning Go and to be honest I found it quite hard to write it given the constraints imposed by Go's type system. Now it compiles and it seems to be working. To use it in my code, I needed additional helper functions with specific return types. All in all it seems to be a ugly hack with a lot of repetition. I'd like to see if this is the best one can do in Go.

Note: especially terrible is the distinction between map[string]string and map[string]interface{}. I though that interface{} would represent every type, but apparently not in this case. If I remove that part from the switch statement, the program does not work because map[string]string is not handled by the map[string]interface{} branch.

package main

import (
    "fmt"
    "strconv"
)

func main() {
    test := map[string]interface{}{
        "root": map[string]interface{}{
            "inner": []interface{}{
                map[string]string{"name": "obj1"},
                map[string]string{"name": "obj2"},
            },
        },
    }
    fmt.Println(TraverseToStr(test, []string{"root", "inner", "0", "name"}))
}

func TraverseToUint64(data interface{}, path []string) uint64 {
    val, err := Traverse(data, path)
    if err != nil {
        return 0
    } else {
        return val.(uint64)
    }
}

func TraverseToStr(data interface{}, path []string) string {
    val, err := Traverse(data, path)
    if err != nil {
        return ""
    } else {
        return val.(string)
    }
}

func TraverseToMap(data interface{}, path []string) map[string]interface{} {
    val, err := Traverse(data, path)
    if err != nil {
        return make(map[string]interface{})
    } else {
        return val.(map[string]interface{})
    }
}

func TraverseToArray(data interface{}, path []string) []interface{} {
    val, err := Traverse(data, path)
    if err != nil {
        return make([]interface{}, 0)
    } else {
        return val.([]interface{})
    }
}

func Traverse(data interface{}, path []string) (interface{}, error) {
    var ok bool
    current := data
    for _, p := range path {
        switch current.(type) {
        case map[string]string:
            if current, ok = current.(map[string]string)[p]; !ok {
                return nil, fmt.Errorf("key not found in map: %s", p)
            }
        case map[string]interface{}:
            if current, ok = current.(map[string]interface{})[p]; !ok {
                return nil, fmt.Errorf("key not found in map: %s", p)
            }
        case []interface{}:
            if i, err := strconv.ParseInt(p, 10, 64); err == nil {
                array := current.([]interface{})
                if int(i) < len(array) {
                    current = array[i]
                } else {
                    return nil, fmt.Errorf("index %d out of bounds for %v", i, array)
                }
            } else {
                return nil, fmt.Errorf("integer required, got: %s", p)
            }
        default:
            return nil, fmt.Errorf("cannot traverse %T\n", current)
        }
    }
    return current, nil
}
```

Readability is essential for correct and maintainable programs.

For example,

traverse.go:

package main

import (
    "fmt"
    "strconv"
)

func main() {
    test := map[string]interface{}{
        "root": map[string]interface{}{
            "inner": []interface{}{
                map[string]string{"name": "obj1"},
                map[string]string{"name": "obj2"},
            },
        },
    }
    fmt.Println(
        TraverseToStr(test, []string{"root", "inner", "0", "name"}),
    )
}

func TraverseToUint64(data interface{}, path []string) (uint64, error) {
    val, err := Traverse(data, path)
    if err != nil {
        return 0, err
    }
    return val.(uint64), nil
}

func TraverseToStr(data interface{}, path []string) (string, error) {
    val, err := Traverse(data, path)
    if err != nil {
        return "", err
    }
    return val.(string), nil
}

func TraverseToMap(data interface{}, path []string) (map[string]interface{}, error) {
    val, err := Traverse(data, path)
    if err != nil {
        return nil, err
    }
    return val.(map[string]interface{}), nil
}

func TraverseToArray(data interface{}, path []string) ([]interface{}, error) {
    val, err := Traverse(data, path)
    if err != nil {
        return nil, err
    }
    return val.([]interface{}), nil
}

func Traverse(data interface{}, path []string) (interface{}, error) {
    var ok bool
    current := data
    for _, p := range path {
        switch current.(type) {
        case map[string]string:
            if current, ok = current.(map[string]string)[p]; !ok {
                return nil, fmt.Errorf("key not found in map: %s", p)
            }
        case map[string]interface{}:
            if current, ok = current.(map[string]interface{})[p]; !ok {
                return nil, fmt.Errorf("key not found in map: %s", p)
            }
        case []interface{}:
            i, err := strconv.ParseInt(p, 10, 64)
            if err != nil {
                return nil, fmt.Errorf("integer required, got: %s", p)
            }
            array := current.([]interface{})
            if i < 0 || i >= int64(len(array)) {
                return nil, fmt.Errorf("index %d out of bounds for %v", i, array)
            }
            current = array[i]
        default:
            return nil, fmt.Errorf("cannot traverse %T\n", current)
        }
    }
    return current, nil
}

Playground: https://play.golang.org/p/fMcDZr1AZy_X

Commentary:

Readability is essential for correct and maintainable programs.

fmt.Println(
    TraverseToStr(test, []string{"root", "inner", "0", "name"}),
)

The primary line of code should be obvious. The first read through, we focus on the primary line, returning later to read error handling and other secondary details.

TraverseToStr is the primary code, fmt.Println is a secondary detail. We highlight the primary code on a separate line.

func TraverseToUint64(data interface{}, path []string) (uint64, error) {
    val, err := Traverse(data, path)
    if err != nil {
        return 0, err
    }
    return val.(uint64), nil
}

In Go, don't ignore errors.

Unnecessary conditional indentation is hard to read. An idiosyncratic coding style is extrememly hard to read. Return immediately on error. CodeReviewComments: Indent Error Flow

func TraverseToMap(data interface{}, path []string) (map[string]interface{}, error) {
    val, err := Traverse(data, path)
    if err != nil {
        return map[string]interface{}{}, err
    }
    return val.(map[string]interface{}), nil
}

Replace make(map[string]interface{}), a built-in function call, with map[string]interface{}{}, a composite literal value.

func TraverseToMap(data interface{}, path []string) (map[string]interface{}, error) {
    val, err := Traverse(data, path)
    if err != nil {
        return nil, err
    }
    return val.(map[string]interface{}), nil
}

However, on error, return the zero value, nil for a map.

i, err := strconv.ParseInt(p, 10, 64)
if err != nil {
    return nil, fmt.Errorf("integer required, got: %s", p)
}
array := current.([]interface{})
if i < 0 || i >= int64(len(array)) {
    return nil, fmt.Errorf("index %d out of bounds for %v", i, array)
}
current = array[i]

Replace a stream of code with the basic Go error pattern.

Don't overuse the "if expression; simple-statement statement" form.

if i < 0 || i >= int64(len(array)) {
    return nil, fmt.Errorf("index %d out of bounds for %v", i, array)
}

When you see this error message,

invalid operation: i < len(array) (mismatched types int64 and int)

and this fix,

if int(i) < len(array) {
}

stop and think. i may be negative. i is int64, int(i) will discard high-order bits when int is implemented as 32-bits.

The Go Programming Language Specification

The Go Programming Language

"I though that" is not sufficient justification. Use authoritative sources.

Go is strongly typed. Types interface{}, map[string]string, and map[string]interface{} are distinct and have different memory layouts.

When the code is readable, it's possible to read much of the code and prove it correct.

Sometimes it can be hard to establish invariants for maps. "The iteration order over maps is not specified and is not guaranteed to be the same from one iteration to the next." The Go Programming Language Specification. Also, since the Go 1.12 release, "fmt: Maps are now printed in key-sorted order to ease testing." Which leads some to believe, incorrectly, that maps are sorted.

For this code, it's important to note that maps are not read iteratively in a for loop in random order. All map accesses are deterministic.


Largest Prime Factor redux There have been a number of questions asking for comments on code for the Project Euler problem to compute the largest prime factor of a number. A primary intent of this task is to find the factor as quickly as possible.

I decided to implement the code in C++ using at least some approximation of the best techniques/algorithms of which I was aware1.

#include <iostream>
#include <vector>
#include <time.h>

typedef unsigned long long integer;

const integer input = 60085147514377;

class prime_table {
    std::vector<integer> primes;
public:
    prime_table(integer num) {
        integer max = ceil(sqrt(num));
        std::vector<bool> table;
        table.resize(max, false);

        for (int i = 2; i < max; i++) {
            if (!table[i]) {
                primes.push_back(i);
                for (integer temp = 2 * i; temp < max; temp += i)
                    table[temp] = true;
            }
        }
    }

    unsigned size() {   return primes.size();   }

    integer back() { return primes.back(); }

    integer operator[](unsigned index) {    return primes[index];   }

    integer prime_factor(integer input) {
        for (int i = 0; primes[i] * primes[i] <= input; i++)
            if (input % primes[i] == 0)
                return prime_factor(input / primes[i]);
        return input;
    }
};

int main() {
    prime_table s(input);

    integer current = 1;

    clock_t start = clock();
    for (int i = 0; i < s.size(); i++) {
        if ((input % s[i]) == 0) {
            if (s[i]>current)
                current = s[i];
            integer candidate = input / s[i];
            candidate = s.prime_factor(candidate);
            if (candidate > current) {
                current = candidate;
                if (current > s.back())
                    break;
            }
        }
    }
    clock_t stop = clock();
    std::cout << "\n" << current << "\n";
    std::cout << "Time: " << double(stop - start) / CLOCKS_PER_SEC << " seconds\n";
}

Unfortunately, this has the exact opposite problem most attempts do: rather than taking far too long to run, it runs so fast that much of the time it simply shows the minimum interval (1 millisecond) the clock implementation I'm using can measure. Worse, a substantial part of the time, it shows up as running in 0 milliseconds.

To try to get a better idea of the speed, I chose a somewhat larger number to factor -- specifically I tacked 77 on the end to get of the input suggested by Project Euler to get 60085147514377. The code above gives the largest prime factor as 345937 (which Wolfram Alpha agrees is the largest prime factor of this number) in about 9 or 10 milliseconds.

Specific question: implicit in almost any question like this is one of whether further improvements in speed can be achieved easily. I'm reasonably certain there is room for improvement in the sieving part.

I'm also somewhat uncertain about the number theory part--I think this is correct, and it produces correct results for every test I've tried, but I feel slightly less than 100% certain that there couldn't be some input for which it could produce an incorrect result.

1. I'm aware, for example, that the sieve of Atkins is at least theoretically faster than the sieve of Eratosthenes, but it apparently requires fairly non-trivial code to actually achieve that theoretical speedup so I haven't attempted to implement it here.

Slight improvement in the sieve.
I doubt it is significant for max.

prime_table(integer num) {
    integer max = ceil(sqrt(num));
    std::vector<char> table(max, false);   // Making this char rather than bool
                                           // probably makes a tiny difference in speed.
                                           // Don't do an extra re-size do it all up
                                           // front in one allocation.

    // Initialize with the first 2 primes.
    // We don't need to mark multiples of 2 and 3 in the table as true.
    // because of the way we increment in the loop. We basically skip
    // all cells in table {for table[x] where (x%2 ==  0 || x%3 == 0)}
    // so we don't even consider multiples of 2 and 3.
    primes.push_back(2);
    primes.push_back(3);
    int inc = 4;

    // Increment by 2 then 4 (repeat)  (represented by `inc`)
    // This effectively ignores all multiples of 2 and 3.
    for (int i = 5; i < max; i += inc) {
        inc = 6 - inc;
        if (!table[i]) {
            primes.push_back(i);
            for (integer temp = 2 * i; temp < max; temp += i)
                table[temp] = true;
        }
    }
}

You needed the smallest prime here:

integer prime_factor(integer input) {
    for (int i = 0; primes[i] * primes[i] <= input; i++)
        if (input % primes[i] == 0)
            return prime_factor(input / primes[i]);
    return input;
}

This may result in a lot of wasted work (if primes[0] works you did not need the other primes). To only do as much work as you need you may want to try and implement a lazy sieve. That way you only calculate as many primes as you actually need. That being said the sieve is already fast so the extra code and complexity may outweigh the advantages of being lazy.

But to facilitate your tests (to see if a lazy sieve is worthwhile) you could update the code to use std::begin() and std::end() on the primes container. That way it can use your current technique. But if you change the container for primes into something that calculates primes on the fly you don't need to modify your code.

integer prime_factor(integer input)
{
    for (auto prime: primes)   // use std::begin() and std::end in the background (I think)
    {
        if ((prime * prime) > input) {
            break;
        }
        if (input % prime == 0) {
            return prime_factor(input / prime);
        }
    }
    return input;
}

Here is an attempt at a Lazy prime Sieve. Maybe that will help.

class LazyPrime
{
    std::vector<integer>    primes;
    integer                 max;
    std::vector<char>       table;
    integer                 inc;
    integer                 lastPrimeFound;

    struct LazyPrimeIterator
    {
        LazyPrime&   parent;
        std::size_t  current;
        LazyPrimeIterator(LazyPrime& p, int v) : parent(p), current(v) {}
        LazyPrimeIterator& operator++()     {current = parent.next(++current);return *this;}
        integer            operator*()      {return parent.primes[current];}
        bool               operator!=(LazyPrimeIterator const& rhs) const
        {return current != rhs.current;}
    };

    public:
        typedef LazyPrimeIterator           iterator;
        LazyPrime(integer num)
            : max(ceil(sqrt(num)))
            , table(max, false)
            , inc(2)
            , lastPrimeFound(5)
        {
            primes.push_back(2);
            primes.push_back(3);
            primes.push_back(5);
        }

        std::size_t next(std::size_t current)
        {
            if (current < primes.size())
            {    return current;
            }

            // Assume primes retrieved in order.
            assert(current == primes.size());

            for (integer temp = 2 * lastPrimeFound; temp < max; temp += lastPrimeFound){
                table[temp] = true;
            }

            // Increment by 2 then 4 (repeat)  (represented by `inc`)
            // This effectively ignores all multiples of 2 and 3.
            for (integer temp = lastPrimeFound + inc; temp < max; temp += inc) {
                inc = 6 - inc;
                if (!table[temp]) {
                    lastPrimeFound = temp;
                    primes.push_back(lastPrimeFound);
                    return current;
                }
            }
            lastPrimeFound = max - inc;
            return -1; // Same as end.
        }

        iterator begin() { return LazyPrimeIterator(*this, 0);}
        iterator end()   { return LazyPrimeIterator(*this, -1);}
};


Avoiding RequireJS circular dependencies My app module bootstraps a Backbone.js application.
I need app to be available in every other view for easy access to router, triggering navigation, etc.

I never ported an app to RequireJS before, and I got stuck on circular dependency problem, and this answer isn't quite clear to me, so I'm not sure if I applied the solution correctly.

I'm using AMD sugar style throughout the app, so here's what I came up with:

app.js

define(function (require, exports) {

  'use strict';

  var _ = require('underscore'),
      Backbone = require('backbone'),
      Router = require('Router'),
      AppView = require('common/views/app_view'),

  // ...

  return _.extend(exports, {
    start: function () {
      this.router = new Router();
      Backbone.history.start();

      this.appView = new AppView();
      this.appView.render();
    },

    // ...
  });

});

Extending exports instead of directly returning an object allows me to do app = require('app') in any view of the app without worrying about circular dependencies.

All other modules in the app return value normally (i.e. I'm not using exports anywhere else).

Is this an OK strategy for a Require.js Backbone app?
Are there better solutions (that will also work with optimizer or Almond)?

An example of how I'm using app in a (rather deeply nested) view:

zine_item_view.js

define(function (require) {

  'use strict';

  var View = require('common/views/view'),
      Misc = require('common/helpers/misc'),
      app = require('app');

  var ZineItemView = View.extend({
    template: 'templates/social/items/zine_item.html',

    events: {
      'click': 'showZine'
    },

    showZine: function (e) {
      if (!Misc.shouldHandleClick(e)) {
        return;
      }

      app.presenter.showZine({
        zine: this.model,
        route: app.router.routeToZineTrending(this.model.id)
      });
    }
  });

  return ZineItemView;

});

There are few ways to solve your problem with Backbone.js + Require.js and executing functions or changing routes from view.

1. Using Backbone.Router

Here is approach I've used multiple times:

'use strict';

// Your view --------------
define([
    'jquery'
    'backbone'
    'common/views/view',
    'common/helpers/misc'
], function ($, Backbone, View, Misc) {

    var ZineItemView = View.extend({
        ...
        showZine: function (e) {
          if (!Misc.shouldHandleClick(e)) {
            return;
          }
          // Pass all control from view to the router,
          // see http://stackoverflow.com/a/20311253/1363799
          Backbone.history.navigate('yourUrl_to_zine');
        }
    });

    return ZineItemView;
});

// Main block --------------
require([
    'jquery'
    'backbone',
    'app',
    'ZineItemView'
], function($, Backbone, App, ZineItemView) {
    ...

    var app = new App();

    var zineItemView = new ZineItemView({...});

    // Router way
    var router = Backbone.Router.extend({
      routes: {
        "yourUrl_to_zine": "showZine"
      },
      showZine: function() {
            // Here you gave access to zineItemView.model and zineItemView.model.id
            app.presenter.showZine({
                zine: zineItemView.model,
                // I think next row becames obsolete
                route: app.router.routeToZineTrending(zineItemView.model.id)
            });
        }
    });
});

2. Event-driven approach

'use strict';

// Your view --------------
define([
    'jquery',
    'backbone',
    'common/views/view',
    'common/helpers/misc'
], function ($, Backbone, View, Misc) {

    var ZineItemView = View.extend({
        ...
        showZine: function (e) {
          if (!Misc.shouldHandleClick(e)) {
            return;
          }

          var view = this;

          // Trigger global event with desired parameters
          $(window).trigger('showZine.App', {
            zine: view.model,
            modelId: view.model.id // probably it's obsolete, because you can get it from zine property
          });
        }
    });

    return ZineItemView;
});

// Somewhere in app block --------------
// lodash is a dependency, because we use bind method
var App = {
    initialize: function() {
        $(window).on('showZine.App', _.bind(this.presenter.showZine, this) );
    },
    presenter: {
        showZine: function(event, paramsObj) {
            // Here you have paramsObj,
            // with paramsObj.zine = zineItemView.model
            // and paramsObj.modelId = zineItemView.model.id

            // Call app.router.routeToZineTrending from here, but be aware about value of `this` and `app` variables
        }
    }
};

3. Syntax of module defining (optional)

It preferentual, but seems to be a standart (it's easier to see dependencies):

'use strict';

define([
    'common/views/view',
    'common/helpers/misc',
    'app'
], function (View, Misc, app) {

    var ZineItemView = View.extend({
        ...
    });

    return ZineItemView;
});


Given a query string s and a set of all possible query strings, return all strings in the set that have s as a prefix Implement an autocomplete system. That is, given a query string s and
  a set of all possible query strings, return all strings in the set
  that have s as a prefix.

  For example, given the query string de and the set of strings [dog,
  deer, deal], return [deer, deal].

    class DailyCodingProble11 {

        public static void main(String args[]) {

            String[] words = { "dog", "deer", "deal" };
            Trie trie = new Trie();
            for (String word : words) {
                trie.insert(word);
            }
            trie.search("de", trie.root, 0).stream().forEach(word -> System.out.println(word));
            trie.search("do", trie.root, 0).stream().forEach(word -> System.out.println(word));
        }
    }

        class TrieNode {
            public Character content;
            public TrieNode[] children = new TrieNode[26];
            public boolean isWord;

            TrieNode(Character ch) {
                this.content = ch;
            }
        }

        class Trie {
            TrieNode root;

            Trie() {
                root = new TrieNode('*');
            }

            public void insert(String word) {
                TrieNode currentNode = root;
                for (int i = 0; i < word.length(); i++) {
                    Character ch = word.charAt(i);
                    if (currentNode.children[ch - 'a'] == null) {
                        currentNode.children[ch - 'a'] = new TrieNode(ch);
                    }
                    currentNode = currentNode.children[ch - 'a'];

                }
                currentNode.isWord = true;
            }

            public List<String> search(String searchTxt, TrieNode currentNode, int index) {
                List<String> results = new ArrayList<>();
                Character ch = searchTxt.charAt(index);
                if (currentNode.children[ch - 'a'] == null) {
                    return results;
                }
                currentNode = currentNode.children[ch - 'a'];
                if (index == searchTxt.length() - 1) {
                    findWords(currentNode, new StringBuilder(searchTxt), results);
                    return results;
                }
                return search(searchTxt, currentNode, ++index);
            }

            public void findWords(TrieNode currentNode, StringBuilder sb, List<String> results) {

                for (TrieNode child : currentNode.children) {
                    if (child != null) {
                        if (child.isWord == true) {
                            results.add(sb.append(child.content).toString());
                        }
                        findWords(child, new StringBuilder(sb).append(child.content), results);
                    }
                }
            }

 }

How can I improve my solution? Are there any code improvements?

Hide implementation details of Trie

root should be private
current search and findWords methods should be private
write public version of search: should take only a String and call the private one
make TrieNode a private static inner class of Trie

Change storage of children

Storing 26 pointers, most of which will be null in regular usage, is wasteful. You should use a more compact data structure; perhaps a TreeMap<Character, TrieNode>. You could even extend TreeMap for a cleaner interface.

Only store characters once

You effectively store each character in two places: explicitly in TrieNode.content, and implicitly by the node's position in its parent's children array (or in the key of the children map if you take my recommendation above). If you grab the character in the previous round of recursion, you should not need the TrieNode.content field at all.

Fix bug using StringBuilder

In findWords if a node is a word and has children, the character will be appended twice.

Minor tweaks

write a Trie(Iterable<String> content) constructor
no need to store '*' in root
use List.forEach() instead of List.stream().forEach()
separate search functionality into findNode and findWords
change if (blah == true) to if (blah)
pass functions instead of using lambdas when possible
fix indentation -- your IDE should do this for you

Finally, variables names can often be shorter if their purpose is clear in context. For example: currentNode vs current. A lot of people say to "use descriptive variables names"; however, brevity can also help with readability as well.

My stab at the changes

class Trie {
    private static class TrieNode extends TreeMap<Character, TrieNode> {
        public boolean isWord;
    }

    private TrieNode root;

    public Trie(Iterable<String> content) {
        this();
        content.forEach(this::insert);
    }

    public Trie() {
        root = new TrieNode();
    }

    public void insert(String word) {
        TrieNode current = root;
        for (int i = 0; i < word.length(); i++) {
            current = current.computeIfAbsent(word.charAt(i),
                              k -> new TrieNode());
        }
        current.isWord = true;
    }

    public List<String> search(String word) {
        List<String> results = new ArrayList<>();

        TrieNode node = findNode(word, root, 0);
        if (node == null) {
            return results;
        }

        findWords(node, new StringBuilder(word), results);
        return results;
    }

    private TrieNode findNode(String word, TrieNode current, int index) {
        if (index == word.length()) {
            return current;
        }

        Character ch = word.charAt(index);
        if (!current.containsKey(ch)) {
            return null;
        }

        return findNode(word, current.get(ch), ++index);
    }

    private void findWords(TrieNode current, StringBuilder sb, List<String> results) {
        current.forEach((Character ch, TrieNode child) -> {
            StringBuilder word = new StringBuilder(sb).append(ch);
            if (child.isWord) {
                results.add(word.toString());
            }
            findWords(child, word, results);
        });
    }
}

class TrieTest {
    public static void main(String args[]) {
        Trie trie = new Trie(Arrays.asList(new String[] { "dog", "dee", "deer", "deal" }));

        trie.search("de").forEach(System.out::println);
        System.out.println();

        trie.search("do").forEach(System.out::println);
        System.out.println();
    }
}


Number-guessing game with maximum and minimum values This is a simple guessing game where you are prompted for a minimum and maximum value, then you must guess the number the computer chooses! This is the first program I have written so any and all suggestions are welcome.

def guessinggame():
    print "Welcome to the Guessing Game!."
    while True:
        try:
            numrange1 = int(raw_input("What is the minimum number you want?: "))
            break
        except ValueError:
            print "This must be an integer!"
    while True:
        try:
            numrange2 = int(raw_input("What is the maximum number you want?: "))
            break
        except ValueError:
            print "This must be an integer!"
    rnumber = random.randint(numrange1, numrange2)
    i = 0
    while True:
        x = int(raw_input("What is your guess?: "))
        if rnumber > x:
            print "Low!"
            i = i + 1
        elif rnumber < x:
            print "High!"
            i = i + 1
        elif rnumber == x:
            print "Correct!"
            i = i + 1
            print i
            break
    again = raw_input("Would you like to play again? (y/n): ")
    if again == 'y':
        guessinggame()
guessinggame()

You are inappropriately using the guessinggame() function as if it were a goto label.  After playing the game repeatedly, you will end up with a deep stack trace (which you can see by hitting ControlC), which is a bad sign.  A proper design would be:

def guessing_game():
    # Code to play one game

play = True
while play:
    guessing_game()
    play = ('y' == raw_input('Would you like to play again? (y/n): '))

Curiously, you check that the bounds are integers, but you don't bother to validate the guesses.  You also don't check that the minimum is less than the maximum.

All three prompts would benefit from a shared integer-prompting routine:

def prompt_int(prompt, err_msg='This must be an integer!'):
    while True:
        try:
            return int(raw_input(prompt))
        except ValueError:
            print err_msg

Getting that clutter out of the way makes your main function more readable:

def guessing_game():
    print "Welcome to the Guessing Game!."

    rnumber = None
    while rnumber is None:
        lower = prompt_int('What is the minimum number you want?: ')
        upper = prompt_int('What is the maximum number you want?: ')
        if lower < upper:
            rnumber = random.randint(lower, upper)
        else:
            print "The minimum must be less than the maximum!"

    tries = 0
    while True:
        guess = prompt_int("What is your guess?: ")
        tries += 1
        if guess < rnumber:
            print "Low!"
        elif guess > rnumber:
            print "High!"
        elif guess == rnumber:
            print "Correct!"
            print tries
            break

I've renamed numrange1 to lower, numrange2 to upper, i to tries, and x to guess.  Incrementing the loop counter is common to all three branches and can be factored out.


Combinations with replacement I am still new to Scala and wrote a small snippet to find all the combinations with replacement of a sequence (e.g. cwr(ab, 3) should give aaa, aab, abb, bbb).
The slow way would be to generate all permutations, sort them, and throw out duplicates.  I'd like to know if this code is idiomatic Scala style, and if I'm making any bad performance mistakes.

def sortedRanges(lo: Int, hi: Int, reps: Int) : List[List[Int]] =
    if (reps == 0) List(Nil) else (
    List.range(lo,hi).flatMap(
        (x:Int) => (sortedRanges(x,hi,reps-1).map((xs:List[Int]) => x :: xs ))
    )
)

def combinations_with_replacement[T](stuff: List[T], reps: Int) : List[List[T]] =
  sortedRanges(0,stuff.length, reps).map( (xs:List[Int]) => xs.map(stuff(_)))

Here are the changes I would make to your code in terms of both style and efficiency:

    def sortedRanges(lo: Int, hi: Int, reps: Int): List[List[Int]] = reps match {
        case 0 => Nil :: Nil
        case _ =>
          List.range(lo, hi) flatMap (x =>
            sortedRanges(x, hi, reps - 1) map (xs =>
              x :: xs))
      }

    def combinationsWithReplacement(stuff: Vector[T], reps: Int): List[List[T]] =
      sortedRanges(0, stuff.length, reps) map (xs =>
        xs map (x =>
          stuff(x)))

Style Comments

Methods which take functions as parameters (flatMap and map in this case) should be invoked using infix notation. That is, if there were a god of Idiomatic Scala Style he would prefer xs map (...) to xs.map(...)

Defined functions should use camel case, e.g. combWithRep over comb_with_rep.

In general, and you will get a feel for this the more you use Scala, pattern matching is preferred over if-statements. You will notice that in your function sortedRanges I've swapped your if-else statement for a pattern match on the value reps.

The last style tip I have for you is to break a chain of higher order functions over several lines. This last one can be fudged in some cases, but in general I find that it improves readability.

Efficiency Comment

The one change that I made in the name of efficiency was to change the container type of stuff from List[T] to Vector[T]. The reason I made this change is that accessing an item in a List takes linear time on the length of the list, whereas accessing an item in a Vector is almost constant. And as you know, you are accessing elements of stuff by index is in the last line of combinationsWithReplacement.


Method to count unread messages I currently use this method to count unread messages for a user (self here):

def unread_messages_count
  number = 0
  conversations = Conversation.where {|conversation| conversation.user1 == self || conversation.user2 == self }
  conversations.each do |conversation|
    number += conversation.messages.select{|message| !message.read && (message.user != self) }.count
  end
  number
end

However, this seems highly inefficient to me since I'm iterating on every conversation found to look for unread messages.

Any input on how to make this method smarter will be much appreciated !

About your code:

As a general rule, the imperative pattern acc = 0; xs.each { |x| acc += f(x) }; total = acc should be replaced by the functional expression total = xs.reduce(0) { |acc, x| acc + f(x) } or total = xs.map { |x| f(x) }.reduce(0, :+). Given that Rails provides an Enumerable#sum, it can be simplified even more: total = xs.map { |x| f(x) }.sum.
That said, this is not the way you do things in Rails, you should use associations and scopes. The code will be much faster (all the processing is performed by the database engine) and more reusable.

I'd write:

class User
  has_many :conversations, ->(user) { Conversation.with_user(user) }
  has_many :messages, through: :conversations

  def unread_messages_count
    messages.not_sent_by(self).unread.count
  end
end

class Conversation
  scope :with_user, ->(user) { where("? IN (user1_id, user2_id)", user.id) }
end

class Message
  scope :unread, -> { where(read: false) }
  scope :not_sent_by, ->(user) { where.not(user: user) }
end

Note that we could write the where-condition of with_user directly in the has_many block, but this way we respect encapsulation: the User model knows nothing about the attributes of the Conversation table.


Human readable display of a list of things Just for fun, I created this small function that prints an iterator in a String:

use std::fmt::{Display, Write};

fn enumerate<T: Display, I: IntoIterator<Item = T>>(it: I) -> String {
    let mut it = it.into_iter().peekable();
    let mut result = String::new();

    if let Some(first) = it.next() {
        write!(result, "{}", first).unwrap();
        if it.peek().is_some() {
            while let Some(item) = it.next() {
                match it.peek() {
                    Some(_) => write!(result, ", {}", item),
                    None => write!(result, " and {}", item),
                }.unwrap();
            }
        }
    }

    result
}

fn main() {
    assert_eq!(enumerate(Vec::<i32>::new()), "");
    assert_eq!(enumerate(vec![1]), "1");
    assert_eq!(enumerate(vec![1, 2]), "1 and 2");
    assert_eq!(enumerate(vec![1, 2, 3]), "1, 2 and 3");
    assert_eq!(enumerate(vec![1, 2, 3, 4]), "1, 2, 3 and 4");
}

But I am not really satisfied with it. It is not really readable, and I am switching between next and peek in nested blocks. I wonder if this is possible to write the function in a more functional manner?

It's often more readable to write trait bounds (e.g. T: Display) in a where clause rather than directly next to the declaration. Also, there are some types of bounds that cannot be written with the angle brackets and must instead be written in a where clause (there are examples below). When you have a where clause, put all the bounds here; don't put bounds in both places, because then it's easy for someone reading the code to miss the bounds in the angle brackets.
You don't need the T type parameter on your function; you can write the bound I::Item: Display to get the same effect.
The if it.peek().is_some() is unnecessary; if that expression returns false, the program wouldn't enter the while let loop anyway, and that loop is the only statement in the if.
By using DoubleEndedIterator::next_back, we can extract the last item from the iterator. This lets us remove the peeks to determine whether you should use a comma or "and" as the separator. However, this restricts the types of iterators that the function accepts, because the iterator must now implement DoubleEndedIterator. In order to write this requirement, we must write the bound I::IntoIter: DoubleEndedIterator in a where clause; we can't write this within the angle brackets.

fn enumerate<I>(it: I) -> String
where
    I: IntoIterator,
    I::Item: Display,
    I::IntoIter: DoubleEndedIterator,
{
    let mut it = it.into_iter();
    let mut result = String::new();

    if let Some(first) = it.next() {
        write!(result, "{}", first).unwrap();

        if let Some(last) = it.next_back() {
            while let Some(item) = it.next() {
                write!(result, ", {}", item).unwrap();
            }

            write!(result, " and {}", last).unwrap();
        }
    }

    result
}

  I wonder if this is possible to write the function in a more functional manner?

Building upon the code above, we can replace the while let loop with a fold.

fn enumerate<I>(it: I) -> String
where
    I: IntoIterator,
    I::Item: Display,
    I::IntoIter: DoubleEndedIterator,
{
    let mut it = it.into_iter();
    let mut result = String::new();

    if let Some(first) = it.next() {
        write!(result, "{}", first).unwrap();

        if let Some(last) = it.next_back() {
            result = it.fold(result, |mut result, item| {
                write!(result, ", {}", item).unwrap();
                result
            });

            write!(result, " and {}", last).unwrap();
        }
    }

    result
}

write! doesn't "feel" functional, though, because it mutates its operand rather than returning a new value. Therefore, in this case, I don't think the version with fold is really more readable than the version with while let.

By using itertools, a third-party crate published on crates.io, we can simplify the code a bit and give it a more functional feel by using its join adapter. Its implementation is more or less what we were doing before.

extern crate itertools;

use std::fmt::{Display, Write};
use std::iter;

use itertools::Itertools;

fn enumerate<I>(it: I) -> String
where
    I: IntoIterator,
    I::Item: Display,
    I::IntoIter: DoubleEndedIterator,
{
    let mut it = it.into_iter();

    if let Some(first) = it.next() {
        if let Some(last) = it.next_back() {
            let mut result = iter::once(first)
                .chain(it)
                .join(", ");

            write!(result, " and {}", last).unwrap();
            result
        } else {
            format!("{}", first)
        }
    } else {
        String::new()
    }
}


Insertion sort in Java I've written a stable implementation of insertion sort, that sorts an array in ascending order.

Please let me know of any improvements/optimisations that can be made.

import java.util.List;

public class InsertionSort {
    public static void start(List<Integer> arrayToSort){
        for(int i = arrayToSort.size() - 1; i > 0; i--){    // i is the leftmost index of sorted elements
            // j is the index at which the new element inserted in the sorted part of the array
            for(int j = arrayToSort.size() - 1; j >= i; j--){
                if(arrayToSort.get(i - 1) > arrayToSort.get(j)){
                    insert(i - 1, j + 1, arrayToSort);
                    break;
                }
            }
        }
    }

    private static void insert(int source, int destination, List<Integer> arrayToSort){
        arrayToSort.add(destination, arrayToSort.get(source));
        arrayToSort.remove(source);
    }
}

Unnecessary work

Consider a list of \$n\$ elements, then insert(1,0) will move approximately \$2n\$ elements in the array when all it really had to do was swap the two elements.

Note that an optimised insert(source, destination) only needs to move source - destination elements. As this looks like self-learning or possibly homework, I'll leave the details to you.

Use binary search

You can binary search for the insertion point to improve the performance even further.


Creating a array of custom objects from JSON, sent using PHP from a MySQL database I'm using PHP to send data in JSON to create an array of custom objects in Swift. I've been told retrieveData() is bad code because it's in an infinite loop. I'm not sure on how to clean it up so it can be more efficient. On a side note, is there a way I can save the custom objects in an array that is easier to save to UserDefaults without NSCoder?

The app is basically a blog reader. It gets data from a server's database and displays it in a tableview.

MainController.swift

var jsonArray: NSMutableArray = []
var mainArray = [Blog]()

// Retrieving Data from Server
func retrieveData() {

    let getDataURL = "http://example.com/receiving.php"
    let url: NSURL = NSURL(string: getDataURL)!

    do {
        let data: Data = try Data(contentsOf: url as URL)
        jsonArray = try JSONSerialization.jsonObject(with: data, options: .mutableContainers) as! NSMutableArray

        // Looping through jsonArray
        for i in 0..<jsonArray.count {

        // Create Blog Object
        guard let bID: String = (jsonArray[i] as AnyObject).object(forKey: "id") as? String,
            let bName: String = (jsonArray[i] as AnyObject).object(forKey: "blogName") as? String,
            let bStatus1: String = (jsonArray[i] as AnyObject).object(forKey: "blogStatus1") as? String,
            let bStatus2: String = (jsonArray[i] as AnyObject).object(forKey: "blogStatus2") as? String,
            let bURL: String = (jsonArray[i] as AnyObject).object(forKey: "blogURL") as? String,
            let bType: String = (jsonArray[i] as AnyObject).object(forKey: "blogType") as? String,
            let bDate: String = (jsonArray[i] as AnyObject).object(forKey: "blogDate") as? String,
            let bPop: String = (jsonArray[i] as AnyObject).object(forKey: "blogPop") as? String

            else {
                print("Error: (Creating Game Object)")
                return
            }

            // Add Blog Objects to mainArray
            mainArray.append(Blog(blogName: bName, andBlogStatus1: bStatus1, andBlogStatus2: bStatus2, andBlogURL: bURL, andBlogID: bID, andBlogType: bType, andBlogDate: bDate, andBlogPop: bPop))
        }
    }
    catch {
        print("Error: (Retrieving Data)")
    }
    myTableView.reloadData()
}

Blog.swift - Handles the Blog custom objects that it gets from retrieveData

import UIKit

class Blog: NSObject {

// Strings
var blogName: String
var blogStatus1: String
var blogStatus2: String
var blogURL: String
var blogID: String
var blogType: String
var blogDate: String
var blogPop: String

// Converting Strings into Objects
init(blogName bName: String,
     andBlogStatus1 bStatus1: String,
     andBlogStatus2 bStatus2: String,
     andBlogURL bURL: String,
     andBlogID bID: String,
     andBlogType bType: String,
     andBlogDate bDate: String,
     andBlogPop bPop: String)
{
    self.blogName = bName
    self.blogStatus1 = bStatus1
    self.blogStatus2 = bStatus2
    self.blogURL = bURL
    self.blogID = bID
    self.blogType = bType
    self.blogDate = bDate
    self.blogPop = bPop
    super.init()
  }
}

Let me try to clean it little bit.

1 First of all you made jsonArray global. I think it's not necessary.
You can declare it locally where needed.

2 retrieveData() function looks very messy.
You can abstract that creation of Blog object.

3 You can improve for loop code

So here is what I would propose:

class Blog: NSObject {

  var blogName: String!
  var blogStatus1: String!
  var blogStatus2: String!
  var blogURL: String!
  var blogID: String!
  var blogType: String!
  var blogDate: String!
  var blogPop: String!

  init?(from jsonObject: AnyObject) {
    guard let bID: String = jsonObject.object(forKey: "id") as? String,
        let bName: String = jsonObject.object(forKey: "blogName") as? String,
        let bStatus1: String = jsonObject.object(forKey: "blogStatus1") as? String,
        let bStatus2: String = jsonObject.object(forKey: "blogStatus2") as? String,
        let bURL: String = jsonObject.object(forKey: "blogURL") as? String,
        let bType: String = jsonObject.object(forKey: "blogType") as? String,
        let bDate: String = jsonObject.object(forKey: "blogDate") as? String,
        let bPop: String = jsonObject.object(forKey: "blogPop") as? String else {
            print("Error: (Creating Game Object)")
            return nil
    }
    blogName = bName
    blogStatus1 = bStatus1
    blogStatus2 = bStatus2
    blogURL = bURL
    blogID = bID
    blogType = bType
    blogDate = bDate
    blogPop = bPop
    super.init()
  }
}

So outside world will not have to worry about converting json object into Blog.

Then simply use this abstraction as:

var mainArray = [Blog]()

func retrieveData() {

  let getDataURL = "http://example.com/receiving.php"
  let url: NSURL = NSURL(string: getDataURL)!

  do {
    let data: Data = try Data(contentsOf: url as URL)
    let jsonArray = try JSONSerialization.jsonObject(with: data, options: .mutableContainers) as! NSMutableArray

    // Looping through jsonArray
    for jsonObject in jsonArray {
      if let blog = Blog(from: jsonObject as AnyObject) {
        mainArray.append(blog)
      }
    }
  }
  catch {
    print("Error: (Retrieving Data)")
  }
  myTableView.reloadData()
}


Optimization for add function in queue I would prefer if more experienced users could give pointers on how I can optimize and think better when writing code.

Using Homework Questions as reference, as this is a homework related question. This should fall into the second category. I know the code is working and that no memory is leaking. The implementation nor the queue.h can not be changed.

queue.h:

#define MAX_PRIO 100
typedef const char Datatyp;

#ifndef QUEUE_H
#define QUEUE_H

struct Qelement {                           // type of a queue
    struct Qelement *next;                  // pointer to next element
    int prio;                               // order in queue
    Datatyp *dataptr;                       // pointer to data
};

typedef struct Qelement *Queue;

queue.c: (This is were feedback is wanted)

// creating an empty queue
Queue new_queue ()
{
    Queue q = (Queue)malloc(sizeof(struct Qelement));
    q->next = NULL;
    q->prio = MAX_PRIO;
    q->dataptr = NULL;
    return q;
}

// adding a new element to queue based on priority
// higher priority means earlier in queue
void add (Queue q, int priority, Datatyp *d)
{
    // initiation
    Queue previous = q;
    Queue element = (Queue)malloc (sizeof(struct Qelement));
    Queue tmp = NULL;

    while (priority <= previous->prio)
    {
        if (previous->next != NULL)
        {
            tmp = previous;
            previous = previous->next;
        }
        else if (tmp != NULL)
        {
            tmp = previous;
            previous = previous->next;
            break;
        }
        else
        {
            break;
        }
    }

    element->prio = priority;
    element->dataptr = d;

    if (tmp == NULL)
    {
        element->next = NULL;
        previous->next = element;
    }
    else
    {
        element->next = previous;
        tmp->next = element;
    }
}

The code is then used along these lines. (This part can not be changed)

int main () {
    const char *daniel = "Daniel";
    const char *lisa = "Lisa";
    const char *a[] = {"Kalle", "Olle", "Eva", lisa, "Stina",
                      "Peter", "Anna", daniel, "Johan", "Maria"};

    Queue q = new_queue();
    int i;

    for (i=0; i<10; i++) {
        add(q, i%4, a[i]);
    }
}

Your variable's names confused me; I'd suggest renaming 'previous' to 'current', and 'tmp' to 'previous'.
You don't do anything sensible if malloc fails.
The following ...

else if (tmp != NULL)
{
    tmp = previous;
    previous = previous->next;
    break;
}
else
{
    break;
}

... could be rewritten as ...

else
{
    if (tmp != NULL)
    {
        tmp = previous;
        previous = NULL;
    }
    break;
}

IMO you should ignore the alleged priority assigned to the head q element (because you never want to put anything in front of it).

1st version

In summary you could rewrite it to be something like this:

bool add (Queue q, int priority, Datatyp *d)
{
    // initiation
    Queue element = (Queue)malloc (sizeof(struct Qelement));
    if (element == NULL)
        return false;
    element->prio = priority;
    element->dataptr = d;

    // loop until end of list or higher priority than next item
    for (; (q->next != NULL) && (q->next->prio >= priority); q = q->next)
    { } // an empty statement

    // insert element
    element->next = q->next;
    q->next = element;
    return true;
}

Advantages of this version over your implementation:

No danger of adding in front of the first, passed-in q element, even if the passed-in priority is greater than MAX_PRIO
No extra local variables (you have 2: tmp and previous)
No if or else statements (you have 5)
It's obvious what the algorithm is (search the list after the first node until you find the right spot, then insert into the list there)

2nd version

It's not clear whether your Queue new_queue() is part of the code which we're allowed to review.

Assuming that we are allowed to review/change that, I would add, "**Don't create "first element" which represents (contains a first pointer to) the queue: instead the queue should be represented by a pointer to the first real element."

Your first node (allocated using new_queue) might be what's called a "sentinel node" (see references here and here). It's also possible to create a queue without using a sentinel node: the "queue" is simply a pointer to the first node (instead of being a pointer to a node which contains a pointer to the first real node).

void add (Queue* qptr, int priority, Datatyp *d)
{
    // initiation
    Queue element = (Queue)malloc (sizeof(struct Qelement));
    element->prio = priority;
    element->dataptr = d;

    // get first element of queue
    Queue q = *qptr;
    if ((q == NULL) // queue was empty
        || (priority > q->prio)) // higher priority than first queue item
    {
        // insert at front of queue
        element->next = q;
        // caller's qptr now points to the new first node
        *qptr = element;
        return;
    }

    // else insert later in the queue using the same code as above
    for (; (q->next != NULL) && (q->next->prio >= priority); q = q->next)
    { }

    // insert element
    element->next = q->next;
    q->next = element;
}

... used like this ...

int main () {
    const char *daniel = "Daniel";
    const char *lisa = "Lisa";
    const char *a[] = {"Kalle", "Olle", "Eva", lisa, "Stina",
                      "Peter", "Anna", daniel, "Johan", "Maria"};

    // not q = new_queue()
    // instead let q be pointer to first real node
    // and q is null represent an empty list
    Queue q = NULL;

    for (int i=0; i<10; i++) {
        // pass Queue* qptr = &q (address of queue)
        // so that add can modify the value contained in q
        // (i.e. modify the node to which q is pointing)
        add(&q, i%4, a[i]);
    }

    // q is now a pointer to the first element
}

  Sadly, I can not change add to be a bool either. Is there any other way of handling it (except of very obscured ones) without changing add to a bool?

Some C conventions are:

return int (often 0 for success and -1 for failure)
return something useful like 'Queue' (the most recently-added element) or 0 (if a new element couldn't be created
throw an exception (if it's C++)
Include <stdbool.h>
Ignore the fact that malloc might fail.

  Tell me about it, that is how I did it at first, and it turned out that the main function gave me a billion errors because apparently the first element of the queue should only be a pointer to the first real element.

I tested the above code by adding the following statement to the end of main:

// q is now a pointer to the first element

for (;q;q=q->next)
{
    printf(q->dataptr);
    printf("\r\n");
}

It generated the following output (which I think is correct):

Lisa
Daniel
Eva
Anna
Olle
Peter
Maria
Kalle
Stina
Johan


A hotel greeter named Alfred Is there any way of improving this code? (Here's a JSFiddle with an example and comments)

Example: A hotel greeter can greet guests and accepts tips, but it's impossible to see the total of tips a greeter has received.

<!DOCTYPE html>

            []___
           /    /\     O
          /____/__\   /|\
          |[][]||||   / \
                     ALFRED

        <button onclick="javascript:alfred('greet')">Arrive at Hotel</button>
        <button onclick="javascript:alfred('acceptTip', 10)">Tip Alfred $10</button>

Goals:

Keep it DRY
Keep it MVC
Make sure anyone with access to a "greeter" can never manipulate the view or model directly.

Limitations:

To avoid clutter, avoid type checking to avoid simple errors unless it is necessary to guarantee integrity.

Some specific thoughts:

Is there a better way of passing model and view into the controller?
Is there a better way of communicating with the controller?
Is there a good way of making a more general view?
Any other improvements?

From the comments, I guess you would like a theoretical review ;)

This:

<button onclick="javascript:alfred('greet')">Arrive at Hotel</button>
<button onclick="javascript:alfred('acceptTip', 10)">Tip Alfred $10</button>

in my mind should be wired by the controller,  that is, the linking of UI elements to data and UI changes
This:

greet: function () {
    this.model.view.innerHTML = '"' + this.model.greeting + '"';
},

has your controller access data straight in your view, updating the DOM. This is wrong in my mind. The view should have functions that the controller can call with either the model as a parameter or already passed in advance.
Object.create(controllerPrototype); <- Any reason you are eschewing new and prototype ?
Not that it truly matters but, method === "model" will not catch new String('model')
args = [].slice.call(arguments, 1); works as well as the ternary

All in all, I think this could use some more polishing.


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

Currently I'm doing:

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

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

To give me something like:

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

Is there a nicer/cleaner approach?

I assume you "own" the controller?
If so, I'd serialize and deserialize the list as a comma separated string on each side.
It'd also be nice if criteria.JobTypes is an empty list instead of a possible null, then you don't need the ?? below.

var redirectUrl = Url.Action("search", new {
    // ...
    jobTypes = String.Join(criteria.JobTypes ?? new string[0], ",")
}

And do jobTypes.split(",") in the action.

URL would look a bit nicer too:

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


C code, rewritten to use a Person struct without pointers I'm new to C and I have been assigned the task of converting some code that uses structs with pointers to code that doesn't use pointers. I would be interested to know if the code I have written is 'good' code and how I could improve it. Here is the code I have written which creates a Person struct then creates a few sample persons.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct person {
    char *name;
    int age;
    int height;
    int weight;
};

typedef struct person Who;

struct person Person_create(char *name, int age, int height, int weight)
{

    Who w;

    w.name = strdup(name);
    w.age = age;
    w.height = height;
    w.weight = weight;

    return w;
}

void Print_person(Who w)
{
    printf("Name: %s\n", w.name);
    printf("\tAge: %d\n", w.age);
    printf("\tHeight: %d\n", w.height);
    printf("\tWeight: %d\n", w.weight);
}

int main(int argc, char *argv[])
{
    struct person ryan = Person_create("Ryan McGovern", 37, 182, 67);
    struct person sandy = Person_create("Sandy McGovern", 32, 167, 49);

    printf("Ryan is at memory location: %p\n", &ryan);
    Print_person(ryan);

    printf("Sandy is at memory location: %p\n", &sandy);
    Print_person(sandy);

    ryan.age += 2;
    ryan.weight += 4;

    Print_person(ryan);

    return 0;
}

Prints out:

Ryan is at memory location: 0x7fff52b7ea68
Name: Ryan McGovern
    Age: 37
    Height: 182
    Weight: 67
Sandy is at memory location: 0x7fff52b7ea50
Name: Sandy McGovern
    Age: 32
    Height: 167
    Weight: 49
Name: Ryan McGovern
    Age: 39
    Height: 182
    Weight: 71

Original code using pointers (from the book Learn C the Hard Way by Z. A. Shaw):

#include <stdio.h>
#include <assert.h>
#include <stdlib.h>
#include <string.h>

struct Person {
    char *name;
    int age;
    int height;
    int weight;
};

struct Person *Person_create(char *name, int age, int height, int weight)

{

    struct Person *who = malloc(sizeof(struct Person));
    assert(who != NULL);

    who->name = strdup(name);
    who->age = age;
    who->height = height;
    who->weight = weight;

    return who;
}

void Person_destroy(struct Person *who)
{
    assert(who != NULL);
    free(who->name);
    free(who);
}

void Person_print(struct Person *who)
{
    printf("Name: %s\n", who->name);
    printf("\tAge: %d\n", who->age);
    printf("\tHeight: %d\n", who->height);
    printf("\tWeight: %d\n", who->weight);
}

int main(int argc, char *argv[])
{
    struct Person *ryan = Person_create("Ryan McGovern", 37, 182, 67);

    struct Person *sandy = Person_create("Sandy McGovern", 32, 167, 48);

    // print them out and where they are in memory
    printf("Ryan is located at memory location %p:\n", ryan);
    Person_print(ryan);

    printf("Sandy is at memory location %p:\n", sandy);
    Person_print(sandy);

    // Make them age 20 years and print them out again
    ryan->age += 20;
    ryan->height -= 2;
    ryan->weight += 4;
    Person_print(ryan);

    sandy->age += 20;
    sandy->weight += 10;
    Person_print(sandy);

    // Clean up
    Person_destroy(ryan);
    Person_destroy(sandy);

    return 0;
}

This is an interesting problem. Removal of pointers can improve the safety of code. It's much more difficult in C than something like C++, though, as C doesn't have many other options like references or smart pointers. Regardless, here are some thoughts on your code.
Use Your typedefs
You've made a type definition for struct person named Who. But you only appear to be using it two of the five places where you need them. Many C programmers dislike using typedef with a struct. I am not one of those programmers. I think they make the code much nicer and easier to understand. You've made the typedef, so you should use it everywhere. Person_create() should return a Who not a struct person. Also in main() you should be creating 2 Whos, not 2 struct persons.
Removing pointers
You've said the goal was to remove pointers from your struct, but you're still using one for the name field. Why? You could make it a fixed-length array if you truly want to remove all use of pointers. You'll need to ensure that you don't copy anything too long into it, in that case, though. You could define a constant for the maximum length of a name in that case. Something like this:
const size_t kMaxNameBytes = 32;    // Maximum number of bytes including NUL terminator

struct person {
    char name [ kMaxNameBytes ];
    int age;
    int height;
    int weight;
};

typedef struct person Who;

Now you have a struct that has no pointers. It is bigger, however. The disadvantage with having no pointers is that every struct must carry all of its data within the struct leading to much larger pieces of data on the stack. If you want an array of thousands of persons, it may be too large to fit on the stack.
As mentioned above, you'll also need to be more careful with your copies of the name field, now, too. Instead of just calling w.name = strdup(name);, you'll have to do something like this:
size_t name_len = MIN(strlen(name), kMaxNameBytes - 1);
memcpy(w.name, name, name_len);
w.name [ name_len + 1 ] = '\0';

Note all the extra work you now have to do to ensure that the string doesn't go over the maximum length and that it is also properly NUL terminated. On the other hand, copying a whole person is easier because you don't have to manually copy the name string like you do in your version. You can simply memcpy() the bytes from one struct to the other because it's self-contained.
Handling Failure
Note that your code doesn't handle the failure of strdup(). If strdup() can't allocate the memory, it will return NULL. Your code doesn't check the return value and will happily set the name to NULL and keep going. This is likely to cause a memory error somewhere else in your program.


Sorting strings by length - functional Python I'm trying to port this little F# snippet while staying pythonic:

["something"; "something else"; "blah"; "a string"] |> List.map (fun p -> p, p.Length) |> List.sortBy snd

In case you don't speak F#, it gets the length of each string, then sorts by length.

Output:

[("blah", 4); ("a string", 8); ("something", 9); ("something else", 14)]

In Python, this is the best I could do so far:

sorted([(p, len(p)) for p in ["something", "something else", "blah", "a string"]], key=lambda a:a[1])

While correct, this doesn't look very elegant to me, or maybe it's just my non-pythonic eye.

Is this pythonic code? How would you write it? Maybe an imperative style is more appropriate?

data = ["something", "something else", "blah", "a string"]
result = [(x, len(x)) for x in sorted(data, key = len)]

Basically, its more straightforward to sort first then decorate. Although, I'm not sure why you would need the length of the list in your tuple. If you don't really need it sorting by length can be much shorter.

EDIT:

If all I wanted was to output the data, I'd do it like this:

for string in sorted(data, key = len):
    print string, len(string)

If you really wanted to eliminate the two references to len you could do:

mykey = len
for string in sorted(data, key = mykey):
    print string, mykey(string)

But unless you are reusing the code with different mykey's that doesn't seem worthwhile.


Order a list of tuples or a NumPy array in a specific format I have a list of tuples or a NumPy array (it can be either of them as the variable comes as a list and will end up being a NumPy array) and I want to order them in a specific way (that I am not able to phrase it).

Let's make an example:

boundary=[(1, 2), (1, 3), (3, 4), (2, 4)];

Desired output:

[(1, 2), (2, 4), (4, 3), (3, 1)]

(or a NumPy array of 2x4 will do also)

The logic behind it is to get the first row, read its last value and find that value in the array. Get that tuple, swap it to make the found value the first one (if needed) and repeat.

The idea is that ...N),(N... , N is the same. There will always be 2 and only 2 repetitions of the numbers.

I can figure out how to do it "C-like", iterating, searching, swapping etc, but I wonder if there is a pythonic way (those amazing one-liners) of achieving it. Ultimately I am just trying to increase my Python-fu.

boundary=np.array(boundary)
ordered=np.zeros([boundary.shape[0],boundary.shape[1]]);
idx=0
idy=0
for i in range(len(boundary)):
    ordered[i,0]=boundary[idx,idy]
    ordered[i,1]=boundary[idx,(idy+1)%2]
    ix,iy=np.where(boundary==boundary[idx,(idy+1)%2])
    for j in range(len(ix)):
        isused=np.where(np.all(ordered==boundary[ix[j],],axis=1))
        if isused[0].size==0:
            idx=ix[j]
            idy=iy[j]
            print idx, idy
            break

This is really a graph problem! Draw a graph with a vertex for every number in your list, and an edge between vertices \$a\$ and \$b\$ if there is a pair \$(a, b)\$ or \$(b, a)\$ in your list:

What you want to do is to find a path in this graph that visits all the edges. Such a path is known as an Eulerian path. Your condition that each number appears exactly twice in the list means that the path must return to the original vertex: that makes it an Eulerian cycle.

To find this cycle, we must first represent the graph using a suitable data structure. Here we can use the adjacency list representation (really, adjacency set):

from collections import defaultdict

def graph(edges):
    """Given a list of edges, return a map from vertex to a set of
    adjacent vertices.

    >>> graph([(1, 2), (1, 3), (3, 4), (2, 4)])
    defaultdict(<class 'set'>, {1: {2, 3}, 2: {1, 4}, 3: {1, 4}, 4: {2, 3}})

    """
    g = defaultdict(set)
    for v, w in edges:
        g[v].add(w)
        g[w].add(v)
    return g

Then, to find the path, we can use Hierholzer's algorithm. This is really simple: we just pick a starting vertex and then repeatedly pick any unvisited edge from that vertex. Your condition that each number appears exactly twice in the list of edges means that the algorithm can't get stuck: there are exactly two edges meeting each vertex, so having gone in on one we can go out on the other:

def eulerian_path(edges, v):
    """Generate an Eulerian path over the given collection of edges,
    starting at the vertex v.

    >>> list(eulerian_path([(1, 2), (1, 3), (3, 4), (2, 4)], 1))
    [(1, 2), (2, 4), (4, 3), (3, 1)]

    """
    g = graph(edges)
    while True:
        try:
            w = g[v].pop()
        except KeyError:
            return
        g[w].remove(v)
        yield v, w
        v = w

What might happen, however, is that your data contains multiple disjoint cycles. For example if the input is:

[(1, 2), (2, 3), (3, 1), (4, 5), (5, 6), (6, 4)]

then this corresponds to the graph:

The eulerian_path algorithm only finds one of the cycles. What should you do in ths case? Stop at the end of the first cycle? Keep going and find all the other cycles? Maybe this can't happen in your problem, so raise an error? You didn't explain what your problem really is, so I can't tell you what the right thing is to do in this case.

Update

If you're determined to stick to NumPy, then consider using the Compressed Sparse Graph Routines. The idea is to use scipy.sparse.csr_matrix to convert your array into a CSR matrix:

>>> edges = np.array([(1, 2), (1, 3), (3, 4), (2, 4)])
>>> m = scipy.sparse.csr_matrix((np.ones(len(edges)), (edges[:,0], edges[:,1])), shape=(5, 5))

and then call scipy.sparse.csgraph.depth_first_order to get the vertices on the Eulerian path:

>>> scipy.sparse.csgraph.depth_first_order(m, 1, False)[0]
array([1, 2, 4, 3], dtype=int32)

If you need to turn this array of vertices back into an array of edges (as in the OP), you could use numpy.vstack and numpy.roll, like this:

>>> np.vstack((vertices, np.roll(vertices, -1))).T
array([[1, 2],
       [2, 4],
       [4, 3],
       [3, 1]], dtype=int32)