Spring Data Redis - mapping object relationships? I'm designing an app that will process a set of social media messages which are posted by users. Each message is then scored and there is a reporting layer that will spit out a set of reports. The app is implemented with spring-boot and has to use a Redis DB.

I'm more familiar with RDBM databases and perhaps I'm being too much of an ORM mapping mindset to this issue.

The main rules i need to enforce

A user can post multiple messages.
A message can have multiple scores.
It should be possible to chart a user's scores over time.

I've started with the logical mapping of data that i believe i need to record in the app. There are three main objects and I'm using spring-data-redis to define the @RedisHash, @Id, @Index and @Reference fields.

Message

The raw message details with thread, date, user and message content.

@RedisHash("messages")
public class Message {

    @Id String messageId;
    @Indexed String threadId;
    @Reference String userId;
    String message;
    Date date;
    @Reference List<User> userScores;
}

Score

A record of a score that each message has been recorded with

@RedisHash("scores")
public class Score {
    @Id String id;
    @Reference String messageId;
    Long score;
    @Reference User user;
}

User

The user details with title

@RedisHash("users")
public class User {

    @Id String userId;
    String grade;
    @Reference List<Score> scores;
}

I believe the Message and User objects are correctly defined, but my concern is I've over complicated the Score object mapping. Any comments or advise would be welcome.

There's nothing really wrong with your modelling approach. If you feel confident with the modelling you are good to go.

Data modelling in Redis depends on the functional and non-function requirements you need to meet. In Redis and your case this boils down to whether read performance is more important than write performance, and whether performance is an issue at all.

In my opinion (and after having developed quite a few commercial-grade products based on Redis), Redis has not been made with object relationships in mind. At its core it is just a very fast, efficient and lightweight key-value based database, i.e. you lookup a value by its (complex) key. That's it. Simple, plain and wonderful.

With this said, I would strongly recommend you to keep data models as simple as possible, when working with Redis. In classic database scheme design your are encouraged to perform database normalizations. However, Redis is not a RDMBS and thus is used most efficiently, when object relations are kept at a minimum (or even better avoided altogether, if possible).

The @Reference annotation actually is just a convenience to developers, taking care of automatically loading referenced objects from Redis database. On a side note, please keep in mind that referenced objects need to be updated/saved separately from the referencing object (related). Resolution of references in Spring Data Redis just mean additional queries under the hood and thus mean less read performance.

For maximum read performance, I would suggest the following changes to your model. Please note that this not necessarily the best model for your purpose, as many constraints of your project remain in the dark. Just consider it as an alternative to your "classic" model you may want to think about.

@RedisHash("messages")
public class Message {
    @Id String messageId;
    @Indexed String threadId;
    @Reference User user;
    String message;
    Date date;
    List<Score> userScores;
}

@RedisHash("users")
public class User {
    @Id String userId;
    String grade;
    List<Score> scores;
}

public class Score {
    String scoreId
    String messageId;
    Long score;
    User userId;
}

This approach basically eliminates the object relationships between Message and Score and User and Score by embedding Score information into the Message and User hash objects as standalone data. So you gain substantial read performance at the cost of data redundancy.

When querying a message (e.g. for message rendering) this means Redis Data just needs one lookup to fetch the full data from database (because message scores are already part of the stored value). The same is valid for querying a user for charting his/her scores. However, the drawback with this approach is, that you need to write/update the Score twice whenever a new score is added to a message by a user: once for the update to the user object and once for the update to the message object. However, using @Reference you still had to write the score object once, then write the updated references to User and Message, respectively.

Another (and probably more important) drawback is the increased memory usage due to the data duplication in User and Message objects. As Redis holds all its data in RAM at the time of this writing (albeit in a very efficient manner), this can become an issue if you deal with a huge amount of users and messages.

The proposed model should work particularly well in a strongly asymmetric producer-consumer model, where you have a lot more consumers (people just reading messages) than producers (users contributing messages and scores to messages). YMMV depending on the actual project style and setup.


Verbose mailing function I have multiple forms that send an email. As of this moment I have 3 forms that send mail and vary only by the number of inputs:

One requires just an email
Another requires a name and email
Another requires name, email, and a comment

Instead of making 3 separate functions to mail the inputs away, I figured I could consolidate them into 1. The issue here is a lot of if statements. The code all works, but spans around 30 lines, which seems like too much.

function sendMail()
{
$comment = null;
$to =  "destination@mail.com";
$copy = "mymail@mail.com";
$subject = "Email..";
if (isset($_POST["contactSubscribe"]) || isset($_POST["questionSubmit"]) || isset($_POST["submit"])) {
  foreach ($_POST as $field => $value) {
    if ($field == "contactName") {
      $contactName = $value;
    }
    if ($field == "email") {
      $email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
    }
    if ($field == "comment") {
      $comment = $value;
    }
  }
}
if (!is_null($contactName) && is_null($comment) ) {
  $message = "Name: $contactName Email: $email";
}
if (is_null($comment) && is_null($contactName)) {
  $message = "inputname=i_" . $email;
}
if (!is_null($contactName) && !is_null($comment) ) {
  $message = "Name: $contactName Email: $email Message: $comment";
}
mail($to, $subject, $message);
mail($copy, $subject, $message);
}

I can slightly improve your function, like this:

function array_whitelist($array,$whitelist)
// only allow certain keys in an array, in reaction to comment by tim
{
  $list = explode(',',$whitelist);
  return array_intersect_key($array,array_flip($list));
}

function sendMail()
{
  // check form submission
  if (is_array($_POST))
  {
    // initialize
    $to      = 'destination@mail.com';
    $copy    = 'mymail@mail.com';
    $subject = 'Email..';
    $message = '';
    // make all posted input fields into local variables
    $inputs  = array_whitelist($_POST,'email,contactName,comment');
    extract($inputs);
    // filter email address
    $email = filter_var($email,FILTER_SANITIZE_EMAIL);
    // add some fields to message
    if (isset($contactName)) $message .= "Name: $contactName".PHP_EOL;
                        else $message .= "Name: i_".$email.PHP_EOL;
    if (isset($comment)) $message .= "Message: $comment".PHP_EOL;
    // send mails
    mail($to, $subject, $message, "BCC: $copy");
  }
}

It is clear you seem to overdo the conditions, they are not used very effectively. But I think you have removed some essential parts, so this is never going to work.

Perhaps it is time you start to think about a more general email function, not just one that will fit your three forms. What if a fourth form comes along? So more than looking at what I did, think about generalizing your email routines. You could make a class. Seperate the building of the mail message from the sending of the emails. You can find enough examples here (search for: 'PHP Email Class').

