Normalize integer types to float range I wrote two template functions to take an integer type and output a float in the given range. One function takes signed integers and the other takes unsigned integers.

The functions are used to normalize audio PCM data into a range that an encoder is expecting, such as [0.0f, 1.0f]. The source audio data can come in many forms (signed, unsigned, 1 byte, 2 bytes, etc) so I thought template functions would be ideal for this.

The caller has the responsibility of calling the function with an appropriately sized type containing the sample to normalize (e.g. int16_t). The functions use a lookup-table to determine the max/min values the type can contain based on the size of the integer type.

util.cc

#include <stdint.h>

const uintmax_t MAX_VALUE_PER_BYTES_UNSIGNED[] = {
    0,
    uintmax_t(UINT8_MAX),
    uintmax_t(UINT16_MAX),
    uintmax_t(UINT16_MAX) + uintmax_t(UINT8_MAX),
    uintmax_t(UINT32_MAX),
    uintmax_t(UINT32_MAX) + uintmax_t(UINT8_MAX),
    uintmax_t(UINT32_MAX) + uintmax_t(UINT16_MAX),
    uintmax_t(UINT32_MAX) + uintmax_t(UINT16_MAX) + uintmax_t(UINT8_MAX),
    uintmax_t(UINT64_MAX)
};

const intmax_t MAX_VALUE_PER_BYTES_SIGNED[] = {
    0,
    intmax_t(INT8_MAX),
    intmax_t(INT16_MAX),
    intmax_t(INT16_MAX) + intmax_t(INT8_MAX),
    intmax_t(INT32_MAX),
    intmax_t(INT32_MAX) + intmax_t(INT8_MAX),
    intmax_t(INT32_MAX) + intmax_t(INT16_MAX),
    intmax_t(INT32_MAX) + intmax_t(INT16_MAX) + intmax_t(INT8_MAX),
    intmax_t(INT64_MAX)
};

const intmax_t MIN_VALUE_PER_BYTES_SIGNED[] = {
    0,
    intmax_t(INT8_MIN),
    intmax_t(INT16_MIN),
    intmax_t(INT16_MIN) + intmax_t(INT8_MIN),
    intmax_t(INT32_MIN),
    intmax_t(INT32_MIN) + intmax_t(INT8_MIN),
    intmax_t(INT32_MIN) + intmax_t(INT16_MIN),
    intmax_t(INT32_MIN) + intmax_t(INT16_MIN) + intmax_t(INT8_MIN),
    intmax_t(INT64_MIN)
};

util.h

#include <stdint.h>
#include <limits>
#include <cmath>

extern const uintmax_t MAX_VALUE_PER_BYTES_UNSIGNED[];
extern const intmax_t MAX_VALUE_PER_BYTES_SIGNED[];
extern const intmax_t MIN_VALUE_PER_BYTES_SIGNED[];

template <typename T>
float normalizeToRangeFloatSigned(T num, float rangeMin, float rangeMax) {
    float f = 0.0f;

    if (!std::numeric_limits<T>::is_integer) {
        return f;
    }

    const float rangeDiff = rangeMax - rangeMin;
    if (!rangeDiff || rangeDiff < 0.0f) {
        return f;
    }

    uintmax_t uintRangeMax = MAX_VALUE_PER_BYTES_UNSIGNED[sizeof(T)];

    const intmax_t tRangeMin = MIN_VALUE_PER_BYTES_SIGNED[sizeof(T)];
    const intmax_t tRangeMax = MAX_VALUE_PER_BYTES_SIGNED[sizeof(T)];
    const intmax_t tRangeMinAbs = std::abs(tRangeMin);
    float percent = 0.0f;

    if (num < 0) {
        if (num == tRangeMin) {
            percent = 0.0f;
        } else {
            percent = (tRangeMinAbs - T{-1} * num) / float(uintRangeMax);
        }
    } else if (!num) {
        percent = 0.5f;
    } else {
        if (num == tRangeMax) {
            percent = 1.0f;
        } else {
            percent = (num + tRangeMinAbs) / float(uintRangeMax);
        }
    }

    f = (percent) * rangeDiff + rangeMin;

    return f;
}

template <typename T>
float normalizeToRangeFloatUnsigned(T num, float rangeMin, float rangeMax) {
    float f = 0.0f;

    if (!std::numeric_limits<T>::is_integer) {
        return f;
    }

    const float rangeDiff = rangeMax - rangeMin;
    if (!rangeDiff || rangeDiff < 0.0f) {
        return f;
    }

    uintmax_t uintRangeMax = MAX_VALUE_PER_BYTES_UNSIGNED[sizeof(T)];
    f = (num / float(uintRangeMax)) * rangeDiff + rangeMin;

    return f;
}

I don't have much experience with templates so any feedback is welcome. Also, any improvements to the functions time-efficiency.

<limits> already provides a set of limits on values that can be stored in the various types. It's also a template, so it's easy to invoke on a template parameter. As a quick demo of that particular part of things:

template <class T>
void showminmax(T) {
    std::cout << "min: " << std::numeric_limits<T>::min() <<
               "\nmax: " << std::numeric_limits<T>::max();
}

int main() {
     std::cout << "int\n";
     showminmax(1);

     std::cout << "\nunsigned long long\n";
     showminmax(1ULL);
}

I believe the code to normalize the number to the range [0..1] can be simplified quite a bit as well, to something on this order:

template <class T>
float normalize(T t) {
    static_assert(std::is_integral<T>::value, "Input must be integral");
    float min = std::numeric_limits<T>::min();
    float max = std::numeric_limits<T>::max();
    float range = max - min;

    return (t - min) / range;
}

This should work for either signed or unsigned types. Here's a quick bit of demo code to exercise it, and show the results:

int main() {
    std::cout << "char(min):   " << normalize(std::numeric_limits<char>::min()) << "\n";
    std::cout << "char (1/4):  " << normalize(char(std::numeric_limits<char>::min() >> 1)) << "\n";
    std::cout << "char (0):    " << normalize('\0') << "\n";
    std::cout << "char (3/4):  " << normalize(char(std::numeric_limits<char>::max() >> 1)) << "\n";
    std::cout << "char (max):  " << normalize(std::numeric_limits<char>::max()) << "\n\n";

    std::cout << "int(min):   " << normalize(std::numeric_limits<int>::min()) << "\n";
    std::cout << "int (1/4):  " << normalize(std::numeric_limits<int>::min() >> 1) << "\n";
    std::cout << "int (0):    " << normalize(0) << "\n";
    std::cout << "int (3/4):  " << normalize(std::numeric_limits<int>::max() >> 1) << "\n";
    std::cout << "int (max):  " << normalize(std::numeric_limits<int>::max()) << "\n\n";

    std::cout << "uint (0):   " << normalize(0U) << "\n";
    std::cout << "uint (1/4): " << normalize(std::numeric_limits<unsigned int>::max() / 4) << "\n";
    std::cout << "uint (mid): " << normalize(std::numeric_limits<unsigned int>::max() / 2) << "\n";
    std::cout << "uint (max): " << normalize(std::numeric_limits<unsigned int>::max()) << "\n\n";

    std::cout << "ULL (0):   " << normalize(0ULL) << "\n";
    std::cout << "ULL (1/4): " << normalize(std::numeric_limits<unsigned long long>::max() / 4) << "\n";
    std::cout << "ULL (mid):  " << normalize(std::numeric_limits<unsigned long long>::max() / 2) << "\n";
    std::cout << "ULL (max):  " << normalize(std::numeric_limits<unsigned long long>::max()) << "\n";
}

Note that twos complement is asymmetric, so the values produced are technically just a tiny bit off (well, off from what you might expect, anyway). This will normally be lost in the rounding for any type with more than ~24 input bits, but if the input type is smaller than that (e.g., most implementations of char and short) a signed value of 0 won't give precisely 0 as the result (unless you have a machine with something like signed-magnitude or ones complement int, anyway).


Update a table based on other table I created this simple macro that works fine for its purpose: allow the user to create a list of updates that need to be made based on groups in a target table (to basically do mass updates).

However, I am very concerned about the performance of this macro, it would take less time to do it manually if the list of updates becomes significantly long. I started optimizing the code using arrays but that seems not be enough.

What I am going to look into next is the use of dictionaries, is anyone aware of the most optimal way to achieve this?

Sub UpdateManualUpdates()
    Application.ScreenUpdating = False
    Dim lookUpSheet As Worksheet, updateSheet As Worksheet
    Dim valueToSearch As String
    Dim i As Long, t As Long

    Set lookUpSheet = Worksheets("Manual price changes")
    Set updateSheet = Worksheets("Price Build-up")

    Dim lookUpSheetArray As Variant
    Dim updateSheetArray As Variant

    lastRowLookup = lookUpSheet.Cells(Rows.Count, "F").End(xlUp).Row
    lastRowUpdate = updateSheet.Cells(Rows.Count, "B").End(xlUp).Row

    lookUpSheetArray = lookUpSheet.Range("A1:F" & lastRowLookup).Value
    updateSheetArray = updateSheet.Range("A1:AW" & lastRowUpdate).Value

    For i = 6 To lastRowLookup 'i = 2 to last to omit the first row as that row is for headers

        valueType = lookUpSheetArray(i, 5) 'lookUpSheet.Cells(i, 5) 'Type of update - Both, Planning group or GC
        valueGroup = lookUpSheetArray(i, 3) 'Family group
        valueGC = lookUpSheetArray(i, 4) 'GC
        ValueChange = lookUpSheetArray(i, 6) 'What is the % change
        'above get the values from the four column into variables

        For t = 6 To lastRowUpdate
            'AW is column 49 target column to update
            'M is target column for group, 13
            'C is target column for GC, 3
            If valueType = "Both" Then
                If updateSheetArray(t, 13) = valueGroup And updateSheetArray(t, 3) = valueGC Then
                    updateSheet.Cells(t, 49) = ValueChange
                End If
            End If
            If valueType = "Planning group" Then
                If updateSheetArray(t, 13) = valueGroup Then
                    updateSheet.Cells(t, 49) = ValueChange
                End If
            End If
            If valueType = "GC" Then
                If updateSheetArray(t, 3) = valueGC Then
                    updateSheet.Cells(t, 49) = ValueChange
                End If
            End If
        Next t
    Next i
    Application.ScreenUpdating = True
End Sub

It looks like you'll have to set up three dictionaries that reference the same data. In your case, the dictionaries are based on your TARGET data that should change: updateSheetArray. You need three dictionaries because you want to access the data three different ways. The trick is in choosing a unique value from the data to be used as the key into the data. The first two values are simple, single column values: PlanningGroup and GC.

Since one of your choices is "Both", you have to create a key from the data that combines both of those fields into a single value. This can be done on the worksheet itself in the form of a visible or hidden column (and thus also pulled into a memory array), or you can combine the fields in your VBA code. My preferred method for this is to combine the two (or more) fields into a single string in the code, though each situation is different. You can do a simple concatenate or concatenate with a delimiter, it does't matter.

Before we get to the dictionary part of the answer, I will suggest a few items in the code review part of this answer.

Always use Option Explicit. You might be doing this, but it's good to make sure it's visible in your questions (or answers) so it doesn't come up like this.
Declare your variables as close as possible to the first use of the variable. It saves lots of back and forth searching for declarations, plus your code is now a little grouped into "logic blocks".
Define constants for "magic numbers". These are typically a straight numerical value you've hard-coded into your routines. Any other developer that comes along will have to scratch their head to figure out why you're starting a loop from index 6. You also might need to use the same value in multiple places, so creating a Const once isolates the definition and then you only have to change it once.

