Designing a text based guessing game with a branching decision tree I am hoping to make a quick decision tree using the if statement in excel vba. The point is to have a message with a prompt show up and then it will take the person through the prompts based on the answers. Can my current approach be improved upon in any way?

Sub Decision_tree()

    a = MsgBox("Want To Play A Game?", 3)

    If a = 6 Then
    MsgBox "Let the games begin!", 1

    ElseIf a = 7 Then
    MsgBox "Okay. Press ctrl K to start over."
    End

    Else
    MsgBox "Sad. Come back later. Press ctrl K to start."
    End
    End If

    a = MsgBox("are you a boy?", vbYesNo)

    If a = 6 Then '**
       b = MsgBox("does your names start before O?", vbYesNo)
       If b < 7 Then d = MsgBox("do you live in logan?", vbYesNo)
            ElseIf b = 7 Then
                   f = MsgBox("Is your name 4 letters?", vbYesNo)
                     If d = 6 Then
                       h = MsgBox("Did you serve your mission with Josh?", vbYesNo)
                       ElseIf f = 6 Then
                        MsgBox "Your name is Ryan"
                         ElseIf f = 7 Then
                        MsgBox "your name is Trevon"
                            ElseIf d = 7 Then
                                   j = MsgBox("Did you serve a mission with Josh?")
                                   ElseIf h = 6 Then
                                   MsgBox "Your name is Cade"
                                    ElseIf h = 7 Then
                                           K = MsgBox("are you a DJ?", vbYesNo)
                                             ElseIf j = 6 Then
                                             MsgBox "your name is Houston"
                                              ElseIf j = 7 Then
                                              MsgBox "your name is Cameron"
                                                ElseIf K = 6 Then
                                                MsgBox "your name is Jack with a j"
                                                 ElseIf K = 7 Then
                                                  MsgBox "your name is burton"

    ElseIf a = 7 Then
       c = MsgBox("does your name start Before N?", vbYesNo)
        ElseIf c = 6 Then
            e = MsgBox("does your name have a K in it?", vbYesNo)
            ElseIf c = 7 Then
                   g = MsgBox("do you own a jeep?", vbYesNo)
                   ElseIf e = 6 Then
                      i = MsgBox("Are you from Idaho?", vbYesNo)
                       ElseIf g = 6 Then
                        MsgBox "your name is olivia"
                        ElseIf g = 7 Then
                        MsgBox "your name is tiffany"
                            ElseIf i = 6 Then
                            MsgBox "Your name is Kasey"
                             ElseIf i = 7 Then
                              MsgBox "Your name is Mckenna"

       End If
    End If

End Sub

Let me start with a few quick things to enhance readability and then give some pointers how one could make this more maintainable, if one wanted to.

Magic Numbers

One thing that makes your code hard to follow is that you use magic numbers instead of named constants at multiple places. You already use vbYesNo for the MsgBox options most of the time. I wonder why you use 3 instead of vbYesNoCancel in the first message box. Moreover, the code would be more intuitive, if you replaced 6 by vbYes and 7 by vbNo. Whenever you want to use an explicit number in code, ask yourself whether there is a constant for that already and, if not, define a constant of your own and give it a semantic name.

Indentation

As @Greedo has pointed out in the comments, the strange indentation might be an artifact of how you have posted the code. However, as is, the indentation makes the code unreadable. ElseIfs should line up with the corresponding If statements and their content should be indented one level deeper. Then, you know which level of the tree you are in while reading the code.

Reuse of Variables

It is generally confusing in code, if you reuse a local variable. There is no benefit from reusing the variable a here. Still, the reader of the code has to know that it has been reused for an entirely different question in the second part of the procedure.

Naming

Code should always strive to be self-documenting. The most important part in this is good naming of variables and procedures. They should always have a name indicating what they are/do; variables, properties and functions should be nouns and procedures should be verbs. Here, expressiveness trumps shortness: there is no problem with having a long name if you cannot express the meaning in a short name. If this leads to a very long name, it might be a smell that you should split something up.
In you specific case, all your answers are single letter variables without any meaning, which is not ideal. E.g. a better name for e, or rather e = vbYes, would be nameStartsWithK.
Note that applying semantic naming would have avoided the reuse of the variable a automatically since there is no sensible common name for the two uses.

Declaring Variables

Do yourself a favor and always declare you local variables, e.g. using Dim a As Long right before using a the first time. This documents the variables type and subsequent assignment with an incompatible type will result in an error instead of unexpected behaviour. (VBA does a lot of helpful implicit conversions, though.)

Option Explicit

Please, always use Option Explicit on top of your component. You can let the editor add it automatically for new components by ticking Require Variable Declaration under Tools->Options->Editor. This will force you to declare all your variables. A very helpful side-effect is that it will catch typos in uses of your variables. E.g. is you try to reference nameStartsWithK but type nameStartWithK, it will complain that nameStartWithK is not declared. Without Option Explicit, you get a new variable of type Variant instead, which is really not what you want here.

Use of End

The End statement is something that usually should never be used. In general, it is unexpected behaviour that a called procedure terminates the entire program. Instead, the procedure should probably use Exit Sub in order to return execution to its caller.

Use of Single-Line If Statements

Single-line If statements like If b < 7 Then d = MsgBox("do you live in logan?", vbYesNo) are generlly a bit confusing because there is no End If one starts to expect out of habit. However, this is amplified tremendously without proper indentation since it is not immediate clear for the reader what the next ElseIf refers to.

Select Case Statement

For your first If block, a Select Case statement would probably be semantically more appropriate.

Select a
    Case vbYes
        MsgBox "Let the games begin!"
    Case vbNo
        MsgBox "Okay. Press ctrl K to start over."
    Case Else
        MsgBox "Sad. Come back later. Press ctrl K to start."
End Select

Note that I have removed the option 1 in the first case, which is vbOKCancel. More on this, next.

User Experience

Your first query to the user whether he wants to play a game has two somewhat strange features.

When you confirm that the game starts, you do this in a message box with option vbOKCancel but never react to the result. As the user, I would expect that clicking the Cancel will abort the game. If the effect of Cancel is the same as OK, do not display a Cancel button.
It is usually percieved as rather annoying, if a dialog pops up in response to cancelling. This is exactly what happens in your first query to the user.

Using Indirection to Improve Maintainability

When speaking about maintainability and flexibility one has to ask what you might want to change in the future and what is a hindering proper automated testing. In the case of this simple game, the latter is probably not a real concern, but for parts of larger applications, it is.

The main point of possible change I see here, is the presentation to the user. Not entirely by coincidence, this is also what would hinder any automated testing since blocking dialogs are a pain for that.

This is a bit more advanced, but what you could do to abstract away the use of the message box is to add a new class YesNoUserInteraction with a single function

Public Function UserAgrees(ByVal questionText As String) As Boolean
    UserAgrees = (MsgBox(questionText , vbYesNo) = vbYes)
End Function

The you new it up and save it in a variable Dim userInteraction As YesNoUserInteraction in your code Set userInteraction = New YesNoUserInteraction and use that instead of the message box.

Going one step further, you can split out the part newing up the object and pass it as a parameter to the procedure.

Public Sub Decision_tree()
    Dim userInteraction As YesNoUserInteraction
    Set userInteraction = New YesNoUserInteraction
    DecisionTreeImpl userInteraction
End Sub

Private Sub DecisionTreeImpl(ByVal userInteraction As YesNoUserInteraction)
    ...
End Sub

This has the benefit that another class could implement YesNoUserInteraction and that could be provided as the user interaction to the implementation instead. This way, you can change the presentation entirely, e.g. using a custom user form, or automate the answers for unit testing.

The way it is, the intent to allow other implementations is not really clear though. We can go one step further and introduce a dedicated interface class IYesNoUserInteraction for the interaction.

Public Function UserAgrees(ByVal questionText As String) As Boolean
End Function

Then, we can turn YesNorUserInteraction into YesNoMsgBoxInteraction and implement the interface.

Implements IYesNoUserInteraction

Private Function IYesNoUserInteraction_UserAgrees(ByVal questionText As String) As Boolean
    UserAgrees = (MsgBox(questionText , vbYesNo) = vbYes)
End Function

To get the right names and signatures for the members to implement, you can use the two drop downs at the top of the code pane.

Finally, we can use it in the setup code.

Public Sub Decision_tree()
    Dim userInteraction As IYesNoUserInteraction
    Set userInteraction = New YesNoMsgBoxInteraction
    DecisionTreeImpl userInteraction
End Sub

Private Sub DecisionTreeImpl(ByVal userInteraction As IYesNoUserInteraction)
    ...
End Sub

This makes it clear that we can provide anything to the procedure that honors the contract to take a question and return an answer in the form of a Boolean with True = Yes.

Obviously, this does not deal with the first question to the user, since you want to allow cancellation. You could do that in a similar way as presented here, but not with a simple Boolean.

Separation of Responsibilities

The last observation above points to a further thing that could be improved in your code. Currently, it violates the so-called Single Responsibility Principle. In essence, it states that any unit of computation, e.g. a procedure, should be responsible for one and only one thing. Yours actually is for two: asking the user whether he wants to play at all and playing the game.

What you could do instead is to extract a function and a procedure. The function could ask whether the user wants to play and return True if he wants to play and False, otherwise. Then, the procedure actually playing the game is only called if the user wanted to play.

Since the procedure is not large, this does not seem to be much of a problem that it might change for entirely different reasons: changing how a user is asked and playing the game differently. However, if you ever want to write something more complicated, you should get into the habit of splitting things up into small units with a defined purpose. That makes reading the code and figuring out what it actually does later on much easier.


Faster palindrome checker in Java I wonder if there could be a faster palindrome checker in Java. The best one I have is O(n/2) which is O(n) eventually:

  public static boolean isPalindromeNaive(String s) {
        StringBuilder sb = new StringBuilder(s);
        StringBuilder sbReverse = sb.reverse();

        if (s.equals(String.valueOf(sbReverse))){
            return true;
        }
        return false;
    }

    public static boolean isPalindromLessNaive(String s) {
        if (s.length()%2==0) {
            for (int i = 0; i<(s.length()/2); i++) {
                if (s.charAt(i) != s.charAt(s.length() - 1 - i)) {
                    return false;
                }
            }
        }
        else {

            for (int i=0; i<(s.length()/2); i++) {
                if (s.charAt(i)!=s.charAt(s.length()-1-i)) {
                    return false;
                }

            }

        }

        return true;
    }

Also am I correct that my naive method is slower than less naive method? what is the time complexity of StringBuilder reverse() method?
// suggestion for code design/pattern(s) are really welcomed!

A palindrome checker has to have a worst case of \$O(n)\$ because you have to check each character to verify that it is a palindrome.