Also don't forget the next thing you will want to do is send HTML email messages, and add attachments. That's when it becomes really complex. Email HTML and attachments are just plain tricky. Look for a library that can do these things for you, there are some very good ones out there (https://github.com/PHPMailer/PHPMailer, and the like).


INTEST on Codechef in Python 3 I have written a program in Python 3 for the INTEST problem on Codechef (included in question). The program is taking a lot of time (55.76s) in Python 3. The same code takes almost 1/10 of the time time (4.97s) in C.

  The purpose of this problem is to verify whether the method you are
  using to read input data is sufficiently fast to handle problems
  branded with the enormous Input/Output warning. You are expected to be
  able to process at least 2.5MB of input data per second at runtime.

  Input

  The input begins with two positive integers \$n\$ \$k\$ (\$n, k\le10^7\$). The next
  \$n\$ lines of input contain one positive integer \$t_i\$, not greater than
  \$10^9\$, each.

  Output

  Write a single integer to output, denoting how many integers \$t_i\$ are
  divisible by \$k\$.

  Example

  Input:

7 3
1
51
966369
7
9
999996
11

  Output:

4

My questions are:

How is it that the Python 3 code is taking so much time or which line of code specifically is taking the most time?
Any modifications that can speed up the code?

My CodeChef submissions with time and code.

import sys
__author__ = 'Gourav Chawla'
"""
    Problem Code: INTEST
    Problem URL: http://www.codechef.com/problems/INTEST
    Compatability: Python 3.x
"""

n, k = input().split()
n = eval(n)
k = eval(k)

inputVar = 0
count = 0

# inputVar = [eval(x) for x in input().split()]

inputVar = list(map(int, sys.stdin.readlines()))

for i in inputVar:
   if i % k == 0:
       count += 1

print(count)

I think that there must be something wrong with the way CodeChef runs Python, or with the way it reports the times. The time limit for the problem is 8 seconds, but your Python submissions are passing even though they take 55 seconds and so ought to be out of time even with the five times allowance given to Python programs.

On my laptop, if I create about 80 megabytes of input:

import random
n, k = 10**7, 17
f = open('cr93327.data', 'w')
f.write('{} {}\n', n, k)
for _ in range(n):
    f.write('{}\n'.format(random.randrange(0, 10**7)))

Then this C program runs in 3.3 seconds:

#include <stdio.h>

int main() {
    unsigned n, k, i, m, count = 0;
    scanf("%u %u", &n, &k);
    for (i = 0; i < n; i++) {
        scanf("%u", &m);
        count += (m % k == 0);
    }
    printf("%u\n", count);
    return 0;
}

And this Python 3 program runs in 6.1 seconds:

import sys

n, k = map(int, next(sys.stdin.buffer).split())
count = 0
for i in map(int, sys.stdin.buffer):
    if not i % k:
        count += 1
print(count)

So Python is about twice as slow as C, not ten times as slow.


String to list of pairs I have an input like this:

x = '415,252,428,233,428,214,394,207,383,205,390,195'

And I want to end up with this

y = [(415,252),(428,233),(428,214),(394,207),(383,205),(390,195)]

The closes I got is

tmp = [int(i) for i in x.split(',')]
y = zip(tmp[::2], tmp[1::2])

How do I get rid of the intermediate list? I need alternative to [::2] that could be used on a generator.

To reduce memory usage you can exploit the mechanics of zip and iter / iterator / generator expression.

Make tmp an iterator.

You can achieve this by changing the brackets from [] to (); changing it from a list comprehension to a generator expression.

You can alternately wrap the list comprehension in an iter call. However that would still be using \$O(n)\$ memory.
In case you don't know what an iterator is, than it's a very useful design pattern.

In short the iterator contains in it some data, and a single method to get the next bit of information. In Python the next function calls this method.

Take the iterator a list provides:

>>> my_data = iter([1, 2, 3, 4])
>>> next(my_data)
1
>>> next(my_data)
2

If you think of zip in terms of iterators, then you'll notice that with two input it works something like:

def zip(a, b):
    while True:
        yield next(a), next(b)

This means if a and b are the same then zip will get the first value and second value in a tuple. Then it will get the third and fourth, until it gets all values.

And so to improve memory usage you can change your code to:

tmp = (int(i) for i in x.split(','))
y = zip(tmp, tmp)

Note: this still runs in \$O(n)\$ space, as x.split() also makes an intermediary list. However it has cut the code from four intermediary lists to just one.

This is a common pattern, as provided in the itertools docs.

  def grouper(iterable, n, fillvalue=None):
    "Collect data into fixed-length chunks or blocks"
    # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx"
    args = [iter(iterable)] * n
    return zip_longest(*args, fillvalue=fillvalue)

y = grouper((int(i) for i in x.split(',')), 2)


Excel sheets with employees and dates I have more than 20 sheets in Excel and one main sheet (all programs with 200 names). Each sheet has a column with names and 24 months (Jan 18 to Dec 18, Jan 19 to Dec 20). Each sheet names is a subset of the main sheet.

Main sheet (all programs) has 200 names and 24 months (values to be calculated based on other sheets). The other sheet has names and values for each month respective to the main sheet. I need to take each name in main sheet and search the name in all other sheets, and if present sum all same column values and insert in the main sheet.

For 1 name I need to do calculation on 34 cells (for 200 names * 34 cells = 6800 cells). It's taking almost 20 minutes with my code. Is there any other way I can do it or any modification which improves the performance?

Main Sheet has name "employee1"

Sheet1

Sheet2

Value on the main sheet should be calculated respect to months

Dim sheetCount As Integer
Dim datatoFind
Private Sub CommandButton1_Click()
    Dim mainSheet As String: mainSheet = "All Programs"
    Dim nameColumnStart As String: nameColumnStart = "A"

    Dim namesStart As Integer: namesStart = 1
    Dim namesEnd As Integer: namesEnd = 200

    Dim startColumn As Integer: startColumn = 10 'J Column'
    Dim EndColumn As Integer: EndColumn = 33 'AG Column'

    namesStart = InputBox("Please enter start value")
    namesEnd = InputBox("Please enter end value")

    Dim temp_str As String
    Dim total As Single
    On Error Resume Next
    Sheets(mainSheet).Activate
    lastRow_main = ActiveCell.SpecialCells(xlLastCell).Row
    lastCol_main = 34
    For vRow = namesStart To namesEnd
        temp_str = Sheets(mainSheet).Cells(vRow, "A").Text
        datatoFind = StrConv(temp_str, vbLowerCase)
            For vCol = startColumn To EndColumn
                total = Find_Data(vCol)
                Worksheets(mainSheet).Cells(vRow, vCol).Value = total
            Next vCol
    Next vRow
    Sheets(mainSheet).Activate
    'MsgBox ("Calculated all values")'
End Sub

Private Function Find_Data(ByVal ColumnName As Integer) As Single
    Dim counter As Integer
    Dim currentSheet As Integer
    Dim sheetCount As Integer
    Dim str As String
    Dim lastRow As Long
    Dim lastCol As Long
    Dim val As Single

    Find_Data = 0

    currentSheet = ActiveSheet.Index

    If datatoFind = "" Then Exit Function

    sheetCount = ActiveWorkbook.Sheets.Count

    For counter = 2 To sheetCount

        Sheets(counter).Activate

        lastRow = ActiveCell.SpecialCells(xlLastCell).Row
        lastCol = ActiveCell.SpecialCells(xlLastCell).Column

        For vRow = 1 To lastRow
                str = Sheets(counter).Cells(vRow, "A").Text
                If InStr(1, StrConv(str, vbLowerCase), datatoFind) Then
                        val = Sheets(counter).Cells(vRow, ColumnName).Value
                        Find_Data = Find_Data + val
                End If
        Next vRow

    Next counter

End Function

Global Variables

Dim sheetCount As Integer
Dim datatoFind

Global variables make the code harder to maintain, modify, and debug.  It would be better to pass the data as parameters between the sub routines.  This makes it easier to determine exactly what data is being passed into your subroutines.

Private or Public modifiers should be used instead of Dim when declaring a global variable.

Constant Expressions

Dim mainSheet As String: mainSheet = "All Programs"
Dim nameColumnStart As String: nameColumnStart = "A"
Dim startColumn As Integer: startColumn = 10 'J Column'
Dim EndColumn As Integer: EndColumn = 33 'AG Column'

The variables above should be declared as constants.

Const mainSheet As String = "All Programs"
Const nameColumnStart As String = "A"
Const startColumn As Integer = 10 'J Column'
Const EndColumn As Integer = 33 'AG Column'

namesStart and namesEnd

Why initiate the values below if you are not going to use the initial values?

Dim namesStart As Integer: namesStart = 1
Dim namesEnd As Integer: namesEnd = 200

namesStart = InputBox("Please enter start value")
namesEnd = InputBox("Please enter end value")

Consider using Application.InputBox because you can specify the type of data it returns.

Dim namesStart As Integer
Dim namesEnd As Integer
Const namesStartDefault As Integer = 1
Const namesEndDefault As Integer = 200

namesStart = Application.InputBox(Prompt:="Please enter start value", Default:=namesStartDefault, Type:=1)
namesEnd = Application.InputBox(Prompt:="Please enter end value", Default:=namesEndDefault, Type:=1)

If namesStart < namesStartDefault Then
    MsgBox "Start vaule must be greater than or equal to " & namesStartDefault, vbCritical
    Exit Sub
End If

Selecting and Activating Objects

Selecting and Activating Objects should be avoided unless absolutely necessary, watch Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset).   I would wager to bet that of the 20 minutes that it takes to run your code 19+ minutes are spent needlessly activating worksheets.

Using Application.ScreenUpdating = False would probably cut the time in half.

Function Find_Data

SpecialCells(xlLastCell) should only be used when you don't know the data structure.

lastCol isn't used.

 lastCol = ActiveCell.SpecialCells(xlLastCell).Column

vRow is never declared.  The v prefix implies a variant when it should clearly be long.

ColumnName implies a string value.  I would use CoumnIndex instead.

Refactored Code

Private Const mainSheet As String = "All Programs"

Private Sub CommandButton1_Click()
    Application.ScreenUpdating = False
    Const LastColumn = 34
    Dim namesStart As Integer
    Dim namesEnd As Integer
    Const namesStartDefault As Integer = 1
    Const namesEndDefault As Integer = 200

    namesStart = Application.InputBox(Prompt:="Please enter start value", Default:=namesStartDefault, Type:=1)
    namesEnd = Application.InputBox(Prompt:="Please enter end value", Default:=namesEndDefault, Type:=1)

    If namesStart < namesStartDefault Then
        MsgBox "Start vaule must be greater than or equal to " & namesStartDefault, vbCritical
        Exit Sub
    End If

    Dim r As Long, c As Long
    With ThisWorkbook.Worksheets(mainSheet)
        For r = namesStartDefault To namesEndDefault
            For c = 2 To LastColumn
                .Cells(r, c).Value = Find_Data(.Cells(r, 1).Value, c)
            Next
        Next
    End With

End Sub

Private Function Find_Data(ByVal EmployeeName As String, ByVal ColumnIndex As Integer) As Single
    Dim result As Single
    Dim ws As Worksheet
    Dim r As Long
    For Each ws In ThisWorkbook.Worksheets
        With ws
            If Not .Name = mainSheet Then
                For r = 2 To .Range("A" & .Rows.Count).End(xlUp).Row
                    If InStr(1, .Cells(r, 1).Value, EmployeeName, vbTextCompare) > 0 Then
                        result = result + .Cells(r, ColumnIndex).Value
                    End If
                Next
            End If
        End With
    Next
    Find_Data = result
End Function


Picking a random state from a set based on probabilities I'm writing a Markov Chain implementation, and need to be able to pick a random state from a set of states, where each state has a different probability of being chosen. In my caffeine-high, I wrote this:

(defn random-state
  "Accepts a map mapping a state to the frequency that it will be chosen.
  Assumes that the frequencies add up to 100%. Will have undesired behavior if that isn't the case."
  [state-frequency-map]
  (let [r (rand)]
    (loop [[[state prob] & rest-states] (seq state-frequency-map)
           prob-sum 0
           last-sum 0]

      (let [new-sum (+ prob-sum prob)]
        (if (<= last-sum r new-sum)
          state
          (recur rest-states new-sum prob-sum))))))

I had very little idea of how I was going to achieve this going in, so I kind of cobbled it together. Basically, it works by maintaining a sum of probabilities, and checking if a randomly generated number (between 0 and 1) falls between the last and current probability sums. For example:

The probability map is {:a 0.3, :b 0.3, :c 0.4}.
The randomly drawn number is 0.7.
0.7 is greater than the sums of :a and :b's probabilities (0.6), but less than :c's, so :c is chosen this time.

This works, but like I said, I kind of just threw it together as I figured out what I was going to do, and now I'm in mind-lock and can't think of improvements for it. Is this optimal? Are there any ways of making this more idiomatic via better use of built-ins? I'm looking for any suggestions here. I'd also be open to suggestions of entirely different approaches of solving this.
A solution that isn't O(n) with respect to the number of states would be nice.

Example of its use showing that it does indeed pick the states with the correct frequencies:

(let [tries 1e7]
  (mapv (fn [[s f]] [s (/ f tries)]) ; Get the percentage occurrence
    (frequencies
      (for [n (range tries)]
        (random-state
          {\a 0.2
           \b 0.2
           \c 0.5
           \d 0.1})))))

=> [[\b 0.2000032] [\a 0.2000432] [\c 0.4999343] [\d 0.1000193]]

There could potentially be millions of states, since each distinct word in the text I scan will be become a state. And they'll never change once calculated. I'm going to scan over a book, calculate the probabilities, then serialize the state/probability map to use later.

Given this, there are a couple more efficient approaches, depending on how much preprocessing you want to do. There's a great article, Darts, Dice, and Coins, that outlines this further and ends with an O(1) algorithm, Vose's alias method.

I think the most straightforward, though, is a binary search tree-based approach ("roulette wheel selection"). Same basic idea as your current code, fill [0, 1) with proportional sections corresponding to your state probability map:

[  :a  | :b |   :c   |:d|  :e  )
0    0.25  0.4     0.76 0.8    1

but then instead of looping over a list, use something like a TreeMap that is designed to answer these kinds of queries in O(log n).

