Swift complexities with Dictionary I have been trying to learn the various aspects of the languages I know so far to do the Daily Challenges in reddit (/r/DailyProgrammer). The one which I am working on just now asks for an alphabetised count of the occurrences of letters in a string e.g: "Hello World" -> "d:1 e:1 h:1 l:3 o:2 r:1 w:1".

I have tried to write this seemingly simple challenge in Swift (I had already done it in C#, JavaScript and F#), but after longer than should have been the case, I have arrived at the following solution which seems a bit messy to me:

func countLetters(input: String) -> String  {

    // Convert to Lowercase
    var lower = input.lowercaseString

    // Filter out invalid characters ([a-z] only)
    var validCharacters = filter(lower) { $0 >= Character("a") && $0 <= Character("z") }

    // Create a dictionary to build the letter counts
    var letterCounts = Dictionary<Character, Int32>()
    // Go through the characters and add to the dictionary
    for i in validCharacters {
        if letterCounts[i] != nil { // If already exists, add one to the count
            letterCounts[i] = letterCounts[i]! + 1
        } else {
            letterCounts[i] = 1 // If doesn't exist, add to the dictionary with a count of 1
        }
    }

    // Unfortunately the Dictionary does not seem to be sorted / sortable, so we need to
    // create an Array of the stringified version to allow sorting
    var stringifiedCounts = Array<String>()

    // Convert each dictionary item to a 'a:1' type string
    for (k,v) in letterCounts {
        stringifiedCounts.append("\(k):\(v) ")
    }

    // Sort the list
    sort(&stringifiedCounts)

    // Concatenate the results
    var output = ""
    for i in stringifiedCounts {
        output += i
    }

    // Remove the last space
    return output.substringToIndex(advance(output.startIndex, countElements(output) - 1))
}

I was going to ask the question in my reply on Reddit, but I am quite late with it, so I doubt anyone will be looking.

For those who have been more regularly using Swift, is there a cleaner way to deal with Dictionaries so that I don't have to covert it to an array before sorting? What are some of the cleverer ways to get the same results?

The following looks like it is much simpler and most of the way there:

func countLetters (string:String) -> String {
  var dict : [Character:Int] = [:]
  for letter in string {
    dict[letter] = 1 + (dict[letter] ?? 0)
  }

 //  dict -> array of tuples -> sort based on tuple .0
 var dictSorted = map(dict) { $0 }.sorted { $0.0 < $1.0 }

 // your string to return.
 return " ".join (dictSorted { (key,val) -> String in return "\(key):\(val)" })
}


Project Euler 30 - Digit fifth powers in Rust I am trying to move from Python to a systems programming language so this is a bit of a challenge for me.

fn main() {
    let mut total_pow = 0;
    for x in 2..360000 {
        let values: Vec<char> = x.to_string().chars().collect();
        let mut total = 0;
        for i in values {
            let value = i.to_digit(10).unwrap();
            total += value.pow(5);
        }
        if total == x {
            total_pow += x
        }
    }
    println!("Numbers {}", total_pow);
}

It runs in just under 1 second, which I think is slow:

$ time cargo run
Total: 443839
real  0m0.894s
user  0m0.888s
sys   0m0.012s

After building with the --release flag the time has greatly improved:

$ time cargo run --release
Numbers 443839

real  0m0.083s
user  0m0.072s
sys   0m0.012s

Some of my concerns are:

Converting x, the loop value, to a char array and then converting each char back into a digit to perform pow on. Is there an easier way? In Python it is really simple (list(str(x))).
I feel the speed of this is really slow.
Best practices for rust/systems level code. I realize this is a fairly simple program but I want to make sure that I learn the good habits early.

If you want to measure the execution time of your program, run it directly, rather than going through Cargo, which adds some overhead of its own.

$ time cargo run --release
     Running `target/release/pe30`
Numbers 443839

real    0m0.111s
user    0m0.087s
sys     0m0.027s

$ time ./target/release/pe30
Numbers 443839

real    0m0.045s
user    0m0.040s
sys     0m0.003s

(Cargo's overhead was much worse in the 0.2.0 nightly I first tried this with...)

The Rust compiler generates quite suboptimal code in debug builds, so don't worry too much about it.

Now for the code review proper:

let values: Vec<char> = x.to_string().chars().collect();

There is no need to collect() if all you're going to do is iterate on the collection. chars() returns a struct that implements Iterator, so you can use directly in a for loop.

let values = x.to_string().chars();

Oops, that doesn't compile:

src/main.rs:4:22: 4:35 error: borrowed value does not live long enough
src/main.rs:4         let values = x.to_string().chars();
                                   ^~~~~~~~~~~~~

We need to keep the value of x.to_string() alive, since the value of x.to_string().chars() refers to it. (You can discover that from the fact that Chars has a lifetime parameter ('a).) Let's split up the expression:

let x_str = x.to_string();
let values = x_str.chars();

But rather than using the builtin to_string(), we can generate the digits of a number arithmetically:

fn digits(mut n: u32) -> Vec<u32> {
    if n == 0 {
        vec![0]
    } else {
        let mut digits = Vec::new();
        while n > 0 {
            digits.push(n % 10);
            n /= 10;
        }

        digits
    }
}

fn main() {
    let mut total_pow = 0;
    for x in 2..360000 {
        let digits = digits(x);
        let mut total = 0;
        for digit in digits {
            total += digit.pow(5);
        }
        if total == x {
            total_pow += x
        }
    }
    println!("Numbers {}", total_pow);
}

(digits produces the digits from least-significant to most-significant; the order isn't important in this algorithm.)

At this point, I think the code is pretty optimal. However, Rust also encourages functional-style programming. Why not take a look?

First, let's change this loop:

let mut total = 0;
for digit in digits {
    total += digit.pow(5);
}

into a fold:

let total = digits.iter().fold(0, |sum, digit| sum + digit.pow(5));

fold lets you produce a single value from an Iterator. You specify the initial value, then a function to call on each item that returns the accumulator to pass to the next item, or to return if the end is reached. When you compile with --release, fold is often inlined.

We can do the same again with the outer loop:

let total_pow = (2..360000).fold(0, |total_pow, x| {
    let digits = digits(x);
    let total = digits.iter().fold(0, |sum, digit| sum + digit.pow(5));
    if total == x {
        total_pow + x
    } else {
        total_pow
    }
});

This big closure looks a bit messy, though. Let's break up the problem further. First, we want to obtain the list of numbers that can be written as the sum of fifth powers of their digits. Then, we want to sum that list.

For the first task, we need to eliminate numbers that don't apply. To keep only some items from a list, we can use filter. filter returns a new Iterator, on which we can apply fold to get the sum.

fn main() {
    let total_pow = (2..360000).filter(|&x| {
        x == digits(x).iter().fold(0, |sum, digit| sum + digit.pow(5))
    }).fold(0, |total_pow, x| {
        total_pow + x
    });
    println!("Numbers {}", total_pow);
}

If you're using a nightly compiler, you can also use sum (unstable at the time of writing). For the outer fold, we can use sum directly. For the inner fold, we could first use map to turn the sequence of digits into a sequence of their fifth powers, then sum them up. (I had to add a type annotation on total_pow, for some reason.)

#![feature(iter_arith)] // for Iterator::sum()

fn digits(mut n: u32) -> Vec<u32> {
    if n == 0 {
        vec![0]
    } else {
        let mut digits = Vec::new();
        while n > 0 {
            digits.push(n % 10);
            n /= 10;
        }

        digits
    }
}

fn main() {
    let total_pow: u32 = (2..360000).filter(|&x| {
        x == digits(x).iter().map(|digit| digit.pow(5)).sum()
    }).sum();
    println!("Numbers {}", total_pow);
}

Functional style can make program quite shorter, but also somewhat hard to read when you're not used to it. I encourage you to get used to it, because you're likely to encounter this style again when reading other people's Rust code.


Best way to get two columns from a table Basically I have taken over a project and found a table that has the following video and video title fields in it (why they didn't create a separate linked table is beyond me):

[VIDEOURL]
[VIDEOTITLE]
[COUKVIDEO1URL]
[COUKVIDEO2URL]
[COUKVIDEO3URL]
[COUKVIDEO4URL]
[COUKVIDEO1TITLE]
[COUKVIDEO2TITLE]
[COUKVIDEO3TITLE]
[COUKVIDEO4TITLE]

Can this query be improved to get a single list of titles and urls from the above fields:

SELECT
    t.VIDEOURL,
    t.VIDEOTITLE
FROM
(
    SELECT VIDEOURL, VIDEOTITLE FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
    UNION
    SELECT [COUKVIDEO1URL] AS VIDEOURL, [COUKVIDEO1TITLE] AS VIDEOTITLE FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
    UNION
    SELECT [COUKVIDEO2URL] AS VIDEOURL, [COUKVIDEO2TITLE] AS VIDEOTITLE FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
    UNION
    SELECT [COUKVIDEO3URL] AS VIDEOURL, [COUKVIDEO3TITLE] AS VIDEOTITLE FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
    UNION
    SELECT [COUKVIDEO4URL] AS VIDEOURL, [COUKVIDEO4TITLE] AS VIDEOTITLE FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
) t
WHERE t.VIDEOURL != ''
AND t.VIDEOTITLE != ''

There is no telling how the database will optimize the query plan for this, but I would expect that the best query plan would involve applying the predicates to the tables before the union is done. Doing a UNION of multiple small result sets would be more efficient than doing a union on large sets and then filtering the results.

The UNION operation requires a lot of scanning because UNION is also a DISTINCT process (you only have one record of each value in the result).

So, two things.

If you are sure that the values in in a column-pair will not also be in any of the other column-pairs, then you can replace the UNION with the faster UNION ALL.

If you are sure that a column-pair is unique, or empty, then you can avoid the DISTINCT process completely.

I would write the query as:

    SELECT VIDEOURL,
           VIDEOTITLE
    FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
    WHERE [VIDEOURL] != ''
      AND [VIDEOTITLE] != ''
  UNION ALL
    SELECT [COUKVIDEO1URL] AS VIDEOURL,
           [COUKVIDEO1TITLE] AS VIDEOTITLE
    FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
    WHERE [COUKVIDEO1URL] != ''
      AND [COUKVIDEO1TITLE] != ''
  UNION ALL
    SELECT [COUKVIDEO2URL] AS VIDEOURL,
           [COUKVIDEO2TITLE] AS VIDEOTITLE
    FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
    WHERE [COUKVIDEO2URL] != ''
      AND [COUKVIDEO2TITLE] != ''
  UNION ALL
    SELECT [COUKVIDEO3URL] AS VIDEOURL,
           [COUKVIDEO3TITLE] AS VIDEOTITLE
    FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
    WHERE [COUKVIDEO3URL] != ''
      AND [COUKVIDEO3TITLE] != ''
  UNION ALL
    SELECT [COUKVIDEO4URL] AS VIDEOURL,
           [COUKVIDEO4TITLE] AS VIDEOTITLE
    FROM [VincentV5].[dbo].[MARCMSITEMPRESENTATION]
    WHERE [COUKVIDEO4URL] != ''
      AND [COUKVIDEO4TITLE] != ''

If values in one column-pair can appear in other column pairs, then change the UNION to UNION ALL.

If the values can be duplicated in a column-pair, then add DISTINCT to each SELECT.

Do both DISTINCT and UNION ALL if you need to.

The idea is to make the data as small as possible before you start doing the big merging processes.


List Any vs Count, which one is better for readability? Resharper thinks I should change this

while (dockPanel.Contents.Count() > 0)

to the following:

while (dockPanel.Contents.Any())

Without thinking twice about it, I switched to the any version because .Any() is more readable for me. But a coworker of mine sees the change and asked why I did it. I explained that this improves readability but his objection was the following:

Any is an ambiguous word.
People may not be familiar with LINQ, so they might not be aware of what Any does at first glance.
Count > 0 is universal. You don't have to know C# to find out what you're trying to do.

The confusion is probably compounded by the fact that I'm working overseas. People here speak little to no English here.

Which method should I stick with?

For me, it's about intent. What does your code's business logic say if you read it in English (or your native language)?

It usually comes to me as "If there are any employees who are in the management role, then show a particular option panel".

And that means .Any(). Not .Count(). Very few times will I find myself using .Count() unless the underlying rule talks about "more than one" or "more than two" of something.

As a bonus, .Any() can be a performance enhancement because it may not have to iterate the collection to get the number of things. It just has to hit one of them. Or, for, say, LINQ-to-Entities, the generated SQL will be IF EXISTS(...) rather than SELECT COUNT ... or even SELECT * ....


Print some elements from input I am studying Python, so it is very easy task. I want to get best and most elegant way to solve each simple task.
The task is:

  Write a program that reads the integers from the console one number per line.
  For each entered number check:

  if the number is less than 10, skip that number;
  if the number is greater than 100, stop to read the numbers;

  Print all numbers that does not satisfy those two conditions.

lst = []
num = 0
while num <=100:
    num = int(input())
    if num > 100:
        break
    if num >= 10:
        lst.append(num)

print('\n'.join(str(value) for value in lst))

Your code is pretty good. Change num <=100 to True and remove the un-needed num out of the while loop, and it's good.
You should also expect user input to not be a number and handle it as you need.

Since comprehensions are a cleaner than map and filter, you may want to steer clear of them. Some people stick to never using them, I however use them if it makes the input clearer, such as map(str, user_input()).

You can then adjust your code slightly to get the iterative approach of:

list_ = []
while True:
    try:
        num = int(input())
    except ValueError:
        continue
    if num > 100:
        break
    if num >= 10:
        list_.append(num)

However, you may want a functional approach. This can be better as then you can pick and mix functions, but some don't like it as much.

Make an infinite generator, of user input.
itertools.takewhile the input is good.
Filter input.

def user_input():
    while True:
        yield input()

def to_int(numbers):
    for num in numbers:
        try:
            yield int(num)
        except ValueError:
            continue

nums = itertools.takewhile(lambda n: n <= 100, to_int(user_input()))
print('\n'.join(str(n) for n in nums if n >= 10))

Python is multi-paradigm, and so you can mix them. And so as-long as it works and is clear, it's fine. I personally would use:

def user_input():
    while True:
        try:
            yield int(input())
        except ValueError:
            continue

def filter_numbers(numbers):
    for num in numbers:
        if num > 100:
            break
        if num >= 10:
            yield num

print('\n'.join(str(n) for n in filter_numbers(user_input())))


Building objects in javascript, without "if(!a[k]) a[k] = []" When building objects using reduce, I often have crappy code like this:

function mapArticlesByTagAndId(articles) {
    return articles.reduce((collector = {}, article) => {
        const { tag, id } = article;
        if(!collector[tag]) collector[tag] = {};

        collector[tag][id] = article;
        return collector;
    })
}

I would love to one line this, but I can't figure out a way to do it.
If only assigning a property to undefined created an object =P [Obviously this would cause different problems haha]

Here's one variant which gets rid of the explicit if at the cost of potential useless reassignment. This, however, does not take away the necessity of ensuring that the k key has a collection object in place...

const result =
  myArr.reduce((collector, { k, j, v }) => {
    collector[k] = collector[k] || [];
    collector[k][j] = v;
    return collector;
  }, {});

Assuming, ES6 syntax is fine with you, here's another variant. Please notice that it the spread operator (... will result in copying the ...collector[k] item-by-item). I.e. this is less performant and may be an issue for large objects.

const result =
  myArr.reduce((collector, { k, j, v }) => {
   collector[k] = collector[k] ? [...collector[k], v] : [ v ];
   return collector;
  }, {});

which may be compacted further:

const result =
  myArr.reduce((collector, { k, j, v }) =>
    (collector[k] = collector[k] ? [...collector[k], v] : [ v ], collector),
    {});

Update 1

const result =
  myArr.reduce((collector, { k, j, v }) =>
    (collector[k] = !collector[k] ? [v] : (collector[k].push(v), collector[k])),
    {});


Copying data from an Excel sheet to Word This code copies a specific range and a chart from an Excel sheet to Word. This code works fine but it is very slow. I have used delays(Wait) to work properly while dealing with clipboard copy-paste. If I don't use delays as error occurs.

This program copies 140 Excel-ranges and 140 charts, so it takes a lot of time. Any suggestions to make the program execute faster?

Sub ExcelToWord()
    Dim fileName, Pathname As String
    Dim WB As Workbook
    Dim mychart As ChartObject
    Pathname = "c:\Charts\"
    vArr = Array("A", "B", "C", "D", "E", "F", "G", "H", "I", "J")
    For Each vFile In vArr
        fileName = Dir(Pathname & vFile & "\" & "*.xlsx")
        Dim WdObj As Object
        Set WdObj = CreateObject("Word.Application")
        WdObj.Documents.Add
        'Loop for all files begins
        Do While fileName <> ""
            Set WB = Workbooks.Open(Pathname & vFile & "\" & fileName)
            ActiveSheet.Columns("C:C").Select
            Selection.Insert Shift:=xlToRight, CopyOrigin:=xlFormatFromLeftOrAbove
            Dim rLastCell As Range
            Dim ColLtr As String
            Set rLastCell = ActiveSheet.Cells.Find(What:="*", After:=ActiveSheet.Cells(1, 1), LookIn:=xlFormulas, Lookat:= _
            xlPart, SearchOrder:=xlByColumns, SearchDirection:=xlPrevious, MatchCase:=False)
            ColLtr = Replace(Cells(1, rLastCell.Column).Address(True, False), "$1", "")
            ActiveSheet.Columns(ColLtr).Copy
            ActiveSheet.Columns("C").Select
            ActiveSheet.Paste
            Application.CutCopyMode = False
            Dim lastrow As Long, lastcol As Long
            Dim rngTemp As Range
            With ActiveSheet
                If Application.WorksheetFunction.CountA(.Cells) <> 0 Then
                    lastrow = .Cells.Find(What:="*", _
                                          After:=.Range("A1"), _
                                          Lookat:=xlPart, _
                                          LookIn:=xlFormulas, _
                                          SearchOrder:=xlByRows, _
                                          SearchDirection:=xlPrevious, _
                                          MatchCase:=False).Row
                    lastcol = 3
                Else
                    lastrow = 1: lastcol = 1
                End If
                Set rngTemp = .Range("A1:" & Split(.Cells(, lastcol).Address, "$")(1) & lastrow)
                Set mychart = ActiveSheet.ChartObjects(1)
            End With
            WdObj.Visible = True
            WdObj.Activate
            rngTemp.Select
            Application.Wait (Now + TimeValue("0:00:01"))
            Selection.Copy
            DoEvents
            Application.Wait (Now + TimeValue("0:00:01"))
            WdObj.Selection.PasteSpecial
            Application.CutCopyMode = False
            mychart.Select
            Application.Wait (Now + TimeValue("0:00:01"))
            Selection.Copy
            DoEvents
            Application.Wait (Now + TimeValue("0:00:01"))
            WdObj.Selection.PasteSpecial
            Application.CutCopyMode = False
            Const wdPageBreak = 7
            Application.Wait (Now + TimeValue("0:00:01"))
            WdObj.Selection.InsertBreak (wdPageBreak)
            Application.Wait (Now + TimeValue("0:00:01"))
            Application.CutCopyMode = False
            WB.Close SaveChanges:=False
            fileName = Dir()
        Loop
        Dim fname As String
        fname = "Converted and Combined.docx"
        Application.CutCopyMode = False
        With WdObj
            .ChangeFileOpenDirectory "C:\Charts\" & vFile & "\"
            .ActiveDocument.SaveAs fileName:=fname
        End With
        With WdObj
            .ActiveDocument.Close
            .Quit
        End With
        Set WdObj = Nothing
    Next
End Sub

Things you could try:

Set shorter wait times. This shows you how. This is likely the main source of delay. You can assess if this is true by timing the total time, and calculating the time taken with each vfile.
Avoid Selecting unless mandatory. For instance, replace

ActiveSheet.Columns("C").Select
ActiveSheet.Paste

with

ActiveSheet.Columns("C").PasteSpecial


Generic Null/Empty check for each property of a class I have created a method to check for null/empty values of class properties and if any null property is found I'm stopping the checking process and returning the result as true. I've used a solution from here and following is the code I have implemented:

/// To check the properties of a class for Null/Empty values
/// </summary>
/// <param name="obj">The instance of the class</param>
/// <returns>Result of the evaluation</returns>
public static bool IsAnyNullOrEmpty(object obj)
{
    //Step 1: Set the result variable to false;
    bool result = false;

    try
    {
        //Step 2: Check if the incoming object has values or not.
        if (obj != null)
        {
            //Step 3: Iterate over the properties and check for null values based on the type.
            foreach (PropertyInfo pi in obj.GetType().GetProperties())
            {
                //Step 4: The null check condition only works if the value of the result is false, whenever the result gets true, the value is returned from the method.
                if (result == false)
                {
                    //Step 5: Different conditions to satisfy different types
                    dynamic value;
                    if (pi.PropertyType == typeof(string))
                    {
                        value = (string)pi.GetValue(obj);
                        result = (string.IsNullOrEmpty(value) ? true : false || string.IsNullOrWhiteSpace(value) ? true : false);
                    }
                    else if (pi.PropertyType == typeof(int))
                    {
                        value = (int)pi.GetValue(obj);
                        result = (value <= 0 ? true : false || value == null ? true : false);
                    }
                    else if (pi.PropertyType == typeof(bool))
                    {
                        value = pi.GetValue(obj);
                        result = (value == null ? true : false);
                    }
                    else if (pi.PropertyType == typeof(Guid))
                    {
                        value = pi.GetValue(obj);
                        result = (value == Guid.Empty ? true : false || value == null ? true : false);
                    }
                }
                //Step 6 - If the result becomes true, the value is returned from the method.
                else
                    return result;
            }
        }
    }
    catch (Exception ex)
    {
        throw ex;
    }

    //Step 7: If the value doesn't become true at the end of foreach loop, the value is returned.
    return result;
}

I want to extend this code to another end that instead of sending the bool result as true or false, I'll clone the incoming object and based on the values inside the properties can send the object.

For example:

Let's say this is my model: Order.cs

public class Order
{

    [DataMember]
    public Guid OrderId { get; set; }

    [DataMember]
    public string Owner { get; set; }

    [DataMember]
    public string Info { get; set; }

    [DataMember]
    public string Recipient { get; set; }

    [DataMember]
    public int Test { get; set; }

    [DataMember]
    public DateTime CreatedOn { get; set; }
}

Now if there is any null or empty value in any of the properties, I'll be returning a new class which contains only those values which are either null and empty and the status of it as:

//This class is the output class from the IsAnyNullOrEmpty() method and has only those properties which are either null or empty
public class ProcessedModel
{
    public string Property1 { get; set; }   //has null value in type String (set property vaule to "Null value")

    public string Property2 { get; set; } //has empty value in type String (set property vaule to "Empty value")

    public string Property3 { get; set; } //has 0 value in type int (set property vaule to "0 value")
}

Am I following the right approach?

First of all I'd reduce indentation. It makes your code really too hard to read. Doing that you will see it may be simplified.

First of all try/catch: a catch-all block where you rethrow the exception is useless (and you may even throw away line number information.)

This code:

if (obj != null)
{
    // Do something...
}

May be replaced simply with:

if (obj == null)
    return false;

You do not need to declare a local variable for result, simply return its value where appropriate.

You do not have to check condition result == false inside your foreach loop (you will still enumerate through all properties), simply break the loop and exit immediately.

You do not need to use to dynamic, when casting you have the right type at compile-time. Also move all this code inside a separate function:

private static bool IsNullOrEmpty(object obj) {
}

Let's see how to fill this function. Let's read the value of the property:

object value = pi.GetValue(obj);

First of all a common case, valid for all reference types:

if (Object.ReferenceEquals(value, null))
    return true;

Then explicitly check for strings:

if (value is string && String.IsNullOrEmpty((string)value))
    return true;

For Int32 (note that in your original code value <= 0 ? true : false is redundant it may be simplified to value <= 0). BTW are you sure you want to consider negative numbers as empty values?!

if (value is int)
    return ((int)value) <= 0;

Boolean does not need to be checked in this case (null case has been already handled). We just need to work out Guid:

if (value is Guid)
    return ((Guid)value) == Guid.Empty;

That's all. Note that now or in future you may want to check for Nullable<T> and or other types. With the exception of strings and negative integer numbers you can also do something like this:

if (Object.ReferenceEquals(value, null))
    return true;

var type = value.GetType();
return type.IsValueType
    && Object.Equals(value, Activator.CreateInstance(type));

What left in your original function? In this example let me use simplified check (negative numbers are not empty and I do not check for String.Empty against strings however it handles nullable types), feel free to pick the one works for you:

public static bool IsAnyNullOrEmpty(object obj)
{
    if (Object.ReferenceEquals(obj, null))
        return true;

    return obj.GetType().GetProperties()
        .Any(x => IsNullOrEmpty(x.GetValue(obj)));
}

private static bool IsNullOrEmpty(object value)
{
    if (Object.ReferenceEquals(value, null))
        return true;

    var type = value.GetType();
    return type.IsValueType
        && Object.Equals(value, Activator.CreateInstance(type));
}

Last note: if working with databases you may also want to check equality with DBNull.Value.


Associating a lock/mutex with the data it protects I've recently come across some areas of code in our solution that have a few thread-safety problems.  Thread-safety is a minefield at best so I thought I'd have an attempt at writing a few little utility classes that would make it easier to write safe multi-threaded code.
The general idea is to link a resource with the lock(s) that protect it.  If the resource is accessed outside of all of it's locks then a runtime exception is thrown.

Imagine a developer had written the following (example) caching object but had forgotten to take the _imageLock when clearing the cache.

Using the traditional .net locking mechanism (of just locking on an Object) we might corrupt the Dictionary if two threads call Add() and ClearCache() at the same time.

Using these utility classes, Padlock and Protected<T>, will instead throw an exception in the ClearCache() method because the Protected<T> object is being accessed outside of it's lock. This exception should surface during the developers normal testing procedure, before the code is even checked in.

public class SomeThreadSafeCache
{
    Padlock _imageLock = new Padlock();
    Protected<Dictionary<string, byte[]>> _imageCache;

    public SomeThreadSafeCache()
    {
        //The variable _imageCache is associate with the _imageLock Padlock
        //i.e. _imageCache is only available when we're locking on _imageLock.
        _imageCache = new Protected<Dictionary<string, byte[]>>(
            new Dictionary<string, byte[]>(),
            _imageLock);
    }

    public void AddImage(string key, byte[] data)
    {
        using (_imageLock.Lock())
        {
            _imageCache.Value.Add(key, data);
        }
    }

    public void ClearCache()
    {
        //Throws an exception because the protected
        //resource is accessed outside of it's lock.
        _imageCache.Value.Clear();
    }
}

(I know that .net now comes with Concurrent collections which make my example pointless but the general idea of linking a resource with it's lock is what I'm trying to get at)

The Protected<T> class is able to be associated with as many Padlocks as you might want.

public class SomeOtherThreadSafeObject
{
    Padlock _lock1 = new Padlock();
    Padlock _lock2 = new Padlock();

    Protected<int> _i;

    public SomeOtherThreadSafeObject()
    {
        _i = new Protected<int>(0, _lock1, _lock2);
    }

    public void IncrementI()
    {
        using (_lock1.Lock())
        {
            using (_lock2.Lock())
            {
                _i.Value++;
            }
        }
    }
}

(A trivial example for illustrative purposes only, the Interlocked.Increment function would do a better job.)

If either _lock1 or _lock2 is not taken then

_i.Value++

will throw an exception stating that the object is not fully locked.

Enforcing the order in which the locks are taken are outside the scope of the utility classes.  I'm also trying to avoid the old ICollection.SyncRoot pattern by not allowing

Protected<T>.Padlock.Lock()

The locking policy itself should be up to the implementors of the class.  All Protected<T> is interested is in is knowing if it's fully protected before allowing access to it's internal value.

What I'm asking for a review of is the general approach/idea. Does this seem like a sensible system?  Are there any glaring problems with it that I've not thought of?  If anyone wants to see the actual implementation code then I can post it on Github (or somewhere similar).

(I'm aware that this abuses the IDisposable/Using pattern but for added thread-safety I think it will be worth it for us).

I confess that I think your approach looks painful. As you can see from the other answers, it's insufficient to protect your data outside the container class. And there's confusion in your example code as to whether Protected is the container or ThreadSafeCache is the container. I think instead that you need to revisit the principle of encapsulation. When you have a collection that requires multi-threaded updates and access, you protect that collection with a wrapper class. The wrapper encapsulates the underlying collection in its entirety. Think "private". If you don't want to expose the lock, then you don't expose an iterator/enumerator. Rather, you do something similar to the List's ForEach method:

public void ForEach(Action<T> t){
   lock(_items) foreach(var item in _items) t.Invoke(item);
}


Check the address String for presence of a New Brunswick abbreviation I need to check if there are any abbreviation of New Brunswick province in the address. Acceptable abbreviations are: "NB", "N.B.", "N-B", "N.-B." and any combination of upper-lower cases. It shouldn't be a part of another word, hence the "\b" separators.

The hardest part was checking the dot in the end, because of the greediness of the "\b". So, the part "\.\b" doesn't work, because the dot is within the word boundary, so it is being "attached" to "\b".

Is there a way to avoid such behaviour and are there other suggestions to improve on the regex?

return addressLine?.matches(".*\\b([nN]-?[bB]\\b|[nN]\\.-?[bB]\\.).*")

Depending on your requirements for word breaks the following should do nicely:

/.*\b[nN](\.)?-?[bB]\1?\s.*/

Caveat only checks for a space after N?B?

Or in as in your question language(ruby?):

return addressLine?.matches(".*\\b[nN](\\.)?-?[bB]\\1?\\s.*"


Finding pairs of complementary numbers This is the "Pairs with Positive Negative values" problem from geeksforgeeks.org.

  Given an array of distinct integers, print all the pairs having positive value and negative value of a number that exists in the array.

  NOTE: If there is no such pair in the array , print "0".

  Input:
  The first line consists of an integer T i.e number of test cases. The first line of each test case consists of an integer n. The next line of each test case consists of n spaced integers.

  Output:
  Print all the required pairs in the increasing order of their absolute numbers.

Here is my code:

def findPairs(l):
    listLen=len(l)
    nums=set()
    for i in range(listLen):
        if (-1*l[i]) in l[i+1:]:
            nums.add(abs(l[i]))
            continue
    nums=sorted(nums)
    #print (nums)
    pairs=[]
    for num in nums:
        pairs.extend([-1*num,num])

    return pairs

T=int(input())
pair=[]
for i in range(T):
    input()
    pair=findPairs(list(map(int,input().split())))
    if len(pair):
        print(" ".join(str(x) for x in pair))
    else:
        print("0")

You definitely incorporated the response to your previous questions, such as:

Extract code into functions.
Always indent with 4 spaces.
Process input immediately instead of storing all input data and results in lists.

There is still too little (horizontal) whitespace, I suggest to check
your code against the PEP8 coding style,
for example at PEP8 online.

Also function and variable names should be snake_case according to PEP8.

More suggestions:

Variable names: T is too short and non-descriptive, num_tests
would be better. pair is better named pairs.
There is no need to pre-assign pair=[].
The value of the iterator variable i in the main loop is not needed,
a common convention is use _ instead:

for _ in range(T):

Negation as in -1*l[i] can be simplified to -l[i]
When printing the result list

print(" ".join(str(x) for x in pair))

you can use map instead, as you already do when reading the input:

print(" ".join(map(str, pair)))

Improving the performance:

Your algorithm has two nested loops traversing through the list, i.e. the
time complexity is \$ O(n^2) \$ as a function of the list length.

This can be improved by sorting the list first. One option would be to sort
the numbers in increasing order, e.g.

-3 -1 1 2 3 6

for the first test case, and then traverse the list from both ends to find
pairs. The time complexity is \$ O(n \log n) \$ for the sorting, and
 \$ O(n) \$ for the traversal, so this would be faster for large input.

Another option is to sort the number in increasing absolute value, e.g.

 -3 3 -1 1 2 6

Again a linear sweep of the sorted list is now sufficient to find matching pairs,
which are now consecutive entries.

Here is a possible implementation of the second approach:

def find_pairs(numbers):
    numbers.sort(key=lambda x: x + 0.5 if x >= 0 else -x)
    pairs = []
    for pair in [(x, y) for x, y in zip(numbers, numbers[1:]) if x == -y]:
        pairs.extend(pair)
    return pairs

using

sort() with a custom key. This relies on the given fact that the numbers are
distinct integers.
zip and list comprehension to enumerate pairs of consecutive list entries.


Producing all possible combinations of a list I've written a function that produces all combinations of elements in a given list:

createGroups :: [a] -> [[(a, a)]]
createGroups li@(x : xs) = map (\el -> map (\el2 -> (el, el2)) xs) li

These nested maps seem quite messy -- is there a better way to do this?

It is possible that I'm misunderstanding the problem, but based on your description, I would expect the type of createGroups to be [a] -> [(a, a)] (A flat list, not a list of lists.)

I believe there is bug in your code. Take a look:

> createGroups [1..3]
[[(1,2),(1,3)],[(2,2),(2,3)],[(3,2),(3,3)]]

This result is not what I would expect based on your problem description. Given a list of [1, 2, 3], I would expect the output to be [(1,2),(1,3),(2,3)].

One way to achieve this output is the following:

createGroups :: [a] -> [(a, a)]
createGroups [] = []
createGroups (x:xs) = map ((,) x) xs ++ createGroups xs

> createGroups [1..3]
[(1,2),(1,3),(2,3)

Here is another function that outputs a "flat list" containing the same pairs as your original function, however it's been rewritten to use list comprehension:

createGroups :: [a] -> [(a, a)]
createGroups ls @ (_ : xs) = [(x, pair) | x <- ls, pair <- xs]

> createGroups [1..3]
[(1,2),(1,3),(2,2),(2,3),(3,2),(3,3)]

Lastly, here is a function that produces output identical to yours, rewritten using the monad instance of list:

createGroups :: [a] -> [[(a, a)]]
createGroups li@(x : xs) = do
  first <- li
  return (getPairs first xs)
  where
    getPairs item pairings = do
      pair <- pairings
      return (item, pair)

> createGroups [1..3]
[[(1,2),(1,3)],[(2,2),(2,3)],[(3,2),(3,3)]]

Hopefully this gives you some ideas of different ways to write your function.


Reversing a linked list I implemented reversing a linked list in iterative way, just by knowing we need 3 pointers and I have never seen a code before, when I coded. I ran this code and tested it, it works fine. Even for null element and one element. But all the implementations on net have slightly different code. I was just wondering, how my algorithm is.

Do you guys see any major concern in my implementation?

int reverse(List *l) {

 if(!l) {
        printf("List is NULL \n");
        return -1;
 }

 if(!l->head) {
        printf("List is empty \n");
        return -1;
 }

 Node *prev_node = l->head;
 Node *curr_node = l->head->next;

 while(curr_node) {
        prev_node->next = curr_node->next;
        curr_node->next = l->head;
        l->head = curr_node;
        curr_node = prev_node->next;
 }

 return 0;
}

If you don't care about the origin list, we can use the head of it as the iterator, You should return the new head after reverse, or the new list will lost.

#include <stdio.h>

struct Node
{
    int data;
    Node* next;
    Node(int value, Node* ptr):data(value), next(ptr){}
};

// Reverse list and return the new head
Node* reverse_list(Node* head)
{
    Node* p = NULL;
    Node* q = head;
    while(q)
    {
        Node* t = q->next;
        q->next = p;
        p = q;
        q = t;
    }

    return p; // new head
}

void print_list(Node* head)
{
    Node* t = head;
    while(t)
    {
        printf("%d\n", t->data);
        t = t->next;
    }
}

void main()
{
    Node* head = new Node(1, NULL);
    Node* p = head;
    for(int i = 2; i < 10; ++i)
    {
        p->next = new Node(i, NULL);
        p = p->next;
    }
    print_list(head);

    Node* newHead = reverse_list(head);
    print_list(newHead);

    getchar();
}


Alternative to env that accepts arguments It is commonly known that env does not allow passing of arguments - at least not reliably. There are various workarounds described here, but they felt to hacky and unreadable to me. That's why I developed an alternative, ex.

ex:

#!/usr/bin/env bash

# save the string given in the sheband line
shebang_line="$1"

# remove it from $@
shift

# execute
# not quoting shebang_line is intentional
eval $(echo $shebang_line "$@")

test.py

#!/usr/local/bin/ex python "-i"
# quoting the arguments is possible
import sys

print('hello', sys.argv)

It works surprisingly well, but I don't like the eval-echo thing. I tried experimenting with arrays, but haven't found a solution. Any tips?

I don't recommend calling your program ex.  That name has a well established meaning in the Unix world as the name of a text-editing tool.


Selecting from a database into a DataGridView I have a piece of code which grabs data from an SQL database and displays it in a DataGridView. This code works perfectly fine for me.

It has been pointed out to me that the below code is bad programming:

Imports System.Data.SqlClient

Public Class Form1

Dim dbConnection As SqlConnection
Dim dbCommand As SqlCommand
Dim dbAdapter As SqlDataAdapter
Dim DataSet As New DataSet
Dim strSQL As String
Dim dbCount As String

Public Sub SQLConnect()
dbConnection = New SqlConnection("Data Source=connectionhere\sqlexpress;Initial Catalog=line_log;Integrated Security=True")
dbConnection.Open()
End Sub

Public Sub SQLCommand()
dbCommand = New SqlCommand(strSQL, dbConnection)
End Sub

Public Sub SQLAdapter()
dbAdapter = New SqlDataAdapter(strSQL, dbConnection)
End Sub

Public Sub SQLDisconnect()
dbConnection.Close()
End Sub

Public Sub DGVLoad()
Try
SQLConnect()
strSQL = "SELECT * FROM [Products]"
SQLAdapter()
DataSet.Clear()
dbAdapter.Fill(DataSet)
SQLDisconnect()
DataGridView1.DataSource = DataSet.Tables(0)

Catch ex As Exception
MsgBox(ex.ToString)
End Try
End Sub

Private Sub Form1_Load(sender As Object, e As EventArgs) Handles MyBase.Load
DGVLoad()
End Sub

Could someone explain to me why what I am doing is bad practice?

Three possible problems that I see.

dbConnection = New SqlConnection("Data Source=connectionhere\sqlexpress;Initial Catalog=line_log;Integrated Security=True")

You have a hard-coded connection string. If you want a different database location then you need to edit the code. Perhaps it's better to have the connection string in an editable config file.

The original version which prompted this review ...

strSQL = "SELECT [item] FROM [table] WHERE ([item] = 'value')"

... seems to build the select statement by string manipulation of the value and therefore may be vulnerable to SQL injection.

You have SQL code and Form code in the same source file. It may be better to separate these into different classes so that they can be tested and re-used independently.


FutureTaskWrapper for Java I did not like that get synchronizes every time I call it - because I may be calling it more than once. So I wrote the wrapper for the future interface which keeps not synchronized local variable for completion state and caches the result. I am very sure it is thread safe.
Please have a look:

package i;

import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.FutureTask;
import java.util.concurrent.RunnableFuture;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;

public class FutureTaskWrapper<V> implements RunnableFuture<V> {

    private FutureTask<V> localFuture;
    private V result;
    //this one is not synchronized on purpose. It does not have to be.
    //It will either use wrapped class' done mechanism or eventually will     update and it will use cached result.
    private boolean done = false;

    public FutureTaskWrapper(FutureTask<V> instance) {
        localFuture = instance;
    }

    @Override
    public void run() {
        localFuture.run();
    }

    @Override
    public boolean cancel(boolean mayInterruptIfRunning) {
        return localFuture.cancel(mayInterruptIfRunning);
    }

    @Override
    public boolean isCancelled() {
        return localFuture.isCancelled();
    }

    @Override
    public boolean isDone() {
        if (done) {
            return true;
        } else {
            return localFuture.isDone();
        }
    }

    @Override
    public V get() throws InterruptedException, ExecutionException {
        if (done) {
            System.out.println("using cached version.");
            return result;
        } else {
            System.out.println("using version version from future.");
            result = localFuture.get();
            done = true;
            return result;
        }
    }

    @Override
    public V get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException {
        if (done) {
            System.out.println("using cached version.");
            return result;
        } else {
            System.out.println("using version version from future.");
            result = localFuture.get(timeout, unit);
            //could clean up localFuture here.
            done = true;
            return result;
        }
    }

    //main method for testing.
    public static void main(String[] args) throws ExecutionException, InterruptedException {
        ExecutorService executor = Executors.newCachedThreadPool();
        FutureTask<String> future = new FutureTask<>(() -> {
            Thread.sleep(1000L);
            return "future";
        });
        FutureTaskWrapper<String> wrapper = new FutureTaskWrapper(future);
        executor.submit(future);
        wrapper.get();
        wrapper.get();
        wrapper.get();
        wrapper.get();

        executor.shutdownNow();
    }
}

I am very sure it is thread safe.

Actually, it's not because the done is not in happens-before relations with the result.

But you can reach the desired behavior without extending anything: just check isDone() and call get() only if it was true.


Simple Morse code interpreter This is a simple Morse code interpreter with a CLI. It's a naive table-lookup implementation, and uses my-py for type checking. It automatically detects the input (English or Morse) and converts accordingly. It has the simplest possible CLI that I could think of. There is a git repo here.

I don't often write Python; I write Ruby for work and Clojure in my free time, but I do love Python and have a bit of experience with it. I welcome any suggestions on how to write more "Pythonic" code, simpler code or interfaces, or how to make the code more robust. Ideally, I want it to "just work" and be able to take even malformed data. Unfortunately, I think I need to keep the msg argument surrounded by quotes from the command line, otherwise the shell will interpret a single or double dot as the current or parent directory.

I'm considering using the Boyer-Moore algorithm instead of a lookup table, but I'm not sure if that's worth it.

import re
import argparse
from typing import Dict

eng = {
    "a" : ".-",
    "b" : "-...",
    "c" : "-.-.",
    "d" : "-..",
    "e" : ".",
    "f" : "..-.",
    "g" : "--.",
    "h" : "....",
    "i" : "..",
    "j" : ".---",
    "k" : "-.-",
    "l" : ".-..",
    "m" : "--",
    "n" : "-.",
    "o" : "---",
    "p" : ".--.",
    "q" : "--.-",
    "r" : ".-.",
    "s" : "...",
    "t" : "-",
    "u" : "..-",
    "v" : "...-",
    "w" : ".--",
    "x" : "-..-",
    "y" : "-.--",
    "z" : "--..",
    " " : "/",
    "0" : "-----",
    "1" : ".----",
    "2" : "..---",
    "3" : "...--",
    "4" : "....-",
    "5" : ".....",
    "6" : "-....",
    "7" : "--...",
    "8" : "---..",
    "9" : "----.",
    "," : ",",
    "." : ".",
    "!" : "!",
    "?" : "?",
} # type: Dict[str, str]

morse = {v: k for k, v in eng.items()} # type: Dict[str, str]

def morsep(msg: str) -> bool:
    if msg[0][0] in ['.', '-', '/']:
        return True
    else:
        return False

def morse_to_eng(msg: str) -> str:
    out = "" # type: str
    split_msg = re.split(" ", msg) # type: list
    filtered_msg = [e for e in split_msg if e] # type: list
    for char in filtered_msg:
        c = morse[char] # type: str
        out += c
    return("".join(out))

def eng_to_morse(msg: str) -> str:
    out = "" # type: str
    for char in msg:
        c = eng[char] # type: str
        out += "{} ".format(c)
    return("".join(out))

def decode(msg: str) -> str:
    "Main control flow."
    if morsep(msg) == True:
        return(morse_to_eng(msg))
    else:
        return(eng_to_morse(msg))

if __name__ == '__main__':
    parser = argparse.ArgumentParser(description='Translate English to Morse code, and vice versa.')
    parser.add_argument('-v', '--version', help='Show the version.', action='version', version='Morse 0.1')
    parser.add_argument('msg', type=str, nargs=1,
                        help='The message to be decoded. Input type will be automatically detected and converted. \
                        Must be wrapped in quotes.')

    args = parser.parse_args()
    out = decode(args.msg[0]) # type: str
    print("{}".format(out))

Overall, this code was very simple and easy to follow. I just have a few nit-picks to point out.

The function syntax

def func_name(param_name: type) -> return_type:

is new in Python 3. However, it is not a very common thing to be used in Python, and I don't think it looks very pythonic.

Your code will work just as well without making functions like this, so I recommend that you stick to using the usual syntax:

def func_name(param_name):

This clunky statement:

if msg[0][0] in ['.', '-', '/']:
    return True
else:
    return False

can be reduced to a more pythonic and clean version:

return msg[0][0] in ['.', '-', '/']

Comments like this:

# type: list

are really unnecessary and just add noise to your code. Just from reading your code, someone reviewing it should be able to determine what type each variable is.

You need to do some more error catching.

For example, say you are translating a string from morse code to english. What if you encounter a character that is not a valid morse character? Your program will raise an error here:

for char in msg:
    c = eng[char] # <======
    out += "{} ".format(c)

Therefore, this is what you should do:

Encase that line in a try/except.
In the except, throw a custom InvalidMorseLetter exception.
Exit

Here is what I mean:

try:
    c = eng[char] # type: str
except KeyError:
    raise(InvalidMorseLetter)

where InvalidMorseLetter is:

class InvalidMorseLetter(Exception):
    pass

Read here and here.

In the code, I wrote to catch a KeyError because that is the error that would be raised if an invalid dictionary key is used.

You should use a custom exception because it specifically describes what went wrong with your specific code, rather than just a general KeyError.

You should do something very similar to this for when you are translating english to morse.

Thanks to Zenohm for this.

In your translation functions, you should not be constantly modifying a single string each iteration:

c = morse[char] # type: str
out += c # <======================

The reason: python strings are immutable. Therefore, by constantly changing the string, you are actually creating a new string each and every time. This is very inefficient.

As Zenohm posted in the comments, you should be doing something along the lines of this:

return ''.join("{} ".format(eng[char]) for char in msg))

  While the modifying of a single string does not a bad habit make and
  isn't really a problem here, I think it would be good if he started
  avoiding the modifying of an immutable object right off the bat.
  return ''.join("{} ".format(eng[char]) for char in msg) Could be used
  in place of his original string modification and would help to
  encourage better practice in future.


finding names of un-named list elements I have a naming problem: lists with un-named elements.  I wish to get the names of the elements in the list, without going back upstream to where the list was created. An example is modelList below:

## naming problem
lmNms <- c( "mod1", "mod2", "mod3", "mod4", "mod5", "mod6")
lapply(lmNms,
       function(N) assign(N, lm(runif(10) ~ rnorm(10)), env = .GlobalEnv))
modelList <- list(mod1, mod2, mod3, mod4, mod5, mod6)

I have written the below function which takes the list and the environment as arguments (as well as an argument to return names only), looks for a matching object in the target environment, and then attaches that name to the element and returns the list.

nameListObjects <- function(LIST, ENV = NULL, NAMES.ONLY = FALSE) {
    if(is.null(ENV)) ENV <- .GlobalEnv
    for(i in seq_along(LIST)){
        # check the class of all objects in the target environment
        classMatches <- sapply(ls(ENV),
                               function(N) class(get(N)) == class(LIST[[i]]))
        # see which objects of matching class are all.equal to the subject
        TF <- sapply(names(classMatches[classMatches]),
                 function(N) is.logical(all.equal(LIST[[i]], get(N))))
        names(LIST)[i] <- names(classMatches[classMatches])[TF]
    }
    if(NAMES.ONLY) names(LIST) else LIST
}

R>nameListObjects(modelList, NAMES.ONLY=TRUE)
[1] "mod1" "mod2" "mod3" "mod4" "mod5" "mod6"

This took me to the edge of my R ability.

I'm interested in better ways to do this (i presume there are multiple better ways), problems with this approach, and any other comments.

You could have used identical to compare objects. However, I would recommend representing all objects by their MD5 digests, so you can then use match to find the matches:

nameListObjects <- function(LIST, ENV = NULL, NAMES.ONLY = FALSE) {

  if(is.null(ENV)) ENV <- .GlobalEnv
  require(digest)
  list.md5   <- sapply(LIST, digest)
  env.names  <- ls(envir = ENV)
  env.md5    <- sapply(env.names, function(x)digest(get(x, envir = ENV)))
  list.names <- env.names[match(list.md5, env.md5)]

  if(NAMES.ONLY) list.names else setNames(LIST, list.names)
}

nameListObjects(modelList, NAMES.ONLY=TRUE)
[1] "mod1" "mod2" "mod3" "mod4" "mod5" "mod6"


JavaScript Anagram checker I have written a code that finds whether the entered words are anagrams or not. I tested and it works, but I know that there are some details that I must change or modify. So what do you think I have to change in this code?

function findAnagram (firstWord, secondWord) {
var testWord = "";
var c = 0;
while (c < secondWord.length || c < firstWord.length) {
    while (secondWord[c] == " " || firstWord[c] == " ") {
    secondWord = secondWord.replace(" ", "");
    firstWord = firstWord.replace(" ", "");
}
c++;
}

if (firstWord.length == secondWord.length) {
    for (var i = 0; i < firstWord.length; i++) {
    for (var k = 0; k < secondWord.length; k++) {
        if (firstWord[i] == secondWord[k]) {
            testWord += firstWord[i];
            secondWord = secondWord.replace(secondWord[k], "")
            break;
        }
    }
}
}
if (firstWord == testWord){
    return "Anagram !";
}
else {
    return "Not an Anagram";
}
}
console.log(findAnagram("funeral", "real fun"));

Your function has a high complexity because of the 2 nested loops.

Here is my implementation. I'm not sure if it's more efficient, but I think it's more readable.

function findAndAppendCharacter(countArray, currentChar) {
    let foundAt = -1

    countArray.forEach((pair, i) => {
        if(pair.letter == currentChar) {
            foundAt = i
        }
    })

    if(foundAt == -1) {
        countArray.push({
            letter: currentChar,
            count: 1
        })

        return
    }

    countArray[foundAt].count++
}

function findAnagram(firstWord, secondWord) {
    firstWord = firstWord.replace(" ", "")
    secondWord = secondWord.replace(" ", "")

    if(firstWord.length != secondWord.length) {
        return "Not an anagram!"
    }

    let letterCountArray = {
        first: [],
        second: []
    }

    for(let i = 0; i < firstWord.length; i++) {
        findAndAppendCharacter(letterCountArray.first, firstWord.charAt(i).toLowerCase())
        findAndAppendCharacter(letterCountArray.second, secondWord.charAt(i).toLowerCase())
    }

    if(letterCountArray.first.length != letterCountArray.second.length) {
        return "Not an anagram!"
    }

    for(let i = 0; i < letterCountArray.first.length; i++) {
        let firstPair = letterCountArray.first[i]
        let secondPair = letterCountArray.second[i]

        if(letterCountArray.first.filter(pair => pair.letter == secondPair.letter && pair.count == secondPair.count).length != 1) {
            return "Not an anagram!"
        }

        if(letterCountArray.second.filter(pair => pair.letter == firstPair.letter && pair.count == firstPair.count).length != 1) {
            return "Not an anagram!"
        }
    }

    return "Anagram!"
}

I'm also curious about a better solution :)


c++ Checking if a string is an isogram So I'm starting out and doing some code challenges in between book and tutorial learning and was hoping to get some general feedback on code readability and use of comments.

I know it's sometimes hard to understand your own code after you wrote it so my question is - how easy is it to understand this one based on comments?

Do you think I'm missing anything crucial? Any general feedback welcome, thanks.

/*
https://edabit.com/challenge/SEzYX5qtTR4WcZeR3
An isogram is a word that has no repeating letters, consecutive or
nonconsecutive.Create a function that takes a string and returns
either true or false depending on whether or not it's an "isogram".
Examples
isIsogram("Algorism") ➞ true
isIsogram("PasSword") ➞ false
// Not case sensitive.
isIsogram("Consecutive") ➞ false
*/

#include <iostream>
#include <ctype.h> // to use the toupper function which takes a letter
character and returns the upper case ASCII integer
using namespace std;

int main()
{
    string input_string;
    string output_string;
    bool isogram_check = true; // by default the program assumes that    everything is an isogram and then tries to find repeating letters to assert that it's not an isogram

    cout << "Enter a string." << endl;
    cin >> input_string;

    for (int i = 0; i < input_string.length(); i++) // this loop takes the input string and converts it to all upper case letters.
    {
        output_string = output_string + char(toupper(input_string[i]));
    }

    // cout << output_string << endl;
    // the above line is to test the toupper output

    for (int i = 0; i < output_string.length(); i++)       // the first loop decides WHAT to compare to something else, the second loops decides what that SOMETHING ELSE is.
    {
        for (int o = i+1; o < output_string.length(); o++) // this loop takes the character at position i and compares it to the character at i+1 then i+2, etc. by iterating o.
        {                                                  // in the above comment, i+1 = o since we iterated i once right at the beginning.
            if (output_string[i] == output_string[o])      // when the loop finds two of the same character within the string it brakes out of both loops
            {
                isogram_check = false;
                break;
            }
        }

        if (isogram_check == false)
        {
            break;
        }
    }

    if (isogram_check == false)
    {
        cout << input_string << " is not an isogram.\n";
    }
    else
    {
        cout << input_string << " is an isogram.\n";
    }
}

Avoid using namespace std.

Keep your lines shorter.  Having to constantly scroll back and forth to read lines is unpleasant.  Most of your comments should be on one (or more) lines right before the statement being commented on.  Also, avoid obvious things in comments (like "resets i to use in the next loop.").

Declare variables as close as possible to their first use.

Since isogram_check is a bool, you can test it with !isogram_check instead of isogram_check == false.

With a slight change in algorithm (searching characters before the ith one), your two while loops can be combined into one. Although there are other ways to do this that could be better (for some definition of "better"), your implementation is a quick first pass that works well for short strings (but less well as the strings get longer).


Executing an SQL command on a number of files Is this thread safe?

I have a program that executes an SQL command on a number of files (selected by the user).

Here is where I create the threads:

var allThreads = new List<Task<bool>>();
if (fileList != null)
{

    foreach (var file in fileList)
    {
        var fileName = Path.GetFileName(file);
        if (File.Exists(file))
        {
            Log.AddToLog("Beginning to Process File: " + fileName);
            if (!ValidateFile(file))
            {
                Log.AddToLog(String.Format("Error! {0} is not a valid file for the {1}. Skipping this file.", fileName, JsonSettingsList.NameId));
                IsErrorsInLog = true;
            }
            else
            {
                var fileToProcess = file; //need to make copy for threading loop, otherwise it acts screwy
                var newTask = Task.Factory.StartNew<bool>(() => ProcessFile(fileToProcess));
                allThreads.Add(newTask);
            }
        }
        else
        {
            Log.AddToLog(String.Format(CultureInfo.CurrentCulture, "File {0} Does not Exist. File not processed.", fileName));
            IsErrorsInLog = true;
        }
    }

    //check result, if any of them return true, then there are errors in the log, and the user needs to be alerted.
    foreach (var task in allThreads)
    {
        try
        {
            var didThreadFail = task.Result;
            if (!IsErrorsInLog)
            {
                IsErrorsInLog = didThreadFail;
            }
        }
        catch (AggregateException aggEx)
        {
            foreach (Exception ex in aggEx.InnerExceptions)
            {
                Log.AddToLog(string.Format("Caught exception '{0}'",
                    ex.Message));
                IsErrorsInLog = true;
            }
        }
        finally
        {
            task.Dispose();
        }
    }
}

and then here is the code where I am actually accessing the database, inside of the ProcessFile() function:

//initialize oracle connection
oraConnection.Open();
using (var command = new OracleCommand(sqlText, oraConnection))
{
    command.Transaction = command.Connection.BeginTransaction();
    foreach (var line in fileByLine.Where((i, index) => index > 0))
    {
        if (!String.IsNullOrWhiteSpace(line))
        {
            //add parameters
            command.ExecuteNonQuery();
        }
    }

    command.Transaction.Commit();
    Log.AddToLog("Successfully Processed " + filePath);
}

oraConnection.Close();

The comments are where I have omitted unnecessary code for the sake of simplicity.

if (fileList != null)

You can reduce nesting (which is usually a good thing) by turning this into

  if (fileList == null) { return; }

The same applies to

if (File.Exists(file))

It's better to have

if (!File.Exists(file)) { continue; }

But even better then this would be to filter the list before you start processing it:

fileList.Where(File.Exists)

Checking the fileList for null is an indicator that some other part of the code might be implemented in a wrong way_. In C# we prefer to have an empty list rather then a null.

Parallel.ForEach

Have you already tried the Parallel.ForEach? It would greatly simplify your code:

Parallel.ForEach(fileList.Where(File.Exists), fileName =>
{
    // process the file
});

This method has a lot of overloads so you'll probably find something that suits your needs.

With one of them you could for example create a connection for each parallel loop and dispose it at the end of the processing.

Here's a bigger example that might work for you. This will use the same connection for several files.

try
{
    var parallelOptions = new ParallelOptions
    {
#if DEBUG
        MaxDegreeOfParallelism = 1
#else
        MaxDegreeOfParallelism = Environment.ProcessorCount
#endif
    };

    var parallelLoopResult = Parallel.ForEach
    (
        source: fileList.Where(File.Exists),
        parallelOptions: parallelOptions,
        localInit: () => new
        {
            Connection = /* init the oracle connection*/
        },
        body: (fileName, loopState, i, local) =>
        {
            // process the file
            // use the local.Connection
            return local; // pass the connection to the next loop
        },
        localFinally: local =>
        {
            local.Connection.Dispose();
        }
    );
}
catch (AggregateException ex)
{
    // handle exceptions
}

See this question on SO, it compares your solution with Parallel.ForEach:

Parallel.ForEach vs Task.Factory.StartNew


Simplifying GPA calculator I'm only two weeks into learning Java and just finished creating my first program.  I have a feeling I've added unnecessary things, but not sure. I'd like someone to take a look at it and let me know if there are ways to simplify or approach the code.  Basically, I'm just trying to create a simple GPA calculator.

import java.util.Scanner;

class GpaCalculator {

    public static void main(String[] args) {
        Scanner myScanner = new Scanner(System.in);
        char c1;
        double gpa;
        double g1, g2, g3, g4, g5, g6;

        g1 = 0;
        g2 = 0;
        g3 = 0;
        g4 = 0;
        g5 = 0;
        g6 = 0;

        System.out.println("What is your grade for class 1?");
        c1 = myScanner.findWithinHorizon(".", 0).charAt(0);
        if (c1 == 'a' || c1 == 'A') {
            g1 = 4;
        }
        if (c1 == 'b' || c1 == 'B') {
            g1 = 3;
        }
        if (c1 == 'c' || c1 == 'C') {
            g1 = 2;
        }
        if (c1 == 'd' || c1 == 'D') {
            g1 = 1;
        }
        if (c1 == 'f' || c1 == 'F') {
            g1 = 0;
        }
        System.out.println("What is your grade for class 2?");
        c1 = myScanner.findWithinHorizon(".", 0).charAt(0);
        if (c1 == 'a' || c1 == 'A') {
            g2 = 4;
        }
        if (c1 == 'b' || c1 == 'B') {
            g2 = 3;
        }
        if (c1 == 'c' || c1 == 'C') {
            g2 = 2;
        }
        if (c1 == 'd' || c1 == 'D') {
            g2 = 1;
        }
        if (c1 == 'f' || c1 == 'F') {
            g2 = 0;
        }
        System.out.println("What is your grade for class 3?");
        c1 = myScanner.findWithinHorizon(".", 0).charAt(0);
        if (c1 == 'a' || c1 == 'A') {
            g3 = 4;
        }
        if (c1 == 'b' || c1 == 'B') {
            g3 = 3;
        }
        if (c1 == 'c' || c1 == 'C') {
            g3 = 2;
        }
        if (c1 == 'd' || c1 == 'D') {
            g3 = 1;
        }
        if (c1 == 'f' || c1 == 'F') {
            g3 = 0;
        }
        System.out.println("What is your grade for class 4?");
        c1 = myScanner.findWithinHorizon(".", 0).charAt(0);
        if (c1 == 'a' || c1 == 'A') {
            g4 = 4;
        }
        if (c1 == 'b' || c1 == 'B') {
            g4 = 3;
        }
        if (c1 == 'c' || c1 == 'C') {
            g4 = 2;
        }
        if (c1 == 'd' || c1 == 'D') {
            g4 = 1;
        }
        if (c1 == 'f' || c1 == 'F') {
            g4 = 0;
        }
        System.out.println("What is your grade for class 5?");
        c1 = myScanner.findWithinHorizon(".", 0).charAt(0);
        if (c1 == 'a' || c1 == 'A') {
            g5 = 4;
        }
        if (c1 == 'b' || c1 == 'B') {
            g5 = 3;
        }
        if (c1 == 'c' || c1 == 'C') {
            g5 = 2;
        }
        if (c1 == 'd' || c1 == 'D') {
            g5 = 1;
        }
        if (c1 == 'f' || c1 == 'F') {
            g5 = 0;
        }
        System.out.println("What is your grade for class 6?");
        c1 = myScanner.findWithinHorizon(".", 0).charAt(0);
        if (c1 == 'a' || c1 == 'A') {
            g6 = 4;
        }
        if (c1 == 'b' || c1 == 'B') {
            g6 = 3;
        }
        if (c1 == 'c' || c1 == 'C') {
            g6 = 2;
        }
        if (c1 == 'd' || c1 == 'D') {
            g6 = 1;
        }
        if (c1 == 'f' || c1 == 'F') {
            g6 = 0;
        }

        gpa = (g1 + g2 + g3 + g4 + g5 + g6) / 6;

        System.out.println("Your GPA is " + gpa + ". ");

        if (gpa > 3.00) {
            System.out.println("Gret job, you're on your way to success.");
        } else if (gpa > 2.00) {
            System.out.println("You did OK, but better than average.");
        } else {
            System.out.println("You didn't do too well. You should look into getting a tutor.");
        }
    }
}

First off, both Grade and Class (GradedClass to avoid confusion with java.lang.Class) make sense as enums.

enum Grade {
    A(4),B(3),C(2),D(1),F(0);

    private final int value;

    Grade(int value) {
        this.value = value;
    }

    int getValue() {
        return value;
    }
}

enum GradedClass {
    ONE, TWO, THREE, FOUR, FIVE, SIX;

    @Override
    public String toString() {
        return "class " + (ordinal() + 1);
    }
}

This will allow a much simpler calculation loop :

    public double calculateGpa() {
        return calculateTotal() / GradedClass.values().length;
    }

    private double calculateTotal() {
        double total = 0d;
        for (GradedClass gradedClass : GradedClass.values()) {
            Grade grade = userInterface.askGradeFor(gradedClass);
            total += grade.getValue();
        }
        return total;
    }

Note that I've hidden away the scanner behind a UserInterface interface, in order to separate calculation from UI code.

It's implemented by the ConsoleUI class :

class ConsoleUI implements UserInterface {
    Scanner myScanner = new Scanner(System.in);

    @Override
    public Grade askGradeFor(GradedClass gradedClass) {
        System.out.println(MessageFormat.format("What is your grade for {0}?", gradedClass));
        String input = myScanner.findWithinHorizon(".", 0);
        return Grade.valueOf(input.toUpperCase());
    }

    @Override
    public void presentGpa(double gpa) {
        if (gpa > 3d) {
            System.out.println("Great job, you're on your way to success.");
        } else if (gpa > 2d) {
            System.out.println("You did OK, but better than average.");
        } else {
            System.out.println("You didn't do too well. You should look into getting a tutor.");
        }
    }

}

The Main class simply ties everything together :

public class Main {

    public static void main(String[] args) {
        UserInterface ui = new ConsoleUI();
        GpaCalculator gpaCalculator = new GpaCalculator(ui);

        double gpa = gpaCalculator.calculateGpa();
        ui.presentGpa(gpa);
    }

}


Avoid if/else in constructor - NavigableMap? I have the following code, based on the input to the constructor I need to initialise some values which are wrapped in a simple POJO. These values are constant as you can see.

However I don't like the if/else construct in the set method and had read about NavigableMap as an alternative for holding range values. Any thoughts on how to improve/clean the below or indeed use this construct? I would like to avoid conditional logic where possible.

private Calibrated calibratedValues;

public CalibrationProperties(long odNumber) {
    setCalibratedValues(odNumber);
}

private void setCalibratedValues(long odNumber) {
if (odNumber < 762) {
    this.calibratedValues = new Calibrated(K0, H0, K0_INV, H0_INV, F0);
} else if (odNumber < 866) {
    this.calibratedValues = new Calibrated(K1, H1, K0_INV, H0_INV, F1);
} else if (odNumber < 1011) {
    this.calibratedValues = new Calibrated(K2, H2, K2_INV, H2_INV, F2);
} else {
    this.calibratedValues = new Calibrated(K3, H3, K3_INV, H3_INV, F3);
}

//Two exceptions
if (odNumber == 858){
    this.calibratedValues = new Calibrated(K2, H2, K2_INV, H2_INV, F2);
}
if (odNumber == 1005){
    this.calibratedValues = new Calibrated(K3, H3, K3_INV, H3_INV, F3);
}
}

public Calibrated getCalibratedValues() {
return calibratedValues;
}

/** Convenience class used to hold calibrated values */
static class Calibrated {
private double[] k;
private double[] h;
private double[] kinv;
private double[] hinv;
private double f;

Calibrated(double[] k, double[] h, double[] kinv, double[] hinv, double f) {
    this.k = k;
    this.h = h;
    this.kinv = kinv;
    this.hinv = hinv;
    this.f = f;
}

public double[] getK() {
    return k;
}

public double[] getH() {
    return h;
}
...

A NavigableMap would just move the current logic into an object.  You'd get something like

final static NavigableMap<Long, Calibrated> OD_CALIBRATIONS = new TreeMap();

static {
    OD_CALIBRATIONS.put(Long.MIN_VALUE, new Calibrated(K0, H0, K0_INV, H0_INV, F0));
    OD_CALIBRATIONS.put(762, new Calibrated(K1, H1, K0_INV, H0_INV, F1));
    OD_CALIBRATIONS.put(858, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_CALIBRATIONS.put(859, new Calibrated(K1, H1, K0_INV, H0_INV, F1));
    OD_CALIBRATIONS.put(866, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_CALIBRATIONS.put(1005, new Calibrated(K3, H3, K3_INV, H3_INV, F3));
    OD_CALIBRATIONS.put(1006, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_CALIBRATIONS.put(1011, new Calibrated(K3, H3, K3_INV, H3_INV, F3));
}

private void setCalibratedValues(long odNumber) {
    this.calibratedValues = OD_CALIBRATIONS.floorEntry(odNumber).getValue();
}

Note:  I'm not sure about the map key initializations.  They get kind of weird since you are mixing less than ranges with single value equalities.  You could pull the exceptions out of the NavigableMap and put them in a regular Map instead.  That would make the code look something like

final static MAP<Long, Calibrated> OD_EXCEPTION_CALIBRATIONS = new HashMap();
final static NavigableMap<Long, Calibrated> OD_CALIBRATIONS = new TreeMap();

static {
    OD_EXCEPTION_CALIBRATIONS.put(858, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_EXCEPTION_CALIBRATIONS.put(1005, new Calibrated(K3, H3, K3_INV, H3_INV, F3));

    OD_CALIBRATIONS.put(Long.MIN_VALUE, new Calibrated(K0, H0, K0_INV, H0_INV, F0));
    OD_CALIBRATIONS.put(762, new Calibrated(K1, H1, K0_INV, H0_INV, F1));
    OD_CALIBRATIONS.put(866, new Calibrated(K2, H2, K2_INV, H2_INV, F2));
    OD_CALIBRATIONS.put(1011, new Calibrated(K3, H3, K3_INV, H3_INV, F3));
}

private void setCalibratedValues(long odNumber) {
    if (OD_EXCEPTION_CALIBRATIONS.containsKey(odNumber)) {
        this.calibratedValues = OD_EXCEPTION_CALIBRATIONS.get(odNumber);
    } else {
        this.calibratedValues = OD_CALIBRATIONS.floorEntry(odNumber).getValue();
    }
}

Note that calling setCalibratedValues with both 762 and 763 will give each the same Calibrated object.  If that's not what you want, you could give Calibrated a copy constructor and make a new one each time.  Also note that this method requires creating all the calibrations at the beginning.  This can be wasteful if you only use one per program invocation.  Not so bad now, but what if you get more calibration sets?

It would be possible to put the rules into a file or files and then read them into the objects.  That would eliminate most of the constants.  You'd have to make sure that you read the file by the first time that you needed the values.  Perhaps put them into a Singleton object?  At worst case then, the Singleton constructor could read the file the first time that it was needed.  Or you could prime it earlier.

Some things that I don't like about setCalibratedValues that you didn't mention:

What's an odNumber?  Unless odNumber means something, you might as well call it n.  If odNumber does mean something, you should tell me so that I know it too.  If odNumber isn't a standard name, then I'd much prefer it was written out.  Why write out number which should be obvious from the type and leave od obscured?
Similarly, do k, h, k_inv, h_inv, and f have meaning?  I'd have trouble editing that code, as I have no idea what each of those means.  I sort of think that k and k_inv have something to do with each other, but no idea what.
Also, why are most of them arrays?  This should be understandable from the code, and it isn't here.
Your constants are named K0, H1, etc.  Why not give these more meaningful names rather than just 0, 1, 2, 3?  Of course, this could also argue in favor of moving these out of code entirely.  If position is all that separates them, why give them names at all?
You're repeating the exact same calibratedValues and overwriting previous values.

Why not something like

private void setCalibratedValues(long odNumber) {
    if (odNumber < 762) {
        this.calibratedValues = new Calibrated(K0, H0, K0_INV, H0_INV, F0);
    } else if (odNumber < 866 && odNumber != 858) {
        this.calibratedValues = new Calibrated(K1, H1, K0_INV, H0_INV, F1);
    } else if (odNumber < 1011 && odNumber != 1005) {
        this.calibratedValues = new Calibrated(K2, H2, K2_INV, H2_INV, F2);
    } else {
        this.calibratedValues = new Calibrated(K3, H3, K3_INV, H3_INV, F3);
    }
}

This reduces your repeated code.  Of course, it won't work as well if the exceptions aren't just off by one in the ranges.  Although if 1050 were supposed to get the 0 set, then you could use an || to move that value from the 3 set to the 0.

We might be able to suggest something better if we knew what odNumber, k, h, k_inv, h_inv, and f were and why they are related.  For example, if odNumber is the Orson Dines catalog number of a set and the other numbers are dimensions of the parts, you might put those in a database.  Then you could read the values from the database as necessary.


PHP handle array when query database with one-to-many relationship Well, I am writing a web service using PHP to return data from a DB in JSON format. I need to query 3 tables, which have a one-to-many relationship, like shown below:

Contact Group ---(has)---> Contact ---(has)---> Phone number

Here I want to get an array of each group, in each group, get an array of all contacts and in each contact, get an array of all phone numbers. Because I am new to PHP, this might not be following best practices for PHP. The code I wrote is below:

$sql = "SELECT r.contact_id, contact_name, group_id, group_name, phone_id, phone_number FROM contact r LEFT JOIN contact_group s ON r.contact_group = s.group_id LEFT JOIN phone_numbers t ON r.contact_id = t.contact_id ORDER BY group_id, contact_id, phone_id";
$query = mysqli_query($conn,$sql);
$data = array();

while ($row = mysqli_fetch_array($query)) {
    $data[$row["group_id"]]["group_id"] = $row["group_id"];
    $data[$row["group_id"]]["group_name"] = $row["group_name"];

    $data[$row["group_id"]]["contacts"][$row["contact_id"]]["id"] = $row["contact_id"];
    $data[$row["group_id"]]["contacts"][$row["contact_id"]]["name"] = $row["contact_name"];

    $data[$row["group_id"]]["contacts"][$row["contact_id"]]["phone_number"][$row["phone_id"]]["id"] = $row["phone_id"];
    $data[$row["group_id"]]["contacts"][$row["contact_id"]]["phone_number"][$row["phone_id"]]["number"] = $row["phone_number"];
}

I think my code is very ugly and seems difficult to read. Could you help me review my code and refactor it to make it more beautiful or introduce new ways to solve that problem?

First, let's start with the way your query is formatted.  I would strongly recommend you get in the habit of breaking your query across lines when they get long like this.  I oftentimes use heredoc/nowdoc synteax for this.

For example:

$sql = <<<EOT

SELECT
  r.contact_id,
  contact_name,
  group_id,
  group_name,
  phone_id,
  phone_number
FROM contact r
LEFT JOIN contact_group s
  ON r.contact_group = s.group_id
LEFT JOIN phone_numbers t
  ON r.contact_id = t.contact_id
ORDER BY group_id, contact_id, phone_id

EOT;

This gives you a nice, clean visual presentation for your query in your code.

I think your query as written is a little odd, in that you specify the table for contact_id but not for any of the other columns you are selecting.  Some of your column names may be ambiguous across tables, so I try to make it a practice when writing join queries to specify the table names for ALL columns to remove ambiguity for the reader.

I would also suggest aliasing the columns.  If you want to refer to contact_id in your PHP code like you are doing, then you MUST alias r.contact_id to this like r.contact_id AS contact_id.

Finally, you should consider meaningful table aliases if using them (I actually typically do not use them except for sub-selects that need to be aliased).  What do r, s, and t mean? Why not c, cg, pn or similar?

That could leave you with a query like:

$sql = <<<EOT

SELECT
  contact.contact_id AS contact_id,
  contact.contact_name AS contact_name,
  contact_group.group_id AS group_id,
  contact_group.group_name AS group_name,
  phone_numbers.phone_id AS,
  phone_numbers.phone_number AS phone_number
FROM contact
LEFT JOIN contact_group
  ON contact.contact_group = contact_group.group_id
LEFT JOIN phone_numbers t
  ON contact_group.contact_id = phone_numbers.contact_id
ORDER BY group_id, contact_id, phone_id

EOT;

This is a little more verbose, and might take you a minute or two of extra typing when initially writing the query, but it has the effect of removing any ambiguity for what your query is doing for the reader.

Outside of format, your query seems odd in that it doesn't seem to match the relations you have noted.  You said the relationships are:

Contact Group ---(has)---> Contact ---(has)---> Phone number

Yet your joins would seem to suggest relations of:

Contact ---(has)---> Contact Group ---(has)---> Phone number

Can a contact exist that does not belong to a contact_group?  Your query suggests this is the case (due to LEFT JOIN), which would beg the question, how are you handling the nulls that could be returned for contact_group fields in such a case?

You are taking a very standard approach to populating a multi-dimensional array from a results set that represents one-to-many relationships.

I do question however if this is an appropriate approach given the fact that you are using LEFT JOINs here. As noted above, you could have null values for the contract group information and the phone number information.  You don't seem to be considering this at all.

This is especially critical given the fact that the contact group ID is the very first dimension in your array.  Are you really intending to group on null in this first dimension (technically such values would actually be assigned to an empty string array key)?

Since JSON can only represent two data structures - a numerically-indexed array, or an object - I would think that a numerically-indexed array would make more sense as the first dimension, meaning you might end up with JSON like:

[
    {"group_id": 1, ... },
    {"group_id": 2, ... },
    ...
]

I think numerically-indexed arrays might also be used to hold the contacts and phone number data nested underneath the contact group.

You are treating all values from the result set as strings (the way they are represented by default when retrieving from mysqli_fetch_array()).  Is this desired behavior, or does it make sense to cast primary key id's, for example, as integers?

Putting aside the problems noted above around LEFT JOINS and null.  I would suggest that you consider working with each row of your results set as an object.  IMO, this can make your code a lot cleaner and, in most cases, actually more truly represents what you are working with (an object representation retrieved from DB).

Taking this approach, as well as taking the approach noted on using numerically-indexed arrays for contact groups, contacts, and phone numbers, you might end up with something more like this:

$data = array();
$group = new stdClass();
$group->group_id = 0;
while ($row = mysqli_fetch_object($query)) {
    // build new contact group if needed
    if((int)$row->group_id !== $group->group_id) {
        // break existing reference between $group and $data
        // data in $data will remain after this dereferencing
        unset($group);
        $group = new stdClass();
        $group->group_id = (int)$row->group_id;
        $group->group_name = $row->group_name;
        $group->contacts = array();
        // build reference from group to location in $data
        // this allows you to simply work with $group here in loop
        // as opposed to $data[$index] which requires you to manually
        // track index values
        // taking similar apporach below for contacts as well eliminates
        // $data[$group_index]->contacts[$contact_index]->phone_numbers[]
        // types of references
        $data[] =& $group;

        // reset $contact
        unset($contact);
        $contact = new stdClass();
        $contact->id = 0;
    }
    // build new contact if needed
    if((int)$row->contact_id !== $contact->id) {
        // dereference contact
        unset($contact);
        // create new contact
        $contact = new stdClass();
        $contact->id = (int)$row->contact_id;
        $contact->name = $row->contact_name;
        $contact->phone_numbers = array();
        $group->contacts[] =& $contact;
    }
    // add phone number to current contact
    $phone = new stdClass();
    $phone->id = (int)$row->phone_id;
    $phone->number = $row->phone_number;
    $contact->phone_numbers[] = $phone;
}
unset($group, $contact, $phone);

You will note that taking this approach means that you are truly only building a single logical representation (a stdClass object) for each contact group and contact and simply adding phone numbers to the current contact and adding contacts to the current group.   I think thinking in terms of objects here really makes the code more meaningful in a real-world sense than building out a convoluted associative array.  Considering the JSON serialization converts both objects and associative arrays to object notation anyway, there really seems to be no value to using associative array here.


Given entrance and exit logs, find the population maximum I was given this problem during an interview. And I solved it using Python. Would like to get feedback to see how I can improve my interview response.

  Busiest Time in The Mall

  The Westfield Mall management is trying to figure out what the busiest
  moment at the mall was last year. You’re given data extracted from the
  mall’s door detectors. Each data point is represented as an integer
  array whose size is 3. The values at indices 0, 1 and 2 are the
  timestamp, the count of visitors, and whether the visitors entered or
  exited the mall (0 for exit and 1 for entrance), respectively. Here’s
  an example of a data point: [ 1440084737, 4, 0 ].

  Note that time is given in a Unix format called Epoch, which is a
  nonnegative integer holding the number of seconds that have elapsed
  since 00:00:00 UTC, Thursday, 1 January 1970.

  Given an array, data, of data points, write a function
  findBusiestPeriod that returns the time at which the mall reached its
  busiest moment last year. The return value is the timestamp, e.g.
  1480640292. Note that if there is more than one period with the same visitor peak, return the earliest one.

  Assume that the array data is sorted in an ascending order by the
  timestamp.

"""
input:  data = [ [1487799425, 14, 1],
                 [1487799425, 4,  0],
                 [1487799425, 2,  0],
                 [1487800378, 10, 1],
                 [1487801478, 18, 0],
                 [1487801478, 18, 1],
                 [1487901013, 1,  0],
                 [1487901211, 7,  1],
                 [1487901211, 7,  0] ]

output: 1487800378 # since the increase in the number of people
                   # in the mall is the

"""

def find_busiest_period(data):

  people = 0
  max_time = 0
  max_people = 0
  for i in range(len(data)):

    if data[i][2] == 1:
      people += data[i][1]
    else:
      people -= data[i][1]

    if (i < len(data)-1 and data[i][0] == data[i+1][0]):
      continue

    if people > max_people:
      max_people = people
      max_time = data[i][0]
  return max_time

data = [ [1487799425, 14, 1],
                 [1487799425, 4,  0],
                 [1487799425, 2,  0],
                 [1487800378, 10, 1],
                 [1487801478, 18, 0],
                 [1487801478, 18, 1],
                 [1487901013, 1,  0],
                 [1487901211, 7,  1],
                 [1487901211, 7,  0] ]

test = find_busiest_period(data)
print(test)

Algorithm
I don't understand the purpose of comparing the timestamp of the next datum here:

if (i < len(data)-1 and data[i][0] == data[i+1][0]):
  continue

The two events happened during the same second, but it's reasonable to assume that they are ordered, and therefore we should consider the total within that single second, unless the problem statement says otherwise.
Without that constraint, we have no need for the index i, and can consider just the members of the input data; we can give the elements meaningful names:
  for time,quantity,direction in data:

Now, we know we won't find a new maximum when people are exiting (assuming we're not given negative numbers of people), so we can move the test into the += branch:
    if direction == 1:
      # Some people entered
      people += quantity
      # Have we reached a new maximum?
      if people > max_people:
        max_time, max_people = time, people
    elif direction == 0:
      # Some people left
      people -= quantity
    else:
      raise ValueError(direction)

General review

PEP8 recommends four spaces per indent level.

This doc-comment is both incomplete and incorrect:

"""
output: 1487800378 # since the increase in the number of people
                   # in the mall is the
"""

The doc-comment is in the wrong place (it should be just within the function body).

We should use a main guard.

Consider using doctest to provide more test cases.

Improved code
def find_busiest_period(data):
    """
    Find the timestamp when the greatest number of people
    are in the building.

    >>> find_busiest_period([]) is None
    True

    >>> find_busiest_period([ [0, 0, 2] ])
    Traceback (most recent call last):
        ...
    ValueError: 2

    >>> find_busiest_period([ [0, -5, 0] ])
    Traceback (most recent call last):
        ...
    ValueError: -5

    >>> find_busiest_period([ [0, 5, 1], [2, 5, 1], [3, 5, 0] ])
    2

    >>> find_busiest_period([ [1487799425, 14, 1], \
                              [1487799425, 4,  0], \
                              [1487799425, 2,  0], \
                              [1487800378, 10, 1], \
                              [1487801478, 18, 0], \
                              [1487801478, 18, 1], \
                              [1487901013, 1,  0], \
                              [1487901211, 7,  1], \
                              [1487901211, 7,  0] ])
    1487901211
    """
    people = 0
    max_time = None
    max_people = 0

    for time,quantity,direction in data:
        if quantity < 0:
            raise ValueError(quantity)
        if direction == 1:
            # Some people entered
            people += quantity
            # Have we reached a new maximum?
            if people > max_people:
                max_time, max_people = time, people
        elif direction == 0:
            # Some people left
            people -= quantity
        else:
            raise ValueError(direction)

    return max_time

if __name__ == "__main__":
    import doctest
    doctest.testmod()


Align text (left, center, right) without additional packages I wrote this short subroutine to left, center, right-align a multiline string.

How can I make it better (shorter, prettier, optimized)?  Can you offer some suggestions?

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

# A bit of text
$_ = <<END;
     Lorem ipsum dolor sit amet consectetur adipiscing elit
          Curabitur pretium odio dictum nisl posuere, vitae mollis nibh facilisis
   Curabitur tempus tincidunt ante eget facilisis
     Pellentesque ut accumsan dui, nec semper odio
        Ut libero lacus, fermentum quis ultricies quis, tempus nec augue
                   Mauris tincidunt hendrerit accumsan
END

# Test each alignment case
for my $align ('left', 'center', 'right') {
    print align($_, $align)."\n";
}

# Alignment subroutine
sub align {
    local $_ = shift;
    my ($align) = @_;    # Text to align, Alignment type
    my $re = qr/^(.*)/m;
    s/^[ \t]+|[ \t]+$//gm;   # Remove trailing/leading spaces

    # Get longest line
    my $max = 0;
    length($1) > $max and $max = length($1) while /$re/g;

    if    ($align eq 'left')   { s|$re|$1 . " " x ($max-length($1))|ge }
    elsif ($align eq 'center') { s|$re|" " x (($max-length($1))/2) . $1|ge }
    elsif ($align eq 'right')  { s|$re|" " x ($max-length($1)) . $1|ge }

    return $_;
}

Welcome to Perl, and to Code Review :)

There are a couple of things that could be improved:

It's good that you use strict. I recommend that you also use warnings.
If you are using any recent Perl (say, anything since v5.10.1), then you can gain access to many convenient new features. You can do this by either specifying a minimum version (such as use v5.10) or by explicitly asking for a feature (use feature 'say'). The say feature is exactly like print, but appends a newline automatically.
Please do not use here-docs like <<END. Always make it clear whether the here-doc behaves like a double-quoted string <<"END" or like a verbatim section <<'END'. Omitting quotes around the end marker behaves like double quotes, and can lead to bugs. Use single quotes if you don't want Perl to touch the string contents.
Avoid the $_ variable whenever possible. Instead, use a variable with a good name such as $unaligned_text. As an aside, do not declare a $_ variable with my. This is currently still allowed, but will probably be forbidden in upcoming versions of perl:

  $_ is by default a global variable. However, as of perl v5.10.0, you can use a lexical version of $_ by declaring it in a file or in a block with my. […] Though this seemed like a good idea at the time it was introduced, lexical $_ actually causes more problems than it solves. If you call a function that expects to be passed information via $_, it may or may not work, depending on how the function is written, there not being any easy way to solve this. Just avoid lexical $_, unless you are feeling particularly masochistic. For this reason lexical $_ is still experimental and will produce a warning unless warnings have been disabled. As with other experimental features, the behavior of lexical $_ is subject to change without notice, including change into a fatal error.

  – from perldoc -v '$_'

If you want to match any whitespace character in a regex, consider using the \s charclass rather than [ \t]. In this case, [ \t] may be preferable, but it will not match Unicode whitespace.
Unless needed, do not put regexes into separate variables. Rather, specify them in-place. If you do put a regex object into a variable (which here makes sense, as you are reusing it), use a proper name, such as $match_line.
Some regex modifiers change how a regex is compiled, others how a regex is matched. When creating a regex object with qr//, it is compiled. The /m flag changes the behaviour of the ^ and $ regex operators. By specifying this flag at the place of matching, you may be triggering multiple recompilations, slightly degrading performance. Instead: my $match_line = qr/.../m. The /g and /e flags change how matching and substitutions work, so they can't be used with qr//. For more information, see Regexp Quote-Like Operators in perldoc perlop.
Instead of matching and changing each line, consider splitting the input into an array of lines. Then loop over each line and apply the required transformation. Afterwards, join the lines together again. This happens to avoid all that hassle with regexes.
If an invalid alignment is passed, then you should throw an error rather than returning the input unaligned. I recommend you prefer the Carp module over the builtin die and warn.

Here is your align function, as I would write it.

sub align {
    my ($text_to_align, $alignment) = @_;
    my @lines = split /\n/, $text_to_align;

    # Trim whitespace, and find the longest line.
    # This is an OK use-case for the $_ variable.
    my $max_length = 0;
    for (@lines) {
        # I prefer \A and \z over ^ and $ because they always match at
        # string beginning and end, and are never influenced by /m.
        s{\A\s+}{};
        s{\s+\z}{};
        $max_length = length if length > $max_length;
    }

    # Perform the alignment, depending on the alignment type.
    # The alignment is performed in-place.
    if ($alignment eq 'left') {
        for my $line (@lines) {
            my $padding = $max_length - length $line;
            $line .= " " x $padding;
        }
    }
    elsif ($alignment eq 'right') {
        for my $line (@lines) {
            my $padding = $max_length - length $line;
            $line = (" " x $padding) . $line;
        }
    }
    elsif ($alignment eq 'center') {
        for my $line (@lines) {
            my $padding = $max_length - length $line;
            # we divide the $padding into two halves: $left and $right.
            # But what if $padding is an odd number?
            # Let's put the smaller padding $left.
            my $left = int($padding / 2);
            my $right = $padding - $left;
            $line = (" " x $left) . $line . (" " x $right);
        }
    }
    else {
        # this happens if an invalid $aligment was passed as argument.
        # Let's throw an error. We could use `die`, but that doesn't tell
        # an user of our function where *he* made an error. Instead, we use
        # the Carp module:
        require Carp;
        Carp::croak("Alignment must be one of left, right, center.");
    }

    # join the lines back together again, and return
    return join "\n", @lines;
}