The constants I've defined for my example solution are:

Private Const LOOKUP_TYPE_COL As Long = 5
Private Const LOOKUP_GROUP_COL As Long = 3
Private Const LOOKUP_GC_COL As Long = 4
Private Const LOOKUP_CHANGE_COL As Long = 6
Private Const UPDATE_GROUP_COL As Long = 13
Private Const UPDATE_GC_COL As Long = 3
Private Const UPDATE_CHANGE_COL As Long = 49
Private Const START_OF_LOOKUP_DATA As Long = 2
Private Const START_OF_UPDATE_DATA As Long = 6

I've made these constants Private at the global module level, but they easily could be restricted in scope to the single Sub in the code. (The example Sub below is a bit long and I could have separated it into smaller functions, I'll leave that as an exercise for the reader :) ).

One problem I noticed in your code is when you're calculating the last row. Your statement is good but you've missed a worksheet reference when using Rows.Count. Since the Rows is not qualified with a worksheet reference, it is counting the rows on the currently active sheet. My habit is to wrap the statement in a With block, just to make sure I have to correct reference:

Dim lookUpSheet As Worksheet
Dim lastRowLookup As Long
Dim lookUpSheetRange As Range
Dim lookUpSheetArray As Variant
Set lookUpSheet = Worksheets("Manual price changes")
With lookUpSheet
    lastRowLookup = .Cells(.Rows.Count, "F").End(xlUp).Row
    Set lookUpSheetRange = .Range("A1").Resize(lastRowLookup, 6)
    lookUpSheetArray = lookUpSheetRange.value
End With

Dim updateSheet As Worksheet
Dim lastRowUpdate As Long
Dim updateSheetRange As Range
Dim updateSheetArray As Variant
Set updateSheet = Worksheets("Price Build-up")
With updateSheet
    lastRowUpdate = .Cells(.Rows.Count, "B").End(xlUp).Row
    Set updateSheetRange = .Range("A1").Resize(lastRowUpdate, 49)
    updateSheetArray = updateSheetRange.value
End With

You are copying data from the worksheet Range into a memory-based array, and that will give you a much faster execution. The bonus is that you're not interacting with the screen, so Application.ScreenUpdating = False is not necessary. (You'll see another change to support this below)

Now comes the Dictionary build up. We're working with your target data, which is the sheet that needs to be updated. You want to be able to find any row in your data with a single reference. I'm assuming the data on your updateSheet could be hundreds or thousands of lines long. Notice in the loop below that I'm creating three dictionaries, including using a combination key to pick up the option to reference entries for "both".

Also, I figured that there could be more than one row in the data that may match the update criteria. So the dictionary keeps track by building a (string) list of row numbers to use later.

Dim groupKeys As Dictionary
Dim gcKeys As Dictionary
Dim bothKeys As Dictionary
Set groupKeys = New Dictionary
Set gcKeys = New Dictionary
Set bothKeys = New Dictionary

Dim updateGroup As String
Dim updateGC As String
Dim bothKey As String
Dim existingList As String
Dim i As Long
For i = START_OF_UPDATE_DATA To UBound(updateSheetArray, 1)
    updateGroup = updateSheetArray(i, UPDATE_GROUP_COL)
    updateGC = updateSheetArray(i, UPDATE_GC_COL)

    If Not groupKeys.Exists(updateGroup) Then
        groupKeys.Add updateGroup, CStr(i)
    Else
        existingList = groupKeys(updateGroup)
        groupKeys(updateGroup) = existingList & "," & CStr(i)
    End If

    If Not gcKeys.Exists(updateGC) Then
        gcKeys.Add updateGC, CStr(i)
    Else
        existingList = gcKeys(updateGC)
        gcKeys(updateGC) = existingList & "," & CStr(i)
    End If

    bothKey = updateGroup & updateGC
    If Not bothKeys.Exists(bothKey) Then
        bothKeys.Add bothKey, CStr(i)
    Else
        existingList = bothKeys(bothKey)
        bothKeys(bothKey) = existingList & "," & CStr(i)
    End If
Next i

Now that we have the dictionaries complete, we only need a single pass through the lookUpSheet data. We'll get the list of rows from the selected dictionary, then update only those rows in the memory-based array NOT directly on the worksheet. So this inside loop is restricted to ONLY the rows needing updated values. This is where you get your other burst of speed.

Dim valueGroup As String
Dim valueGC As String
Dim valueType As String
Dim valueChange As Double
Dim updateRows As Variant
For i = START_OF_LOOKUP_DATA To UBound(lookUpSheetArray, 1)
    valueType = lookUpSheetArray(i, LOOKUP_TYPE_COL)
    valueGroup = lookUpSheetArray(i, LOOKUP_GROUP_COL)
    valueGC = lookUpSheetArray(i, LOOKUP_GC_COL)
    bothKey = valueGroup & valueGC

    valueChange = lookUpSheetArray(i, LOOKUP_CHANGE_COL)

    updateRows = -1      'reset to a non-array value
    Select Case valueType
        Case "Planning group"
            If groupKeys.Exists(valueGroup) Then
                updateRows = Split(groupKeys(valueGroup), ",")
            End If

        Case "GC"
            If gcKeys.Exists(valueGC) Then
                updateRows = Split(gcKeys(valueGC), ",")
            End If

        Case "Both"
            If bothKeys.Exists(bothKey) Then
                updateRows = Split(bothKeys(bothKey), ",")
            End If

    End Select

    '--- update the values if we found the rows to update
    If IsArray(updateRows) Then
        Dim j As Long
        For j = LBound(updateRows, 1) To UBound(updateRows, 1)
            updateSheetArray(CLng(updateRows(j)), UPDATE_CHANGE_COL) = valueChange
        Next j
    End If
Next i

The final step is to copy the modified memory array back to the worksheet (which is why I created the updateSheetRange variable earlier).

updateSheetRange.value = updateSheetArray

Here is the whole solution in a single module:

Option Explicit

Private Const LOOKUP_TYPE_COL As Long = 5
Private Const LOOKUP_GROUP_COL As Long = 3
Private Const LOOKUP_GC_COL As Long = 4
Private Const LOOKUP_CHANGE_COL As Long = 6
Private Const UPDATE_GROUP_COL As Long = 13
Private Const UPDATE_GC_COL As Long = 3
Private Const UPDATE_CHANGE_COL As Long = 49
Private Const START_OF_LOOKUP_DATA As Long = 2
Private Const START_OF_UPDATE_DATA As Long = 6

Sub UpdateManualUpdates()
    Dim lookUpSheet As Worksheet
    Dim lastRowLookup As Long
    Dim lookUpSheetRange As Range
    Dim lookUpSheetArray As Variant
    Set lookUpSheet = Worksheets("Manual price changes")
    With lookUpSheet
        lastRowLookup = .Cells(.Rows.Count, "F").End(xlUp).Row
        Set lookUpSheetRange = .Range("A1").Resize(lastRowLookup, 6)
        lookUpSheetArray = lookUpSheetRange.value
    End With

    Dim updateSheet As Worksheet
    Dim lastRowUpdate As Long
    Dim updateSheetRange As Range
    Dim updateSheetArray As Variant
    Set updateSheet = Worksheets("Price Build-up")
    With updateSheet
        lastRowUpdate = .Cells(.Rows.Count, "B").End(xlUp).Row
        Set updateSheetRange = .Range("A1").Resize(lastRowUpdate, 49)
        updateSheetArray = updateSheetRange.value
    End With

    '--- build up the dictionaries for the UPDATE array where the keys
    '    are single or multiple fields and the entry is CSV list of
    '    row numbers that match the given key
    Dim groupKeys As Dictionary
    Dim gcKeys As Dictionary
    Dim bothKeys As Dictionary
    Set groupKeys = New Dictionary
    Set gcKeys = New Dictionary
    Set bothKeys = New Dictionary

    Dim updateGroup As String
    Dim updateGC As String
    Dim bothKey As String
    Dim existingList As String
    Dim i As Long
    For i = START_OF_UPDATE_DATA To UBound(updateSheetArray, 1)
        updateGroup = updateSheetArray(i, UPDATE_GROUP_COL)
        updateGC = updateSheetArray(i, UPDATE_GC_COL)

        If Not groupKeys.Exists(updateGroup) Then
            groupKeys.Add updateGroup, CStr(i)
        Else
            existingList = groupKeys(updateGroup)
            groupKeys(updateGroup) = existingList & "," & CStr(i)
        End If

        If Not gcKeys.Exists(updateGC) Then
            gcKeys.Add updateGC, CStr(i)
        Else
            existingList = gcKeys(updateGC)
            gcKeys(updateGC) = existingList & "," & CStr(i)
        End If

        bothKey = updateGroup & updateGC
        If Not bothKeys.Exists(bothKey) Then
            bothKeys.Add bothKey, CStr(i)
        Else
            existingList = bothKeys(bothKey)
            bothKeys(bothKey) = existingList & "," & CStr(i)
        End If
    Next i

    '--- now compare each row of the lookup data to find it in the update
    '    data and make the appropriate change to the memory array
    Dim valueGroup As String
    Dim valueGC As String
    Dim valueType As String
    Dim valueChange As Double
    Dim updateRows As Variant
    For i = START_OF_LOOKUP_DATA To UBound(lookUpSheetArray, 1)
        valueType = lookUpSheetArray(i, LOOKUP_TYPE_COL)
        valueGroup = lookUpSheetArray(i, LOOKUP_GROUP_COL)
        valueGC = lookUpSheetArray(i, LOOKUP_GC_COL)
        bothKey = valueGroup & valueGC

        valueChange = lookUpSheetArray(i, LOOKUP_CHANGE_COL)

        updateRows = -1      'reset to a non-array value
        Select Case valueType
            Case "Planning group"
                If groupKeys.Exists(valueGroup) Then
                    updateRows = Split(groupKeys(valueGroup), ",")
                End If

            Case "GC"
                If gcKeys.Exists(valueGC) Then
                    updateRows = Split(gcKeys(valueGC), ",")
                End If

            Case "Both"
                If bothKeys.Exists(bothKey) Then
                    updateRows = Split(bothKeys(bothKey), ",")
                End If

        End Select

        '--- update the values if we found the rows to update
        If IsArray(updateRows) Then
            Dim j As Long
            For j = LBound(updateRows, 1) To UBound(updateRows, 1)
                updateSheetArray(CLng(updateRows(j)), UPDATE_CHANGE_COL) = valueChange
            Next j
        End If
    Next i

    '--- all of the requested updates are complete, copy the array back to the worksheet
    updateSheetRange.value = updateSheetArray
End Sub


Counting points on a given irregular grid that are inside or outside a circle Given some points that are represented as (X, Y) coordinates, my task is to count the points that are inside a circle, on the circle, and outside the circle.

The input is given in the following way: first, the number n is provided. Then, n numbers are inputted that represent the SetX. Afterwards, the numbers for m and SetY are given in the same way.

The points that I should consider are Cartesian product of SetX and SetY. For example, given SetX: {0, 3, 4} and SetY: {1, 4}, the points would be: (0, 1), (0, 4), (3, 1), (3, 4), (4, 1), (4, 4).

The maximum element of SetX is guaranteed to be the same as the maximum element in SetY, and must be an even number. The center of previously mentioned circle is determined as (max/2, max/2), with radius max/2.

Additional limitations: n and m are at least 1 and no more than 107. Elements of SetX and SetY are no greater than 107.