(defn states->treemap
  "Builds a navigable map of `partial sum -> state`"
  [states]
  (->> states
    (reduce (fn [[entries sum] [k v]]
              (let [sum' (+ sum v)]
                [(conj entries [sum' k]) sum']))
            [[] 0])
    first
    (into {})
    (java.util.TreeMap.)))

(defn random-state [^java.util.NavigableMap nm]
  (.getValue
    (.ceilingEntry nm (rand))))

That's not "pure Clojure", obviously, but we're on the JVM and it's pretty idiomatic to pull from Java as needed. Otherwise, your Clojure looks fine to me, but definitely check out that article for further depth.


User and Score models I have a couple of models - User & Score. I am displaying data from those models to my template. Data from Score is filtered based on data from User. I am using nested for loops and an if condition within the template to get my desired result. However, I want to know if there is a better way of coming up with the same result because once the data gets huge by number, the nested for loop is going to use up a lot of time and resource.

models.py

class User(models.Model):
    name = models.CharField(max_length=20)
    age = models.IntegerField()

class Score(models.Model):
    user = models.ForeignKey('User')
    subject = models.CharField(max_length=20)
    score = models.IntegerField()

views.py

def Home(request):
    users = User.objects.all()
    scores = Score.objects.all()
    return render(request, "home.html", {'users': users, 'scores': scores})

home.html (template)

{% for user in users %}
    {% for score in scores %}
    {% if score.user == user %} {{ user }} -- {{ score.subject }} -- {{ score.score }} <br>{% endif %}
    {% endfor %}
    {% endfor %}

Result

Michel -- Physics -- 80
Michel -- Chemistry -- 90
Zarif -- Physics -- 100
Zarif -- Biology -- 80

This question is a simplified version of this (Django ORM usage in template). If you can, maybe answer over there too!

Your nested loop seems to serve the purpose of grouping the output by user.
Since Score has a link to User,
you can achieve the same effect by getting the scores ordered by the user field, like this:

def Home(request):
    scores = Score.objects.order_by('user')
    return render(request, "home.html", {'scores': scores})

And then in the template you can eliminate one for and one if:

{% for score in scores %}
{{ score.user }} -- {{ score.subject }} -- {{ score.score }} <br>
{% endfor %}


Intersection of two sorted vectors in C++ Intersection of two sorted vectors in C++ - can this be written any better?

vector<int> intersection(vector<int>& nums1, vector<int>& nums2) {
        vector<int> result;
        int l = 0, r = 0;
        while(l < nums1.size() && r < nums2.size()){
            int left = nums1[l], right = nums2[r];
            if(left == right){
                result.push_back(right);
                while(l < nums1.size() && nums1[l] == left )l++;
                while(r < nums2.size() && nums2[r] == right )r++;
                continue;
            }
            if(left < right){
            while(l < nums1.size() && nums1[l] == left )l++;
            }else while( r < nums2.size()  && nums2[r] == right )r++;
        }
            return result;
    }

Indentation

Your indentation is not consistent. This makes the code hard to read and maintain. It should be fixed so you don't give other people headaches.

    if(left < right){
    while(l < nums1.size() && nums1[l] == left )l++;
    }else while( r < nums2.size()  && nums2[r] == right )r++;

That is basically unreadable giberish (opinion of Martin).
Using namespace std; is super bad

This is mention in nearly every C++ review. There is a large article on the subject here: Why is “using namespace std” considered bad practice?. The second answer is the best in my opinion (Martin) see
Multiple declarations in one is bad (thanks to terrible syntax binding rules)

The one declaration per line has been written about adnausium in best practice guides. Please for the sake of your reader declare one variable per line with its own exact type.

The syntax binding rules alluded to above is:

int* x, y;   // Here x is int* and y in int
             // confusing to a reader. Did you really mean to make y an int?
             // Avoid this problem be declaring one variable per line

Typically, functions like this would be based on iterators to work on any container

Here your code is limited to only using vectors. But the algorithm you are using could be used by any container type with only small modifications. As a result your function could provide much more utility being written to use iterators.

The standard library was written such that iterators are the glue between algorithms and container.
It would be a lot simpler, if not necessarily more efficient at runtime, to just use some hash sets.
This function could be generic in T rather than assuming int.
The repeated conditions make me feel like there's simplification waiting here, although exactly what that is eludes me in the two minutes I'm spending on this.
Should take by const ref, not ref, so that you can operate on temporaries.


Optimization of comparing two collections and get the changes I use the following code to get the changes between two collections. Objects are "joined" using a primary key. Any tips on performance issues or other optimizations appreciated.

/// <summary>
/// Gets the changes [Deleted, changed, inserted] comparing this collection to another.
/// </summary>
/// <param name="local">The source collection.</param>
/// <param name="remote">The remote collection to comare agains.</param>
/// <param name="keySelector">The primary key selector function</param>
/// <returns></returns>
public static ChangeResult<TSource> CompareTo<TSource, TKey>(this IEnumerable<TSource> local, IEnumerable<TSource> remote, Func<TSource, TKey> keySelector)
{
    if (local == null)
        throw new ArgumentNullException("local");
    if (remote == null)
        throw new ArgumentNullException("remote");
    if (keySelector == null)
        throw new ArgumentNullException("keySelector");

    var remoteKeyValues = remote.ToDictionary(keySelector);

    var deleted = new List<TSource>();
    var changed = new List<TSource>();
    var localKeys = new HashSet<TKey>();

    foreach (var localItem in local)
    {
        var localKey = keySelector(localItem);
        localKeys.Add(localKey);

        /* Check if primary key exists in both local and remote
         * and if so check if changed, if not it has been deleted
         */
        TSource changeCandidate;
        if (remoteKeyValues.TryGetValue(localKey, out changeCandidate))
        {
            if (!changeCandidate.Equals(localItem))
                changed.Add(changeCandidate);
        }
        else
        {
            deleted.Add(localItem);
        }
    }
    var inserted = remoteKeyValues
                    .Where(x => !localKeys.Contains(x.Key))
                    .Select(x => x.Value)
                    .ToList();

    return new ChangeResult<TSource>(deleted, changed, inserted);
}

/// <summary>
/// Immutable class containing changes
/// </summary>
public class ChangeResult<T>
{
    public ChangeResult(IList<T> deleted, IList<T> changed, IList<T> inserted)
    {
        Deleted = new ReadOnlyCollection<T>(deleted);
        Changed = new ReadOnlyCollection<T>(changed);
        Inserted = new ReadOnlyCollection<T>(inserted);
    }

    public IList<T> Deleted { get; private set; }
    public IList<T> Changed { get; private set; }
    public IList<T> Inserted { get; private set; }
}

Usage

var changes = Col1.CompareTo(Col2, x => x.UniqueId);

I would first make ChangeResult<T> truly immutable using sealed and readonly. It's unfortunate that we don't have true immutable syntax for automatic properties, but space is cheap, they say:

/// <summary>
/// Immutable class containing changes
/// </summary>
public sealed class ChangeResult<T> : IChangeResult<T>
{
    private readonly ReadOnlyCollection<T> deleted;

    private readonly ReadOnlyCollection<T> changed;

    private readonly ReadOnlyCollection<T> inserted;

    public ChangeResult(IList<T> deleted, IList<T> changed, IList<T> inserted)
    {
        this.deleted = new ReadOnlyCollection<T>(deleted);
        this.changed = new ReadOnlyCollection<T>(changed);
        this.inserted = new ReadOnlyCollection<T>(inserted);
    }

    public IList<T> Deleted
    {
        get
        {
            return this.deleted;
        }
    }

    public IList<T> Changed
    {
        get
        {
            return this.changed;
        }
    }

    public IList<T> Inserted
    {
        get
        {
            return this.inserted;
        }
    }
}

and because it's good design to program to interfaces, I extracted one:

public interface IChangeResult<T>
{
    IList<T> Deleted
    {
        get;
    }

    IList<T> Changed
    {
        get;
    }

    IList<T> Inserted
    {
        get;
    }
}

Then, in your CompareTo<TSource, TKey>() method, I have it return said interface. Also, I expand the deleted and changed lists to the capacity of the local enumerable (converted to a list locally so an expensive enumerable doesn't get iterated multiple times). This may be overkill in some cases, but in the worst cases, you wind up eliminating potentially expensive list expansion reallocations. A classic speed-vs-space tradeoff. I would then posit that this is actually a great algorithm to go parallel with, as long as you don't care about the order in the three lists:

/// <summary>
/// Gets the changes [Deleted, changed, inserted] comparing this collection to another.
/// </summary>
/// <typeparam name="TSource"></typeparam>
/// <typeparam name="TKey"></typeparam>
/// <param name="local">The source collection.</param>
/// <param name="remote">The remote collection to compare against.</param>
/// <param name="keySelector">The primary key selector function</param>
/// <returns></returns>
public static IChangeResult<TSource> CompareTo<TSource, TKey>(
    this IEnumerable<TSource> local,
    IEnumerable<TSource> remote,
    Func<TSource, TKey> keySelector)
{
    if (local == null)
    {
        throw new ArgumentNullException("local");
    }

    if (remote == null)
    {
        throw new ArgumentNullException("remote");
    }

    if (keySelector == null)
    {
        throw new ArgumentNullException("keySelector");
    }

    local = local.ToList();

    var remoteKeyValues = remote.ToDictionary(keySelector);
    var deleted = new List<TSource>(local.Count());
    var changed = new List<TSource>(local.Count());
    var localKeys = new HashSet<TKey>();

    Parallel.ForEach(
        local,
        localItem =>
        {
            var localKey = keySelector(localItem);

            lock (localKeys)
            {
                localKeys.Add(localKey);
            }

            /* Check if primary key exists in both local and remote
             * and if so check if changed, if not it has been deleted
             */
            TSource changeCandidate;

            if (remoteKeyValues.TryGetValue(localKey, out changeCandidate))
            {
                if (changeCandidate.Equals(localItem))
                {
                    return;
                }

                lock (changed)
                {
                    changed.Add(changeCandidate);
                }
            }
            else
            {
                lock (deleted)
                {
                    deleted.Add(localItem);
                }
            }
        });

    var inserted = remoteKeyValues
        .AsParallel()
        .Where(x => !localKeys.Contains(x.Key))
        .Select(x => x.Value)
        .ToList();

    return new ChangeResult<TSource>(deleted, changed, inserted);
}


Handling multiple IO operations that return optional values The following Haskell code reads a Pandoc document from the DB and then serves it to the user after making a transformation, after the first run it is stored in memory via IORef State, inside a [(String, Pandoc)] so the document can be quickly looked up again, without hitting the DB or doing the transformation. I don't feel like this code reflects that this is what is happening that well, there is a lot of handling Maybe's that I feel clouds the actual steps that make up this action.

module Api.ReadDocument (Route, action) where

import Control.Monad.Reader
import Data.Aeson.Types
import Data.Int
import Data.IORef
import Data.List
import Database.Persist.Postgresql (ConnectionPool, fromSqlKey, runSqlPool)
import Servant
import Servant.Auth.Server
import Text.Pandoc (Pandoc, def, readHtml, writeJSON)
--
import Config
import Queries
import State
import Types.DB
import Types.Pandoc

type Route = "read" :> Capture "id" Int64 :> Get '[JSON] Pandoc

action :: IORef State -> ConnectionPool -> AuthResult Account -> Int64 -> Handler Pandoc
action state pool (Authenticated _) id = viaCache state pool id
action _ _ _ = throwAll err401

viaCache :: IORef State -> ConnectionPool -> Int64 -> Handler Pandoc
viaCache state pool id = do
  let key = show id
  state' <- liftIO $ readIORef state
  maybe (retrieve state pool id) (return . snd) $ find ((== key) . fst) (cached state')

retrieve :: IORef State -> ConnectionPool -> Int64 -> Handler Pandoc
retrieve state pool id = do
  doc <- runSqlPool (Queries.getDocumentFromId id) pool
  maybe (throwError err404) (loadDocument state id) doc

loadDocument :: IORef State -> Int64 -> Document -> Handler Pandoc
loadDocument state id doc = do
  let key = show id
      pd = transform . documentBody $ doc
  _ <- liftIO $ updateState (CacheFile (key, pd)) state
  return pd

Previously this was one function, which contained nested case statements like so (pseudo code):

action state pool (Authenticated _) id = do
  state' <- fromState state              -- get the state
  case find id state of                  -- find the data
    Just (_, doc) ->
      return doc                         -- return cached document
    Nothing -> do
      doc <- getDocumentFromId id pool   -- get document from db
      case doc of
        Just d -> do
          let doc = performOp $ d        -- perform computation on doc
          _ <- saveToState state doc     -- save the document to in memory cache
          return doc                     -- serve document
        Nothing ->                       -- document not actually in DB
          throwError err404

This is obviously much more explicit but I didn't like the nested case statements, and obviously there is a bit more to the code than what I have typed out here.

Here's how your pseudocode can be rewritten with bounded indentation using MaybeT.

action state pool (Authenticated _) id = cached <|> retrieve <|!> throwError err404 where
  cached = snd <$> MaybeT (find id <$> fromState state)
  retrieve = do
    doc <- performOp <$> MaybeT (getDocumentFromId id pool)
    _ <- lift $ saveToState state doc
    return doc

infix 2 <|!>
(<|!>) :: Monad m => MaybeT m a -> m a -> m a
mx <|!> y = runMaybeT mx >>= maybe y pure


Create boustrophedon path in Matplotlib I have following code that creates parallel swathes of specified sizes. Can someone suggest a better way of doing this?

import matplotlib.pyplot as plt
import numpy as np
len=10; width=5;
pts=[];
for i in xrange(1,width+1):
    y_curr=i-1;
    if y_curr%2==0 :
        for j in xrange(0,len+1):
            pts.append([j,y_curr]);
    elif y_curr%2==1:
        for j in  xrange(1,len+2):
            pts.append([len+1-j,y_curr]);
pts=np.asarray(pts);
plt.plot(pts[:,0], pts[:,1])
plt.axis([-1, len+1, -1, width])
plt.show()

First of all, please remove all those semicolons. In Python, semicolons and newlines are two ways of indicating the end of a statement. Putting them both there is redundant.

Speaking of newlines, you should probably put one after your imports as a separator and probably another one between your len=10 and width=5. An interesting read is PEP 8, the official Python style guide.

As long as you're changing len=10 anyway, it's generally considered bad practice to define a variable with the same name as a built-in function because it masks the built-in. You use a full word for width; maybe it would be better to do the same for length.

There are some other spacing things I don't like. I would put some spaces around your operators, for example, and after the commas when passing arguments to xrange() and when creating your lists.

Now that we're done with style changes, on to the logic part.

I see some unnecessary +1s in your code. For example, your outer loop uses for i in xrange(1, width+1), yet the only place where i is used is y_curr=i-1. If you're just going to subtract 1 from it anyway, why add 1 to it in the first place? You can keep it simply for y_curr in xrange(width):.

Similarly, your second inner loop can be simplified as:

for j in xrange(len + 1):
    pts.append([len - j, y_curr])

You may notice now that your inner for loops are very similar. You can then simplify your code a little like this:

for y_curr in xrange(width):
    for j in xrange(len + 1):
        if y_curr % 2 == 0:
            pts.append((j, y_curr))
        else:
            pts.append((len - j, y_curr))

or even:

for y_curr in xrange(width):
    for j in xrange(len + 1):
        pts.append((j if y_curr % 2 == 0 else len - j, y_curr))

And if you're in the mood, you could use a list comprehension:

pts = [(j if y_curr % 2 == 0 else len - j, y_curr) for y_curr in xrange(width) for j in xrange(len + 1)]

though I don't recommend that because it is not at all readable.

I also decided to change to using tuples instead of lists. This is because the immutable nature of tuples makes them generally more efficient than lists. In addition, you probably noticed that I've used only one argument to xrange() in several places. When that is done, it assumes that the starting value is 0 and the stop is the supplied argument. This is the desired behavior in most cases, including yours.

I'm not sure why you're using NumPy here. You can do this very simply with standard Python:

plt.plot(*zip(*pts))

You could even keep separate lists in the first place:

x = []
y = []
for y_curr in xrange(width):
    for j in xrange(len + 1):
        x.append(j if y_curr % 2 == 0 else len - j)
        y.append(y_curr)

plt.plot(x, y)


Job execution on records supplied by database I have a requirement to check all records in a table which is big (Around 1.5-2 million records). What I am doing is very simple, just get all the records and loop through one by one, and do things. I have a quartz job which will run every 2 minutes to check things.

ApplicationDetailCheckingJob.java

public class ApplicationDetailCheckingJob implements Job {

@Autowired
private DatabaseService databaseService;

@Value("${EnableJob}")
private boolean enableJob;

@Override
public void execute(JobExecutionContext jobExecutionContext) {
    if (enableJob) {
        try {
            LogService.info(this.getClass().getName(), "Started Checking...");
            //Get All Records
            List<Applications> allApplicationDetails = databaseService.getAllAppDetails();
                //Perform Check For Each Application
                allApplicationDetails.stream().forEachOrdered((app) -> {
                    //Check For Columns Values
                    //If SendTweet = 1, Send tweet to the Application's User
                    //If SendEmail = 1, Send Email to User
                    //If Send SMS = 1, Send SMS to User
                    //Rest is also similar procedures
                });
            LogService.info(this.getClass().getName(), "Finished checking.");
        } catch (Exception e) {
            LogService.error(this.getClass().getName(), e);
        }
    } else {
        LogService.info(this.getClass().getName(), "ApplicationDetailChecking Job is Disabled");
    }
 }
}

DatabaseService.java

public List<Applications> getAllAppDetails() {

    String sqlQuery = "SELECT * FROM Applications";
    List<Applications> appDetails = defaultJdbcTemplate.query(sqlQuery, BeanPropertyRowMapper.newInstance(Applications.class));

    LogService.info(this.getClass().getName(), "Number of records found = " + appDetails.size());

    return appDetails;
}

Can I improve these methods? Is there a better way?

If I use it in the live DB, I doubt the job can be finished before it's started again after 2 mins.

For starters code-wise, I'm guessing you can try allApplicationDetails.parallelStream().

Try to use more method references if possible as there are potential performance advantages to doing so.

Your try-catch is unusually wide, if an Exception is thrown from the first stream element, no further processing is done. Are you sure that is OK?

How is jobExecutionContext being used?

Other more worthy considerations...

  What I am doing is very simple, just get all the records and loop through one by one, and do things.

Can you batch some of these operations together? For example, do your 'downstream' libraries accept multiple inputs at once, or do they only send individually?

Do you really need to retrieve all 22 columns of 1.5 to 2 million records? Are there perhaps temporal columns on the table so that you can only fetch the most recently updated rows? Or a subset of the columns to minimize network traffic?

The bigger question is... can your actions be farmed off to an ExecutionService provider so that they can be performed asynchronously?

Simple in-memory processing of 1.5 to 2 million records in 2 minutes is relatively trivial with recent processing power. Having to incur network latencies (from emailing, twitter-ing tweeting, SMS-ing) is obviously going to introduce the 'timeout' issues that you are witnessing.

Most of these questions aren't really going to be covered by a code review (seeing how I've only dedicated four sentences to that), so perhaps you may want to drop by Programmers.SE for some of the more hard-hitting questions I've asked.


Simple array implementation without bounds-checking I am preparing myself for some job interviews. This is a simple array (with the error handling left out for brevity). Any input or suggestions on style and the use of templates would be appreciated.

    #include <iostream>
    template <class T>
    class Array {
    public:
      explicit Array(const int&);
      Array(const int&, const T&);
      Array(const Array&);
      virtual ~Array();
      T getVal(const int&) const;
      void setVal(const int&, const T&);
      T& operator[] (T);
      Array& operator= (const Array&);

      template <class U>
      friend std::ostream& operator<< (std::ostream&, const Array<U>&);

    private:
      T* arr;
      int size;
    };

    template <class T>
    Array<T>::Array(const int& init_size) {
      arr = new T[init_size];
      size = init_size;
    }

    template <class T>
    Array<T>::Array(const int& init_size, const T& init_value) {
      arr = new T[init_size];
      size = init_size;
      for (int i = 0; i < size; ++i) {
        arr[i] = init_value;
      }
    }

    template <class T>
    Array<T>::Array(const Array& original) {
      size = original.size;
      arr = new T[size];
      for (int i = 0; i < size; ++i) {
        arr[i] = original.arr[i];
      }
    }

    template <class T>
    Array<T>::~Array() {
      delete [] arr;
    }

    template <class T>
    T Array<T>::getVal(const int& index) const {
      return arr[index];
    }

    template <class T>
    void Array<T>::setVal(const int& index, const T& value) {
      arr[index] = value;
    }

    template <class T>
    T& Array<T>::operator[] (T index) {
      return arr[index];
    }

    template <class T>
    Array<T>& Array<T>::operator= (const Array& copy) {
      if (arr == copy.arr)
        return *this;
     size = copy.size;
      delete [] arr;
      arr = new T[size];
      for (int i = 0; i < size; ++i) {
        arr[i] = copy.arr[i];
      }
      return *this;
    }

    template <class T>
    std::ostream& operator<< (std::ostream& out, const Array<T>& self) {
      out << "[ ";
      for (int i = 0; i < self.size-1; ++i) {
        out << self.arr[i] << ", ";
      }
      out << self.arr[self.size-1] << " ]";
      return out;
    }

In the constructor that just takes a size. I would initialize all members to their default value even if POD.

template <class T>
Array<T>::Array(const int& init_size)
{
  arr = new T[init_size]();
                  //    ^^^^^  Add braces. on types it makes no difference.
                  //           On POD it will force zero initialization.
  size = init_size;
}

Use the initializer lists in your constructors.

template <class T>
Array<T>::Array(const int& init_size)
    : arr(new T[init_size]())
    , size(init_size)
{}

Use copy and swap idiom for assignment operator (it automatically provides the strong exception guarantee).

template <class T>
Array<T>& Array<T>::operator= (const Array& copy)
{
    if (arr == copy.arr)  // This is the wrong test.
        return *this;     // Though it works as a side effect you will confuse the maintainer.

    size = copy.size;
    delete [] arr;       // breaks the strong exception gurantee.
                         // What it the next line fails? Then you are left with an object
                         // in an inconsistent state. (arr points at de-allocated memory)
    arr = new T[size];

    for (int i = 0; i < size; ++i)
    {
        arr[i] = copy.arr[i];
    }
    return *this;
}

If you want to do this the hard way then it should look like this:

template <class T>
Array<T>& Array<T>::operator= (const Array& copy)
{
    if (this == &copy)    // Return quickly on assignment to self.
    {   return *this;
    }

    // Do all operations that can generate an expception first.
    // BUT DO NOT MODIFY THE OBJECT at this stage.
    T* tmp = new T[size];
    for (int i = 0; i < size; ++i)
    {
        tmp[i] = copy.arr[i];
    }

    // Now that you have finished all the dangerous work.
    // Do the operations that  change the object.
    std::swap(tmp, arr);
    size = copy.size;

    // Finally tidy up
    delete tmp;    // Notice the swap above.

    // Now you can return
    return *this;
}

Alternatively use the copy and swap idiom

template <class T>
Array<T>& Array<T>::operator=(Array const& rhs)
{
    // 1 Copy
    Array    copy(rhs);     // Use the copy constructor to make a safe copy.

    // 2 Swap
    swap(arr,   copy.arr);
    swap(size,  copy.size);

} // 3 as the local copy goes out of scope it destroys the old array.

Note this can be optimized too:

Array<T>& Array<T>::operator=(Array rhs) // 1 implicit copy as parameter is passed by value
{
    // 2 Swap
    swap(arr,   copy.arr);
    swap(size,  copy.size);

} // 3 as the local copy goes out of scope it destroys the old array.

You don't need a get/set methods. Infact get/set is an obvious indication of bad design.

template <class T>
T Array<T>::getVal(const int& index) const {
  return arr[index];
}

template <class T>
void Array<T>::setVal(const int& index, const T& value) {
  arr[index] = value;
}

Just remove both these methods. What you want to do is return a reference to the internal object in your overload of operator[] (which you do) bit the index is an integer usually.

template <class T>
T& Array<T>::operator[] (size_t index)
{                //      ^^^^^^^    Changed type here.
  return arr[index];
}

You also need to provide a verision to use when your array is const.

template <class T>
T const& Array<T>::operator[] const (size_t index)
//^^^^^   Here                ^^^^^  and here.
{
  return arr[index];
}

If you provide an output operator you should generally by symmetric and provide an input operator.


Simple Java program - Coding bat sumNumbers The problem is stated as follows:

Given a string, return the sum of the numbers appearing in the string, ignoring all other characters. A number is a series of 1 or more digit chars in a row. (Note: Character.isDigit(char) tests if a char is one of the chars '0', '1', .. '9'. Integer.parseInt(string) converts a string to an int.)
sumNumbers("abc123xyz") → 123
sumNumbers("aa11b33") → 44
sumNumbers("7 11") → 18

Here's the program I wrote based on Character.isDigit(char) and Integer.parseInt(string).
public int sumNumbers(String str) {
  int sum = 0;
  for (int i = 0; i < str.length(); i++) {
      if (Character.isDigit(str.charAt(i))) {
          int count = 0;
          for (int j = i; j < str.length(); j++) {
              if (Character.isDigit(str.charAt(j))) count++;
              else break;
          }
          sum += Integer.parseInt(str.substring(i, i + count));
          i += count;
      }
  }
  return sum;
}

I'm not sure if I should make it more modular since it's pretty straightforward. I was wondering if I could do this without count. Also, I know it's nit-picky, but I want to know if the way I format my for loops and if else statements is good style. This is just a standalone program, so I'm not really concerned about the public/private issue.

There are a couple things you can do here to make things simpler. The first is to make your inner loop a while loop instead of a for loop. With this idea, you  can build up each number char by char, using Character.getNumericValue() which will return 1 for '1' etc. With charVal, we can incrementally build up our number by multiplying the by 10 and adding the new digit. When we reach a non-digit, we add the current num to our sum, and set sum to 0. Since the code now only operates one char at a time, we don't need i, or j either. With these changes, your updated code is

public int sumNumbers(String str) {
    int sum = 0;
    int num = 0;
    for (char ch : exampleString.toCharArray()) {
        int digit = Character.getNumericValue(ch);
        if (digit >= 0 && digit <= 9) {
            num = num * 10 + digit;
        } else {
            sum += num;
            num = 0;
        }
    }
    return sum + num;
}


Is this a correct way to write a convenience constructor? I was reading more about the instancetype on Stack Overflow and now my question is:

For every convenience constructor that I write, do I need to have the corresponding one using init?

For example:

RandomItem.h

#import <Foundation/Foundation.h>

@interface RandomItem : NSObject

@property (nonatomic, copy) NSString *aString;

+(instancetype)itemWithString:(NSString *)string;

@end

RandomItem.m

#import "RandomItem.h"

@implementation RandomItem

+(instancetype)itemWithString:(NSString *)string
{
    RandomItem *anItem = [[self alloc] init];
    anItem.aString = string;
    return anItem;
}

@end

And compare this when using an -initWithString: constructor as well:

RandomItem.h

#import <Foundation/Foundation.h>

@interface RandomItem : NSObject

@property (nonatomic, copy) NSString *aString;

-(id)initWithString:(NSString *)string;
+(instancetype)itemWithString:(NSString *)string;

@end

RandomItem.m:

#import "RandomItem.h"

@implementation RandomItem

-(id)initWithString:(NSString *)string
{
    self = [super init];
    if (self)
    {
        self.aString = string;
    }
    return self;
}

+(instancetype)itemWithString:(NSString *)string
{
    return [[self alloc] initWithString:string];
}

@end

I suppose that both cases are fine, but I would like to hear your comments.

Thanks!

Personally I would prefer the second option because it makes it easier/less weird for subclasses to use and override alloc-init.

As a side node I would point to some reasons to reconsider user properties in your constructors.

Should I refer to self.property in the init method with ARC?
Friday Q&A 2009-11-27: Using Accessors in Init and Dealloc
Don’t Message self in Objective-C init (or dealloc)


Dependency injection in constructor vs Supplying argument in utility functions I have the following:

public class Person{
    public string FirstName {get; set;} = "";
    public string MiddleName {get; set;} = "";
    public string LastName {get; set;} = "";
    public string Title {get; set;} = "";
}

public class Program
{
    public static void Main()
    {
        var person = new Person {FirstName = "Daniel", MiddleName = "W", LastName = "Craig", Title = "Mr"};
    }
}

I am writing a service class that reports back a user's

Full name (First + Middle + Last names)
Title + Full name
First name and last name

Using dependency injection, I have the following:

(1)

public class NameService{

    private readonly Person _person;
    public NameService(Person person){
        _person = person;
    }

    public string GetFullName(){
        var fullName = $"{_person.FirstName} {_person.MiddleName} {_person.LastName}";

        return fullName;
    }

    public string GetFullNameWithTitle(){
        var fullNameWithTitle = $"{_person.Title} {_person.FirstName} {_person.MiddleName} {_person.LastName}";

        return fullNameWithTitle;
    }

    public string GetFirstLastName(){
        var fullName = $"{_person.FirstName} {_person.LastName}";

        return fullName;
    }
}

Which I can use like this:

var person = new Person {FirstName = "Daniel", MiddleName = "W", LastName = "Craig", Title = "Mr"};
var service = new NameService(person);

var fullName = service.GetFullName();

However, I can also define the service like this:

(2)

public class NameService
{
    public NameService(){}

    public string GetFullName(Person person)
    {
        var fullName = $"{person.FirstName} {person.MiddleName} {person.LastName}";
        return fullName;
    }

    public string GetFullNameWithTitle(Person person)
    {
        var fullNameWithTitle = $"{person.Title} {person.FirstName} {person.MiddleName} {person.LastName}";
        return fullNameWithTitle;
    }

    public string GetFirstLastName(Person person)
    {
        var fullName = $"{person.FirstName} {person.LastName}";
        return fullName;
    }
}

And then get a person's full name like so:

    var person = new Person {FirstName = "Daniel", MiddleName = "W", LastName = "Craig", Title = "Mr"};
    var service = new NameService();

    var fullName = service.GetFullName(person);

Which is the better approach? My thoughts:

(1)

Looks like the service is designed on a "per Person" basis. This means calling another person's full name will require instantiating a new service with a new Person argument (and that's okay).

(2)

This is a lightweight constructor that allows different Person object to be supplied into GetFullName(person) to allow different rendition of full name.

What other pros and cons can you think of?

If you use second option you should probably declare  methods as static. Since you dont have any "data" associated with NameService object instance(service).

Like this:

public static string GetFullName(Person person)
{
    var fullName = $"{person.FirstName} {person.MiddleName} {person.LastName}";
    return fullName;
}

And then you can call...

NameService.GetFullName(person);

... without object initialazing - without line var service=new NameService();


Is this circular linked list better than the fast & slow pointer method? I have a faster way to find if a list is circular linked list, in contrast to the solution that Cracking the Coding Interview has offered:

In the below algorithm, fast-pointer will either hit null (in case of linear list) or hit starting point/origin (in case of circular) quicker than conventional (fast & slow pointer approach).

Reason: because in fast & slow pointer approach, we wait for the fastPointer == slowPointer condition to determine if it's a circular list. But fastPointer would reach its origin/starting point first before hitting slow pointer. (what's the point of crossing over the starting point and continuing the hunt for slowPointer?)

Isn't the below example a better algorithm?  If so, then why have a fast & slow pointer approach?

(I didn't consider starting point as "node outside of loop" scenario while asking question.)

public boolean findCircular(Node someRandomNodeOfList) {

    Node fast = someRandomNodeOfList;
    while (true) {
        if ((fast.next == null) || (fast.next.next == null)) {
            return false;
        }
        else if (fast.next == someRandomNodeOfList|| fast.next.next == someRandomNodeOfList) {
            return true;
        }
        else {
            fast = fast.next.next;
        }
    }
    return false;
}

This works when you're starting from inside the cycle. However, the following is also a valid circular list:

N1 --> N2 --> N3 --> N4
       ^             v
       +-------------+

Your algorithm works when starting from N2, N3 or N4, but not when starting from N1.


Periodically synching data from an API to database is extremely slow I have a scheduled task where I synch all the data from one table (via an API) to another. There are about 10,000 rows that are being updated and the new ones get created. This is what I came up with and for some reason, it is extremely slow.

def sync_data_from_hub():
    stats = tasks.sync_clients_from_hub()

    return stats

tasks.py

# 1. This method calls an external API to fetch all the rows.
def sync_clients_from_hub():
    url = resolve_hub_url('clients')

    response = requests.get(url, headers=headers)

    if response.status_code != 200:
        print('api response error: {}'.format(response))
        return

    try:
        response_payload = response.json()
    except ValueError:
        return

    # response from API
    clients_data = response_payload.get('data')

    for (key, value) in clients_data.items():
        # Check whether there are any actual changes in this row
        # by comparing the last updated date
        if services.should_update_data_from_hub(value, constants.SyncTypes.CLIENT):
            user, action = services.update_or_create_client_from_hub_data(value)

            if action == constants.SyncActions.CRT:
                result['new'] += 1
            if action == constants.SyncActions.UPD:
                result['update'] += 1

    # soft delete clients which are not present in payload but still available in database.
    if clients_data:
        discarded_clients = models.Client.objects.exclude(pk__in=list(map(int, clients_data.keys()))).filter(
            is_active=True)

        for client in discarded_clients:
            client.is_active = False
            client.save()

            result['deleted'] += 1

    return result

service.py

# logic to determine whether we should update the row or not
def should_update_data_from_hub(hub_data: dict, sync_type: str) -> bool:
    last_updated_date_from_api = hub_data.get('updatedAt', None)

    hub_data['id'] = int(hub_data.get('client', ''))

    # This line hits DB to fetch the row corresponding to the one in the API
    entity = get_client_by_pk(hub_data['id'])

    if not entity:
        return True

    last_updated_date_local = entity.last_updated_date

    if not last_updated_date_local:
        return True
    # The last updated dates have different timezones, so I have to adjust the timezone before comparison.
    last_updated_date_tz = last_updated_date.replace(tzinfo=timezone.utc).astimezone(tz=None)

    last_updated_date_local_formatted = last_updated_date_tz.strftime('%Y-%m-%d %H:%M:%S')

    return last_updated_date_formatted != last_updated_date_from_api

# Actual method to update the data
def update_or_create_client_from_hub_data(client_data: dict):
    client_data['id'] = int(client_data.get('client', None))

    # Set `is_active` based on the `archived` flag.
    if client_data.get('archive', None) == '1':
        client_data['is_active'] = False
    else:
        client_data['is_active'] = True
    ...

    # Set `last_updated_date`
    client_data['last_updated_date'] = client_data.get('updatedAt', datetime.datetime.now())

    serializer = serializers.ClientCreateSerializer(data=client_data)

    if not serializer.is_valid():
        print('client sync errors: {}'.format(serializer.errors))
        return None, None

    # This line hits DB to fetch the row corresponding to the one in the API
    client = get_client_by_pk(serializer.validated_data['id'])

    if client:
        action = constants.SyncActions.UPD

        for (key, value) in serializer.validated_data.items():
            setattr(client, key, value)

        client.save()
    else:
        action = constants.SyncActions.CRT

        client = models.Client.objects.create(**serializer.validated_data)

    return client, action

def get_client_by_pk(pk: int) -> Optional[models.Client]:
    return models.Client.objects.filter(pk=pk).first()

How can I optimize it to perform better? Should I load all the rows from DB in memory first rather than fetching them one by one and doing a comparison?

Error handling

if response.status_code != 200:
    print('api response error: {}'.format(response))
    return

is better represented as a simple call to response.raise_for_status().

Also, this:

try:
    response_payload = response.json()
except ValueError:
    return

is dangerous. Silently failing should (at the least) be left up to the caller to decide whether an exception should be caught and swallowed, caught and printed, or allowed to fall through.

Tuple unpacking

for (key, value) in clients_data.items():

can be

for key, value in clients_data.items():

Undeclared variable

        if action == constants.SyncActions.CRT:
            result['new'] += 1
        if action == constants.SyncActions.UPD:
            result['update'] += 1

Where does result come from? You don't show this. Is it a defaultdict?

Incorrect default

hub_data['id'] = int(hub_data.get('client', ''))

will not work in the case that the key is missing. You can't convert '' to an int. Perhaps you meant:

hub_data['id'] = int(hub_data.get('client', '0'))

Ellipses

This is more of a meta-comment, but

# Set `is_active` based on the `archived` flag.
if client_data.get('archive', None) == '1':
    client_data['is_active'] = False
else:
    client_data['is_active'] = True
...

should show the ellipses as a comment to avoid breaking syntactical validity, and you should provide rationale as to why this code is elided. Eliding code harms the potential for you to receive a meaningful review.

Optimization

  How can I optimize it to perform better?

It's nearly impossible to say. You haven't run a profiler, which you should; you've elided too much code for us to run this thing; you haven't shown any example data; and you haven't given us an API endpoint (or if that's not possible, at least a sample of a payload the endpoint would return).


Calculating entropy of a string We're calculating entropy of a string a few places in Stack Overflow as a signifier of low quality.

I whipped up this simple method which counts unique characters in a string, but it is quite literally the first thing that popped into my head. It's the "dumbest thing that works".

/// <summary>
/// returns the # of unique characters in a string as a rough
/// measurement of entropy
/// </summary>
public static int Entropy(this string s)
{
  var d = new Dictionary<char, bool>();
  foreach (char c in s)
      if (!d.ContainsKey(c)) d.Add(c, true);
  return d.Count();
}

Is there a better / more elegant / more accurate way to calculate the entropy of a string?

Efficiency is also good, though we never call this on large strings so it is not a huge concern.

Won't this work too?

string name = "lltt";
int uniqueCharacterCount = name.Distinct().Count();

will return 2


Java 8 file read method I have a method which extracts filedata and converts it into a String array:

import java.io.BufferedReader;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.DataInputStream;
import org.apache.james.mime4j.message.BodyPart;
import org.apache.james.mime4j.message.Message;
import org.apache.james.mime4j.message.Multipart;
import org.apache.james.mime4j.message.TextBody;

protected String[] extractLedesText(byte[] fileData) {
    // Remove the BOM if present
    byte[] array = { (byte) 0xEF, (byte) 0xBB, (byte) 0xBF };
    byte[] data =  { fileData[0], fileData[1], fileData[2] };
    if (fileData.length > 3 && Arrays.equals(data, array)) {
        fileData = ArrayUtils.subarray(fileData, 3, fileData.length-1);
    }

    String ledes = new String(fileData);
    if (ledes.startsWith("MIME")) {

        try {
            ledes = null;
            Message signed = new Message(new ByteArrayInputStream(fileData));
            for (BodyPart part : ((Multipart) signed.getBody()).getBodyParts()) {
                if (part.getMimeType().equalsIgnoreCase("text/plain")) {
                    TextBody tb = (TextBody) part.getBody();
                    ByteArrayOutputStream baos = new ByteArrayOutputStream();
                    tb.writeTo(baos);
                    return extractLedesText(baos.toByteArray());
                }
            }
            throw new BaseApplicationException(
                    "No MIME part found with MIME type of 'text/plain' while parsing submitted invoice file.");
        } catch (IOException ioe) {
            throw new BaseApplicationException(ioe);
        }
    } else {
        return ledes.split("\\[]");
    }

}

For example, below is the leads file:

LEDES98BI V2[]
INVOICE_DATE|INVOICE_NUMBER|CLIENT_ID|LAW_FIRM_MATTER_ID|INVOICE_TOTAL[]
20150301|INV-Error_Test1|160|LF_MAT_1221|22[]
20150301|INV-Error_Test1|160|LF_MAT_1221|22[]
20150301|INV-Error_Test1|160|LF_MAT_1221|22[]

The extractLedesText method converts the above file data to string array of lines.

We recently upgraded to Java 8 and am wondering whether this method can be optimized further.

It looks like extractLedesText() is trying to do three things:

Strip away any BOM from fileData.
If this is a MIME message, extract only the TextBody part and pass that recursively into this method again.
Else just do a split("\\[]") to get our desired String[] array.

So...

Data massaging

You can use a helper method to achieve this:

private static byte[] filterBOM(byte[] fileData) {
    if (fileData.length < 3) {
        return fileData;
    }
    final byte[] array = { (byte) 0xEF, (byte) 0xBB, (byte) 0xBF };
    final byte[] data =  { fileData[0], fileData[1], fileData[2] };
    return fileData.length > 3 && Arrays.equals(data, array) ?
        Arrays.copyRangeOf(fileData, 3, fileData.length) : fileData;
}

I think it may be better to do a fileData.length < 3 check before you construct your data array. Also, I am using the Arrays utility class instead of ArrayUtils to copy part of the array.

Extracting the TextBody from a MIME message

You can make use of try-with-resources for both your ByteArrayInputStream and ByteArrayOutputStream instances, and I suppose this is where we can employ a bit of Stream trickery...

private static byte[] getTextBody(final String ledes) {
    final TextBody tb;
    try (final InputStream input = new ByteArrayInputStream(ledes.getBytes(StandardCharsets.UTF_8))) {
        tb = ((Multipart) new Message(input).getBody()).getBodyParts().stream()
                .filter(part -> part.getMimeType().equalsIgnoreCase("text/plain"))
                .findFirst().orElseThrow(() -> new BaseApplicationException(
                    "No MIME part found with MIME type of 'text/plain' while parsing submitted invoice file."))
                .getBody();
    } catch (IOException e) {
        throw new BaseApplicationException(e);
    }
    try (final ByteArrayOutputStream output = new ByteArrayOutputStream()) {
        tb.writeTo(output);
        return output.toByteArray();
    } catch (IOException e) {
        throw new BaseApplicationException(e);
    }
}

We stream on the List from getBodyParts(), filtering those where their MIME type is "text/plain" and then look for the first matching BodyPart. If there isn't any, we call orElseThrow() with a new BaseApplicationException(...), otherwise we will getBody() and writeTo() your ByteArrayOutputStream. The output of this method is a byte[] array if there are no thrown Exceptions.

Putting it all together

protected String[] extractLedesText(byte[] fileData) {
    final String ledes = new String(filterBOM(fileData));
    return ledes.startsWith("MIME") ? extractLedesText(getTextBody(ledes)) : ledes.split("\\[]");
}


Idiomatic conditionals in F# I'm learning F#, and even though I'm able to do whatever I want, parts of my code looks really bad. I would love to get some suggestions on how to improve a couple of functions involving some conditional logic.

let stripLast (punct : char) (word : string) =
    if word.Length > 0 && word.LastIndexOf(punct) = word.Length - 1
    then word.Substring(0, word.Length - 1)
    else word

let countWord (dictionary : Dictionary<string, int>) word =
    if word = "" || (blacklisted word) then "" |> ignore
    elif dictionary.ContainsKey(word)  then dictionary.[word] <- dictionary.[word] + 1
    else dictionary.[word] <- 1

I'm guessing there are ways to make these much more elegant. For instance, the second function uses side-effects on a Dictionary and has return type unit, but I added a branch where I had to use ignore in (probably) a silly way. Please don't change the functionality, but show me a nicer way to write the code.

This is probably the way I would write it:

let stripLast punct word =
  match String.length word with
  | n when n > 0 && word.[n-1] = punct -> word.[..n-2]
  | _ -> word

let countWord (dictionary : Dictionary<_,_>) = function
  | null | "" -> ()
  | word when blacklisted word -> ()
  | word ->
    match dictionary.TryGetValue(word) with
    | true, n -> dictionary.[word] <- n + 1
    | _ -> dictionary.Add(word, 1)

Here's a rundown of the changes:

In stripLast:

Use String.length instead of a type annotation (punct can also be inferred)
Capture the length (which is used multiple times) using pattern matching
Check only the last char (LastIndexOf searches the whole string)
Use string slicing over Substring for conciseness

In countWord:

Allow the type args for Dictionary<_,_> to be inferred
Use pattern matching to clearly show the cases to be handled
Replace calls to ContainsKey and Item ([]) with one call to TryGetValue

A more functional approach, assuming you have a list of words, might be:

words
  |> Seq.countBy (stripLast stripChar)
  |> Seq.filter (fun (word, _) ->
    match word with
    | null | "" -> false
    | _ -> not (blacklisted word))


Find out if attributes are identical I have an array of active record objects. All objects have an attribute result and the value of result can be either 'failed' or 'passed'.

My goal is to find out if all result attributes are identical. If not then I will have to return mixed but if they are then I return the value of the attributes.

I currently have this code:

if tests.map(&:result).uniq.count != 1
   'mixed'
else
   tests.first.result
end

But that first line is quite ugly. I'm sure there is a better way to find out if all results are identical or not but I can't think of any.

You could try the appropriately named #all? method:

reference = tests.first.result
if tests.all? { |test| test.result == reference }
  reference
else
  'mixed'
end

Edit: As tokland points out, you'll want to check if tests is empty before any of this. Otherwise, tests.first will of course be nil.

But you could also do the check in the database layer with something like:

if Test.select(:result).distinct.count > 1
  'mixed'
else
  Test.first.try(:result)
end

Obviously you'll want to scope the query more specifically.

Also note that here, an empty collection returns a nil (because if Test.first is nil, then .try(:result) will also be nil).


Best way to identify a computer in LAN C# I have a web application to manage computers. Every registered computer is locally querying the latest information and updates the database with it in a 10 minute cycle.

But the computer is only allowed to update its information and nothing else. Currently I'm identifying the computer like this:

I get its hostname from the "remote_addr" variable

string senderHostname = (Dns.GetHostEntry(Request.ServerVariables["remote_addr"]).HostName).Split('.')[0];

Then I compare the hostname of the sender computer with the hostname of the computer which should be updated.

if (senderHostname.Equals(targetHostname, StringComparison.InvariantCultureIgnoreCase)) {
 // allowed
}
else {
 // forbidden
}

If they are the same, then I'm allowing the update, otherwise not.

I know that this code is very very ugly and I would be interested in a best practice to do this.

All the code:

protected void Page_Load(object sender, EventArgs e)
{
    try
    {
        clientMachineName = (Dns.GetHostEntry(Request.ServerVariables["remote_addr"]).HostName).Split('.')[0];
    }
    catch (Exception)
    {
        clientMachineName = "n/a";
    }

    if (!String.IsNullOrEmpty(Request.QueryString["status"]) && !String.IsNullOrEmpty(Request.QueryString["value"]))
    {
        String macAddress = Request.QueryString["status"];
        String statusCode = Request.QueryString["value"];

        if (clientMachineName.Equals(getHostName(macAddress), StringComparison.InvariantCultureIgnoreCase))
        {
            ComputerDbCommunicator communicator = new ComputerDbCommunicator();
            communicator.insertIntoComputerDb("UPDATE [Computer] SET [statuscode] = " + statusCode + " WHERE [macaddress] = '" + macAddress + "'");

            communicator.closeConnections();
            communicator = null;

            Response.Clear();
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "text/plain");
            Response.Write(statusCode);
            Response.Flush();
            Response.End();
        }
        else
        {
            Response.Clear();
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "text/plain");
            Response.Write("Operation not allowed");
            Response.Flush();
            Response.End();
        }
    }
    else if (!String.IsNullOrEmpty(Request.QueryString["status"]))
    {
        String macAddress = Request.QueryString["status"];
        String statusCode = null;
        ComputerDbCommunicator communicator = new ComputerDbCommunicator();
        SqlDataReader dr = communicator.queryComputerDb("SELECT [statuscode] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
        if (dr.HasRows)
        {
            while (dr.Read())
            {
                statusCode = dr[0].ToString();
            }
            dr.Close();

            Response.Clear();
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "text/plain");
            Response.Write(statusCode);
            Response.Flush();
            Response.End();

            communicator.closeConnections();
            communicator = null;
        }
    }
}