I would expect your first solution to be slower than your second solution, as it checks each character twice.  Unless your compiler's optimization is really brilliant, this should be slower.  But the proof's in the pudding.  If you really want to know, test it on a million strings and see.  Analysis is great but testing is better.

        if (s.length()%2==0) {
            for (int i = 0; i<(s.length()/2); i++) {
                if (s.charAt(i) != s.charAt(s.length() - 1 - i)) {
                    return false;
                }
            }
        }
        else {

            for (int i=0; i<(s.length()/2); i++) {
                if (s.charAt(i)!=s.charAt(s.length()-1-i)) {
                    return false;
                }

            }

        }

You notice anything about the two loops?  They're identical.  You can just do one loop with no outer if/else:

        for (int i = 0, j = s.length() - 1; i < j; i++, j--) {
            if (s.charAt(i) != s.charAt(j)) {
                return false;
            }
        }

This works because you start from the ends and work your way to the middle.  It will also work if you say i <= j (which checks the middle character if there is one), but that's not necessary.  The middle character will always equal itself.  You don't need to check it.

The thing that I don't like about s.length()-1-i is that it puts the burden on the compiler to realize that s.length() - 1 is constant relative to the loop.  Also, it's conceivable that decrement is faster than subtraction in some implementations.  Doing it this way means that we know that s.length() - 1 would only be calculated once.  The other way we're relying on the compiler to optimize it.

I don't know how the StringBuilder reverse is implemented, but it's possible to implement it without changing the underlying data at all.  Just index in the opposite direction.  Even if they do copy it to a new structure, that can be done in \$O(n)\$ time.  The comparison is also \$O(n)\$, so asymptotically that doesn't matter.


Async querying SQL in IIS let reply =  ref ""