My solution is as follows:

#include <iostream>

inline double distance_squared(const unsigned int&, const unsigned int&);

unsigned int center = 0;

int main()
{
    unsigned int n, m, i, inside = 0, on_circut = 0, outside = 0, tmp, distance_sqr;

    std::cin >> n;

    unsigned int set_X[n];
    for(i = 0; i < n; ++i){
        std::cin >> set_X[i]; // reading the set of Xs
        if(set_X[i] > center) center = set_X[i]; // finding maximum
    }
    center = center >> 1; //div by 2

    std::cin >> m;

    while(m--){ // reading elements of Y one by one
        std::cin >> tmp; //
        for(i = 0; i < n; ++i){ // for each pair of tmp (being Y coordinate) and a number from Xs (being coord. X)...
            distance_sqr = distance_squared(set_X[i], tmp); // find the distance^2 and compare it to radius^2
            if(distance_sqr == center*center){ ++on_circut; continue; }
            if(distance_sqr > center*center) ++outside;
            else ++inside;
        }
    }

    std::cout << inside << ' ' << on_circut << ' ' << outside;

    return 0;

}

inline double distance_squared(const unsigned int& x, const unsigned int& y){
    return (x - center)*(x - center) + (y - center)*(y - center);
}

The solution provides correct output, but it appears to be too slow. The online judge gives the "time limit exceeded" on majority of the tests. Please do note that in the original code I do not use std::cin and std::cout, but custom functions that I omitted, since I am certain that they are significantly faster than iostream functions, plus, they are quite long compared to the rest of the program.

Is there any other way I could optimise the code? Is my approach generally bad in terms of time complexity?

Well, you are manually testing every point \$(X_i, Y_j)\$ to see where it falls. You shouldn't, really.
Remember the sage advice: If in doubt, sort.

A better algorithm goes:

Read all X-coordinates.
Change them to squared distance from center-line.
Sort them.
Read an Y-coordinate.
Calculate the squared-distance from center-line for X-coordinates leading to the point sitting on the circle. \$\Delta x^2 = r^2 - (y - r)^2\$
Use binary search to determine how many elements from point 3 are smaller/equal/greater than the target.
Repeat at 4 until done.

Your algorithm was \$O(n \cdot m)\$. This one is \$O((n + m) \cdot \log(n))\$.

As an aside, if you simply double the given coordinates before processing them, you don't have to insist on the largest being even.

Now let's take a look at your code:

unsigned only has a minimal maximum of \$2^{16}-1\$, less than \$10^7\$. Even if it had 32 value-bits as on most desktops, \$2^{32}-1\$ is still below \$10^{14}\$. So, use long long which has a guaranteed 64 value-bits.
Why do you use double as the result of distance_squared()? If you did the calculation with double it would be accurate, though slow, but you only convert implicitly on return, which is far too late to help.
An unsigned is cheaply copied. Passing it by constant reference is actually a pessimization, luckily the optimizer will inline it so no harm done.
return 0; is implicit for main().
You should always finish your output with a newline. Not doing so means the prompt after calling the program won't be on a new line, and on ancient systems could have even less desirable outcomes.
The one time you use continue; it doesn't actually have any effect.
Some people like putting single-statements on the same line as the corresponding if, else if or else. I would desist as it impairs readability.
All else being equal, try to define any variable in the smallest scope you can, so a reader doesn't have to trace it all over the place. Concentration is a resource best preserved where possible.
inline only has the effect of allowing multiple equivalent definitions left. If it's only used in the current TU, better mark it static to get internal linkage.
Also consider defining a function before its use so you don't have to add a separate declaration. Remember, don't repeat yourself (DRY).
Restrict line-length so there's no horizontal scrolling. Horizontal scrolling, especially couple with vertical scrolling, is the bane of readability.


Search for a value in tuple of lists/vectors I guess there is some smarter way to implement such simple piece of code, but I couldn't figure out other solution by myself. Could it be done recursively without some sort of if's?

import           Data.Vector (Vector)
import qualified Data.Vector

type SeqV = (Vector Integer, Vector Integer, Vector Integer)

seqv :: SeqV
seqv = (Data.Vector.fromList[1..5], Data.Vector.fromList[10..15], Data.Vector.fromList[20..25])

-- | searchs for n in SeqV from left to right
-- returns first found value
find :: Integer -> SeqV -> Maybe Integer
find n (a,b,c) =
  case f a of
       Just v -> Just v
       Nothing -> case f b of
         Just v  -> Just v
         Nothing -> f c

  where f v = let f = Data.Vector.filter (==n) v
                  in if Data.Vector.length f > 0 then
                                               Just (Data.Vector.head f)
                                               else Nothing

find :: Integer -> SeqV -> Maybe Integer
find n (a,b,c) | elem n a || elem n b || elem n c = Just n
find _ _ = Nothing

There's also (<|>) :: Maybe a -> Maybe a -> Maybe a and asum :: [Maybe a] -> Maybe a, but if you simplified your question and find above doesn't unsimplify, post the whole code and there might be an unreasonably short answer again. I also have the feeling wherever you use find could be simplified further since find only really returns a boolean.


Executing a program with a temporary directory and measuring the running time I need a code review for best practices and code correctness of the following piece of code. run executes a program and validates in output. If the output is valid status can be found in VALID_SOLUTIONS.

elapsed = None
tmpdir = tempfile.mkdtemp(prefix='structure-%s-' % os.path.basename(instance))
try:
  for r in range(options.repeat):
    run_returncode, run_status, run_elapsed = run(
        r, path, program, tmpdir, options.timeout, options.satisfiable)
    if run_status not in VALID_SOLUTIONS:
      returncode, status, elapsed = run_returncode, run_status, None
      break

    if r == 0:
      # Set the expected results
      returncode, status = run_returncode, run_status
      elapsed = [run_elapsed]
    else:
      if returncode != run_returncode or status != run_status:
        # Results don't match
        returncode, status, elapsed = None, 'inconsistent_results', None
        break
      else:
        # Extra set
        elapsed.append(run_elapsed)
except Exception as e:
  # Unexpected error in script itself
  print(e, file=sys.stderr)
  #traceback.print_exc()
  status = 'error'
except KeyboardInterrupt as e:
  print('interrupted', file=sys.stderr)
  returncode, status, elapsed = None, 'interrupted', None
finally:
  if status in VALID_SOLUTIONS:
    avg = sum(elapsed) / len(elapsed)
    std = (sum((e - avg)**2 for e in elapsed) / len(elapsed)) ** 0.5
    ci = 1.96 * std / (len(elapsed) ** 0.5)
    print(instance, returncode, status, avg, ci)
  else:
    print(instance, returncode, status, None, None)

  sys.stdout.flush()
  exit(int(status not in ['interrupted'] + VALID_SOLUTIONS))

Well, firstly, don't be afraid of empty lines. This code is quite hard to read.

Secondly, functions is good invention. ;) That code as it is now is a bare piece of code, with no indication of where most of the parameters come from, and it seems that the acceptable solutions is a global, as you have capitalized it? That doesn't make much sense to me.

Just as you have a function called run() to run the program, it would make sense to have a function to run it multiple times, with the tests for that you have.

I'm also not sure why you trap KeyboardInterrupt, without seeming to actually do anything with it, except setting the status, and then testing the status. You could just do exit directly from that exception handler, that would be clearer.

Otherwise it seems fine. Feedback on correctness is impossible when your code isn't a running example. But then again, tests generally work better than code reviews there, although they are not exclusive.


Diving into PHP's MVC - the application's core I recently started programming in php using the mvc-pattern, which resulted in one of best choices, I did so far, but I read through many articles showing "easy" examples and some of them were quite advanced (based on my opinion).

I started learning the pattern by looking up some code and I ended up here: "php-mvc"-repository by panique

I changed some lines of code and my best approach up to this point is this:

<?php

define( 'DEFAULT_CONTROLLER', 'controller' );
define( 'DEFAULT_METHOD', 'index' );

define( 'CONTROLLERS_DIR', './application/controllers/' );

/**
 * Gets $_GET['url'], while assuming it will be used in the sense of MVC,
 * it returns information for the controller.
 *
 * @returns string Returns $_GET['url']
 */
function getUrl()
{
    $url = filter_input( INPUT_GET, 'url', FILTER_SANITIZE_URL );
    return ( isset( $url ) && !empty( $url ) ? $url : ( defined( 'DEFAULT_CONTROLLER' ) ? DEFAULT_CONTROLLER : null ) );
}

class Application
{

    const CONTROLLER_INDEX = 0;
    const METHOD_INDEX = 1;
    const DEFAULT_CONTROLLER = DEFAULT_CONTROLLER;
    const DEFAULT_METHOD = DEFAULT_METHOD;

    public $url;
    public $controller, $method, $params;

    public function __construct()
    {
        $this->getUrl();
        $this->splitUrl();

        $this->getController();
        $this->getMethod();
        $this->getParams();

        $this->loadController();
        $this->callMethod();
    }

    /**
     * Retrieves the controller url
     */
    private function getUrl()
    {
        $this->url = getUrl();
    }

    /**
     * Splits the url for better access
     */
    private function splitUrl()
    {
        $this->url = explode( '/', rtrim( $this->url, '/' ) );
    }

    /**
     * Gets the controller from $this->url
     */
    private function getController()
    {
        $this->controller = $this->url[self::CONTROLLER_INDEX];
        unset( $this->url[self::CONTROLLER_INDEX] );
    }

    /**
     * Gets the method from $this->url
     */
    private function getMethod()
    {
        $this->method = ( isset( $this->url[self::METHOD_INDEX] ) ? $this->url[self::METHOD_INDEX] : self::DEFAULT_METHOD );
        unset( $this->url[self::METHOD_INDEX] );
    }

    /**
     * Gets the parameters from $this->url
     */
    private function getParams()
    {
        $this->params = array_values( $this->url );
        if ( empty( $this->params ) )
        {
            $this->params = NULL;
        }
        unset( $this->url );
    }

    /**
     * Loads the controller
     */
    private function loadController()
    {
        if ( !file_exists( CONTROLLERS_DIR . $this->controller . '.php' ) )
        {
            $this->controller = self::DEFAULT_CONTROLLER;
        }
        $this->controller = new $this->controller();
    }

    /**
     * Calls the method
     */
    private function callMethod()
    {
        if ( !method_exists( $this->controller, $this->method ) )
        {
            $this->method = self::DEFAULT_METHOD;
        }
        $this->controller->{$this->method}( $this->params );
    }

}

Basically, what I wanted to do, is making the code clearer and easily readable, but now I am facing a point, where I got stuck.

I really wanted to share my approach, because I wanted you to share some thoughts about it.
Secondly, when I start a project, I always try writing my own little framework, which I require by using spl_autoload_register and the namespace, and I wanted to implement namespaces in the whole project (as it is possible?).
Are there any advices, you would like to share (based on experience, ...)? For example: references using the mvc-pattern, things definitely worth reading

For the actual code review, scroll to the bottom of this article

The "single pattern to rule them all"  pattern

This is a mistake a lot of starting developers are making. They read about how good MVC is. And that everyone should use it. A bit like jQuery, where jQuery is always the answer, independent of the question. It's like using a boat to plow a field because it is really good in the water and everyone like a boatride!

A pattern is not a rule, but a best practice to solve a certain problem. The MVC-pattern helps us solve some of the code-clutter and seperation of concern of domain logic, controlling logic an displaying stuff to the user.