protected String getHostName(String macAddress)
{
    String hostName = null;
    ComputerDbCommunicator communicator = new ComputerDbCommunicator();
    SqlDataReader dr = communicator.queryComputerDb("SELECT [hostname] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
    while (dr.Read())
    {
        hostName = dr[0].ToString();
    }
    dr.Close();

    communicator.closeConnections();
    communicator = null;

    return hostName;
}

You've got some duplication in there;

Perhaps you can create a function with a string argument that will handle response writing?

protected void CreateResponse(string response){
    Response.Clear();
    Response.ClearHeaders();
    Response.AddHeader("Content-Type", "text/plain");
    Response.Write(response);
    Response.Flush();
    Response.End();
}

To me that seems far more concerning than the way in which you retrieve the hostname.


Codility: MaxZeroProduct challenge and solution Test data passes the test, so why does Codility keep failing it? Am I perhaps misunderstanding the requirement?

using System;

class Solution {
    public int solution(int[] A) {
        int max = 0;
        for (var i = 0; i < A.Length; i++) {
            for (var j = i + 1; j < A.Length; j++) {
                for (var k = j + 1; k < A.Length; k++) {
                    var product = A[i] * A[j] * A[k];
                    var zeros = trailingZeros(product);
                    //Console.WriteLine($"{product} - {zeros}");
                    if (zeros > max) {
                        max = zeros;
                    }
                }
            }
        }

        return max;
    }

    private int trailingZeros(int N) {
        if (N == 0) {
            return 0;
        }
        int count = 0;
        while (!(N <= 0 || N % 10 != 0)) {
            count++;
            N /= 10;
        }
        return count;
    }
}

Test data:

A = [7, 15, 6, 20, 5, 10]
A = [25, 10, 25, 10, 32]
A = [1, 1, 110]
A = [90, 76, 300, 10, 3, 5, 9, 1000000000]
A = [100090, 10030, 1000, 90, 87655, 676345]
A = [85, 65, 20, 50, 1, 0]
A = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20]
A = [10, 15, 20, 25, 30, 35, 40, 45, 50, 55, 60, 65, 70, 75, 80, 85, 3, 90]
A = [101, 307, 890, 208, 406, 701, 222, 445, 5432]
A = [7, 15, 6, 20, 5, 10]
A = [90, 70, 80]
A = [900, 800, 700, 600, 500, 551, 662, 773, 884, 995]