let doAuto  =
    async {
        let! resultAA =
            async { return SqlCmd.aaHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultBB =
            async { return SqlCmd.bbHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultCC =
            async { return SqlCmd.ccHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultDD =
            async { return SqlCmd.ddHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultEE =
            async { return SqlCmd.ddHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultFF =
            async { return SqlCmd.ffHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultGG =
            async { return SqlCmd.ggHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultHH =
            async { return SqlCmd.hhHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultII =
            async { return SqlCmd.iiHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultJJ =
            async { return SqlCmd.jjHits query |> Async.RunSynchronously |> List.ofSeq }
        let! resultKK =
            async { return SqlCmd.kkHits query |> Async.RunSynchronously |> List.ofSeq }

        let length1 = resultAA.Length + resultBB.Length

        if length1 > 9 then reply := "hey " + length1.ToString()
        else
            let x = resultCC.Length + resultDD.Length + resultEE.Length + resultFF.Length + resultGG.Length
                        + resultHH.Length + resultII.Length + resultJJ.Length + resultKK.Length + length1
            reply := x.ToString()

    } |> Async.StartImmediate

!reply

The typical SqlCmd.xxHits (in module SqlCmd) built using FSharp.Data.SqlCommandTypeProvider looks like this:

[<Literal>]
let sqlXX = "SELECT ...@startsWith..."

type QueryXX = SqlCommand<sqlXX, ConnectionStringName=...>

let xxHits query =
    let cmdXxHits = QueryXX(connectionString = connectionString, startsWith = query)
    cmdXxHits.AsyncExecute()

I'm pretty inexperienced at using async. Looking for feedback, especially in a couple of areas:

I couldn't think of a better way to get my results out from inside the outer async than the ref. From what I read it sounded like Async.StartImmediate was the best solution for something running in an IIS application.
From the results of the first 2 inner asyncs, I may choose to quit before waiting for any more completions. Should I really be cancelling the remaining asyncs? Does it matter?

Avoid Async.RunSynchronously until absolutely necessary. It is very inefficient by design. Looks like you need:

async { let! xs = SqlCmd.xxxHits query
        return List.ofSeq xs }

Which you should probably abstract into a function.

Once you remove the blocking (RunSynchronously), the ref use also becomes invalid - you are querying the ref potentially before the async changes it - race condition. Try returning it (return) in the main async instead.

If you really want the subqueries to execute in parallel, consider a helper combinator like Async.Parallel rather than starting multiple sub-computations manually.

Your function should return Async<string> instead of string. If the caller of your code wants a string and cannot possibly use Async, they'll call Async.RunSynchronously - make it their problem.

Concerning choosing to quit based on the result of first two asyncs, not sure what you need exactly. Cancellation is an optimization - it may be tricky to get right but may make the code better performing. It's your decision based on the problem domain whether it is worth it or not.


Test connection to database C# I've created a small method to test the connection to a database:

public bool TestConnection(string provider, string serverName, string initialCatalog, string userId, string password, bool integratedSecurity)
{
    var canConnect = false;

    var connectionString = integratedSecurity ? string.Format("Provider={0};Data Source={1};Initial Catalog={2};Integrated Security=SSPI;", provider, serverName, initialCatalog)
                                              : string.Format("Provider={0};Data Source={1};Initial Catalog={2};User ID={3};Password={4};", provider, serverName, initialCatalog, userId, password);

    var connection = new OleDbConnection(connectionString);

    try
    {
        using (connection)
        {
            connection.Open();

            canConnect = true;
        }
    }
    catch (Exception exception)
    {

    }
    finally
    {
        connection.Close();
    }

    return canConnect;
}

Despite the method works it doesn t seems right to me. Is any way to test the connection without having to catch the exception?
Is it possible to achieve the same result in a different way?

It's a little verbose.

I would write

try {
    using(var connection = new OleDbConnection(...)) {
        connection.Open();
        return true;
    }
} catch {
    return false;
}

You should also use OleDbConnectionStringBuilder to properly escape the variables in the connection string.


Get reputation Badge from given score I have written code that gets the reputation badge from given score like this:

I use an if-else statement here:

func GetReputationBadge2(score int) string {

    badge := "badges-off.jpg"
    if score >= 50001 { // Diamond
        badge = "diamond-1.gif"
        if score >= 500001 {
            badge = "diamond-5.gif"
        } else if score >= 200001 {
            badge = "diamond-4.gif"
        } else if score >= 150001 {
            badge = "diamond-3.gif"
        } else if score >= 100001 {
            badge = "diamond-2.gif"
        }
    } else if score >= 4501 { // Gold
        badge = "gold-1.gif"
        if score >= 45001 {
            badge = "gold-5.gif"
        } else if score >= 30001 {
            badge = "gold-4.gif"
        } else if score >= 15001 {
            badge = "gold-3.gif"
        } else if score >= 10001 {
            badge = "gold-4.gif"
        }
    }

    return badge
}

I was kinda thinking that I could make it better using a switch-case statement:

func GetReputationBadge(score int) string {

    badge := "badges-off.jpg"
    caseSwitch := true
    switch caseSwitch {
    case score >= 50001:
        badge = "diamond-1.gif"

        switch caseSwitch {
        case score >= 500001:
            badge = "diamond-5.gif"
        case score >= 200001:
            badge = "diamond-4.gif"
        case score >= 150001:
            badge = "diamond-3.gif"
        case score >= 100001:
            badge = "diamond-2.gif"

        }

    case score >= 4501:
        badge = "gold-1.gif"

        switch caseSwitch {
        case score >= 45001:
            badge = "gold-5.gif"
        case score >= 30001:
            badge = "gold-4.gif"
        case score >= 15001:
            badge = "gold-3.gif"
        case score >= 10001:
            badge = "gold-2.gif"

        }

    default:
        return badge
    }

    return badge
}

Notice that I'm comparing boolean in the switch-case statement. Which are better for performance: using if-else or switch-case statement in this case?

For just a few items, the difference is small. If you have many items you should definitely use a switch.

This question might help: https://stackoverflow.com/questions/29566229/go-switch-string-efficiency


Buffon's Needle experiment I was working on a program that simulates the Buffon's Needle experiment. If anyone is not familiar with it, a Wiki link is provided -  Buffons's Needle

The point of Buffon's experiment is to find the value of pi, or at least approximate it. Usually you draw a couple of lines and then you scatter pins around the board in which you drew the lines in, the more pins the better. Then you take the number of pins crossing the lines and divide that by the number of pins you first had and multiply that by 4.

Below, I have provided the code for my program which is written in Java. Right now, it seems as if there are too many variables/methods. Could someone help refine it so it functions and looks better?

import java.util.Random;
import java.util.Scanner;
public class buffonNeedle {
    public static void main(String[] args) {
        Scanner in = new Scanner (System.in);
        System.out.println("How many darts do you want in each trial?");
        int darts = in.nextInt();
        System.out.println("How many trials do you want conducted?");
        int trials = in.nextInt();
        for(int i = 0; i<trials; i++) {
            xVal(darts);
            yVal(darts);
            hits(darts);
            System.out.println(piCalc(darts));
        }
    }
    public static double [] xVal (int darts){
        double [] xVal = new double[darts];
        Random random = new Random();
        for (int i = 0; i<darts; i++){
            xVal[i] = -1+random.nextDouble()*2;
        }
        return xVal;
    }
    public static double [] yVal (int darts){
        double [] yVal = new double[darts];
        Random random = new Random();
        for (int i = 0; i<darts; i++){
            yVal[i] = -1+random.nextDouble()*2;
        }
        return yVal;
    }
    public static int hits (int darts){
        int hits = 0;
        double [] xVal = xVal(darts);
        double [] yVal = yVal(darts);
        for(int i = 0; i<darts; i++){
            if((Math.pow(xVal[i],2)+Math.pow(yVal[i],2))<=1){
                hits++;
            }
        }
        return hits;
    }
    public static double piCalc(int darts){
        double hits = hits(darts);
        double pi = (hits/darts)* 4;
        return pi;
    }
}

In this loop…

    for(int i = 0; i<trials; i++) {
        xVal(darts);
        yVal(darts);
        hits(darts);
        System.out.println(piCalc(darts));
    }

You are throwing away the values returned from xVal(darts), yVal(darts), and hits(darts), making these calls completely superfluous.  All they are doing is wasting entropy from the random number generator.  The only thing that matters is piCalc(darts) (which calls hits(), which in turn calls xVal() and yVal()).

The code for xVal() is exactly identical to yVal().  You don't need to write the same function twice.


Improving the speed of loop in javascript for objects So I have a loop that works great, but doing some testing I notice it is a bit slow. I get an average time within the loop of about 0.11 seconds. Include that with some arrays containing a length of over 100 and the time starts making a big difference. So I mainly want to improve the speed of the loop without compromising the functionality it already has (which is taking a JSON object/array and turning it into an html format). Below is the code along with the function that is called within the loop.

let replacements = '';
let ind = 0;
for (let w in obj[key]) {
    if (Array.isArray(obj[key])) {
        if (Array.isArray(obj[key][w])) {
            if (obj[key].length > 10) {
                replacements += "Array(" + obj[key].length + ")"
                break
            }
            replacements += ((ind == 0) ? "Array(" : ", Array(") + obj[key][w].length + ")";
        } else {
            if (typeof obj[key][w] === 'object') {
                replacements += ((ind == 0) ? "" : ", ") + "{...}";
            } else {
                replacements += ((ind == 0) ? "" : ", ") + ObjectString(obj[key][w], "preview");
            }
        }
    } else {
        if (Array.isArray(obj[key][w]) || typeof obj[key][w] === 'object') {
            replacements += ((ind == 0) ? "" : ", ") + w + ": " + ((Array.isArray(obj[key][w])) ? "[...]" : "{...}");
        } else {
            replacements += ((ind == 0) ? "" : ", ") + w + ": ";
            replacements += ObjectString(obj[key][w], "preview");
        }
    }
    ind++;
}

function ObjectString(obj, type) { //~0.001-0.003 seconds
    let objString = obj;
    if (typeof objString == "string" && !objString.match(/<.*?>/g))
        objString = "\"" + objString + "\"";
    else if (/<.*?>/g.test(objString))
        objString = ((type == "normal") ? "
" + process(objString).replace(/</g, '&lt;').replace(/>/g, '&gt;') + "
" + objString : "\"...\"");
    else if (typeof objString == "number" || typeof objString == "boolean")
        objString = "" + objString + "";

    return objString
}

function process(str) {//~0.001 seconds

    var div = document.createElement('div');
    div.innerHTML = str.trim();

    return format(div, 0).innerHTML;
}

Hopefully this can be optimized to increase its speed, but if it can't then I would at least like some help to clean it up as I am sure this isn't the most effective way to do it. Thanks!

Edit: added the process function that was previous missing. Also for clarification I am using regular JSON arrays and objects that usually exceed 100 objects in the array. If you want an example of what that data could look like https://en.wikipedia.org/wiki/GeoJSON#Example has a good structure of some of the data I am parsing (containing both objects and arrays).

Continuous repetitive string concatenation is bad for performance because each such operation requires re-hashing of the string due to String interning.
Array enumeration using for-in loop is slower than for-of or a standard for loop.
Things like obj[key][subkey] may be slow in a long loop so cache them in a variable.
Do the proper performance measurements using Devtools Timeline profiler to find bottlenecks.

Here's an arguably more readable and hopefully faster example:

const parts = [];
const group = obj[key];

if (Array.isArray(group)) {
  for (const val of group) {
    if (parts.length) parts.push(', ');
    if (Array.isArray(val)) {
      const tooLong = group.length > 10;
      const len = (tooLong ? group : val).length;
      parts.push(`<font color="green">Array(${len})</font>`);
      if (tooLong) break;
    } else {
      parts.push(objectPreview(val));
    }
  }
} else {
  for (const [key, val] of Object.entries(group)) {
    parts.push(`${parts.length ? ', ' : ''}${key}: ${
      Array.isArray(val) ? '[...]' : objectPreview(val)
    }`);
  }
}
const replacements = parts.join('');

function objectPreview(obj) {
  let str = obj;
  switch (typeof obj) {
    case 'object':
      str = '{...}';
      break;
    case 'string':
      if (/<.*?>/.test(obj)) {
        str = type === 'normal' ?
          `<pre>${process(obj).replace(/</g, '&lt;').replace(/>/g, '&gt;')}</pre>${obj}` :
          '"..."';
      } else {
        str = `<font color="red">"${obj}"</font>`;
      }
      break;
    case 'number':
    case 'boolean':
      str = `<font color="#947cf6">${obj}</font>`;
      break;
  }
  return str;
}


Compile a string into a callable object in C++: a simple JIT based on system, dlopen and dlsym Today I discovered that it is possible to dynamically load code at runtime via dlopen and dlsym (and similar utilities on Windows as well). So the obvious next step was to call the compiler through system to build a piece of code into a shared object, which is then loaded to obtain a function pointer. All wrapped in a template class which becomes callable once "compiled".
The code is pretty much at an embryo state (less than 100 lines including the example), but it works and shows the idea, therefore I would love to hear your opinion.
This thing looks sooo simple and yet sooo powerful that I would really be surprised to be the first one having thought about it.
// compile with something like: g++ -std=c++20 jit.cpp -ldl

#include <dlfcn.h>
#include <cstdlib>
#include <iostream>
#include <filesystem>

template< class >
class JitFunc;

template< class R, class... Args >
class JitFunc<R(Args...)> {
  public:
  using Func = R(Args...);
  using result_type = R;

  private:
  std::string soName;

  void * dlso = nullptr;
  Func * func = nullptr;

  public:
  // remember to call compile() anytime you modify these strings
  std::string name;                 // the function name, which should also be found in the source
  std::string source;               // the source code, remember to use extern "C"
  std::string cxx = "/usr/bin/g++"; // the compiler executable
  std::string soPath = "/tmp";      // where to store the shared object (ideally a ramdisk)
  std::string flags;                // custom compiler flags which you may need

  JitFunc(std::string funcName):
    name(std::move(funcName))
  {}

  ~JitFunc() {
    close();
    removeSo();
  }

  result_type operator()(Args && ... args) const {
    return func(std::forward<Args>(args)...);
  }

  void compile() {
    close();
    removeSo();

    soName = soPath + "/jit_" + name + ".so";
    std::string cmd = cxx;
    auto push = [&cmd](std::string s) { cmd += " "; cmd += std::move(s); };
    push(flags);
    push("-shared -fPIC -o");
    push(soName);
    push("-xc++ - << EOF\n");
    push(source);
    push("\nEOF\n");

    std::system(cmd.c_str());

    dlso = dlopen(soName.c_str(), RTLD_NOW);
    if (!dlso) throw std::runtime_error(dlerror());
    func = (Func*) dlsym(dlso, name.c_str());
    if (!func) throw std::runtime_error(dlerror());
  }

  void close() {
    if (dlso) {
      dlclose(dlso);
      dlso = nullptr;
      func = nullptr;
    }
  }

  void removeSo() {
    if (!soName.empty()) {
      std::filesystem::remove(soName);
      soName.clear();
    }
  }

};

int main() {
  JitFunc<int(int)> f("timestwo");
  f.source =
R"CODE(
#include <iostream>
extern "C" {
int timestwo(int a) { std::cout << "hello from timestwo" << std::endl; return 2*a; }
}
)CODE"; // using a string literal to avoid having to escape all the double quotes

  f.compile();
  std::cout << f(2) << std::endl;

  return 0;
}

Make better use of constructors
Make your constructor take the program source as well as the function name as arguments, and have it compile the code at construction time. This avoids having to manually call compile(), which avoids errors such as forgetting to call that function or calling it twice.
The destructor already calls close() and removeSo(). You should make the latter two functions private, so one cannot accidentily call them manually.
Avoid unnecessary member variables
You should only declare member variables to hold information that you need for a longer period of time. Variables like source, cxx and so on are only used during the compilation step, they are not needed for anything else. Just remove them, and pass them as parameters to compile(), or better to the constructor, as mentioned above.
Avoid using <<EOF with user-provided content
There's a potential issue: what if I do the following?
f.source = R"CODE(
extern "C" {
int timestwo(int a) {
    return 2 * a;
}
}
EOF
sudo rm -rf /
)CODE";

It's better to write the source code to a temporary file, and point the compiler to that temporary file. This way, you won't accidentily execute shell code.
Use cases for JIT compilation
This looks very powerful, but what use cases do you have in mind for it? The example you gave is a bad use case; you could just have written timestwo() directly in the program. The main use case I can think of is to provide some way to evalute expressions provided by a user at runtime. In that case, you really want to sanitize the input. Because even if you avoid the shell exploits mentioned above, what happens if you do:
f.source = R"CODE(
extern "C" {
int timestwo(int a) {
    system("sudo rm -rf /");
    return 2 * a;
}
}
)CODE";

If the code you compile is in any way provided by the user, you have to be extremely careful to avoid potential misuse. So I would only use this if I am absolutely sure I need this functionality and if I can make sure it cannot be abused.


Make a perfect Split based on absolute difference between max of two arrays I was solving this problem in Python3 on the Codechef website.  Here I've to split an array such that the absolute difference between the maximum of both the arrays is maximum.

For example, if the input array is [1, 3, -3] then it can be split in two ways:

Way-1:

[1] and [3,-3] --> Absolute difference between max of arrays is 3-1 = 2

Way-2:

[1,3] and [-3] --> Absolute difference between max of arrays is 3 - (-3) = 6

So the max difference is 6, which is the answer.

Below is my code:

def max_difference(a):
    max_diff = 0
    for i in range(len(a) -1):
        local_diff = abs(max(a[:i+1]) - max(a[i+1:]))
        if max_diff < local_diff:
            max_diff = local_diff
    return max_diff

if __name__ == '__main__':
    arr_count = int(input())
    arr = list(map(int, input().rstrip().split()))
    res = max_difference(arr)
    print(res)

Can you please tell me if this code fine?  What improvements can be done?

local_diff = abs(max(a[:i+1]) - max(a[i+1:]))

For every i the entire aray is walked through to find left and right maxima.
This is complexity O(N²).

One could have two arrays with left_maxima and right_maxima, O(N), so

local_diff = abs(left_maxima[i] - right_maxima[i])

Then the entire complexity is O(N).

The maxima can be filled with a loop over i, either increasing or decreasing, using:

left_maxima[i] = max(left_maxima[i-1], a[i])         // ++i
right_maxima[i] = max(right_maxima[i+1], a[i])       // --i

It is even so, that one array (left or right) is not needed in the final local_diff loop.

What makes this problem so devious is that at index i an evaluation has to happen from past < i and from the "future" > i.


Max depth of tree when all parent pointers are provided Request for optimization, good practices, recommendations.

class NodeMaxDepth {

    private final NodeMaxDepth parent;

    public NodeMaxDepth (NodeMaxDepth parent) {
        this.parent = parent;
    }

    public NodeMaxDepth getParent() {
        return parent;
    }
}

    /**
     * Given a list of nodes with parent pointers,
     * calculate the max depth of tree from the root.
     */
    public final class MaxDepth {

        private MaxDepth ( ) { }

        public static int maxDepth (List<NodeMaxDepth> nodes) {
            if (nodes.size() == 0) { throw new IllegalArgumentException("List of nodes is empty."); }

            final Map<NodeMaxDepth, List<NodeMaxDepth>> parentChild = new HashMap<NodeMaxDepth, List<NodeMaxDepth>>();
            NodeMaxDepth parent = null;

            for (NodeMaxDepth node : nodes) {
                if (node.getParent() == null) parent = node;

                if (!parentChild.containsKey(node.getParent())) {
                    List<NodeMaxDepth> count = new ArrayList<NodeMaxDepth>();
                    parentChild.put(node.getParent(), count);
                }
                parentChild.get(node.getParent()).add(node);
            }

            if (parent == null) {
                throw new IllegalArgumentException("The graph should be acyclic.");
            }

            return height (parentChild, parent);
        }

        private static int height (Map<NodeMaxDepth, List<NodeMaxDepth>> parentChild, NodeMaxDepth parent) {
            assert parentChild != null;
            assert parent != null;

            final List<NodeMaxDepth> childList = parentChild.get(parent);

            if (childList == null) return 0;

            int maxCount = 0;
            for (NodeMaxDepth child : childList) {
                int height = height(parentChild, child);
                if (height > maxCount) {
                    maxCount = height;
                }
            }
            return maxCount + 1;
        }

        public static void main(String[] args) {
            NodeMaxDepth root = new NodeMaxDepth(null);
            NodeMaxDepth nodeLeft = new NodeMaxDepth(root);

            NodeMaxDepth nodeRight = new NodeMaxDepth(root);
            NodeMaxDepth nodeRightRight = new NodeMaxDepth(nodeRight);
            NodeMaxDepth nodeRightRightRight = new NodeMaxDepth(nodeRightRight);

            List<NodeMaxDepth> nodes = new ArrayList<NodeMaxDepth>();
            nodes.add(root);
            nodes.add(nodeLeft);
            nodes.add(nodeRight);
            nodes.add(nodeRightRight);
            nodes.add(nodeRightRightRight);

            System.out.println("Expected 3, Actual: " + maxDepth (nodes));
        }
    }

You shouldn't name your classes which form the data structure you operate on according to the problem you are trying to solve. The classes should be named according to their function in the data structure. Thus NodeMaxDepth should be simply Node. This also removes visual clutter when reading the code as the name is shorter yet conveys the same meaning.
I'm not 100% sure why you build the list of children for each node in a separate data structure - it seems that this should be part of the NodeMaxDepth class. It would also make for a slightly cleaner implementation of building of the children lists as each node only has to add itself to the children collection of its parent.

Something along these lines:

class Node {

    private final Node parent;
    private List<Node> children;

    public Node(Node parent) {
        this.parent = parent;
        children = new List<Node>();
    }

    public Node getParent() {
        return parent;
    }

    public void addChild(Node child) {
        children.add(child);
    }
}

public static int maxDepth(List<Node> nodes) {
    if (nodes.size() == 0) { throw new IllegalArgumentException("List of nodes is empty."); }

    Node root = null;

    for (Node node : nodes) {
        if (node.getParent() == null) {
            if (root != null) {
                throw new IllegalArgumentException("Tree must not have multiple roots");
            }
            root = node;
        }
        else {
            node.getParent().addChild(node);
        }
    }

    if (root == null) {
        throw new IllegalArgumentException("This does not seem to be a tree");
    }

    return height (root);
}

If you want to optimize for memory usage and code size you could resort to a simpler algorithm by simply walking up the tree from each node to the root and remember the longest path. Worst case this would result in O(n^2) runtime if I'm not mistaken but would not require any additional memory for building the children lists. The code would also be much simpler.

As indicated by @200_success you could sacrifice a little bit more memory and find all leave nodes and just walk up from them:

// assume all nodes are leaves
HashSet<Node> leaves = new HashSet<Node>(nodes);

Node root = null;

// remove all parents as they are not leaves
for (Node node : nodes) {
    if (node.getParent() == null) {
        root = node;
    }
    else {
        leaves.remove(node.getParent());
    }
}

if (root == null) {
    throw new IllegalArgumentException("The tree must have a root");
}

int maxHeight = 0;

for (Node leaf : leaves) {
    int height = 1;
    Node current = leaf;
    while (current.getParent() != null) {
        current = current.getParent();
        height += 1;
    }
    if (maxHeight < height) {
        maxHeight = height;
    }
}

return maxHeight;

You don't really check for cycles - just that your tree has a root (you could have a normal tree and a disconnected cycle in the list).


Determine if a point is inside or ouside of an irregular shape on a bitmap So this is my task:

You are given an irregular shape on a bitmap. How do you determine if
a point is inside or ousdie of the shape?

We are allowed to assume that helper functions are already available for us and we don't need to write them as well.
So I decided that I can use a function that returns me the status of a given bit on that bitmap.
int IsBitIsOn(bitmap_ty *bitmap, size_t x, size_t y);

We also can assume that the given point won't be a point on the outline of the shape. Which means the given X,Y coordinates must be either inside or outside the shape and not on one of the outlines.
And here is my full code of the task,
I wrote it as a bitmap that every ON(==1) bit tells me that I'm on an outline, while 0 bit tells me i'm either inside or outside the shape.
int IsInTheShape(bitmap_ty *bitmap, size_t x, size_t y)
{
    int IsIn = 0; /* flag of whether the given point is inside the shape    */

    int ComeFromAbove = 0;

    int start_sequal = 1;

    size_t curr_row = y;
    size_t curr_column = 0;

    while (x != curr_column)
    {
        if (IsBitIsOn(bitmap, curr_column, curr_row))
        {
            IsIn = !IsIn;

            /* starts a sequal of bits that are ON  */
            while (IsBitIsOn(bitmap, curr_column + 1, curr_row))
            {
                /* checks the source direction of the first bit in the sequal */
                if (start_sequal &&
                    IsBitOn(bitmap, curr_column, curr_row - 1) ||
                    isBitOn(bitmap, curr_column - 1, curr_row - 1) ||
                    isBitOn(bitmap, curr_column + 1, curr_row - 1))
                {
                    ComeFromAbove = 1;
                    start_sequal = 0;
                }
                ++curr_column;
            }
            /*  checks the source direction of the last bit in the sequal,
                so if it cuts the direction of the first bit, it means
                the next points are out of the shape    */
            if (ComeFromAbove)
            {
                if (!IsBitOn(bitmap, curr_column, curr_row + 1) &&
                    !isBitOn(bitmap, curr_column - 1, curr_row + 1) &&
                    !isBitOn(bitmap, curr_column + 1, curr_row + 1))
                {
                    ComeFromAbove = 0;
                    IsIn = !IsIn;
                }
            }
        }
        ++curr_column;
    }

    return (IsIn);
}

I draw the following shapes find the problematic points and shapes:

If shape on bitmap is not filled and only one shape present on bitmap:
Start at point (x,y). For each of the four directions (left, right, top, bottom), keep going until either IsBitIsOn is true or a border of the bitmap has been reached. In the former case, point is in shape. Else (if border of bitmap reached), point is not in shape.
Pseudocode:
def IsInTheShape(x, y):
  curr_x = x
  curr_y = y
  # Check left
  while curr_x >= -1:
    if (curr_x == -1):
      return false
    if IsBitIsOn(curr_x, y):
      break

  curr_x = x
  curr_y = y
  # Check right
  while curr_x <= bitmap.width + 1:
    if (curr_x == bitmap.width + 1):
      return false
    if IsBitIsOn(curr_x, y):
      break

  # Check top
  # Check bottom

  # All directions hit IsBitIsOn, so must be in shape
  return true

If shape on bitmap is not filled and one or multiple shapes are present on bitmap:
Same as above, but once an outline has been reached, check all connected outline pieces recursively until the starting point has been reached. If the traversal around the outline visits all of the other outlines from the other three directions, then point is inside shape.
If shape on bitmap is filled:
Suppose that we want to check if (x,y) is in shape. Let (x,y) be cell E in the following grid:
+---+---+---+
| A | B | C |
+---+---+---+
| D | E | F |
+---+---+---+
| G | H | I |
+---+---+---+

Case 1 (point trivially not in shape):

Point is not in shape if E is off: if not IsBitIsOn(E): return false

Case 2 (point not in shape):

Point is not in shape if any of the 8 surrounding bits around E is off:

if not (IsBitIsOn(A) and IsBitIsOn(B) and IsBitIsOn(C) and IsBitIsOn(D) and IsBitIsOn(F) and IsBitIsOn(G) and IsBitIsOn(H) and IsBitIsOn(I)): return false

Case 3 (point in shape):

Point is all of the 8 surrounding bits around E is on:

else: return true

All together, the pseudocode is:
if not IsBitIsOn(E):
  return false
else if not (IsBitIsOn(A) and IsBitIsOn(B) and IsBitIsOn(C) and IsBitIsOn(D) and IsBitIsOn(F) and IsBitIsOn(G) and IsBitIsOn(H) and IsBitIsOn(I)):
  return false
else:
  return true


Scale object based on distance between 2 vertices I made this script so that I can scale objects for 3D printing from Maya.
I had fun making it and I'm sure it could be cleaned up or improved. If it can be cleaned up please let me know.

##  ((Ax - Bx)**2 + (Ay - By)**2 + (Az - Bz)**2)**0.5

import maya.cmds as mc

def defineParameters():
    selPoints = mc.ls(sl = True)
    objName = selPoints[0].split('.')[0]
    print objName
    vtxCoordList = mc.xform(selPoints, q = True, translation = True, ws = True)
    Ax, Ay, Az = vtxCoordList[:-3]
    Bx, By, Bz = vtxCoordList[3:]
    d = float(((Ax - Bx)**2 + (Ay - By)**2 + (Az - Bz)**2)**0.5) #direct distance between both points
    return d

def getDist():
    d = defineParameters()
    #selPoints = mc.ls(sl = True)

    #vtxCoordList = mc.xform(selPoints, q = True, translation = True, ws = True)
    #Ax, Ay, Az = vtxCoordList[:-3]
    #Bx, By, Bz = vtxCoordList[3:]
    #d = float(((Ax - Bx)**2 + (Ay - By)**2 + (Az - Bz)**2)**0.5) #direct distance between both points
    '''
    dx = ((Ax - Bx)**2)**0.5
    print "Distance on X axis is: {0}".format(dx) #distance on X axis
    dy = ((Ay - By)**2)**0.5
    print "Distance on Y axis is: {0}".format(dy) #distance on Y axis
    dz = ((Az - Bz)**2)**0.5
    print "Distance on Z axis is: {0}".format(dz) #distance on Z axis
    '''
    mc.textFieldButtonGrp("currentLength", edit = True, tx = "{0}".format(d))
    print "the distance between points is {0}".format(d)
    return d

def refreshScaleFactor(sf):
    userInput = sf
    scaleFactor = (float(sf))/(float(d))
    print "dist btwn pnts is d: {0} ".format(d)
    print "sf is {0}".format(sf)
    print "user input is {0}".format(userInput)
    print "scale factor is {0}".format(scaleFactor)
    print "-"*10
    return float(scaleFactor)

## scale object to match measurement
def scaleDist():
    b = (float(mc.textFieldButtonGrp("desiredLength",q = True, tx = True))) / defineParameters()
    mc.makeIdentity(objName, apply = True, t = 1, r = 1, s = 1, n = 0, pn = 1)#freeze transformations
    mc.DeleteAllHistory()
    mc.scale(float(b), float(b), float(b), objName)
    print "you scaled by {0}".format(b)
    mc.makeIdentity(objName, apply = True, t = 1, r = 1, s = 1, n = 0, pn = 1)#freeze transformations

defineParameters()

## window
mc.window("Distance Tool", t = "Distance Tool")
mc.rowColumnLayout(numberOfColumns = 2, columnAttach = (1, 'left', 0), columnWidth = [(1,100), (2,300)])
mc.text(l = "Current length")
currentLength = mc.textFieldButtonGrp("currentLength", ed = False, tx = "{0}".format(d), buttonLabel = "Refresh", bc = getDist)

mc.text(l = "Desired length")
desiredLength = mc.textFieldButtonGrp("desiredLength", buttonLabel = "Scale",  bc = scaleDist, tcc = refreshScaleFactor )
mc.showWindow()

I’ll start with some general comments on Python style:

Line should be no more than 79 characters long. (PEP 8: Maximum Line Length)
When you’re specifying a list of keyword arguments to a function, there shouldn’t be a space around the equals sign. (PEP 8: Other Recommendations)

So for example, line 9 becomes:

vtxCoordList = mc.xform(selPoints, q=True, translation=True, ws=True)

When you have lots of keyword arguments, you can put them on newlines (aligned with the opening parens) to make them easier to read. For example, line 63 becomes:

currentLength = mc.textFieldButtonGrp("currentLength",
                                      ed=False,
                                      tx="{0}".format(d),
                                      buttonLabel="Refresh",
                                      bc=getDist)

Python variable naming convention for variables and functions is lowercase_with_underscores rather than dromedaryCase.
Functions are documented with docstrings, which are triple-quoted strings that are the first item within the function body. So in getDist(), you want to move the d= to after your docstring, and you should move the comment above scaleDist() to within the function as a docstring.
I don’t understand any of the calls to maya.cmds, because I’m familiar with neither Maya nor this library. If somebody who knows this library could easily follow these calls, and why you’re making them, then this is probably fine. If not, some extra comments might not go amiss.
I would recommend using slightly longer variable names, particularly for d in defineParameters() and b in scaleDist(). Characters are cheap.

And a few more specific comments:

defineParameters():

I think the name of this function is unclear. It actually returns a distance between two points in 3D space, not parameters. I think it might be better to return the two tuples A, B, and move the distance calculation elsewhere.
Rather than using (…)**0.5, you can use the sqrt() function from the math module. I believe it’s equivalent to what you’ve written, but it makes the float() unnecessary and your intent clearer:

import math

d = math.sqrt( (Ax - Bx)**2 + (Ay - By)**2 + (Az - Cz)**2 )

If you want to be really fancy, you can use numpy.linalg.norm:

import numpy

A = vtxCoordList[:-3]
B = vtxCoordList[3:]
d = numpy.linalg.norm(A - B)

but if you don’t already have numpy installed, then setting it up for this small task is probably overkill.

getDist():

This is the function where you should have your code for computing the distance.

refreshScaleFactor():

I would get rid of the userInput variable, and just use the sf variable directly.
You don’t need the float() around the return value; you can just return scaleFactor directly. As the quotient of two float values, this is automatically a float.

scaleDist():

I think this function is going to throw a NameError when you try to get the value of objName. This variable was only ever defined in the local scope of defineParameters(), but it disappears as soon as that function was done.

I see two possibilities:

Either set up objName as a global variable in defineParameters() (but you should try to avoid global variables where possible); or
Have defineParameters() return a dictionary of parameters: objName, A and B, and then access values as appropriate.

In the mc.scale(float(b), …) line, the float()s around b are unnecessary: again, it’s the quotient of two floats.
I don’t know Maya, so I don’t know what the variables in the two freeze transformation lines mean (t, r, s, …). If it’s obvious to somebody who knows Maya and/or the maya.cmds module, then perhaps you don’t need to explain it; but if not, perhaps a comment would be useful.

Finally, there’s some code that isn’t part of any function at the bottom of the script:

Rather than having this code in global scope, put it in a main() function and then have this at the end of the script:

if __name__ == '__main__':
    main()

This means you can run the file as a script, but you can also import it in another script without it kicking off the main code.


Complex scientific formula for nuclear magnetic resonance I've been reading "clean code" tutorials, and I definitely see the value of using good names that "document themselves" to make intent clear. However, what can be done about complex formulas where describing the meaning of a symbol is difficult?

My solution is to include a reference to the math source, and then use names that relate as closely as possible to the published formula. The code would be unintelligible to another programmer, however, if they didn't have access to the source.

For example, I have a Python program that includes this function factory. The returned function is used elsewhere to plot a graph. The code itself looks like a nightmare:

def dnmr_twosinglet(va, vb, ka, wa, wb, pa):
    """
    Accept parameters describing a system of two exchanging, uncoupled nuclei,
    and return a function that requires only frequency as an argurment,
    that will calculate the intensity of the DNMR lineshape at that frequency.

    :param va: The frequency of nucleus 'a' at the slow exchange limit. va > vb
    :param vb: The frequency of nucleus 'b' at the slow exchange limit. vb < va
    :param ka: The rate constant for state a--> state b
    :param wa: The width at half heigh of the signal for nucleus a (at the slow
    exchange limit).
    :param wb: The width at half heigh of the signal for nucleus b (at the slow
    exchange limit).
    :param pa: The fraction of the population in state a.
    :param pa: fraction of population in state a
    wa, wb: peak widths at half height (slow exchange), used to calculate T2s

    returns: a function that takes v (x coord or numpy linspace) as an argument
    and returns intensity (y).
    """
    """
    Formulas for simulating two uncoupled spin-1/2 nuclei are derived from:
    Sandstrom, J. "Dynamic NMR Spectroscopy". Academic Press, 1982, p. 15.
    """

    pi = np.pi
    pi_squared = pi ** 2
    T2a = 1 / (pi * wa)
    T2b = 1 / (pi * wb)
    pb = 1 - pa
    tau = pb / ka
    dv = va - vb
    Dv = (va + vb) / 2
    P = tau * (1 / (T2a * T2b) + pi_squared * (dv ** 2)) + (pa / T2a + pb / T2b)
    p = 1 + tau * ((pb / T2a) + (pa / T2b))
    Q = tau * (- pi * dv * (pa - pb))
    R = pi * dv * tau * ((1 / T2b) - (1 / T2a)) + pi * dv * (pa - pb)
    r = 2 * pi * (1 + tau * ((1 / T2a) + (1 / T2b)))

    def calculate_intensity(v):
        """
        :param v: frequency
        :return: function that calculates the intensity at v
        """
        _Dv = Dv - v
        _P = P - tau * 4 * pi_squared * (_Dv ** 2)
        _Q = Q + tau * 2 * pi * _Dv
        _R = R + _Dv * r
        return(_P * p + _Q * _R) / (_P ** 2 + _R ** 2)
    return calculate_intensity

However, with the referenced formulae (a subset of which are reproduced below), the meaning of the code should be clear:

$$
  \textrm{v} = -C_0\frac{\bigg\{P\bigg[1+\tau\Big(\dfrac{p_B}{T_{2A}}+\dfrac{p_B}{T_{2B}}\Big)\bigg]+Q R\bigg\}}{P^2+R^2}
$$
$$
P=\tau\bigg[\frac{1}{T_{2A} \cdot T_{2B}}-4\pi^2\Delta\nu^2+\pi^2(\delta\nu)^2\bigg]+\frac{p_A}{T_{2A}}+\frac{p_B}{T_{2B}}\\
Q=\tau[2\pi\Delta\nu-\pi\delta\nu(p_A-p_B)]\\
R=2\pi\Delta\nu\bigg[1+\tau\Big(\frac{1}{T_{2A}}+\frac{1}{T_{2B}}\Big)\bigg]+\pi\delta\nu\tau\Big(\frac{1}{T_{2B}}-\frac{1}{T_{2A}}\Big)
+\pi\delta\nu(p_A-p_B)
$$

etc.

Is this practice considered OK, or could this be improved upon?

I would consider the practice ok in this case.

Your variable try appear to be roughly the "ASCII" equivalent of the ones used in the equation.
You cite the paper where you got the equations from. (This is probably the most important thing to do) This allows the programmer to understand the context.

Remember readability is subjective, look at something like APL for example and you will see that APL strives for code most closely aligned to math. The creator of this language purposely designed the language to be effective at representing math operations (in particular, matrix manipulation) and it takes a vastly different approach from Python.

One interesting thought experimenet: If we grew up and learned that the word "print" was simply "p", "print" would likely be considered not readable and we would replace each case with "p". However, in our world, we consider the opposite to be true in most cases.

For me it is much easier to read dx instead of something like change_in_x because I have learned to appreciate the former.

The only thing I would consider changing is getting rid of pi_squared and just writting it out explicitly (unless there is some worthwhile performance cost in doing so).


Generic Constraints and new() I have have attempted to use generics and constraints to create functions that retrieve and save data from a number of similar objects. Each of these objects describe their own properties. When I use these methods for a particular Type I don't understand why I have to specify new() in the constraint and also create a new version of the type in order to access its underlying properties to assist me. I'm sure there is a better way, but my knowledge in this area is a little lacking.

There are a number of objects that inherit from RawDataAggregatedItem.

public class Project : RawDataAggregatedItem
{
    public override string IdColumnName
    {
        get { return "project_id"; }
    }

    public override string DescriptionColumnName
    {
        get { return "project_description"; }
    }

    public override string GetStoredProcedureName
    {
        get { return "R_projects_sp"; }
    }
}

...

public class Category : RawDataAggregatedItem
{
    public override string IdColumnName
    {
        get { return "asset_category"; }
    }

    public override string GetStoredProcedureName
    {
        get { return "R_asset_category_sp"; }
    }
}

RawDataAggregatedItem has it's own interface:

public interface IRawDataAggregatedItem
{
    string IdColumnName { get; }
    string DescriptionColumnName { get; }
}

public abstract class RawDataAggregatedItem : IRawDataAggregatedItem
{

    public abstract string IdColumnName { get; }
    public virtual string DescriptionColumnName
    {
        get { return string.Empty; }
    }
    public abstract string GetStoredProcedureName { get; }
}

I've created functions that get and save data to the database using generics expecting these different object types:

public RawDataAggregatedItems<T> GetRawDataAggregated<T>(int? id) where T : IRawDataAggregatedItem, new()
    {
        try
        {
            var itemType = new T();
            using (var cn = Connection())
            {
                var cmd = new SqlCommand(itemType.GetStoredProcedureName, cn) { CommandTimeout = _commandTimeout, CommandType = CommandType.StoredProcedure };
                cmd.Parameters.Add(new SqlParameter("@periodId", SqlDbType.Int)).Value = id;
                return new RawDataAggregatedItems<T>(cmd.ExecuteReader());
            }
        }
        catch (Exception ex)
        {
            throw new Exception("Error fetching aggregated rawdata list.", ex);
        }
    }

public void UpdateRawDataAggregated(RawDataAggregatedItem itemToUpdate)
    {
        try
        {
            using (var cn = Connection())
            {
                var cmd = new SqlCommand(itemToUpdate.UpdateStoredProcedureName, cn) { CommandTimeout = _commandTimeout, CommandType = CommandType.StoredProcedure };
                cmd.Parameters.Add(new SqlParameter("@projectid", SqlDbType.VarChar)).Value = ((IRawDataAggregatedItem)itemToUpdate).ID;
                cmd.Parameters.Add(new SqlParameter("@is_flagged", SqlDbType.Bit)).Value = ((IRawDataAggregatedItem)itemToUpdate).isFlaggedForReview;
                cmd.Parameters.Add(new SqlParameter("@is_reviewed", SqlDbType.Bit)).Value = ((IRawDataAggregatedItem)itemToUpdate).isReviewed;
                cmd.ExecuteReader();
            }
        }
        catch (Exception ex)
        {
            throw new Exception("Error updating aggregated rawdata item.", ex);
        }
    }

If feels a bit wrong to

supply new() in the GetRawDataAggregated constraint but the compiler is forcing me to.
New-ing up a Type T (var itemType = new T()) in order to get to T.Property.

Please could you suggest a better way of doing this?

Well GetStoredProcedureName is an instance property - you need an instance of the object to get the value of the property. The value belongs to an instance, not to the type. You either need to use static properties and lose the
generics or introduce a configuration source that maps type to a configuration object.

I'd sugges that the object has no need to know what its stored procedures are called so the latter suggestion seems to be the way to go.

Create a configuration class:

public class DataConfiguration<T>
{
    public string GetStoredProcedureName { get; set; }
}

public class DataConfigurationProvider
{
    public DataConfiguration<T> GetConfiguration<T>()
    {
        // some stuff.
    }
}

Then you can update your functions:

public RawDataAggregatedItems<T> GetRawDataAggregated<T>(int? id) where T : IRawDataAggregatedItem
{
    // _dataConfigurationProvider injected by constructor injection.
    var sprocName = _dataConfigurationProvider.GetConfiguration<T>().GetStoredProcedureName;
    try
    {
        using (var cn = Connection())
        {
            var cmd = new SqlCommand(sprocName, cn) { CommandTimeout = _commandTimeout, CommandType = CommandType.StoredProcedure };
            cmd.Parameters.Add(new SqlParameter("@periodId", SqlDbType.Int)).Value = id;
            return new RawDataAggregatedItems<T>(cmd.ExecuteReader());
        }
    }
    catch (Exception ex)
    {
        throw new Exception("Error fetching aggregated rawdata list.", ex);
    }
}

I've been a bit wooly on the specifics here but hopefully you can see yourself to a working solution!


k-means clustering algorithm implementation Here is my personal implementation of the clustering k-means algorithm.

from scipy.spatial import distance
import numpy as np
import random

# (x,y) coordinates of a point
X = 0
Y = 1

def get_first(k, points):
    return points[0:k]

def cost(cetroids, clusters):
    cost = 0

    for i in range(len(centroids)):
        centroid = centroids[i]
        cluster = clusters[i]
        for point in cluster:
            cost += (distance.euclidean(centroid, point))**2

    return cost

def compute_centroids(clusters):
    centroids = []

    for cluster in clusters:
        centroids.append(np.mean(cluster, axis=0))

    return centroids

def kmeans(k, centroids, points, method, iter):
    clusters = [[] for i in range(k)]

    for i in range(len(points)):
        point = points[i]
        belongs_to_cluster = closest_centroid(point, centroids)
        clusters[belongs_to_cluster].append(point)

    new_centroids = compute_centroids(clusters)

    if not equals(centroids, new_centroids):
        print "Iteration " + str(iter) + ". Cost [k=" + str(k) + ", " + method + "] = " + str(cost(new_centroids, clusters))

        clusters = kmeans(k, new_centroids, points, method, iter+1)

    return clusters

def closest_centroid(point, centroids):
    min_distance = float('inf')
    belongs_to_cluster = None
    for j in range(len(centroids)):
        centroid = centroids[j]
        dist = distance.euclidean(point, centroid)
        if dist < min_distance:
            min_distance = dist
            belongs_to_cluster = j

    return belongs_to_cluster

def contains(point1, points):
    for point2 in points:
        if point1[X] == point2[X] and point1[Y] == point2[Y]:
            return True

    return False

def equals(points1, points2):
    if len(points1) != len(points2):
        return False

    for i in range (len(points1)):
        point1 = points1[i]
        point2 = points2[i]
        if point1[X] != point2[X] or point1[Y] != point2[Y]:
            return False

    return True

if __name__ == "__main__":
    data = [[-19.0748,     -8.536       ],
            [ 22.0108,      -10.9737    ],
            [ 12.6597,      19.2601     ],
            [ 11.26884087,  19.90132146 ],
            [ 15.44640731,  21.13121676 ],
            [-20.03865146,  -8.820872829],
            [-19.65417726,  -8.211477352],
            [-15.97295894,  -9.648002534],
            [-18.74359696,  -5.383551586],
            [-19.453215,    -8.146120006],
            [-16.43074088,  -7.524968005],
            [-19.75512437,  -8.533215751],
            [-19.56237082,  -8.798668569],
            [-19.47135573,  -8.057217004],
            [-18.60946986,  -4.475888949],
            [-21.59368337,  -10.38712463],
            [-15.39158057,  -3.8336522  ],
            [-40.0,          40.0       ]]
    k = 4

    # k-means picking the first k points as centroids
    centroids = get_first(k, data)
    clusters = kmeans(k, centroids, data, "first", 1)

I understood and followed the theory of the algorithm, but as you can see, when running the code, the cost on each iteration of the algorithm increases. I am new to python and I think the problem relies in some misunderstanding from my side regarding list manipulation. Any ideas?

N.b. I'm pretty sure you can find optimised replacements of many of
these functions in either the NumPy or SciPy library.

Edit: I don't immediately see a reason for a slowdown except the
addition of new clusters.  A bigger sample set that shows the behaviour
would be nice, together with some timings and probably running a
profiler on it.

In general you should likely use a NumPy array or matrix instead of
lists of lists as they are optimised for storing numeric data(!).  That
would likely also eliminate the need for some of these functions or
considerably reduce their length.  Also store all data in the same
containers - that way you don't have to create new functions like
equals and contains yourself to compare between different
representations.

The X/Y definitions at the start are more of a WTF for me.  I'd
get rid of that by writing code that's either independent on the
number of dimensions, or just use 0/1 - IMO it's not magic enough
to warrant separate constants.
get_first isn't neccessary - the pattern is obvious enough to not
require encapsulation in a function.  The zero is also optional.
Typo in cost signature, should be centroids.
The pattern for i in range(len(...)): appears a couple of times and
should be replaced with proper iteration over some helper generator.
E.g. in cost the parallel iteration over both centroids and
clusters should be done by zip (itertools.izip in Python 2.7 if
used in the same way).  It can also be simplified by using another
SciPy function,
cdist.
If you still need the index, use enumerate.
You can also often get away with using the squared euclidean if the
exact value isn't relevant, just the relation between different
distances.
The pattern to initialise a list of empty lists should probably use
_ instead of i to make it obvious that the variable serves to
further purpose.
print should be used as a function to make it more uniform.  Also,
use one of the various formatting options (% or format) to get rid
of the additional str calls.
compute_centroids can be simplified with a list comprehension.

Looks like this now:

from scipy.spatial import distance
import numpy as np
import random
from itertools import izip

def cost(centroids, clusters):
    return sum(distance.cdist([centroid], cluster, 'sqeuclidean').sum()
            for centroid, cluster in izip(centroids, clusters))

def compute_centroids(clusters):
    return [np.mean(cluster, axis=0) for cluster in clusters]

def kmeans(k, centroids, points, method):
    clusters = [[] for _ in range(k)]

    for point in points:
        clusters[closest_centroid(point, centroids)].append(point)

    new_centroids = compute_centroids(clusters)

    if not equals(centroids, new_centroids):
        print("cost [k={}, {}] = {}".format(k, method, cost(new_centroids, clusters)))

        clusters = kmeans(k, new_centroids, points, method)

    return clusters

def closest_centroid(point, centroids):
    min_distance = float('inf')
    belongs_to_cluster = None
    for j, centroid in enumerate(centroids):
        dist = distance.sqeuclidean(point, centroid)
        if dist < min_distance:
            min_distance = dist
            belongs_to_cluster = j

    return belongs_to_cluster

def contains(point1, points):
    for point2 in points:
        if point1[0] == point2[0] and point1[1] == point2[1]:
        # if all(x == y for x, y in izip(points1, points2)):
            return True

    return False

def equals(points1, points2):
    if len(points1) != len(points2):
        return False

    for point1, point2 in izip(points1, points2):
        if point1[0] != point2[0] or point1[1] != point2[1]:
        # if any(x != y for x, y in izip(points1, points2)):
            return False

    return True

if __name__ == "__main__":
    data = [[-19.0748,     -8.536       ],
            [ 22.0108,      -10.9737    ],
            [ 12.6597,      19.2601     ],
            [ 11.26884087,  19.90132146 ],
            [ 15.44640731,  21.13121676 ],
            [-20.03865146,  -8.820872829],
            [-19.65417726,  -8.211477352],
            [-15.97295894,  -9.648002534],
            [-18.74359696,  -5.383551586],
            [-19.453215,   -8.146120006],
            [-16.43074088,  -7.524968005],
            [-19.75512437,  -8.533215751],
            [-19.56237082,  -8.798668569],
            [-19.47135573,  -8.057217004],
            [-18.60946986,  -4.475888949],
            [-21.59368337,  -10.38712463],
            [-15.39158057,  -3.8336522  ],
            [-40.0,          40.0       ]]
    k = 4

    # k-means picking the first k points as centroids
    centroids = data[:k]
    clusters = kmeans(k, centroids, data, "first")


Dynamic variable - is that best way to dynamically assign? Normally I could just declare the 'email' variable as the base class for both of EmailInbound & EmailOutbound, then cast-up to the inherited class.
However, I don't think when defining EF codefirst classes you can use inherited classes, only POCOs. I could be wrong. Hence I've used the dynamic keyword. Could I use Object() instead? Some direction appreciated here...

             if (source.ActionTypeId == (int)ActionTypes.EmailInbound  ||
                source.ActionTypeId == (int)ActionTypes.EmailOutbound)
            {
                dynamic email;
                if ( source.ActionTypeId == (int)ActionTypes.EmailInbound )
                {
                    email = source.EmailMessage.EmailInbound;
                }
                else
                {
                    email = source.EmailMessage.EmailOutbound;
                }

                return new ActionModel
                {
                    ActionTypeId = (ActionTypes)source.ActionTypeId,
                    Content = email.Body,
                    Created = email.DateSent,
                    GravatarUrl = HtmlHelperExtensions.getGravatarUrl(email.FromAddress, 25),
                    Subject = email.Subject,
                    User = Mapper.Map<User, UserModel>(source.EmailMessage.User),
                    EmailReplyTo = email.From,
                    ShowReplyButton = true,
                    ThreadUid = source.ThreadUid,
                    IsReply = source.IsReply
                };
            }
...

I think it is quite defendable to use dynamic here. I'm not too familiar with EF, but it could very well be that you can't make the objects type-compatible.

Even if you could, I would not do it if it was only to make this piece of code work.

Declaring email as object will not help in any way.

The only alternative I can think of is to have two code paths for both cases:

            if ( source.ActionTypeId == (int)ActionTypes.EmailInbound )
            {
                email = source.EmailMessage.EmailInbound;
                return new ActionModel
                {
                    ActionTypeId = (ActionTypes)source.ActionTypeId,
                    Content = email.Body,
                    Created = email.DateSent,
                    GravatarUrl = HtmlHelperExtensions.getGravatarUrl(email.FromAddress, 25),
                    Subject = email.Subject,
                    User = Mapper.Map<User, UserModel>(source.EmailMessage.User),
                    EmailReplyTo = email.From,
                    ShowReplyButton = true,
                    ThreadUid = source.ThreadUid,
                    IsReply = source.IsReply
                };
            }
            else
            {
                email = source.EmailMessage.EmailOutbound;
                return new ActionModel
                {
                    ActionTypeId = (ActionTypes)source.ActionTypeId,
                    Content = email.Body,
                    Created = email.DateSent,
                    GravatarUrl = HtmlHelperExtensions.getGravatarUrl(email.FromAddress, 25),
                    Subject = email.Subject,
                    User = Mapper.Map<User, UserModel>(source.EmailMessage.User),
                    EmailReplyTo = email.From,
                    ShowReplyButton = true,
                    ThreadUid = source.ThreadUid,
                    IsReply = source.IsReply
                };
            }

But I think the solution with dynamic is better. You can't catch all errors at compile time. I would reccomend a unit test (two really) so that if anybody changes the database schema, this is detected at build time.


Connection Pool in Java 8 using Strategy and Factory Pattern How can I improve the following code and features?
It is based on active911's C++ connection pool.

The full code and unit tests are here : https://github.com/spakai/connection_pool

the README which explains the code is here : https://github.com/spakai/connection_pool/blob/master/README.md

This project uses Strategy and Factory Design Pattern. This allows ConnectionPool class to be generic. The type of database supported is determined by the type of Factory object that is injected during runtime. The specific factory creates required specific connection class that contains the actual connection to the database. So for example , if you need a MySQL connection pool, pass in a MySQLJDBConnectionFactory object.

package com.spakai;

import java.time.Duration;
import java.time.Instant;
import java.util.Map;
import java.util.Map.Entry;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;

public class ConnectionPool {

  private final ConcurrentLinkedQueue<JdbConnection> pool = new ConcurrentLinkedQueue<>();

  private final Map<JdbConnection,Instant> borrowed = new ConcurrentHashMap<>();

  private final JdbConnectionFactory factory;

  private final long leaseTimeInMillis;

  /**
   * Creates connection objects and pushes it into queue.
   * @param factory Used to create JDBConnection implementations.
   * @param poolSize Number of JDBConnection implementations to create.
   * @param leaseTimeInMillis How long the client can use the connection before it expires.
   */

  public ConnectionPool(JdbConnectionFactory factory, int poolSize, long leaseTimeInMillis) {
    for (int i = 0; i < poolSize; i++) {
      pool.add(factory.create());
    }

    this.factory = factory;
    this.leaseTimeInMillis = leaseTimeInMillis;
  }

  /**
   * Get a JdbConnection object either by the ones available in the queue or replace
   * the first expired connection. When a connection is given to a client, it is tagged with
   * the current time. This enables us to check the duration it has been out and replace if
   * required.
   * @return JDBConnection This contains the actual jdbc connection object to db.
   * @throws ConnectionPoolException Throws if no available connections
   */

  public JdbConnection borrow() throws ConnectionPoolException {
    if (pool.size() > 0) {
      borrowed.put(pool.peek(),Instant.now());
      return pool.remove();
    } else {
      return createReplacementIfExpiredConnFound();
    }
  }

  /**
   * Return a JdbConnection object back to the pool.
   *
   * @param jdbConnection The object retrieved from the pool via borrow()
   * @throws ConnectionPoolException Throws if connection has already been
   *        returned or forced to expire
   */

  public void forfeit(JdbConnection jdbConnection) throws ConnectionPoolException {
    if (borrowed.containsKey(jdbConnection)) {
      borrowed.remove(jdbConnection);
      pool.add(jdbConnection);
    } else {
      throw new ConnectionPoolException("Connection already returned or forced to expire");
    }
  }

  private JdbConnection createReplacementIfExpiredConnFound() throws ConnectionPoolException {
    //check for the first expired connection , close it and create a replacement
    //throw exception if replacement is not possible

    Entry<JdbConnection, Instant> entry =
        borrowed.entrySet().stream()
                        .filter(e -> hasExpired(e.getValue()))
                        .findFirst()
                        .orElseThrow(() -> new ConnectionPoolException("No connections available"));

    entry.getKey().close();
    borrowed.remove(entry.getKey());
    JdbConnection newJdbConnection = factory.create();
    borrowed.put(newJdbConnection,Instant.now());
    return newJdbConnection;
  }

  private boolean hasExpired(Instant instant) {
    return (Duration.between(instant, Instant.now()).toMillis() > leaseTimeInMillis);
  }
}

I'm not exactly an expert in this field so perhaps someone else can provide a more in depth answer but here are my initial thoughts about this code:

  public void forfeit(JdbConnection jdbConnection) throws ConnectionPoolException {

Why do you need to burden the user with that exception? If the connection was already returned or forcibly taken, does the user of this method really care?
I would just silently check if that is the case and ignore the call if it is.

thread safety?

Let's say we have a pool with only 1 connection left. What happens if 2 threads call the borrow()?

Thread one checks if (pool.size() > 0) { and it passes. But now thread one is paused and thread two get's to run.

Thread two also checks if (pool.size() > 0) { and it also passes.

Who will get the connection from the pool after this point?

It can actually get even worse. What if we not only have one connection available in the pool, but there's also an expired one.

Both threads should be able to get a connection right? But both went inside the if block. So 1 gets the connection, the other get's nothing.

The else part isn't executed for either of the threads.

Looking at the javadoc for ConcurrentLinkedQueue I don't think your code even works at the moment.

pool.remove() does not exist. pool.remove(Object obj) does but that returns a boolean to tell if removing the obj succeeded or not.

What you want is problably pool.poll().

How do you decide a good lease time? Sometimes a querry takes a couple of minutes to run. These are fine if you don't run them often. But if the lease time is set too low the query will never be able to finish. (Even tho the use case of that query could be a valid one).

Does factory.create() already set up an actual connection with the database? What if I set up a pool with 20 possible connections but actually only use a single connection for a couple of hours. Do I use up 20 connections with the database?

Why not create a new connection the moment one is asked (if we didn't pass the connection limit yet)?

What should I do if I didn't get a connection? (error no connection available). Should I run an active loop until I eventually get my connection?

You probably want to provide a method that puts the request into a queue (perhaps with a timeout) and gives the first available connection to the first in the queue. This way if too many threads are asking for a connection at once, they'll all eventually get their connection instead of overloading the system with busy-waiting-resource-wasting threads.

Multithreaded solutions are always tricky to get right. In general it's often better to try to search for an existing solution. Since I don't have experience in this field I can't really recommend any specific ones though...


C++ implementation of the Command Pattern Below is a C++ implementation of the Command Pattern. I wrote three receivers that carry out the intended action (making a sound) differently. I also wrote command and invoking classes. I saw no need to write a client class, so main() is the client.

#include <iostream>
#include <string>

// receivers

class Cow {
public:
    void moo() {
        std::cout << "Cow says moo!" << std::endl;
    }
};

class Dog {
    std::string name;
public:
    Dog(const std::string& name) : name(name) {}

    void makeSound() {
        std::cout << name + " barks!" << std::endl;
    }
};

class Car {
    bool engineOn;
public:
    Car() : engineOn(false) {};

    void turnCarOn() {
        engineOn = true;
    }

    void revEngine() {
        if (engineOn) {
            std::cout << "Vroom, vroom, vroom!" << std::endl;
        }
        else {
            std::cout << "Silence!" << std::endl;
        }
    }
};

// commands

class Command {
public:
    virtual void execute() = 0;
};

class CowCommand : public Command {
    Cow cow;
public:
    CowCommand(Cow cow) : cow(cow) {}

    void execute() {
        cow.moo();
    }
};

class DogCommand : public Command {
    Dog dog;
public:
    DogCommand(Dog dog) : dog(dog) {}

    void execute() {
        dog.makeSound();
    }
};

class CarCommand : public Command {
    Car car;
public:
    CarCommand(Car car) : car(car) {}

    void execute() {
        car.turnCarOn();
        car.revEngine();
    }
};

// invoker

class PlatoSays {
    Command* ptr_command;
public:
    PlatoSays() : ptr_command(nullptr) {}

    void setCommand(Command* ptr_cmd) {
        ptr_command = ptr_cmd;
    }

    void invokeCommand() {
        if (ptr_command) {
            std::cout << "Plato says: ";
            ptr_command->execute();
        }
        else {
            std::cout << "Plato has nothing to say!" << std::endl;
        }
    }
};

// client

int main() {
    PlatoSays plato;

    plato.setCommand(new CowCommand(Cow()));
    plato.invokeCommand();

    plato.setCommand(new DogCommand(Dog("Lucky")));
    plato.invokeCommand();

    plato.setCommand(new CarCommand(Car()));
    plato.invokeCommand();

    getchar();
}

Virtual Destructor

If a class has virtual methods it should also have a virtual destructor. Now this is only important if you destroy a class from a pointer to the base class. Since your code leaks all the objects it will not hurt you (Note: I am being facetious). Both leaking and incorrectly destroying objects is a big deal. This is after all not C and thus we like to actually write working programs.

class Command {
public:
    virtual ~Command() {}      // Declare a virtual destuctor in
                               // the base class. All derived types
                               // will inherit a virtual destructor.
    virtual void execute() = 0;
};

Override Specifier

In C++11 we added the override specifier. You are supposed to use this to make virtual functions you are overriding. It provides a nice safety blanket, if somebody in the future changes the virtual function definition then the override specifier in your class will cause an error if you have not changed the definition in your class to match.

class CowCommand : public Command {
    Cow cow;
public:
    CowCommand(Cow cow) : cow(cow) {}

    void execute() override {   // Add override here.
        cow.moo();
    }
};

Don't pass by value (Pass by const ref as a first default).

If you pass by value you are causing a copy of the object. This may be unnecessarily expensive. Pass by const reference. If you need to then you can make a copy before using.

Alternatively you can pass by R-Value reference and move the object in the methods.

    Cow cow;
public:
    // You make two copies of cow here.
    CowCommand(Cow cow)       // A copy is made to pass it to the method.
        : cow(cow)            // A second copy is made passing while
                              // creating the local `cow` object.
    {}

    // I would have done this:
    CowCommand(Cow const& cow)   // Pass a normal reference
        : cow(cow)               // Now make a copy (only 1 copy)
    {}
    CowCommand(Cow&& cow)        // Pass an r-value reference
        : cow(std::move(cow))    // Move the object into cow.
    {}

Don't pass pointers

A pointer has no concept of ownership. Ownership decides who is supposed to delete the object. If I don't know who is supposed to delete I may accidentally delete something that is not dynamic or if I don't delete something I am supposed to then I leak an object.

The idea is that public interfaces should pass things around in a way that ownership of the object is clear (private and internal methods can use pointers for efficiency).

This public function passes a pointer.

void setCommand(Command* ptr_cmd) {
    ptr_command = ptr_cmd;
}

Internally you store it as a pointer. But you don't take ownership (i.e. you don't call delete on the pointer in the destructor). So this object can easily be used incorrectly.

{
    PlatoSays  plato;
    plato.setCommand(new LokiCommand);
}

Here I have created a dynamic object and passed in it setCommand() (which has the only copy of this pointer. At the end of scope the LokiCommand object is leaked.

If you are not going to take ownership then pass by reference. This indicates to the method you can have a link to my object but I will delete it (not you).

void setCommand(Command& ptr_cmd) {
    ptr_command = &ptr_cmd;   // Its OK to store a pointer to the object
                              // Just mean Plato has to be created first
                              // and destroyed last.
}

Note: If you do want to pass ownership. Then wrap your pointer in a smart pointer. This will take care of the ownership.

class PlatoSays {
    std::unique_ptr<Command> ptr_command;
public:
    void setCommand(std::unique_ptr<Command>&& ptr_cmd) {
        ptr_command = std::move(ptr_cmd);
    }

    // Notice the usage of a unique_ptr is exactly the same as the
    // usage for a normal pointer. The only difference is that it
    // Correctly initializes itself (so you don't need a constructor)
    // And when updating it you need to use `std::move()`
    void invokeCommand() {
        if (ptr_command) {
            std::cout << "Plato says: ";
            ptr_command->execute();
        }
        else {
            std::cout << "Plato has nothing to say!" << std::endl;
        }
    }
};

Leaks in your program.

And here we see the exact problem with pointers.

plato.setCommand(new CowCommand(Cow()));
plato.invokeCommand();

plato.setCommand(new DogCommand(Dog("Lucky")));
plato.invokeCommand();

plato.setCommand(new CarCommand(Car()));
plato.invokeCommand();

All three of these objects are leaked.

Use when ownership is not transferred:

PlatoSays plato;
CowCommand   cow(Cow());
DogCommand   dog(Dog("Lucky");
CarCommand   car(Car());

plato.setCommand(cow);
plato.invokeCommand();

plato.setCommand(dog);
plato.invokeCommand();

plato.setCommand(car);
plato.invokeCommand();

Use when ownership is transferred:

PlatoSays plato;
plato.setCommand(std::unique_ptr<Command>(new CowCommand(Cow()));
plato.invokeCommand();

plato.setCommand(std::unique_ptr<Command>(new DogCommand(Dog("Lucky"))));
plato.invokeCommand();

plato.setCommand(std::unique_ptr<Command>(new CarCommand(Car())));
plato.invokeCommand();

Prefer "\n" over std::endl

The only difference is that std::endl flushes the stream. Manual programmer flushing is the usually non optimal and the cause of most slow downs when comparing C++ code to C code speed.


CS50 pset-1 itsa Mario - make a half-pyramid using parameters from user input I'm taking Harvard's introduction to computer science (CS50) course online, and I've written a program that satisfies a coding challenge but I don't understand what I did or how I can improve it. I'm a very green programmer.

The challenge is to get an int from the user between 0 and 23 and use that to set the line height of a half-pyramid drawn in the terminal using hashes, ala the ramp at the ends of the levels in Super Mario Bros:

   ##
  ###
 ####
#####

After about 6 hours of banging my head against the wall I came up with the following:

#include <cs50.h>
#include <stdio.h>

int main()
{
    int height;

    printf("How high do you want your half-pyramid?\n");
    height = GetInt();

     //verify that height is an int between 0 and 23

    while (height > 23 || height < 0)

        {
            printf("Invalid entry:\nChoose a number between 0 and 23\n");
            height = GetInt();
        }

    // draw the pyramid

    for (int line = 0; line < height; line++)
      {
         // draw spaces
        for (int space = line + 2; space < height + 1; space++)
         {
         printf(" ");
         }

         // draw hashes
        for (int hash = line + height + 1; height < hash + 1; hash--)
        {
         printf("#");
        }
        printf("\n");
      }

    return 0;
}

I understand the part that I've commented as "verify that height is an int between 0 and 23," but everything in the "draw the pyramid" section was arrived at by brute force. Just making one thing that kind of looked like it worked and then tweaking it over and over again until it did what I needed it to do. Maybe this is how everyone codes, but I hope not.

Can anyone give me some insight in fairly plain language about what the code here is doing, some ideas about how it might be improved, or just some other ways to go about it?

Organized coding

A line of a pyramid is made by x spaces and y hashes. The number of spaces is given by the total line length minus the number of hashes. The number of hashes increases from 2 to 5, the max line length is 5

static int LINE_LEN = 5;
for (int hashes = 2; hashes <= LINE_LEN; hashes++) {
    print_repeated_char(' ', LINE_LEN - hashes);
    print_repeated_char('#', hashes);
    printf("\n");
 }

Also note the print_repeated_char function avoids repetition.


Finding pairs of numbers within A that add up to N I am working on an interview question from Amazon Software Interview:

  Given an integer N and an array of unsorted integers A find all pairs of numbers within A which add up to N

I have a working solution in Java:

static void allPairsAddToN(int[] array, int n) {
    for(int c=0;c<array.length; c ++) {
        for(int j=c+1; j<array.length; j++) {
            if(array[c] + array[j] == n)
                System.out.println("(" + array[c] +", " + array[j] +")" );
        }
    }
}

Is this the most efficient solution? To me, this is the brute force approach that runs in \$O(n^2)\$. But for this problem, wouldn't you have to try every possibility (unsorted)? Is there some trick where you don't?

If you had a sorted array of n numbers, then finding pairs that sum to N should take O(n) time.

Sorting should take O(n log n) time.

O(n log n + n) is O(n log n).  That's better than O(n2).  Therefore, you would be better off sorting the array first.


Crawling through a YouTube channel via the YouTube API This code uses the YouTube API for crawling through a YouTube channel, parsing responses and syncing the channel videos with a local directory.

Can this be optimised or improved?

import subprocess
import os
import urllib.request as urlreq
import urllib.parse as urlparse
import simplejson as json

playlistIds = []
titles = []

def getPlaylistId():
    data = {}
    data['maxResults'] = '50'
    data['channelId'] = 'UCtESv1e7ntJaLJYKIO1FoYw' # Put the channelId of channel you want to Sync to.
    data['part'] = 'snippet'
    data['key'] =   'AIzaSyAngcF6oKnyEbhk3KyL9Wz1OhSi28JjbzE'
    requestValues = urlparse.urlencode(data)
    request = "https://www.googleapis.com/youtube/v3/playlists?" + requestValues
    string = urlreq.urlopen(request).read().decode('utf-8')
    items = json.loads(string)['items']

    for item in items:
        playlistIds.append(item['id'])
        titles.append(item['snippet']['title'])

def download():
    for ids,title in zip(playlistIds,titles):
            url = "https://www.youtube.com/playlist?list=" + ids
            if not os.path.exists(title):
                os.makedirs(title)
            os.chdir("./" + title)
            url_down = "youtube-dl -o '%(title)s' " + url
            subprocess.call(url_down, shell=True)
            os.chdir("..")

getPlaylistId()
download()

Global variables are almost never needed.  You should pass playlistIds as a parameter.  Well, actually, just don't make getPlaylistId() modify the list; make it return the list.  Even better, make it a generator function that yields each id and title.  That's better because you aren't storing the id's and titles in memory.

Your naming is not in accordance with PEP 8, the Python style guide:

  Function names should be lowercase, with words separated by underscores as necessary to improve readability.

  mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

and for Method Names and Instance Variables:

  Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability.

You seem to have some magic values such as '50', 'UCtESv1e7ntJaLJYKIO1FoYw', etc.  How did you come up with those values?  I would suggest making constants at the beginning of the file for your defaults.  Then, use sys.argv or, if you want to be fancier, argparse to get the values from the user.

Instead of creating a dictionary (data) and then setting all of the values, why not just create a dictionary with the right values in the first place?  That is, data = {'maxResults': '50', 'channelId': 'UctESv1e7ntJaLJYKIO1FoYw', ...}

Instead of defining items just to iterate, why not for item in json.loads(string)['items']:?

Your indentation in download() is odd.  You use four spaces everywhere else, but here you use eight.  Be consistent at the least.

os.chdir("./" + title)

Don't make it a habit to put the dots and the slashes in yourself.  Use os.path.join(os.curdir, title).  os.chdir("..") also would change to os.chdir(os.pardir).

Let's say I want to use your program in mine, but I have a different list of id's to get and a different list of titles.  This is my program:

from downloader import download

def get_ids_and_titles():
    ...
download(get_ids_and_titles())

After the first line, your program just did its thing.  It downloaded everything.  I didn't want to run your program; I wanted to use one of its functions.  You can find out if your program is being run as a module or as a program like this:

if __name__ == '__main__':
    download(getPlaylistId())

If your program is run as a program, __name__ is __main__, but if it is a module, it will be the name of the module.


Saving away URL query parameters in an analytics application We are trying to use NodeJS for analytics. The following code gets the parameter from a URL and dumps it into a flat file. Is there any way to optimize it to get better response time?

var http = require("http");
var url = require("url");
var cluster = require('cluster');
var fs = require('fs');
var numCPUs = require('os').cpus().length;
var stream = [];
if (cluster.isMaster) {
        for (var i = 0; i < numCPUs; i++) {
                cluster.fork();

        }
        cluster.on('death', function(worker) {
                console.log('worker ' + worker.pid + ' died');
        });
}
else{
    var server = http.createServer(function(request, response) {
        response.end();
    });
    stream = fs.createWriteStream("nodejsCluster"+cluster.worker.id+".txt");
    stream.once('open', function(fd) {
        server.listen(8084);
    });
    server.on('request', function(request, response){
            var parsedUrl = url.parse(request.url, true, true);
            var queryAsObject = parsedUrl.query;
            stream.write( ( new Date()  )+'This is the content to write into file '+queryAsObject.test+"\n");
    });
    console.log("Server is listening"+cluster.worker.id);
}

Your server sets up a cluster, then listens for requests. It parses a parameter from the request URL, and writes it to file.

For performance reasons, that write to file should be asyncrhonous. Currently your client has to wait for your log to write before the client continues.... but, here's the real flaw in your code....

... you do not send a response at all to the client!

You need to actually process the response by sending a status code, and ending the communication:

 response.writeHeader(200); // success
 response.end();

Client sessions will just sit there hanging until you do that.