Some even tend to say that MVC isn't a pattern but a way-of-life. Ignore those peeps.

Biggest problem of MVC

Because of the 'MVC solves everything' approach, you ended up writing an Application class that does, a bit of everything but only half and all in the constructor.

The moment you do more then assignments (plus the optional exception throwing for bad data) in a __construct, it's time to walk away from your code, take a break (or a beer) and then come back to look at it - with some fresh eyes.

Now that we've had our beer, try and explain to yourself why you should use the Application class and what problem it can help you solve. But because we are lazy, we simply write up the API for that class.

  The Application class has no dependencies (empty construct) and has no public methods.
  You do however have access to 4 parameters: url, controller, method and params.

Aaah, so it's a kind of parameterbag. But then without any get/set methods and only public variables.

So let's fire up that parameterbag:

$app = new Application();

  FATAL ERROR: controller not found / undefined or something like that

Application class gone wrong

The above example is a developer trying to use your code without reading it - you should never have to read code, the api should be enough.

KISS: Keept It Stupid Simple

Your application class is doing to much. And what happens if I want to run multiple applications? Each with a different url passed in? And what is a 'Application'? What does it represent/do? And more importantly: What problem does it solve?. Ow, and if the words describing the problem don't fit on a post-it, your class is doing to much.

The actual code review

construct

public function __construct()
{
    $this->getUrl();
    $this->splitUrl();

    $this->getController();
    $this->getMethod();
    $this->getParams();

    $this->loadController();
    $this->callMethod();
}

A constructor is there to instantiate the class. It shouldn't do anything. Let the developer choose when to load the controller, or filter the url, ...

Obsolete methods

private function getUrl()
{
    $this->url = getUrl();
}

What is the point here? Instead of having one line in the __construct and doing the assignement there. You added 4 lines. Remove them and add the assignment to the construct:

public function __construct()
{
    $this->url = getUrl();
}

But then there is the problem of tight coupling. A class should have as little as dependencies as possible. And all dependencies should be added in the __construct. If you need ask it:

public function __construct($url)
{
    $this->url = $url;
}

This makes testing a lot easier:

$url = 'controller/method/param1/param2';
$app = new Application($url);

And we have now succesfully decoupled the getUrl() function from our Application class. The Application class shouldn't care how the url is formed. It only tells us that it wants a url.

Misused constants

const DEFAULT_CONTROLLER = DEFAULT_CONTROLLER;
const DEFAULT_METHOD = DEFAULT_METHOD;

Setting a default controller and/or method seems like something that should be accessible as a public method of our Application:

public function setDefaultController($controller)
{
    $this->defaultController = $controller;
}

this gives us some extra sugar:

$app = new Application($url);
//If everything else failes
//we have a simple controller that
//can handle every url and prints out a small error message to the user
//and sends an email to support
$app->setDefaultController('SimpleController');

try {

    //do some heavy lifting, i.e. instantiate the database connection

    //Now that we have access to a lot of sugar
    //Our default controller can be a little more fancy
    //We now show random kittens from our database - sweet
    $app->setDefaultController('FancyController');

} catch($e)
{
    $app->run();
}

Design remarks
Your Application is in fact some kind of Router. It maps urls to controller::method(params). So why not call it a Router?

Good reads

A very good read is how the Symfony framework handles a Request:

Symfony http kernel

A good reads on patterns:

Misuse

And a last one, more of a amusing one but with a good underlying message:

Programming sucks


Correct User Input - x86 Linux Assembly So I am working on an x86 Assembly program for Linux using NASM. This program basically asks the user for their name and their favorite color. After doing this and storing the two strings in variables declared in the .bss section, the program prints "No way name of user, favorite color is my favorite color, too!

The problem I am having is that there are enormous spaces in the output because I do not know how long the string was that the user entered, only the length that I declared the buffer to be.

section .data
    greet:       db 'Hello!', 0Ah, 'What is your name?', 0Ah  ;simple greeting
    greetL:      equ $-greet                                  ;greet length
    colorQ:      db 'What is your favorite color?'            ;color question
    colorL:      equ $-colorQ                                 ;colorQ length
    suprise1:    db 'No way '
    suprise1L    equ $-suprise1
    suprise3:    db ' is my favorite color, too!', 0Ah

section .bss
    name:        resb 20                                      ;user's name
    color:       resb 15                                      ;user's color

section .text
    global _start
_start:

    greeting:
         mov eax, 4
         mov ebx, 1
         mov ecx, greet
         mov edx, greetL
         int 80                                               ;print greet

    getname:
         mov eax, 3
         mov ebx, 0
         mov ecx, name
         mov edx, 20
         int 80                                               ;get name

    askcolor:
         ;asks the user's favorite color using colorQ

    getcolor:
         mov eax, 3
         mov ebx, 0
         mov ecx, name
         mov edx, 20
         int 80

    thesuprise:
         mov eax, 4
         mov ebx, 1
         mov ecx, suprise1
         mov edx, suprise1L
         int 80

         mov eax, 4
         mov ebx, 1
         mov ecx, name
         mov edx, 20
         int 80

         ;write the color

         ;write the "suprise" 3

         mov eax, 1
         mov ebx, 0
         int 80

The code for what I am doing is above. Does anyone have a good method for finding the length of the entered string, or of taking in a character at a time to find out the length of the string?

I have an answer in x86 tasm and written under Windows, so I'm not sure if this will help, apologies if it doesn't. Maybe the way its done will give you some ideas.

.model small
.stack 100h
.data

    colour db 00001111b
    input db 10, 0, " " ;

.code

main:

    call initsegs
    call readstring
    call printstring
    call exit

PROC printstring

    push bx dx cx ax bp
    mov bx, 1; 1st (after zero) byte of input we are after, the length
    xor dx, dx; make sure dx contains zero before we play with dh and dl

    mov dl, input[bx]
    mov cx, dx; cx now has the 2nd byte value in the string, ie the length

    mov ah, 13h ; int 13h of 10h, print a string
    mov al, 1 ; write mode: colour on bl
    mov bh, 0 ; video page zero
    mov bl, colour; colour attribute

    mov dh, 10; row
    mov dl, 10; column
    mov bp, offset input ; es:bp needs to point at string..this points to string but includes its max and length at the start

    add bp, 2 ; skip the first two elements which will be the length and amount used
    int 10h;

    pop bp ax cx dx bx

    ret

ENDP printstring

PROC readstring

    push ax dx

    mov ah, 0ah ; function a of 21h - read a string
    mov dx, offset input ; reads string into DS:DX so DX needs be offset of string variable

    int 21h ; call the interrupt

    pop dx ax

    ret

ENDP readstring

PROC exit

    mov ah, 4ch
    INT 21h

    RET

ENDP Exit

PROC initsegs

    push ax

    mov ax, @DATA
    mov ds, ax
    mov es, ax

    pop ax

    RET

ENDP initsegs

end main


Calculate working hours using Python This is a simple boilerplate / template for a bigger program (I hope), either linked to a DB or including a GUI frontend using PYQT or TKinter.

This is just a little tool I wrote to keep track of my own hours.

from datetime import datetime as dt

FMT = "%H:%M"
PARSE = (f"00:00", FMT)

class WorkingTime:
    def __init__(self, worker, company):
        self.worker = worker
        self.company = company
        # This can be a JSON file or a SQL DB
        self.days_dict = {
            "Monday": {"start": "", "end": ""},
            "Tuesday": {"start": "", "end": ""},
            "Wednesday": {"start": "", "end": ""},
            "Thursday": {"start": "", "end": ""},
            "Friday": {"start": "", "end": ""},
            "Saturday": {"start": "", "end": ""},
            "Sunday": {"start": "", "end": ""},
        }

    def set_start_hour(self, day, hour):
        self.days_dict[day.title()]["start"] = hour

    def set_end_hour(self, day, hour):
        self.days_dict[day.title()]["end"] = hour

    def set_start_and_end(self, day, start, end):
        self.days_dict[day.title()]["start"] = start
        self.days_dict[day.title()]["end"] = end

    def get_start_hour(self, day):
        return self.days_dict[day.title()]["start"]

    def get_end_hour(self, day):
        return self.days_dict[day.title()]["end"]

    def get_start_and_end(self, day):
        return self.days_dict[day.title()]["start"], self.days_dict[day.title()]["end"]

    def calculate_day_hours(self, hours):
        """ takes in a tuple and returns the number of hours"""
        start, end = hours
        try:
            start_time_obj = dt.strptime(f"{start[:2]}:{start[2:4]}", FMT)
            end_time_obj = dt.strptime(f"{end[:2]}:{end[2:4]}", FMT)
            return end_time_obj - start_time_obj
        except ValueError:
            return dt.strptime(*PARSE).strftime(FMT)

    def calculate_week_hours(self):
        """ returns the sum of the week """
        total = dt.strptime(*PARSE)
        for k, v in self.days_dict.items():
            if v.get("start") and v.get("end"):
                hours = self.calculate_day_hours((v.get("start"), v.get("end")))
                total += hours
        return total.strftime(FMT)

    def display_hours_day(self, day, hours):
        """ Returns a string of the hours for that day """
        day_hours = self.get_start_and_end(day)
        rtn_hours = self.calculate_day_hours(day_hours)
        space = " " * (11 - len(day))
        return f"\n\t{day.title()}{space}::  {rtn_hours} Hours\n"

    def display_hours_week(self):
        """ Returns a string of the hours for the week """
        rtn_str = "\n"
        rtn_str += f"\n\tName       ::  {self.worker}\n\tCompany    ::  {self.company}"
        line = "-" * 50
        total_line = '-'*26
        rtn_str += f"\n{line}"
        for k, v in self.days_dict.items():
            if v:
                hours = self.calculate_day_hours((v.get("start"), v.get("end")))
                rtn_str += self.display_hours_day(k, hours)
        total = self.calculate_week_hours()
        rtn_str += f"\t{total_line}"
        rtn_str += f"\n\tTotal      ::  {total} Hours\n"
        rtn_str += f"\n{line}\n\n"
        return rtn_str

wt = WorkingTime('Maffaz', 'IT Circle Consult')

# Set hours
wt.set_start_hour("monday", "0730")
wt.set_end_hour("monday", "1800")
wt.set_start_and_end("TUESDAY", "", "")
wt.set_start_and_end("wednesday", "", "")
wt.set_start_and_end("THUrsday", "", "")
wt.set_start_and_end("FrIDAY", "", "")

# # get hours
print(wt.get_start_hour("Monday"))
print(wt.get_end_hour("Wednesday"))
print(wt.get_start_and_end("Friday"))

# calculate day hours
print(wt.calculate_day_hours(wt.get_start_and_end("monday")))

# calculate week hours
week = wt.calculate_week_hours()
print(week)

# print day hours
tues_hours = wt.get_start_and_end("tuesday")
print(wt.display_hours_day("Tuesday", tues_hours))

# print week hours total
print(wt.display_hours_week())

Extract day.title() to  a separate method. In this case, if you wanted to change how you store the days of the weeks (e.g. Monday, Tuesday ... -> monday, tuesday...), you would have to change day.title() to day.lower() only in one place*.
Implement set_start_and_end() by calling set_start() and set_end(). Identically for getters. The rationale is similar to the previous point: if you decided to change the data structure where you store the time slots, you would have to change less code*.
Magic numbers. e.g. 11. First of all extract such number into a constant with a meaningful name. Btw, is this value really a constant? Will you still want to use this value if the names of the days of the week change (e.g. you decide to use Mon, Tue etc., or names in another language)
Do not do formatting in calculate_*** methods. You have display_** methods responsible for that. Let all the formatting be done there. Btw, maybe format_*** would be a better name?
Another question is how you initialize your days_dict. Maybe you would want to pass a dictionary with some already existing values as an argument to the constructor. Possibly make it optional, and if we don't pass anything initialize it with default values as you do now:

def __init__(self, worker, company, days_dict=None):
    self.worker = worker
    self.company = company
    # This can be a JSON file or a SQL DB
    self.days_dict = days_dict or DEFAULT_DAYS_DICT

Also, maybe it would be nicer to have default values like "Monday": {"start": "0000", "end": "0000"}?
How about having a fluent API?

wt = WorkingTime('Maffaz', 'IT Circle Consult')
        .set_start_hour("monday", "0730")
        .set_end_hour("monday", "1800")
        ...
        .set_start_and_end("FrIDAY", "", "")

For that you would need to return self object from every setter/getter:

def set_start_hour(self, day, hour):
    ...
    return self

* I wrote about situations where you would have to change a lot of code. The problem is actually not with changing it. The problem is that in some situations you might forget to do it and your program will start working incorrectly.


LeetCode 1146: Snapshot Array I'm posting my C++ code for LeetCode's Snapshot Array. If you have time and would like to review, please do so. Thank you!
Problem

Implement a SnapshotArray that supports the following interface:

SnapshotArray(int length) initializes an array-like data structure with the given length.  Initially, each element equals 0.
void set(index, val) sets the element at the given index to be equal to val.
int snap() takes a snapshot of the array and returns the snap_id: the total number of times we called snap() minus 1.
int get(index, snap_id) returns the value at the given index, at the time we took the snapshot with the given snap_id

Example 1:
Input: ["SnapshotArray","set","snap","set","get"]
[[3],[0,5],[],[0,6],[0,0]] Output: [null,null,0,null,5] Explanation:

SnapshotArray snapshotArr = new SnapshotArray(3); // set the length to be 3
snapshotArr.set(0,5);  // Set array[0] = 5
snapshotArr.snap();  // Take a snapshot, return snap_id = 0
snapshotArr.set(0,6);
snapshotArr.get(0,0);  // Get the value of array[0] with snap_id = 0, return 5

Constraints:

1 <= length <= 50000
At most 50000 calls will be made to set, snap, and get.
0 <= index < length
0 <= snap_id < (the total number of times we call snap())
0 <= val <= 10^9

LeetCode Template
class SnapshotArray {
public:
    SnapshotArray(int length) {

    }

    void set(int index, int val) {

    }

    int snap() {

    }

    int get(int index, int snap_id) {

    }
};

/**
 * Your SnapshotArray object will be instantiated and called as such:
 * SnapshotArray* obj = new SnapshotArray(length);
 * obj->set(index,val);
 * int param_2 = obj->snap();
 * int param_3 = obj->get(index,snap_id);
 */

Accepted C++
class SnapshotArray {
public:
    SnapshotArray(const int array_length) {}
    int curr_shot_id = 0;
    std::unordered_map<int, std::vector<pair<int, int>>> id_map;

    // Returns current snapshot O(1)
    const int snap() {
        return curr_shot_id++;
    }

    // Setter with unordered_map
    const void set(const int key, const int shot_id) {
        if (id_map[key].empty() || id_map[key].back().first != curr_shot_id) {
            id_map[key].push_back({curr_shot_id, shot_id});

        } else {
            id_map[key].back().second = shot_id;
        }
    }

    // Getter with binary searching -> O(1) memory O(log N) time
    const int get(const int key, const int shot_id) {
        const auto iter = std::upper_bound(id_map[key].begin(), id_map[key].end(), std::pair<int, int>(shot_id, INT_MAX));
        return iter == std::begin(id_map[key]) ? 0 : std::prev(iter)->second;
    }
};

Reference
LeetCode is a platform only for interviewing and competitive programming. On LeetCode, there is a class usually named Solution (for this post is SnapshotArray) with one or more public functions which we are not allowed to rename.

Question
Discussion

The class template provided provides the necessary public interface, anything else should be private rather than public. Therefore the variable int curr_shot_id and the variable std::unordered_map<int, std::vector<pair<int, int>>> id_map; should be declared after private:.
The variable curr_shot_id should be initialized by the constructor SnapshotArray(int length).
    SnapshotArray(int length)
    : curr_shot_id{0}
    { }

It's not clear that you need a binary search of the map since a map is a direct access memory structure which means that the key will be hashed and that provides a direct reference to the object stored.


AI to control a Ping Pong paddle I am making a Ping Pong game using Html5 Canvas. I am concerned about the movement(_b) method. This method should move the computer paddle (up and down) vertically and follow the ball. Some sort of simple AI. Everything works like it should but there is a lot of repetition of if else blocks that might be handled differently. Like shorter code or at least a bit more elegant.
The _b in the movement(_b) is parameter  for the ball that will be passed as an argument.
The keyword this refers to the computer paddle
The code below might give you an idea of what I am talking about.
class Paddle {
    constructor(x, y, width, height, color) {
        this.x = x
        this.y = y
        this.width = width
        this.height = height
        this.color = color
    }
    // collision  logic here...
}

class Ball {
    constructor(x, y, x_velocity, y_velocity, radius, color) {
        this.x = x;
        this.y = y;
        this.velocity = {
            x: x_velocity,
            y: y_velocity
        }
        this.radius = radius;
        this.color = color;
        this.isBouncingAllowed = true;
    }
    // movement logic here...
}

 class ComputerPaddle extends Paddle {
    constructor(...args) {
        super(...args)
    }

    movement(_b) {
        let centerOfPaddle = this.y + this.height / 2;

        if (centerOfPaddle < _b.y - 80) {
            this.y += 4
        }
        else if (centerOfPaddle > _b.y + 80) {
            this.y -= 4
        }
        else if (centerOfPaddle < _b.y - 60) {
            this.y += 2
        }
        else if (centerOfPaddle > _b.y + 60) {
            this.y -= 2
        }
        else if (centerOfPaddle < _b.y - 40) {
            this.y += 1
        }
        else if (centerOfPaddle > _b.y + 40) {
            this.y -= 1
        }
        else if (centerOfPaddle < _b.y - 20) {
            this.y += 0.5
        }
        else if (centerOfPaddle > _b.y + 20) {
            this.y -= 0.5
        }
    }
    update(_ball_) {
        this.movement(_ball_)
        this.ballInteraction(_ball_)
        this.draw()
    }
}

let player = new PlayerPaddle(20, canvas.height / 2 - sizes.paddle.height / 2, sizes.paddle.width, sizes.paddle.height, 'red')
let computer = new ComputerPaddle(canvas.width - (20 + sizes.paddle.width), canvas.height / 2 - sizes.paddle.height / 2, sizes.paddle.width, sizes.paddle.height, 'cyan')
let ball = new Ball(250, 350, 5, 0, sizes.ball.radius, 'green')

function animate() {
    requestAnimationFrame(animate);
    ctx.clearRect(0, 0, canvas.width, canvas.height)

    ball.update()
    player.update(ball)
    computer.update(ball)
}
animate();

Any suggestions will be much appreciated.
EDIT: I did add the Ball and Paddle constructors. The methods in the classes containing the movement and collision detection logic were not included because that code is over 100 lines long and probably not relevant for my question.

Using a mechanical refactoring approach, you could create an array of objects to encapsulate the offsets and values associated with each offset:
movement(_b) {
  let centerOfPaddle = this.y + this.height / 2;

  let offsets = [
    { offset: 80, value: 4 },
    { offset: 60, value: 2 },
    { offset: 40, value: 1 },
    { offset: 20, value: 0.5 },
  ];

  for (const { offset, value } of offsets) {
    if (centerOfPaddle < _b.y - offset) {
      this.y += value;
      break;
    } else if (centerOfPaddle > _b.y + offset) {
      this.y -= value;
      break;
    }
  }
}

Going further, you could do a more conceptual refactoring and figure out the math behind your logic. For example:
movement(_b) {
  let centerOfPaddle = this.y + this.height / 2;

  for (let i = 80, j = 4; i >= 20; i -= 20, j /= 2) {
    if (centerOfPaddle < _b.y - i) {
      this.y += j;
      break;
    } else if (centerOfPaddle > _b.y + i) {
      this.y -= j;
      break;
    }
  }
}

You could give those loop variables more appropriate names like offset and value.


Backbone view that "starts" the application I use the the View below to get things started.  I'm not interested in Marionette.

This code is what is constructed when the application first loads.  It in turn loads the composite view which is common to all loads. It also constructs other objects needed for the application.

I'm looking for feedback on code structure and not naming conventions.

/***********************************************************************************************************************
** BVApp
*/

    var BVApp = Backbone.View.extend({
        Name: 'BVApp',
        el: window,
        initialize: function () {
            var token = Storage.getToken(),
                pub_req = $A.Reg.get('pub_req');

            // View like objects - inject DOM elements needed
            var BodyInstance = new Body(document.body);
            var ErrorHandlerInstance = new ErrorHandler(document.body);
            var TitleAjaxInstance = new TitleAjax($A.el("#timer"));

            this.setFrame();
            this.loadCommonViews();
            if (pub_req) {
                Page.flip('splash');
            } else if (!token) {
                Page.flip('splash');
            } else {
                this.loadViews(token);
            }
        },
        setFrame: function () {
            var test = $A.el('#super-1');
            if(test){
                test.style.display='none';
            }
            $A.el('#mega_wrap').style.display='inherit';
        },
        loadCommonViews: function () {
            $A.Mod.add(new BVFave({el: $A.el('#fm'), collection: new BCFave([], {data: {h_token: "1FOO", model: "FavoriteReader"}})}));
            $A.Mod.add(new BVFeed({el: $A.el('#at_view'), collection: new BCFeed([], {data: {h_token: "1FOO", model: "FeedReader"}})}));
        },
        loadViews: function (token) {
            var composite_view = $A.Mod.add(new BVComposite());
            composite_view.render(token);
            Page.flip(Storage.get('page'));
        }
    });

You should shorten this up

if (pub_req) {
    Page.flip('splash');
} else if (!token) {
    Page.flip('splash');
} else {
    this.loadViews(token);
}

by merging the first two if statements, like this

if (pub_req || !token) {
    Page.flip('splash');
} else {
    this.loadViews(token);
}

This evaluates in exactly the same order as your code did, but it is shorter.

// View like objects - inject DOM elements needed
var BodyInstance = new Body(document.body);
var ErrorHandlerInstance = new ErrorHandler(document.body);
var TitleAjaxInstance = new TitleAjax($A.el("#timer"));

You have declared these variables but then you don't use any of them, anywhere in the code that you have posted, I can only assume that you don't need these and that you should remove the dead code laying around in your code.


KNN pipeline w/ cross_validation_scores Using the wine quality dataset, I'm attempting to perform a simple KNN classification (w/ a scaler, and the classifier in a pipeline). It works, but I've never used cross_val_scores this way and I wanted to be sure that there isn't a better way. For me, it became a little less straightforward using the cross_val_scores inside the pipeline, including the n range

I've included my entire code, as it's fairly short.

import pandas as pd

import pylab as pl

from sklearn.preprocessing import StandardScaler, LabelEncoder
from sklearn.neighbors import KNeighborsClassifier
from sklearn.pipeline import make_pipeline, Pipeline, FeatureUnion
from sklearn.cross_validation import cross_val_score