Full description of the problem:

  Write a function:

  class Solution { public int solution(int[] A); }

  that, given an array of N positive integers, returns the maximum
  number of trailing zeros of the number obtained by multiplying three
  different elements from the array. Numbers are considered different if they are at different positions in the array.

  For example, given A = [7, 15, 6, 20, 5, 10], the function should
  return 3 (you can obtain three trailing zeros by taking the product of
  numbers 15, 20 and 10 or 20, 5 and 10).

  For another example, given A = [25, 10, 25, 10, 32], the function
  should return 4 (you can obtain four trailing zeros by taking the
  product of numbers 25, 25 and 32).

  Assume that:

  N is an integer within the range [3..100,000]; each element of array A
  is an integer within the range [1..1,000,000,000]. Complexity:

  expected worst-case time complexity is O(N*log(max(A)))
  expected worst-case space complexity is O(N) (not counting the storage required
  for input arguments).

You must consider domain boundaries: A can contain integers in the range [1..1,000,000,000] so the largest possible product would be 1,000,000,000^3 which is way beyond int.MaxValue (and even ulong.MaxValue). One way to overcome this problem while keeping you algorithm as is, is to use System.Numerics.BigInteger instead of int.

This conditional expression while (!(n <= 0 || n % 10 != 0)) has its equivalent in while (n > 0 && n % 10 == 0) which I find more straight forward readable.

These nested loops can be optimized a little bit:

    for (var i = 0; i < A.Length; i++) {
        for (var j = i + 1; j < A.Length; j++) {
            for (var k = j + 1; k < A.Length; k++) {

    for (var i = 0; i < A.Length - 2; i++) {
        for (var j = i + 1; j < A.Length - 1; j++) {
            for (var k = j + 1; k < A.Length; k++) {


Inline PHP IP access log For a website, I've got some inline PHP, posted below.  It's supposed to log traffic to the website, and it does its job fine.  But at the end of the day, I'm not even close to a PHP developer, and this is really just hacked together from Googling and inferring from other, more familiar languages.  How can I improve this?  Is using PHP in the actual file like this bad?  (This is in index.html.)  Also, I don't believe I'm open to an SQL injection attack since the variables are all drawn directly from the server rather than the user, but I could be wrong.

    <?php
        $ip = (!empty($_SERVER["HTTP_CLIENT_IP"]) ? $_SERVER["HTTP_CLIENT_IP"] : $_SERVER["REMOTE_ADDR"]);
        $fwd = (!empty($_SERVER["HTTP_X_FORWARDED_FOR"]) ? "\"".$_SERVER["HTTP_X_FORWARDED_FOR"]."\"" : "null");
        $hostname = gethostbyaddr($ip);

        if($ip !== "127.0.0.1" && $ip !== "192.168.1.1") {
            $con = mysqli_connect("localhost","username","password","database");
            if(!mysqli_connect_errno($con)) {
                $result = mysqli_query($con, "INSERT INTO table_name VALUES(\"".$ip."\", now(), \"".$hostname."\", ".$fwd.")");
            }
        }
    ?>

I don't think its really SQL-injection, but $hostname is not taken from server but from DNS.

Theoretically i can add some kind of malicious domain name and affect your query. But from other hand, domain syntax is really limited. I don't think you can use it to do some serious thing.

Though i would recommend some error protection to avoid SQL-errors.

And correct mysql-escaping can never be bad ))


Slow Flask-SQLAlchemy query using association tables I have two models in Flask-SQLAlchemy (Post and Comment) that have many-to-many relationship that is manifested in the third model (post_mentions):

post_mentions = db.Table(
    'post_mentions',
    db.Column('post_id', db.Integer, db.ForeignKey('posts.id'), primary_key=True),
    db.Column('comment_id', db.Integer, db.ForeignKey('comments.id'), primary_key=True),
)

class Post(db.Model):
    __tablename__ = 'posts'

    id = db.Column(db.Integer, primary_key=True)
    name = db.Column(db.String, unique=True, nullable=False)
    mentions = db.relationship('Comment', secondary=post_mentions, lazy='dynamic')

    def __eq__(self, other):
        return self.name.lower() == other.name.lower()

    def __hash__(self):
        return hash(self.name.lower())

class Comment(db.Model):
    __tablename__ = 'comments'

    id = db.Column(db.Integer, primary_key=True)
    text = db.Column(db.Text, nullable=False)
    created_at = db.Column(db.Integer, nullable=False)

There is also a /posts endpoint that triggers the following query:

# flask and other imports

@app.route('/posts')
def posts():
    page_num = request.args.get('page', 1)
    posts = models.Post.query.join(models.post_mentions)\
                             .group_by(models.post_mentions.columns.post_id)\
                             .order_by(func.count(models.post_mentions.columns.post_id).desc())\
                             .paginate(page=int(page_num), per_page=25)
    return render_template('posts.html', posts=posts)

There are more than 14k+ posts and 32k+ comments stored in SQLite database. As you can see from the snippet above, when someone hits /posts endpoint, SQLAlchemy loads all data at once to the memory and then subsequent queries (e.g. retrieving posts, comments to that posts, etc..) take sub-millisecond time, since data is being served from the memory without hitting the database. Initial load takes 10s+ on my laptop, which is, to put it mildly, suboptimal.

So the question is: Considering that users won't view 97+% of posts, how can I both order posts by number of mentions in comments and load them on demand instead of doing it in one swoop?

Saw your post on indiehackers. I don't know this orm, but generally speaking, I see you have two options.

Decide to preload/precache the data when your app starts and refresh it occasionally, if you insist on having all records available.

But some good advice I've read is : never do in real time what you can do in advance. So... Why not even build some "top posts" table and seed that?


Pretty-print dollar amounts using custom precision for large amounts Given a floating-point number, which is representing a dollar value, I'm aiming to return a rounded, human-readable string. Here are the rules for numbers equal to or greater than 1 million, equal to or greater than 1 but less than 1 million, and, finally, less than 1:

one decimal place for million, two places for billions and three places for trillions
numbers over 999 trillion are displayed in trillions, e.g. 1,000 trillion not 1 quadrillion
if less than 1 million, two decimal places (cents)
if less than 1, then as cents (multiplied by 100)

numbers which don't round up to at least 1 cent count as 0 cents

Also, for numbers greater than 1, there's no display of precision if mantissa is all zeros (e.g. 1.000 trillion becomes 1 trillion and 1.00 becomes 1.)
import math

def convert_float_to_value(number):
    if number == 0.0:
      return "0 cents"
    digits = 1 + math.floor(math.log10(number))
    if digits > 12:
        return f"${round(number / 10**12, 3):,.3f} trillion".replace(".000", "")
    elif digits > 9:
        return f"${round(number / 10**9, 2):,.2f} billion".replace(".00", "")
    elif digits > 6:
        return f"${round(number / 10**6, 1):,.1f} million".replace(".0", "")
    elif digits > 0:
        return f"${round(number, 2):,.2f}".replace(".00","")
    else:
        return f"{round(number * 100):,} cents".replace("1 cents", "1 cent")

I already sense some improvements, like perhaps I should be checking this really is a floating-point value and not an integer, or just turning whatever I get into a float. But what do you think of this approach? There are a number of pretty-printing libraries but these are some very specific rules.

To me your code looks very WET. Everything is using the same core but with minor tweeks here and there. And so we can convert your code to a for loop.
TRANSFORMS = [
    (           12, "$", 10**12,    3, ".3f", " trillion",    ".000",        ""),
    (            9, "$", 10** 9,    2, ".2f",  " billion",     ".00",        ""),
    (            6, "$", 10** 6,    1, ".1f",  " million",      ".0",        ""),
    (            0, "$", 10** 0,    2, ".2f",          "",     ".00",        ""),
    (float("-inf"),  "", 10**-2, None,    "",    " cents", "1 cents",  "1 cent"),
]

def convert_float_to_value(number):
    if number == 0.0:
        return "0 cents"
    digits = 1 + math.floor(math.log10(number))
    for d, *t in TRANSFORMS:
        if digits > d:
            return f"{t[0]}{round(number / t[1], t[2]):,{t[3]}}{t[4]}".replace(t[5], t[6])

From here we can start to see patterns and other improvements.

Excluding cents; t[2], t[3] and t[5] all have the same size.
This means we can build them all from one value.
v = ...
t[2] = v
t[3] = f".{v}f"
t[4] = "." + "0"*v

It makes little sense to use .2f for billions. I've never, until this day, seen "$1.10 billion". It's almost like you've used the wrong word and meant to say "dollars".
To fix this I'd stop using .2f. However this will break how .replace works, and can add some hard to fix bugs.
(Converting 10.01 to 101)
To fix this I'd convert integers to ints and keeps floats as floats.

We haven't built t[3] and t[5] from t[2] for plain dollars as it still requires .2f to make sense.
To deal with this we can make a function pretty_round that takes a number, a number of digets to round to, an option for the output to be a fixed amount of decimal places and any additional formatting. The only thing is that the output has return a string and a float.
def pretty_round(number, ndigits=None, fixed=False, format_spec=""):
    number = round(number, ndigits)
    if number % 1 == 0:
        return number, format(int(number), format_spec)
    return number, format(number, format_spec + (f".{ndigits}f" if fixed else ""))

With the above changes we can see t[5] and t[6] are only there to convert cents from plural to singular.
And so why not just check if the output is singular or plural and append the correct term?

By changing the method of finding digits to use len we can replace float('-inf') with -2 and build t[1] from d.

I am going to reorder the table so that they're grouped better. t[0] should be near t[5] and t[6].
And we can now give them easy to understand names.

TRANSFORMS = [
    (12,    3, False, " trillion", "$",       "",       ""),
    ( 9,    2, False,  " billion", "$",       "",       ""),
    ( 6,    1, False,  " million", "$",       "",       ""),
    ( 0,    2,  True,          "", "$",       "",       ""),
    (-2, None, False,          "",  "", " cents",  " cent"),
]

def pretty_round(number, ndigits=None, fixed=False, format_spec=""):
    number = round(number, ndigits)
    if number % 1 == 0:
        return number, format(int(number), format_spec)
    return number, format(number, format_spec + (f".{ndigits}f" if fixed else ""))

def convert_float_to_value(number):
    digits = len(str(int(number)).lstrip('0-'))
    for exp, dp, fixed, magnitude, prefix, plural, singular in TRANSFORMS:
        if digits > exp:
            number_f, number_s = pretty_round(number / 10**exp, dp, fixed, ",")
            name = singular if number_f == 1 and exp <= 0 else plural
            return f"{prefix}{number_s}{magnitude}{name}"

We can see that TRANSFORMS is not normalized.

All of prefix, plural and singular are refering to dollers and cents.
This can be extracted out into it's own table.
Not only does it reduce how WET your code is it allows us to replace dollars with other monetary notations.
Additionally it allows making changes easier if you want to change from using "$" to "dollars"

The magnitude can change even in English. This is as 1 trillion can equal 1 billion, depending on which scale you use.
Having this baked into the TRANSFORMS table can be problematic in the future.

With your values fixed is the same as not exp.

SHORT_SCALE = [
    "",
    " thousand",
    " million",
    " billion",
    " trillion",
]
USD = [
    ("$", "", ""),
    ("", " cents", " cent"),
]
TRANSFORMS = [
    (12, 3),
    ( 9, 2),
    ( 6, 1),
    ( 0, 2),
    (-2, 0),
]

def pretty_round(number, ndigits=None, fixed=False, format_spec=""):
    number = round(number, ndigits)
    if number % 1 == 0:
        return number, format(int(number), format_spec)
    return number, format(number, format_spec + (f".{ndigits}f" if fixed else ""))

def convert_float_to_value(number, *, scale=SHORT_SCALE, currency=USD):
    digits = len(str(int(number)).lstrip('0-'))
    for exp, dp in TRANSFORMS:
        if digits > exp:
            number_f, number_s = pretty_round(number / 10**exp, dp, not exp, ",")
            prefix, plural, singular = currency[exp < 0]
            name = singular if number_f == 1 and exp <= 0 else plural
            return f"{prefix}{number_s}{scale[int(exp / 3)]}{name}"


Lingo game using Python You guess a word, and if its letter(s) is in the same spot as a hidden word's, you add [] around the letter.  If it's in the hidden word, but not in the same spot, you add()).

As of now, I have a whole bunch of loops which I find makes the program un-compact. I would love to see some tips on how to shorten this program!  Efficiency isn't a problem.  I am looking for compactness.

word = list("tiger")
guesses = 0
holder = 1
while holder == 1:
    s = list(input("Please input what you think the word is"))
    while len(s) > 5 or len(s) < 5:
        print("Please guess a five - letter word!")
        s = list(input("Please input what you think the word is"))
    s2 = s
    for i in range (0, len(word)):
        if word[i] == s[i]:
            letter = ("[",s[i],"]")
            lette = "".join(letter)
            s2[i] = letter
    for i in range (0, len(word)):
        for j in range (0, len(word)):
            if s[i] == word[j]:
                letter2 = ("(", s[i],")")
                letter2 ="".join(letter2)
                s2[i] = letter2
    print(s2)
    guesses = guesses +1
    print ("You have used", guesses, "guesses/guess")
    s3 = ["[t]", "[i]", "[g]", "[e]", "[r]"]
    if s2 == s3:
        print("You WIN!")
        holder = 2

def game(word):
    guesses = 0
    while True:
        user_string = prompt("What do you think the word is? ",
                            lambda s: len(s) == len(word),
                            "Please guess a {}-letter word.".format(len(word)))

        if user_string == word:
            print("You WIN!")
            break

        print([handle_letter(user_string, word, i) for i in user_string])

        guesses += 1
        print ("You have guessed {} times.".format(guesses))

def handle_letter(user_string, word, i):
    if user_string[i] == word[i]:
        return "[" + user_string[i] + "]"
    elif user_string[i] in word:
        return "(" + user_string[i] + ")"
    else:
        return user_string[i]

def prompt(statement, condition, warning):
    user_string = input(statement)
    while not condition(user_string):
        print(warning)
        user_string = input(statement)
    return user_string

def main():
    game("tiger")

if __name__ == '__main__':
    main()

Assuming you are familiar with functions, there should be only two things here that stand out as "weird": the lambda and the list comprehension (this line: print([handle_letter(user_string, word, i) for i in user_string])).

lambdas are just anonymous functions, meaning they are functions that don't have name. I could have easily instead written:

def is_correct_length(user_string):
    return len(user_string) == len(word)

and replaced the lambda line with is_correct_length. It's just shorter this way.*

The list comprehension is a huge code-lines win, but I'd rather you just read http://docs.python.org/2/tutorial/datastructures.html#list-comprehensions than me explain it to you.

*Actually this approach would not work because word is not in scope for the sub-function unless you define it inside of game. lambda has the advantage here of having access to all the variables that in the scope of game, including word, which makes life easier.

If you reeeealy wanted to avoid the lambda you could put the definition of is_correct_length indented and inside of the definition of game, but this is probably not a good move stylistically.

It just occurred to me that this is actually MORE lines of code than you originally had, but I'd say it's better code because it's much more modular. The prompt function, for instance, seems ridiculously useful for games like this one and you can use it everywhere.

If you remove the main() and ifmain wrappers and put thing on one line that I have put on two, you could probably make it shorter, though this is usually not a good idea because readability it far more important than lines of code.

(If you really want to make it shorter - no holds barred - you could do this:

print(["[" + user_string[i] + "]" if user_string[i] == word[i] else "(" + user_string[i] + ")" if user_string[i] in word else user_string[i] for i in range(len(user_string))])

Instead of the current line in game and completely remove handle_letter. However, this is much harder to read so I would advise against it.)