X = pd.read_csv("https://s3.amazonaws.com/demo-datasets/wine.csv")
y = X.pop('high_quality')

le = LabelEncoder()
X.color = le.fit_transform(X.color)

results = []

for n in range(1, 50, 2):
   pipe = make_pipeline(StandardScaler(),
   KNeighborsClassifier(n_neighbors=n))
   c_val = cross_val_score(pipe, X, y, cv=5, scoring='accuracy').mean()
   results.append([n, c_val])

print results

results = pd.DataFrame(results, columns=["n", "accuracy"])

pl.plot(results.n, results.accuracy)
pl.title("Accuracy with Increasing K")
pl.show()

Your usage (and your code) is actually fine, which means there isn't much to review.

The only tip I would give is that having only the mean of the cross validation scores is not enough to determine if your model did well. Imagine, for instance, that you have 4 cv that gave the following accuracy scores : [0.01, 0.90, 0.90, 0.90] / 4 = 0.677 vs another set of hyper-parameters that gave [0.77, 0.79, 0.76, 0.80] / 4 = 0.78. That's a fairly crazy example, because such a big gap in the first cv scores is unrealistic, but it's only an example. The mean isn't very robust to outliers, that's what I mean. In order to have more information, it's great to also keep tracking of the standard deviation. You can also plot it using errorbar in matplotlib.


Find the correct path through a 5 x 5 grid (coderbyte 'Correct path') I've completed the following coderbyte question and would like feedback as to whether it follows Python best practices, is efficient and so forth.

The coderbyte problem:

Have the function calcpath(str) read the str parameter being passed, which will represent the movements made in a 5x5 grid of cells starting from the top left position The characters in the input string will be entirely composed of: r, l, u, d, ?. Each of the characters stand for the direction to take within the grid, for example: r = right, l = left, u = up, d = down. Your goal is to determine what characters the question marks should be in order for a path to be created to go from the top left of the grid all the way to the bottom right without touching previously travelled on cells in the grid.

For example: if str is "r?d?drdd" then your program should output the final correct string that will allow a path to be formed from the top left of a 5x5 grid to the bottom right. For this input, your program should, therefore, return the string rrdrdrdd. There will only ever be one correct path and there will always be at least one question mark within the input string.

Examples

Input: "???rrurdr?",
Output: "dddrrurdrd"

Input: "drdr??rrddd?",
Output: "drdruurrdddd"

Code:

import itertools
import numpy as np

def calcpath(s):
    _map = {"d": 1, "u": -1, "l": -1, "r": 1}

    q_count = s.count("?")
    move_combinations = itertools.product(_map.keys(), repeat=q_count)

    while True:
        arry = np.array([[0]*5 for i in range(5)])
        arry[0][0] = 1  # start at 0, 0

        current_x = 0
        current_y = 0

        try:
            moves = next(move_combinations)
        except StopIteration:
            return None

        ss = s[::].replace("?", "{}").format(*moves)

        for move in ss:

            if move in "lr":
                current_x += _map.get(move)
            elif move in "du":
                current_y += _map.get(move)

            if current_x < 0 or current_x > 4:
                break
            elif current_y < 0 or current_y > 4:
                break
            elif arry[current_x][current_y]:
                break
            else:
                arry[current_x][current_y] = 1

        # check for completion
        if (current_x, current_y) == (4, 4):
            return ss

assert calcpath("???rrurdr?") == "dddrrurdrd"
assert calcpath("drdr??rrddd?") == "drdruurrdddd"
assert calcpath("??ddd??lrlddd?") == None

Unnecessary Import

numpy is not required for this challenge.  You are using none of its special capabilities.  The following:

arry = np.array([[0]*5 for i in range(5)])

could easily and simply be replaced with

arry = [[0] * 5 for _ in range(5)]

Notice the throw-away _ variable being used for the unused loop comprehension variable.  If you don't use it, don't name it.

StopIteration

It is rarely necessary to use try: ... except: ... to catch the StopIteration exception.  You just need to use a loop structure.

    move_combinations = itertools.product(_map.keys(), repeat=q_count)

    while True:
        arry = np.array([[0]*5 for i in range(5)])
        arry[0][0] = 1  # start at 0, 0

        current_x = 0
        current_y = 0

        try:
            moves = next(move_combinations)
        except StopIteration:
            return None

        ...

could be written much more simply as:

    for move in itertools.product(_map.keys(), repeat=q_count):

        arry = [[0] * 5 for _ in range(5)]
        arry[0][0] = 1  # start at 0, 0

        current_x = 0
        current_y = 0

        ...

    return None

Variable Names

_map is an odd local variable name.  A single leading underscore is used to signify private/protected object members.  Local variables are never visible in an outer scope, so do not need to be flagged as "private".  A trailing underscore is usually used to avoid name collisions, in which case the variable should be named map_.  But confusion may be better avoided by naming the variable not with its type but with its role; this dictionary mapping contains directions, so it may be better named directions.

arry is also a poor name.  visited would be a better choice.

True / False

You are storing 0 and 1 in your arry matrix, and then testing for the truthiness of arry[current_x][current_y].  It would be clearer to store False and True in these.

visited = [[False] * 5 for _ in range(5)]

...

    ...
    elif visited[current_x][current_y]:
        break
    else:
        visited[current_x][current_y] = True

Unnecessary Slicing

Neither s.replace(...) nor s[::].replace(...) will modify the original s contents.  There is no need to use slicing to create a copy prior to the replace operation:

        ss = s.replace("?", "{}").format(*moves)

(And as Carcigenicate mentioned, move it out of the loop)

Magic Numbers

The code is littered with 4's and 5's.  What if you want to change this to work with a 5x6 or 4x7 grid?  You need to correctly change a lot of numbers.  Perhaps you should pass in the size of the grid, instead of using hard-coded values:

def calcpath(s, rows=5, columns=5):

    start = (0, 0)
    end = (columns - 1, rows - 1)

    ...

       visited = [[False] * columns for _ in range(rows)]
       current_x, current_y = start

       ...

           if current_x < 0 or current_x >= columns:
               break
           ...

       if (current_x, current_y) == end:
           ...

    ...

Optimizations

There should be several obvious checks you can make.  For the 5x5 grid case:

len(s) must be even
len(s) must be >= 8
len(s) must be < 25

Moreover, the directed counts of the left-right and up-down moves will produce a reduced search space.

With s = "???rrurdr?":

>>> print(*(sum(c == dir for c in s) for dir in "lrud?"))
0 4 1 1 4

There are 0 lefts, 4 rights, so we need to move +0 right to yield a net +4 right; there is 1 up and 1 down, so we need to move +4 down to yield a net +4 down.  We have 4 moves up for grabs, so they all need to be downs.  There is no point checking for combinations including up, left and right moves!

You can expand the logic of this for other cases: "drdr??rrddd?" has a net 4 right, 5 down, so the additional moves must include 1 up.  Then, it could add either a left/right pair, or an up/down pair, so you only need to check permutations of "ulr" and "uud", for a total of 12 possibilities, instead of your \$4^3\$ search space.


Removing commented dead code without removing the legitimate comments This question is the real question asked on StackOverflow. I'm here to review my answer and see how can I optimize it.

Here is the answer text:

This is a basic approach, but it proposes a proof of concept of what might be done. I do it using Bash along with the usage of the GCC -fsyntax-only option.

Here is the bash script:

#!/bin/bash
while IFS='' read -r line || [[ -n "$line" ]]; do
    LINE=`echo $line | grep -oP "(?<=//).*"`
    if [[ -n "$LINE" ]]; then
            echo $LINE | gcc -fsyntax-only -xc -
            if [[ $? -eq 0 ]]; then
                   sed -i "/$LINE/d" ./$1
            fi
    fi
done < "$1"

The approach I followed here was reading each line from the code file. Then, greping the text after the // delimiter (if exists) with the regex (?<=//).* and passing that to the gcc -fsyntax-only command to check whether it's a correct C/C++ statement or not. Notice that I've used the argument -xc - to pass the input to GCC from stdin (see my answer here to understand more). An important note, the c in -xc - specifies the language, which is C in this case, if you want it to be C++ you shall change it to -xc++.

Then, if GCC was able to successfully parse the statement (i.e., it's a legitimate C/C++ statement), I directly remove it using sed -i from the file passed.

Running it on your example (but after removing <- commented code from the third line to make it a legitimate statement):

// Those parameters control foo and bar... <- valid comment
int t = 5;
// int t = 10;
int k = 2*t;

Output (in the same file):

// Those parameters control foo and bar... <- valid comment
int t = 5;
int k = 2*t;

(if you want to add your modifications in a different file, just remove the -i from sed -i)

The script can be called just like: ./script.sh file.cpp, it may show several GCC errors while these are the correct

echo | grep is unwarranted. bash understands regular expressions ("$line" =~ regex), and can do simple substitutions: line=${line#[[:space:]]*\/\/} removes leading whitespaces , followed by the comment, just what we are after.
Replacing the file while reading it looks suspiciously. I recommend to have a destination file, and copy valid lines (and don't copy undesired ones). A perk benefit is that forking sed is not needed anymore.

A side note: the script makes a false positive in cases like

    // Notice that
    // some_valid_c_code;
    // doesn't work, because etc

The part of the comment would be recognized as a dead code, and the output will be

    // Notice that
    // doesn't work, because etc


Generalized Project Euler #4: Largest palindrome from product of two n-digit numbers in Python This solves Project Euler 4: Largest palindrome product using Python (not limited to 3 digit numbers). I need suggestions for improvements to either the Python code or the math/algorithm since time of execution increases exponentially as number of digits in multipliers increase.

A palindromic number reads the same both ways. The largest palindrome made from >the product of two 2-digit numbers is 9009 = 91 × 99.
Find the largest palindrome made from the product of two 3-digit numbers.

Here, I am trying to generalise the problem for n-digit numbers:
import sys
print(sys.version)

'''
A palindromic number reads the same both ways.
The largest palindrome made from the product of two 2-digit numbers is 9009    = 91 × 99.
This program intends to find the largest palindrome made from the product of    two n-digit numbers.
'''

digits_str = input("Enter no. of digits in multipliers : ")
digits = int(digits_str)

min_plier = (10 ** (digits-1))   # Minimum n-digit   number for eg. if digits = 3, min_plier = 100
max_plier = int(("9" * (digits+1))[:digits]);   # Maximum n-digit number for eg. if digits = 3, max_plier = 999

# Calculate product and get palindrome
pallindromes = []
for z in range (max_plier, min_plier , -1):
    max_plier = z          # To avoide repetitive calcualtions.

    for x in range(max_plier, min_plier, -1):
        global pallindromes
        product = z * x

        # Check if product obtained is palindrome and is greater than previously obtained palindrome.
        if (str(product) == str(product)[::-1]) :
            pallindromes.append(product)

print("Largest palindrome is : " + str(max(pallindromes)))

Here's the time required for execution as the number of digits increase:

No. of digits : 2, Largest palindrome : 9009, Time required for execution : 1.403140s
No. of digits : 3, Largest palindrome : 906609, Time required for execution : 1.649165s
No. of digits : 4, Largest palindrome : 99000099, Time required for execution : 39.202920s
No. of digits : 5, Largest palindrome : 9966006699, Time required for execution :  1hr 3min 54.552400s

Instead of build a list of palindromes, calculate max palindrome.
When z * z is lower than max_palindrome, You can break the first for loop (all other palindromes will be lower).
When palindrome is lower than max_palindrome, You can break the second for loop (all other palindromes will be lower). Thanks @Peter Taylor for fix location of if statement.
Convert int to str is expensive. Do it once, not twice.
Based of @Peter Taylor algorithm, set step to -2
Based on Joe Wallis algorithm, modify second loop to range(max_plier, int((z * z) ** 0.5), -2)

Code:

min_plier = 10 ** (digits - 1)  # Minimum n-digit number for eg. if digits = 3, min_plier = 100
max_plier = 10 ** digits - 1    # Maximum n-digit number for eg. if digits = 3, max_plier = 999

max_palindrome = 0

for z in range(max_plier, min_plier, -2):
    if z * z < max_palindrome:
        break

    for x in range(max_plier, int((z * z) ** 0.5), -2):
        product = z * x

        # Check if product is greater than previously obtained palindrome.
        if product < max_palindrome:
            break

        sproduct = str(product)

        # Check if product obtained is palindrome.
        if sproduct == sproduct[::-1]:
            max_palindrome = product

print("Largest palindrome is : %s" % max_palindrome)

Test you solution:

  No. of digits : 4, Largest palindrome : 99000099, Time required for
  execution : 32s

  No. of digits : 5, Largest palindrome : 9966006699, Time required for
  execution : 55min

Test my solution

  No. of digits : 4, Largest palindrome : 99000099, Time required for
  execution : 1ms

  No. of digits : 5, Largest palindrome : 9966006699, Time required for
  execution : 7ms

  No. of digits : 6, Largest palindrome : 999000000999, Time required for
  execution : 67ms

  No. of digits : 7, Largest palindrome : 99956644665999, Time required for
  execution : 373ms

Final soulution and explanation

Lets look at simple example:

mmax = 4
for i in range(mmax, 1 - 1, -1):
    for j in range(mmax, i - 1, -1):
        print(i, j)

  4 4
  3 4
  3 3
  2 4
  2 3
  2 2
  1 4
  1 3
  1 2
  1 1

It's combinations with replacement (not permutation). In this case first palindrome is max palindrome.

digits = int(input("Enter no. of digits in multipliers : "))

def largest_product_two(digits):
    min_plier = 10 ** (digits - 1)  # Minimum n-digit number for eg. if digits = 3, min_plier = 100
    max_plier = 10 ** digits - 1    # Maximum n-digit number for eg. if digits = 3, max_plier = 999

    for z in range(max_plier, min_plier, -2):
        for x in range(max_plier, z - 1, -2):
            product = z * x
            sproduct = str(product)

            # Check if product obtained is palindrome.
            if sproduct == sproduct[::-1]:
                return product

    return None

out = largest_product_two(digits)

print("Largest palindrome is : %s" % out)

Hint to check

For size=3 palindrome is when:

\$100000x + 10000y + 1000z + 100z + 10y + x\$

\$100001x + 10100y + 1100z\$

\$11(9091x + 910y + 100z)\$

For size=4 is similar.

Palindrome is divisible by 11

for z in range(max_plier, min_plier, -2):
    for x in range(max_plier, z - 1, -2):
        product = z * x
        if product % 11 != 0:
            continue

or more pythonic

products = (z * x
            for z in range(max_plier, min_plier - 1, -2)
            for x in range(max_plier, z - 1, -2)
            if z * x % 11 == 0)
for product in products:
    sproduct = str(product)

    # Check if product obtained is palindrome.
    if sproduct == sproduct[::-1]:
        max_palindrome = product
        break

  No. of digits : 6, Largest palindrome : 999000000999, Time required for
  execution : 30ms

  No. of digits : 7, Largest palindrome : 99956644665999, Time required for
  execution : 166ms

  No. of digits : 8, Largest palindrome : 9999000000009999, Time required for
  execution : 51s


Simple Duration calculator using es6 class syntax I've written a simple script that calculates the distance between two points in time and returns the duration.

Example: Start - 11:00, End - 11:45, Duration - 0.75

I wanted to make use of ES6 classes and I'd like feedback on the Duration class in particular. For example, should the validation of the start and end arguments go in the constructor? Or should that be done later?

class Duration {
  constructor(start, end) {
    const reg = RegExp('^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$');

    this.start = reg.test(start) ? start : '00:00';
    this.end = reg.test(end) ? end : '00:00';
  }

  _seconds(time = "00:00") {
    const split = time.split(':');

    return Number(split[0]) * 60 + Number(split[1]);
  }

  // See: https://stackoverflow.com/questions/11832914/round-to-at-most-2-decimal-places-only-if-necessary
  _round(float) {
    return Math.round((float + 0.0001) * 100) / 100;
  }

  get difference() {
    const {
      _round: round,
      _seconds: seconds,
      start,
      end
    } = this;

    return round((seconds(end) - seconds(start)) / 60);
  }

}

function calcDuration(e) {
  e.preventDefault();

  const start = document.getElementById('time-start');
  const end = document.getElementById('time-end');
  const target = document.getElementById('target');

  const duration = new Duration(start.value, end.value);

  target.innerHTML += `<tr><td>${duration.start}</td><td>${duration.end}</td><td>${duration.difference}</td></tr>`;

  start.focus();
}

document.getElementById('time-submit').addEventListener('click', calcDuration);
<link href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css" rel="stylesheet" />

Duration Calculator

Enter a start time and an end time and a conversion to a decimal will be given!

      <form class="time mx-auto">

            Start time:

            <input type="time" id="time-start" class="form-control" name="time-start" required autofocus/>

            End time:

            <input type="time" id="time-end" name="time-end" class="form-control" required/>

            <button id="time-submit" class="btn btn-primary" type="submit" value="Calculate">Calculate</button>

      </form>

            Start
            End
            Duration

NOTE     I'll only be taking a look at your Duration class, not the UI-specific code.

Regex as a constant

You don't need to re-instantiate a new RegExp instance every time a Duration is instantiated.
Move the regex initialization to the global context, or maybe as a static field of the class.
This saves memory as it's not allocated for the same regex over and over whenever you call new Duration.

Also, let's be more descriptive in its naming.
Since it's used for validation, let's name it exactly that.

class Duration {
  static VALIDATION = /^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/;
  constructor(start, end) {
    // ...
  }
  // ...
}

You also don't need to instantiate it with the RegExp function; you can just place it there as a literal as it's known run-time.

Helper methods

...shouldn't be methods at all, but functions instead.
The _seconds and _round do not belong in the Duration class.
They are pure utility functions that don't touch this at all: give them something as input, and something comes out as output.

If you put them in a separate file, they also become more reusable.
Your round function, for example, has nothing to do with Duration and thus you could re-use it in any math-based code -- like a renderer in a 3D engine!

function seconds(time = "00:00") { /* ... */ }
function round(float) { /* ... */ }

class Duration {
  static VALIDATION = /^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/;
  constructor(start, end) { /* ... */ }
  get difference() { /* ... */ }
}

A good rule of thumb that a method should be a util function is that it doesn't use this.
If you feel that the util function is still semantically tied to the class, you should place it as a static method.

This also make the get difference part simpler:

get difference() {
  const { start, end } = this;
  return round((seconds(end) - seconds(start)) / 60);
}

Validation

Constructor is a good place for validation in this case.
If someone passes in some junk instead of valid timestamps, you'd want to catch that immediately as no operation would make sense if the start and end are not valid timestamps.

However, your handling of validation only hides errors under the rug.
Say that someone, by mistake, tries to create a Duration like this:

new Duration("12:00", "1300")

Your validation will successfully test that 12:00 is fine, and assign that to start, but the 1300 will be silently treated as 00:00.
And now my duration is actually (12:00, 00:00), but I wanted (12:00, 13:00).
A silent bug.
No error in console, but my results are weird.
Difficult to debug!

That's why you should throw an exception if the validation fails.
Feel free to scream at me, I want to know that I've messed up.

Here's the idea:

constructor(start, end) {
  if (!Duration.VALIDATION.test(start)) {
    throw new Error(`The "start" argument (${start}) is in wrong format.`)
  } else {
    this.start = start
  }
}

Notice what makes a good error message: it tells me exactly what I've screwed up.
It would be even better if it went on and explained what is a valid argument, but I'll go ahead and assume that it's obvious in this case for brevity.

In order to avoid repeating, you can extract the validation as a method.
The method would act as a no-op function in case everything is OK, but throw if something's wrong.
It's a good candidate for a static method, since it's clearly tightly coupled with the Duration class.

static throwIfError(arg, argName) {
  if (!Duration.VALIDATION.test(arg)) {
    throw new Error(`The "${argName}" argument (${arg}) is in wrong format.`);
  }
  return arg;
}

constructor(start, end) {
  this.start = Duration.throwIfError(start, 'start');
  this.end = Duration.throwIfError(end, 'end');
}

The second argument is just for a prettier message.
Now, when I try my example, new Date("12:00", "1300"), I get an error message saying this:

  Error: The "end" argument (1300) is in wrong format.

Complete code

My full refactored version of the original code is below, along with an example of usage.

function seconds(time = "00:00") {
  const split = time.split(':');
  return Number(split[0]) * 60 + Number(split[1]);
}

// See: https://stackoverflow.com/questions/11832914/round-to-at-most-2-decimal-places-only-if-necessary
function round(float) {
  return Math.round((float + 0.0001) * 100) / 100;
}

class Duration {
  static VALIDATION = /^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/;

  static throwIfError(arg, argName) {
    if (!Duration.VALIDATION.test(arg)) {
      throw new Error(`The "${argName}" argument (${arg}) is in wrong format.`);
    }
    return arg;
  }

  constructor(start, end) {
    this.start = Duration.throwIfError(start, 'start');
    this.end = Duration.throwIfError(end, 'end');
  }

  get difference() {
    const { start, end } = this;
    return round((seconds(end) - seconds(start)) / 60);
  }

}

console.log("From 12:00 to 13:00 => ");
console.log(new Duration("12:00", "13:00").difference);
console.log("From 12:00 to 1300 (!) => ");
console.log(new Duration("12:00", "1300").difference);


Getting valid URL from user input I have a dialog which populates my ConnectionStringData object with the URL of the service that I'm trying to connect to. I then consume that URL with the following property:

    /// <summary>
    /// Where the Service is located.
    /// </summary>
    public string ServiceLocation
    {
        get
        {
            string fullUrl = ConnectionStringData.Location.Trim();

            if (fullUrl.EndsWith("/"))
            {
                fullUrl += "Routes";
            }
            else
            {
                fullUrl += "/Routes";
            }

            Uri uri = null;

            if(Uri.TryCreate(fullUrl, UriKind.Absolute, out uri))
            {
                return uri.ToString();
            }

            throw new UriFormatException(String.Format("{0} is an invalid url", fullUrl));
        }
    }

Should I be doing work like this in the getter? Can I do this better?

This piece:

if (fullUrl.EndsWith("/"))
{
    fullUrl += "Routes";
}
else
{
    fullUrl += "/Routes";
}

can be rewritten to:

fullUrl += fullUrl.EndsWith("/") ? "Routes" : "/Routes";

or:

if (!fullUrl.EndsWith("/"))
    fullUrl += "/";
fullUrl += "Routes";

Since you're using the TryCreate method, there's no need to throw an exception. You just have to notify the caller whether the conversion succeeded or not. Plus, throwing an exception in a get statement is bad practice. This results in following:

if (Uri.TryCreate(fullUrl, UriKind.Absolute, out uri))
    return uri.ToString();
//if no succesful creation:
return String.Empty;

If you do want to throw an exception, use a try/catch block and a regular Uri constructor. This should be placed in a method and not in the get statement then:

try
{
    uri = new Uri(fullUrl, UriKind.Absolute);
    return uri.ToString();
}
catch (UriFormatException ex)
{
    throw;
}

My favor would still be with the TryCreate resulting in following final code:

public string ServiceLocation
{
    get
    {
        var fullUrl = ConnectionStringData.Location.Trim();
        fullUrl += fullUrl.EndsWith("/") ? "Routes" : "/Routes";

        Uri uri = null;

        if (Uri.TryCreate(fullUrl, UriKind.Absolute, out uri))
            return uri.ToString();
        return String.Empty;
    }
}


Recurrence, recursive function Type a number \$n\$. Then calculate \$f(n)\$, where

$$ f(n) =
\begin{cases}
n-10 & \text{if}~ n > 100\\[1.5ex]
f(f(n + 11)) & \text{otherwise}
\end{cases}
$$

The output must show all the computations, such as
$$f(99) = f(f(110)) = f(100) = f(f(111)) = f(101) = 91$$

Is there a shorter and better way I can solve this?

#include <iostream>
using namespace std;

int main(){
    int i=0, n, x, k;

cin>>n;

if(n>100) {
cout<<"f("<<n<<")=";
n=n-10;
}
else {
    cout<<"f("<<n<<")=";
while(n<=100){
loop:
x=0;
k=0;
i++;
n+=11;
        while(x<=i){
            x++;
            cout<<"f(";
        }
        cout<<n;
        while(k<=i){
            k++;
            cout<<")";
        }
        cout<<"=";
    if(n>100){

        x=0;
        k=0;
        i=i-1;
        n=n-10;

        while(x<=i){
            x++;
            cout<<"f(";
        }
        cout<<n;
        while(k<=i){
            k++;
            cout<<")";

        }cout<<"=";
    }

        if(n>100){

        x=0;
        k=0;
        i=i-1;
        n=n-10;

        if(i>=0){

        while(x<=i){
            x++;
            cout<<"f(";
        }
        cout<<n;
        while(k<=i){
            k++;
            cout<<")";

        }cout<<"=";

        goto loop;
    }
         break;
    }
    }
    }cout<<n;
 }

There's no way to tell this in a friendly manner: your indentation is abysmal.

Let's fix that up first. Note that there's plenty left to improve, like giving your operators some space, but I'll leave that as an exercise for yourself. Keep in mind space is cheap and readability is very important in code. If you can add readability by using some extra newlines or space, do it. But only if it helps. Don't throw them needlessly around just for the sake of it.

#include <iostream>
using namespace std;

int main(){
    int i=0, n, x, k;
    cin>>n;
    if(n>100) {
        cout<<"f("<<n<<")=";
        n=n-10;
    }
    else {
        cout<<"f("<<n<<")=";
        while(n<=100){
            loop:
                x=0;
                k=0;
                i++;
                n+=11;
                while(x<=i){
                    x++;
                    cout<<"f(";
                }
                cout<<n;
                while(k<=i){
                    k++;
                    cout<<")";
                }
                cout<<"=";
                if(n>100){
                    x=0;
                    k=0;
                    i=i-1;
                    n=n-10;
                    while(x<=i){
                        x++;
                        cout<<"f(";
                    }
                    cout<<n;
                    while(k<=i){
                        k++;
                        cout<<")";
                }
                cout<<"=";
            }
            if(n>100){
                x=0;
                k=0;
                i=i-1;
                n=n-10;
                if(i>=0){
                    while(x<=i){
                        x++;
                        cout<<"f(";
                    }
                    cout<<n;
                    while(k<=i){
                        k++;
                        cout<<")";
                    }
                    cout<<"=";
                    goto loop;
                }
                break;
            }
        }
    }
    cout<<n;
 }

Much better, but plenty left to improve.

Namespaces

using namespace std; is considered bad practice. Short code is not a requirement in C++, clear code is preferred. It's a thing commonly taught to new C++ programmers because it's 'easier', but it will royally bite you in the behind when conflicts arise.

goto

I'm not going to re-iterate how evil goto is, but please take a look at this well-written answer by 200_success. Basically, one should only use it in C++ if you know exactly what you're doing and there's no better alternative. There usually is a better alternative!

Be consistent!

x++ is usually reserved for for loops. Doing it in while loops indicates you're using the wrong tool for the job. You use x++, x += 1 and x = x + 1 or the negative version of it all over the place. There usually is no reason to write i = i -1, i -= 1 will do.

But the worst of it is:

Your code is simply incomprehensible. It takes too long to understand what it does and all the re-declaring of variables makes it hard to track what's going on. One-letter variables are usually not a good idea, iterators in for loops are an acceptable exception.

You have 6 while loops in your main function. 6!. I'd expect at least 3 different functions to keep this readable, with sensible returns passing the data along and there can't be a good reason to have this many while loops.

So?

Write down what you want to do as a flow-chart. Simplify it as much as possible. Implement that. The result will be half as complicated and twice as readable, at least. What you currently have is the result of a design flaw. I'm afraid a re-design, with this answer in the back of your head, is the only sensible solution here.


Good way to clone an object I have to clone some Entity, then I wrote this piece of code.

public override object Clone()
{
  var CloneUser = base.Clone() as FMSUser;

  CloneUser.Username = this.Username;
  CloneUser.IsEnabled = this.IsEnabled;
  CloneUser.IsNeedPasswordReset = this.IsNeedPasswordReset;
  CloneUser.LastName = this.LastName;
  CloneUser.FirstName = this.FirstName;
  CloneUser.MiddleName = this.MiddleName;
  CloneUser.DistributorID = this.DistributorID;
  CloneUser.IsLocked = this.IsLocked;

  return CloneUser;
}

But then my coworker sent me this code, saying it's better to clone this way, but can't tell me why :

public FMSUser(FMSUser user)
  : EntityBase(user)
{
  this.Username = user.Username;
  this.IsEnabled = user.IsEnabled;
  this.IsNeedPasswordReset = user.IsNeedPasswordReset;
  this.LastName = user.LastName;
  this.FirstName = user.FirstName;
  this.MiddleName = user.MiddleName;
  this.DistributorID = user.DistributorID;
  this.IsLocked = user.IsLocked;
}
public override object Clone()
{
  return new FMSUser(this);
}

Can anynone explain me why the second way is better?

You're not listing your base class, but because of the override and the method's signature in the derived type, I'm assuming something like this:

public abstract class EntityBase : ICloneable
{
   //...
   public abstract object Clone();
}

Here's what MSDN says about ICloneable (emphasis mine):

  An implementation of Clone can perform either a deep copy or a shallow
  copy. In a deep copy, all objects are duplicated; in a shallow copy,
  only the top-level objects are duplicated and the lower levels contain
  references. Because callers of Clone cannot depend on the method
  performing a predictable cloning operation, we recommend that
  ICloneable not be implemented in public APIs.

From that perspective, it's better to consider that "cloning" operation as nothing more than construction of a type - passing this to a constructor, and returning the created instance, sounds like a better plan.

Now, given that a FMSUser can be constructed from another instance, I don't really see a need for the Clone method to even exist at all. On top of it all, it returns an object, and you should avoid exposing System.Object in your public interfaces.

Consider this:

var clone = new FMSUser(existing); // "clone" is FMSUser.

Versus this:

var clone = existing.Clone(); // "clone" is System.Object, requires a cast to get a FMSUser.

Side note:

var CloneUser = base.Clone() as FMSUser;

CloneUser should be named cloneUser, or simply clone. By using PascalCase, you're breaking the naming convention for locals and parameters, which should be camelCase.


Excel Import Data from Different Workbook Dynamic Headers I have this code below that copies data into a master workbook, in sheets "Inputs1", by matching the column headers from the master workbook with the source workbook. If a column header matches, it will copy and paste that column to the related column in the master workbook.

The macro works great for smaller files, but once I run it on larger files it takes almost 2 minutes to run. I also noticed it runs slower when called from a userform instead of from the developer tab directly.

How can I improve this code to run more efficiently and quickly? I still get page flickers too.

Sub Import()

 Application.ScreenUpdating = False

EventState = Application.EnableEvents
Application.EnableEvents = False

CalcState = Application.Calculation
Application.Calculation = xlCalculationManual

PageBreakState = ActiveSheet.DisplayPageBreaks
ActiveSheet.DisplayPageBreaks = False

Dim wbSource As Workbook
Dim wbDest As Workbook
Dim TargetSheet As Worksheet
Dim c As Range
Dim rng As Range
Dim i As Integer
Dim MyRange As Range
Dim SourceSheet As Worksheet
Dim source As String
Dim dest As String
Dim r As Range
Dim msg As String

'Source and destination workbooks defined by cell value

source = Worksheets("Set-Up").Range("B11")
dest = Worksheets("Set-Up").Range("B8")

Set wbSource = Workbooks(source)
Set SourceSheet = wbSource.Worksheets("HFL01 Extract")

Set wbDest = Workbooks(dest)
Set TargetSheet = wbDest.Worksheets("INPUTS1")

With SourceSheet.Range("A1").CurrentRegion
    For Each r In TargetSheet.Range("A1:cc1")
        Set c = .Rows(1).Find(r.Value, , , xlWhole, , 0)
        If Not c Is Nothing Then
            .Columns(c.Column).Copy
            r.PasteSpecial xlPasteValues

        End If
    Next
    Application.CutCopyMode = False
End With

Set fileDialog = Nothing
Set wbSource = Nothing

End Sub

Copying and pasting isn't necessary and interferes with the user's paste cache. Clear the target column of data and then set a range in the target the same size as the source. Then use an assignment statement:

TargetRange.Value = SourceRange.Value


"Up" script for moving up directories quickly A long time ago I created a script for moving up directories very quickly in the command line using the command up. You can find usage notes here.

It's a very simple script with just 8 lines of source code, as follows:

if [ -z "$1" ]; then
    cd ..
else
    for i in `seq 1 $1`;
    do
        cd ..
    done
fi

I've never personally had any problems with it since I made it and started using it myself — but are there any accidental or malicious inputs (particularly with the blind injection of $1) that might cause this to do something bad that I'm not aware of?

Given that the up command isn't used for anything in any command line I'm aware of, I'd like to promote more widespread usage of this script file so that people can type up instead of cd .. all the time, saving three keystrokes (or more if they want to move up more directories) for a very common operation.

This is a decent concept, but by looping on the cd you loose some of the value of the $OLDPWD function in the shell. For example, I often use the special construct cd - in a shell, and that changes directory to the one you were in before.

Your code will make that impossible.

I would instead recommend that you instead build up a chain of ../ string values, like ../../../../ for 4 directories, and then just call cd once, which will preserve the cd - function, and the $OLDPWD.

Additionally, this would be a good feature to include as a function in your code, rather than a script. Bash shell likes functions, and they make life easier.

Finally, if someone supplies a non-number as an argument, it will do odd things.

I played with your code, and came up with:

up () {
    local count=$1
    if [ -z "$count" ]; then
        cd ..
        return
    fi

    test "$count" -eq "$count" || return 1

    local todir=""
    for i in `seq 1 $count`;
    do
      todir="../$todir"
    done
    cd $todir
}

The features of the above code I like are:

it is a function of the shell, so there's no additional script called.
it checks the value is a number, by doing a numeric comparison on the value: test "$count" -eq "$count" (That will throw an error if the inputs are not integers)
it only does a single 'cd', so things like cd - still work.

I would add that to my ~/.bashrc file, or source it in to my current shell.