Image Processing Pipeline I'm currently toying around with some Computer Vision algorithms and always wanted to learn more about templates, so I came up with the idea to build a templated Image Processing Pipeline.

I want the pipeline to be built of several task blocks which I want to plug into each other sequentially. For a specific block, its input and output types are fixed. I ultimately want to be able to inherently control for a block to only accept the next block if the output type of the current is identical to the input type of the next.

         Block_N                 Block_N+1
     _______________         _________________
int |               |  str  |                 | float
--->| str = f1(int) |------>| float = f2(str) |------->
    |_______________|       |_________________|

Consequently, Block_N+2 would have to accept float as input and may have an arbitrary output type.

This is what I was able to come up with:

Abstract base classes:

template<class I>
class AbstractInput {

public:
    virtual ~AbstractInput() { }

    virtual void call( I _input ) = 0;
};

template<class I, class O>
class AbstractPipelineTask : public AbstractInput<I> {

public:
    typedef AbstractInput<O>* Ptr;

    AbstractPipelineTask( AbstractInput<O>* _nextTask = NULL)
        : m_nextTask ( _nextTask ) { }

    virtual void call( I _input ) {

        m_input = _input;
        m_output = executeTask( m_input );

        if ( m_nextTask )
            m_nextTask->call( m_output );
    }

    virtual O executeTask( I& _input ) = 0;

protected:
    AbstractInput<O>* m_nextTask;

    I m_input;
    O m_output;
};

Example implementation:

class Str2IntPipelineTask : public AbstractPipelineTask< std::string, int > {

public:
    Str2IntPipelineTask( AbstractInput<int>* _nextTask = NULL )
        : AbstractPipelineTask( _nextTask ) { }
    virtual ~Str2IntPipelineTask() { }

    virtual int executeTask( std::string& _input ) {

        return atoi( _input.c_str() );
    }
};

class Int2IntPipelineTask : public AbstractPipelineTask< int, int> {

public:
    Int2IntPipelineTask( AbstractInput<int>* _nextTask = NULL )
        : AbstractPipelineTask( _nextTask ) { }
    virtual ~Int2IntPipelineTask() { }

    virtual int executeTask( int& _input ) {

        return ( _input * _input );
    }
};

class Int2FloatPipelineTask : public AbstractPipelineTask< int, float > {

public:
    Int2FloatPipelineTask( AbstractInput<float>* _nextTask = NULL )
        : AbstractPipelineTask( _nextTask ) { }
    virtual ~Int2FloatPipelineTask() { }

    virtual float executeTask( int& _input ) {

        return ( static_cast<float>( _input ) / 42.f );
    }
};

Examples:

int _tmain(int argc, _TCHAR* argv[])
{

    Int2FloatPipelineTask* pipeTask2    = new Int2FloatPipelineTask( );
    Int2IntPipelineTask* pipeTask1      = new Int2IntPipelineTask( pipeTask2 );
    Str2IntPipelineTask* pipeTask0      = new Str2IntPipelineTask( pipeTask1 );

    pipeTask0->call( "42" );

    return 0;
}

What I would really like to know besides some general advice is:

How can I get rid of the raw pointers AbstractInput<O>* m_nextTask? I tried changing the typedef of Ptr to std::shared_ptr<AbstractInput<O>> but that broke the derived classes constructors.
Is it possible to get rid of AbstractInput?

Since none of these classes have private members, they can be structs instead.  As such, you'll no longer need to explicitly give them public inheritance.
Consider making the magic number 42.f a constant so that its intent is known.
This is specific to Visual Studio, thus is non-portable:

int _tmain(int argc, _TCHAR* argv[])

It should be changed to this to fit the standard:

int main(int argc, char* argv[])

In your various executeTask() functions, I believe you're meaning to pass by const&, not just by reference.  You're not modifying the argument, so const is best with them.


3-Sum Problem in Python I attempted the 3-Sum problem on Leetcode, where the problem asks to find all possible triplets of numbers in a given list such that their sum is 0. My code worked, but it exceeded the time limit for 2 of the 313 cases. I believe my solution is in \$O(n^2)\$, but I think there is a line in my code where I sort a triplet within a for-loop, which might change the time complexity.

Here is the problem statement from Leetcode:

  Given an array S of n integers, are there elements a, b, c in S such that a + b + c = 0? Find all unique triplets in the array which gives the sum of zero.

  Note: The solution set must not contain duplicate triplets.

class Solution:
    def threeSumHelper(self, nums, target):
        D = {}
        n = len(nums)
        solutions = []
        for x in range(n):
            complement = target - nums[x]
            if complement in D.values():
                solutions.append([complement, nums[x]])
            D[x] = nums[x]
        return solutions

    def threeSum(self, nums):
        """
        :type nums: List[int]
        :rtype: List[List[int]]
        """
        D = {}
        n = len(nums)
        solutions = {}
        key = 0
        for x in range(n - 2):
            target = -(nums[x])
            pairs = self.threeSumHelper(nums[x+1:n], target)
            if not len(pairs) == 0:
                for pair in pairs:
                    pair.append(nums[x])
                    pair.sort()
                    if not (pair in solutions.values()):
                        if key in solutions:
                            key += 1
                        solutions[key] = pair
        return list(solutions.values())

The algorithm basically iteratively takes one number out of the list of numbers provided and sees whether there are two numbers in the remaining list that sum to the negative of the removed number (so that all three numbers sum to 0). This is done using a dictionary. Then, to avoid repetition, I sort the solutions and store them in a dictionary and return the list of values in the dictionary.

I just want to know if there is any way to make my code faster either by eliminating the need to sort each triplet or otherwise?

Looking at your code, this jumps out at me:

    D = {}

    for x in range(n):

        if complement in D.values():

The D variable here is a dictionary. It is designed for \$O(1)\$ lookup of a key. But you are looping over n items, performing a test (complement in D.values()) that is \$O(m)\$ itself, since D.values() is an iterable and the in test will be a linear search.

I suspect you can get a lot of performance back if you figure out how to not do that: it looks like you are treating D as a list, since you are using x as the key and nums[x] as the value.

I'll point out to you that unlike lists, tuples are immutable and therefore can be hashed, which makes them valid as dict keys. So you can use a dict of tuples. Or a set of tuples.


Get ProductCode from MSI File using DTF in C# I've created this function to grab the MSI ProductCode from the MSI file itself. i couldn't find any native method for it. So I've implemented it the following way:

public static string GetMSIProductCode(string msiFile)
{
    string _result = null;
    string productCodeQuery = "SELECT * FROM Property WHERE Property = 'ProductCode'";
    Session product = Installer.OpenPackage(msiFile, true);
    Database msiDB = product.Database;
    View msiView = msiDB.OpenView(productCodeQuery);
    msiView.Execute();
    Record _record = msiView.Fetch();
    _result = _record.GetString(2);
    msiView.Close();
    msiDB.Dispose();
    return _result;
}

Would you do it any different?

Thanks.

Almost all objects you work with here are disposable so wrapping them with the using would be a good start.
The msiDB variable is not necessary.
It is not necessary now to define the result above the query. Variables should be defined as close to their usage as possible.
We use the _ usually only for private fields not for local variables.
You might consider the implicit type var. It would make your code less verbose.

After applaying this we get:

public static string GetMSIProductCode(string msiFile)
{
    var productCodeQuery = "SELECT * FROM Property WHERE Property = 'ProductCode'";
    using(var product = Installer.OpenPackage(msiFile, true))
    using(var msiView = product.Database.OpenView(productCodeQuery))
    {
        msiView.Execute();
        using (var record = msiView.Fetch())
        {
            return record.GetString(2);
        }
    }
}


For each pair of strings, eliminate the characters they have in common In the below code there are 3 inputs.
First line of input is the number of testcases for which the program will run where (1<=testcases<=10) and i is an int.
For each testcase
Second and third input will be a String where (1<=Stringlength<=10^5)
Output
Now for the output we need to compare both the strings and remove similar characters from both them. The output will be based on which string encounters 0 length first. if none of them is 0 after completion then it will be a draw.
Time of execution is 1.0 sec for each input but my time for 10^5 strings is 2.0. Is there a way to increase its efficiency based on the time.

import java.io.*;
import java.util.*;

public class MyProg{
public static void main(String[] args) throws IOException
{
    BufferedReader br=new BufferedReader(new InputStreamReader(System.in));
    System.out.println("Enter the number of testcases");
    String ts=br.readLine();
    int testcase=Integer.parseInt(ts);
    for(int i=0;i<testcase;i++)
    {
        System.out.println("Enter the 2 strings");
        String sa=br.readLine().replaceAll("\\s+", "");
        String sb=br.readLine().replaceAll("\\s+", "");
        List<Character> s1 = new ArrayList<Character>();
        List<Character> s2 = new ArrayList<Character>();
        for(char c : sa.toCharArray())
            s1.add(c);
        for(char c : sb.toCharArray())
            s2.add(c);
        for(char c:s2)
        {
                s1.remove((Character)c);

        }

        for(char c:sa.toCharArray())
        {
                s2.remove((Character)c);
        }
        if(s1.size()==0 && s2.size()>0)
            System.out.println("You lose some.");
        else if(s1.size()>0 && s2.size()==0)
            System.out.println("You win some.");
        else
            System.out.println("You draw some.");
    }
}}

I hate this code, but it is faster.

You said you are only working with strings made of characters from a-z.
That means 26 characters. The idea is to use 26 counters, each belonging to a character from a-z.

Go through the first(second) string, and every time you find a character c, you increment(decrement) c's counter by 1.

If every counter is zero, that means the strings are permutations of each other, i.e. it's a draw.

Otherwise if every counter is non-negative, you'll know that the first string won; if every counter is non-positive, you'll know that the second string won.

Finally, if some counters have different signs, then it's again a draw.

You can check if all the counters are non-negative like so.
Set a variable allnonneg to true if the first counter is non-negative. Then loop through the remaining counters.

If during the \$i\$-th iteration you find a counter, which is negative, then you change allnonneg to false. Otherwise you keep allnonneg as it is.
If in the end allnonneg is true, then you know that every counter was non-negative. If it's false, then you'll know that there was a counter which was negative. This is because the only way the value of allnonneg can change is if there is a counter which is negative.

To do this manipulation, you need a function f, such that f(allnonneg, charset[i] >= 0) is true precisely when both allnonneg, and charset[i] >= 0 are true. Luckily there's already an operator && which does this, so we get

allnonneg = f(allnonneg, charset[i] >= 0) = allnonneg && (charset[i] >= 0)

which is the same as allnonneg &= (charset[i] >= 0).

private static int decide(String sa, String sb){
    int [] charset = new int[26];

    for(int i = 0; i < sa.length(); charset[sa.charAt(i) - 97] ++, i++);
    for(int i = 0; i < sb.length(); charset[sb.charAt(i) - 97] --, i++);

    boolean allnonneg = charset[i] >= 0;
    boolean allnonpos = charset[i] <= 0;

    for(int i = 1; i < 25; i++) {
        allnonneg &= (charset[i] >= 0);
        allnonpos &= (charset[i] <= 0);
    }

    if(allnonneg && allnonpos)
        return 0; // every counter 0 => draw
    if(allnonneg)
        return 1; // 1st won
    if(allnonpos)
        return 2; // 2nd won
    else
        return 0; // mixed signs => draw
}


Re-worked Workbook_Open, Creating DateStrings So, I had a go at re-writing my Workbook_Open event based on The advice from my previous question. Thoughts?

Private Sub Workbook_Open()

    Dim strTableTitle       As String
    Dim StrTodaysDate       As String
    Dim StrThisDay          As String
    Dim StrThisMonth        As String
    Dim StrThisYear         As String

    Dim strSaveFileFolder   As String
    Dim StrSaveFilename     As String
    Dim StrSaveDate         As String

    Dim SheetDashboard As Worksheet
    Set SheetDashboard = Sheets("Market Dashboard")

        SheetDashboard.Activate
        SheetDashboard.Cells(1, 1).Value = strTableTitle

        StrThisDay = day(Date)
        StrThisMonth = MonthName(month(Date))
        StrThisYear = year(Date)

        StrTodaysDate = StrDay & " " & StrMonth & " " & StrYear
        strTableTitle = "Weekly Market Recap - " & StrDateString

        strSaveFileFolder = "S:\Investments\Regular Reports\Market Insight Reports\Weekly Dashboard\"
        StrDateNumber = StrDay & "." & month(Date) & "." & StrYear
        StrSaveFilename = "Client Facing Dashboard - " & StrDateNumber & ".xlsm"

    Dim lngDay As Long, lngMonth As Long, lngYear As Long
    Dim str3MonthsAgo As String, str6MonthsAgo As String, str9MonthsAgo As String, str12MonthsAgo As String
    Dim strStartDate As String

    strStartDate = "01/" & month(Date) & "/" & year(Date)

        str3MonthsAgo = DateAdd("m", -3, strStartDate)

        str6MonthsAgo = DateAdd("m", -6, strStartDate)

        str9MonthsAgo = DateAdd("m", -9, strStartDate)

        str12MonthsAgo = DateAdd("yyyy", -1, strStartDate)

        '/ Key Rates Headers
        Cells(20, 12) = str3MonthsAgo
        Cells(20, 13) = str6MonthsAgo
        Cells(20, 14) = str9MonthsAgo
        Cells(20, 15) = str12MonthsAgo

        '/ Currencies Headers
        Cells(20, 20) = str3MonthsAgo
        Cells(20, 21) = str6MonthsAgo

        ActiveWorkbook.SaveAs filename:=strSaveFileFolder & StrSaveFilename

End Sub

First thought: It feels really rewarding to see a new User who takes on advice that's offered and proactively uses it to improve.

Second thought: MUCH, MUCH better. Now if you (or somebody else) ever has to come back to this code, it's going to be crystal clear what everything does and how it all works.

One more improvement:
Stop this right now.

Dim strTableTitle       As String
Dim StrTodaysDate       As String
Dim StrThisDay          As String
Dim StrThisMonth        As String
Dim StrThisYear         As String

Dim strSaveFileFolder   As String
Dim StrSaveFilename     As String
Dim StrSaveDate         As String

I  get it, it looks really pretty. I was exactly the same when I started. Here's the thing though good code isn't pretty. Clear, concise, consistent, tidy, but not necessarily pretty. Pretty takes time. Time that could be spent actually improving your code.

Plus, if you ever change any of those variable names, it's going to be a real pain getting it all to line up again, especially if you write one that's longer than the current ones.


Object to object mapping verification - is this extension method useful? In our .NET tests we use NSubstitute & ExpectedObjects.

Testing object expectations involves hand crafting large anonymous objects and when new properties are added we need to go back to these anonymous objects & update them.

Attempting below to get a fluent Object to DTO builder - which fails when a property is missed.

Here is the implementation:

        //CustomerCreatedEvent has all below properties
        var exp1 = @event.ToDto<CustomerCreatedEvent,CustomerDetail>(
            x=> x.AggregateId.As("CustomerId"),
            x => x.Email, // comment this out and test FAILS -> CustomerDetail.Email required
            x => x.FirstName,
            x => x.Surname);

        var exp2 = new {
                CustomerId= @event.AggregateId,
                @event.Email,      // comment this out and test still passes
                @event.FirstName,
                @event.Surname};

        exp1.ToExpectedObject().ShouldMatch(actual);
        exp2.ToExpectedObject().ShouldMatch(actual);

I have 2 questions:

Is my code just adding 'noise'?
Is the implementation code below sound?

public static TResult ToDto(this TSource obj, params Expression>[] items) where TSource : class
    {
        var eo = new ExpandoObject();
        var props = eo as IDictionary;

    foreach (var item in items)
    {
        var member = item.Body as MemberExpression;
        var unary = item.Body as UnaryExpression;
        var body = member ?? (unary != null ? unary.Operand as MemberExpression : null);

        if (member != null && body.Member is PropertyInfo)
        {
            var property = body.Member as PropertyInfo;
            props[property.Name] = obj.GetType()
                .GetProperty(property.Name)
                .GetValue(obj, null);
        }
        else
        {
            var property = unary.Operand as MemberExpression;
            if (property != null)
            {
                props[property.Member.Name] = obj.GetType()
                    .GetProperty(property.Member.Name)
                    .GetValue(obj, null);
            }
            else
            {
                var compiled = item.Compile();
                var output = (KeyValuePair<string, object>)compiled.Invoke(obj);
                props[output.Key] = obj.GetType()
                .GetProperty(output.Value.ToString())
                .GetValue(obj, null);
            }
        }
    }

    TResult result = Activator.CreateInstance<TResult>();
    foreach (var item in props)
    {
        result.GetType().GetProperty(item.Key).SetValue(result, item.Value, null);
    }

    return result;
}

}

Minor nitpick first:

public static T2 ToDto<T1,T2>(this T1 obj, params Expression<Func<T1, dynamic>>[] items) where T1 : class
{

I think putting generic type constraints on a new line makes them easier to spot and adds to readability (especially with long signatures) - consider:

public static T2 ToDto<T1,T2>(this T1 obj, params Expression<Func<T1, dynamic>>[] items)
    where T1 : class
{

I really don't like T1 and T2 for generic type parameter names. T is ok when it's the only parameter, but I'd much, much rather like to see TSource and TResult - it makes it much clearer which one serves which purpose, at a glance.

This also instantly makes it easy to find source a much more meaningful name than obj. I'm all for readable identifiers, so I would like to see props renamed to properties, and eo to expandoObject, or perhaps just value - I will be using these identifiers for the remainder of this post.

I like that you're using var - I don't understand why you're not using it here, since JsonConvert.SerializeObject returns a String:

string json = JsonConvert.SerializeObject(value);  // need json.net

That said, the comment isn't necessary. If you referenced Json.net from a NuGet package, the IDE will tell you when/if a package is missing. If you referenced Json.net from an assembly, the project will fail to build and under "references", Newtonsoft.Json will appear problematic - the comment really doesn't help with anything, it's just noise.

In this block:

if (member != null && body.Member is PropertyInfo)
{
    var property = body.Member as PropertyInfo;
    if (property != null)
        properties[property.Name] = source.GetType().GetProperty(property.Name).GetValue(source, null);
}

The condition if (property != null) will always be true, because body.Member is PropertyInfo is necessarily true in that branch. Hence it can be simplified to this:

if (member != null && body.Member is PropertyInfo)
{
    var property = body.Member as PropertyInfo;
    properties[property.Name] = source.GetType().GetProperty(property.Name).GetValue(source, null);
}

The else branch has a redundant cast - you defined unary as item.Body as UnaryExpression; hence, unary already has the reference you're getting in this superfluous declaration:

var ubody = (UnaryExpression)item.Body;

Just use unary, you don't need ubody at all.

I haven't tested it, but the type parameter in DeserializeAnonymousType<object> seems superfluous as well - Activator.CreateInstance<TResult>() will create a TResult object.. that you can readily return - so instead of this:

var json = JsonConvert.SerializeObject(value);
var anon = JsonConvert.DeserializeAnonymousType<object>(json, Activator.CreateInstance<TResult>());
return ((JObject)anon).ToObject<TResult>();

You'd have that:

var json = JsonConvert.SerializeObject(value);
return JsonConvert.DeserializeAnonymousType(json, Activator.CreateInstance<TResult>());

Notice I'm letting the compiler infer the generic type parameter here.

Lastly, the way chained reflection methods are laid out isn't consistent between the if and the else branch - one has it like this:

properties[property.Name] = source.GetType().GetProperty(property.Name).GetValue(source, null);

The other has it like that:

properties[property.Member.Name] = source.GetType()
    .GetProperty(property.Member.Name)
    .GetValue(source, null);

I like how putting each instruction on a new line makes it easier to see what's going on. Some may argue that it's even easier when the dots line up:

properties[property.Member.Name] = source.GetType()
                                         .GetProperty(property.Member.Name)
                                         .GetValue(source, null);


Rock Paper Scissors in Swift I'm going through the eBook Intro to App Development with Swift by Apple and I am making the Rock, Paper, Scissors app in lesson 20.
This is the assignment that I got:

[...] Name the enum Sign. Add a calculated property to give the emoji that represents the Sign.
[...]
You need to be able to compare two Sign instances to give a GameState.
For example, a player’s .rock and the app’s .paper would give you .lose.
Add an instance method to Sign that takes another Sign, representing the opponent’s turn, as a parameter. The method should return a GameState based on a comparison between self and the opponent’s turn. [...]

The code that I wrote works but I feel like this can be done a lot cleaner. Maybe it's Xcode's indentation that looks a bit weird to me, maybe there's a better solution?
I'll start with GameState.swift (these 4 gamestates were given in the instructions):
import Foundation

enum GameState {
    case start, win, lose, draw
}

And here's Sign.swift:
import Foundation

enum Sign {
    case rock, paper, scissors

    var emoji: String {
        switch self {
        case .rock: return ""
        case .paper: return "✋"
        case .scissors: return "✌"
        }
    }

    func beats(otherSign: Sign) -> GameState {
        switch self {
        case .rock: do {
            if otherSign == .paper {
                return .lose
            } else if otherSign == .scissors {
                return .win
            }
            return .draw
            }
        case .scissors: do {
            if otherSign == .rock {
                return .lose
            } else if otherSign == .paper {
                return .win
            }
            return .draw
            }
        case .paper: do {
            if otherSign == .rock {
                return .win
            } else if otherSign == .scissors {
                return .lose
            }
            return .draw
            }
        }
    }
}

First, the do { ... } blocks inside the cases are not needed:
func beats(otherSign: Sign) -> GameState {
    switch self {
    case .rock:
        if otherSign == .paper {
            return .lose
        } else if otherSign == .scissors {
            return .win
        }
        return .draw
    case .scissors:

    // ...
}

But why use a switch-statement for self and an if-statement for otherSign? The    if-statement is error-prone (we might e.g. forget a case), whereas the switch-statement perfectly matches the enum declaration and the compiler checks the exhaustiveness, i.e. that all cases are treated exactly once:
func beats(otherSign: Sign) -> GameState {
    switch self {
    case .rock:
        switch otherSign {
        case .paper:    return .lose
        case .scissors: return .win
        case .rock:     return .draw
        }
    case .scissors:

    // ...
}

This is already more compact than the original code, but still clear and easy to read.
Now we go one step further: A single switch-statement is sufficient because you can switch on a tuple in Swift:
func beats(otherSign: Sign) -> GameState {
    switch (self, otherSign) {
    case (.rock, .rock):     return .draw
    case (.rock, .paper):    return .lose
    case (.rock, .scissors): return .win

    case (.paper, .rock):     return .win
    case (.paper, .paper):    return .draw
    case (.paper, .scissors): return .lose

    case (.scissors, .rock):     return .lose
    case (.scissors, .paper):    return .win
    case (.scissors, .scissors): return .draw
    }
}

It is clear to the reader that (and how) all possible combinations are handled. Repeated or missing cases are detected by the compiler.
There is also another, completely different option. We can assign the integer values 0, 1, 2 to rock, paper, scissors, respectively, e.g. by making it an integer based enumeration:
enum Sign: Int {
    case rock, paper, scissors

    // ...
}

then the outcome of the comparison can efficiently be computed from the difference of the integer values:
func beats(otherSign: Sign) -> GameState {
    let diff = self.rawValue - otherSign.rawValue

    // ...
}

I'll leave it to you to work out the details (hint: modulo arithmetic), and to decide which variant your prefer: A (verbose) switch-statement with 9 cases, or a compact mathematical calculation.


perl6: html color gradient generator Edit: The comments have been incorporated to this module in GitHub. The final code has been included at the end of this thread. Thanks very much for the detailed and brilliant suggestions.

Inspired in this color gradient generator, this is a gradient generator in perl6. I wonder if the code could be simplified or improved. It converts the hex colour to base10, calculates the gradient colours, and then converts the colour to base16.
my $initial_color = '#FF0000';
my $final_color   = '#00FF00';
my $gradient      = 10;

my @initial   = ($initial_color ~~ /\#(..)(..)(..)/).list.map: { .Str.parse-base(16) };
my @final     = ($final_color   ~~ /\#(..)(..)(..)/).list.map: { .Str.parse-base(16) };
my @range     = @final Z- @initial;
my @increment = @range.map: { $_ / $gradient };

my @color;
@color.push: @initial;
for (1..$gradient) -> $i {
  @color.push: @(@color[$i-1]) Z+ @increment;
}

for (0..$gradient) -> $i {
  @color[$i] = '#' ~ ( @(@color[$i]).map: { .base(16,0).fmt('%02s') } ).join;
}

my $fh = open '/tmp/delete.html', :w;
for (0..$gradient) -> $i {
  $fh.print: "&nbsp;";
}
$fh.print: '&nbsp';
$fh.close;

run <elinks -dump -dump-color-mode 3 /tmp/borrem.html>;

I'm going to limit my initial answer to not include modules, but I'll also cover a module that can help out some here too later.

my $initial_color = '#FF0000';
my $final_color   = '#00FF00';

While there's nothing wrong with using underscores, the tendency in Perl 6 has been to move towards hyphens, which would give us $initial-color and $final-color.

my @initial   = ($initial_color ~~ /\#(..)(..)(..)/).list.map: { .Str.parse-base(16) };

This is a bit of personal preference, I'd write this as

my @initial = .map: *.Str.parse-base(16) with $initial-color ~~ /\#(..)(..)(..)/;

Since it allows you to get rid of most of the parentheses/braces.  Note that in any case, you don't need the .list because your Match is already an Iterable.  But this is of course a pure matter of taste (some might prefer parentheses around the .map to emphasize it's separate from the with postfix.

You can also prestore the regex, particularly if you plan on using it more often:

my $hex-format = /\#(..)(..)(..)/;
my @initial = .map: *.Str.parse-base(16) with $initial-color ~~ $hex-format;

Your next two lines have two different techniques for applying math operators across lists:

my @range     = @final Z- @initial;
my @increment = @range.map: { $_ / $gradient };

You could use a zip operator for both ( @range Z/ ($gradient xx 3) ), or you could opt for a hyper meta operator:

my @range     = @final «-» @initial;
my @increment = @range «/» $gradient

Now for generating the colors themselves, you had

my @color;
@color.push: @initial;
for (1..$gradient) -> $i {
  @color.push: @(@color[$i-1]) Z+ @increment;
}

for (0..$gradient) -> $i {
  @color[$i] = '#' ~ ( @(@color[$i]).map: { .base(16,0).fmt('%02s') } ).join;
}

First the easy things.  If I see a variable @color, I assume it is a single entity with multiple constituent entities (one color, three values — R, G, B).  But in this case, it's a list of colors, so the variable name is best used in plural.  Also, you don't need parentheses around the list for the for loop.  Just for 0..$gradient -> $i works.

Your first for loop uses the previous item to determine the next one, so you have a push outside the loop.  There's a few ways to solve this.  If you have to do an initial step like that, I like to put it in the loop using FIRST (or once, both are functionally equivalent here).

my @color;
for 1..$gradient -> $i {
  FIRST @color.push(@initial);
  @color.push: @(@color[$i-1]) Z+ @increment;
}

But we can actually rewrite this to calculate the value without relying upon the previous entry:

my @color;
for 0..$gradient -> $step {
  @color.push: @initial «+» @increment «*» $step;
}

This could be even further reduced into a one liner, but I don't think that's necessary nor desirable here.  What's great here about the hyper operators is that they read like scalar arithmetic ($initial + $increment * $step) so readability is maintained.

Your second loop can also be simplified quite a bit by just looping on the values.  I think it makes most sense to just put them into a new array

my @html-colors;
for @colors -> @color {
  @html-colors.push: '#' ~ @color.map( *.base(16,0).fmt: '%02s' ).join;
}

Anytime, though, you have a .map that has another method chain in its code block, you have a candidate for a hyper method call ».

my @html-colors;
for @colors -> @color {
  @html-colors.push: '#' ~ @color».base(16,0)».fmt('%02s').join;
}

Basically, ».foo means take every element and .foo it, and it returns a list of the values, which can be ».bar on every element.  Note that .join doesn't have the hyperoperator, because we want to act on the resulting list.

But you could also loop in a rw manner (notice the <->) with

for @colors <-> @color {
  @colors = '#' ~ @color».base(16,0)».fmt('%02s').join;
}

Because now both loops though are 0 .. $gradient, it might make sense to merge them:

my @colors
for 0..$gradient -> $step {
  my @intermediate = @initial »+» @increment »*» $step;
  @colors.push: '#' ~ @intermediate».base(16,0)».fmt('%02s').join;
}

Lastly when you print, remember you can just loop on the color items:

for @colors -> $color {
  $fh.print: "&nbsp;";
}

Which also allows a one liner using the topic variable $_ (and in this case, absent doing anything else, makes more sense to me):

$fh.print: "&nbsp;" for @colors;

Since it's effectively the same loop, this is, of course, a candidate for putting back in the main loop, but there's something to be said for separating the calculations from the final output.

Rewritten, my version would be:

my $initial-color = '#FF0000';
my $final-color   = '#00FF00';
my $gradient      = 10;

my $hex-format = /\#(..)(..)(..)/;

my @initial = .map: *.Str.parse-base(16) with $initial-color ~~ $hex-format;
my @final   = .map: *.Str.parse-base(16) with   $final-color ~~ $hex-format;

my @increment = (@final »-» @initial) »/» $gradient;

my @colors;

for 0..$gradient -> $step {
  my @intermediate = @initial »+» @increment »*» $step;
  @colors.push: '#' ~ @intermediate».base(16,0)».fmt('%02s').join;
}

my $fh = open '/tmp/delete.html', :w;
$fh.print: "&nbsp;" for @colors;
$fh.print: '&nbsp';
$fh.close;


Compare two doubles to N decimal places I have a function that compares two doubles to N decimal places.

#include <cmath>
#include <iostream>

using namespace std;

enum class RelType : int { Less, Equal, Greater };

RelType _compare(double v1, double v2, int multiplier)
{
    if (std::round(v1 * multiplier) == std::round(v2 * multiplier))
        return RelType::Equal;
    if (v1 < v2)
        return RelType::Less;
    return RelType::Greater;
}

template<int N> RelType compare(double, double) { static_assert(false, "Not implemented."); }
template<> RelType compare<5>(double v1, double v2) { return _compare(v1, v2, 100000); }

int main(int argc, char* argv[])
{
    double v1 = 1.0;                        // 1.0000000000000000
    double v2 = (1.0 / 13.67) * 13.67;      // 0.99999999999999989

    cout << std::boolalpha;
    cout << (v1 == v2) << endl;                               // false (not what we expected)
    cout << (compare<5>(v1, v2)==RelType::Equal) << endl;     // true (correct behaviour)
    cout << (v1 > v2) << endl;                                // true (not what we expected)
    cout << (compare<5>(v1, v2)==RelType::Greater) << endl;   // false (correct behaviour)

    return 0;
}

My main two questions are about the efficiency and correctness of the _compare helper function. Are there any pitfalls to consider that might not be obvious at first glance?

The main problem I have with your code is with these templates:

template<int N> RelType compare(double, double) { static_assert(false, "Not implemented."); }
template<> RelType compare<5>(double v1, double v2) { return _compare(v1, v2, 100000); }

They are misleading. The 5 in the second function should be supposed to denote the number of decimal places to be compared, however, it is not implemented to do so, being only used as name decoration. E.g.:

// Comparing 5 decimal places:
cout << (compare<5>(v1, v2) == RelType::Equal) << endl;

// Also comparing 5 decimal places:
// (In actuality, a shady compile error due
// to the static_assert. But we can do better...)
cout << (compare<8>(v1, v2) == RelType::Equal) << endl;

I see no point in having a parameter if it always has to have the same value, so one improvement to the interface would be just getting rid of the templates and having a single function that always compares with 5 decimal places and no other option.

Another possibility, if you need to have the ability to configure the number of decimal palaces at compile time, would be actually using the N parameter to compute the multiplier, instead of hardcoding a value. You can do so with a compile-time power function:

template<int N> RelType compare(double v1, double v1)
{
    constexpr auto multiplier = compile_time_pow(10, N);
    // ... the rest ...
}

Note that in the above link C++11 is utilized, though it is probably possible to achieve the same without constexpr.


Write to a new, modified Excel file from an original one I noticed that I've a HUGE bottleneck in this following function of mine. I can't see how to make it faster.

These are the profiling test results(keep in mind that I'm using a PyQt GUI so times can be stretched):

cProfile Results

def write_workbook_to_file(self, dstfilename):
    self.populaterownumstodelete()
    # actual column in the new file
    col_write = 0
    # actual row in the new file
    row_write = 0
    for row in (rows for rows in range(self.sheet.nrows) if rows not in self.row_nums_to_delete):
        for col in (cols for cols in range(self.sheet.ncols) if cols not in self.col_indexes_to_delete):
            self.wb_sheet.write(row_write, col_write, self.parseandgetcellvalue(row, col))
            col_write += 1
        row_write += 1
        col_write = 0

I ran a cProfile profiling test and write_workbook_to_file() resulted the slowest function in all my application. parseandgetcellvalue() isn't a problem at all.

To make it faster you could move this

(cols for cols in range(self.sheet.ncols) if cols not in self.col_indexes_to_delete)

out of the loop, and use enumerate instead of explicitly incrementing variables.

Revised code:

def write_workbook_to_file(self, dstfilename):
    self.populaterownumstodelete()
    rows = [row for row in xrange(self.sheet.nrows) if row not in self.row_nums_to_delete]
    cols = [col for col in xrange(self.sheet.ncols) if col not in self.col_indexes_to_delete]
    for row_write, row in enumerate(rows):
        for col_write, col in enumerate(cols):
            self.wb_sheet.write(row_write, col_write, self.parseandgetcellvalue(row, col))

In fact you could even move enumerate out of the loop, but I like how the code looks quite clean now.


Checking digits in a number I'm working on an exercise that counts the number of even digits, odd digits, or zeros in a user-input integer (I made it a long so the user could put in more numbers). However I have a bunch of repetitive code when I'm building my output, especially about the pluralization of "[s]". Really, the entire thing feels redundant. How can I factor or simplify the code to be cleaner and most efficient? This isn't homework or anything, but I'm trying to do stuff out of the book to get more practice.

/**************************************************************************************************
 *  Program Name:     Programming Project 4.5: Digit Checks in a Number
 *  Author:           Terry Weiss
 *  Date Written:     September 26, 2015
 *  Program Description:
 *     Design and implement an application that determines and prints the number of odd, even
 *  and zero digits in an integer value read from the keyboard.
 **************************************************************************************************/

import java.util.Scanner;

public class PP4_5
{

    public static void main( String[] args )
    {
        long number;
        boolean validInput = false;
        String numStr;
        Scanner input = new Scanner(System.in);

        do
        {
            System.out.print("Please enter an integer:  ");
            numStr = input.next();

            try {
                number = Long.parseLong(numStr);
                validInput = true;
            } catch (NumberFormatException e) {
                System.out.print("That's not an integer. ");
                number = 0;
            }
        } while (!validInput);

        // Break each digit up by dividing by powers of 10.
        int evens = 0, odds = 0, zeros = 0;
        for (long temp = number; temp > 0; temp /= 10)
        {
            int digit = (int)(temp % 10);

            if (digit == 0)
            {
                zeros++;
                System.out.println(digit + " is a zero digit.");
            }
            else if (digit % 2 == 0)
            {
                evens++;
                System.out.println(digit + " is an even digit.");
            }
            else
            {
                odds++;
                System.out.println(digit + " is an odd digit.");
            }
        }

        String evenStr = " even " + ((evens == 1) ? "digit" : "digits");
        String oddStr = " odd " + ((odds == 1) ? "digit" : "digits");
        String zeroStr = (zeros == 1) ? " zero" : " zeros";

        System.out.println(number + " has " + evens + evenStr + ", " + odds + oddStr + ", and "
                + zeros + zeroStr + ".");
    }

}

Really, the entire thing feels redundant

Well, it all comes down to the choice of programming language. With Java you chose predictability and regularity not power nor shortness. But I can still suggest improvements over this.

Your code does 3 things:

Take an integer input
Count evens, odds and zeros in its digits

2.1 Requires taking the digits of a number
Output this result nicely

3.1 Requires pluralization handling

So you should write three big methods and two smaller helper ones. This will not reduce size but will really increase readibility.

I started the re-factoring by extracting the input method, to show you what I mean in practice:

public class num
{
    public static long integerInput(String prompt) {
        long number;
        boolean validInput = false;
        Scanner input = new Scanner(System.in);

        do
        {
            System.out.print("Please enter an integer:  ");
            String numStr = input.next();

            try {
                number = Long.parseLong(numStr);
                validInput = true;
            } catch (NumberFormatException e) {
                System.out.print("That's not an integer. ");
                number = 0;
            }
        } while (!validInput);
        return number;

    }

    public static void main( String[] args )
    {

        long number = integerInput("The number? ");
        // Break each digit up by dividing by powers of 10.
        int evens = 0, odds = 0, zeros = 0;
        for (long temp = number; temp > 0; temp /= 10)
        {
            int digit = (int)(temp % 10);

            if (digit == 0)
            {
                zeros++;
                System.out.println(digit + " is a zero digit.");
            }
            else if (digit % 2 == 0)
            {
                evens++;
                System.out.println(digit + " is an even digit.");
            }
            else
            {
                odds++;
                System.out.println(digit + " is an odd digit.");
            }
        }

        String evenStr = " even " + ((evens == 1) ? "digit" : "digits");
        String oddStr = " odd " + ((odds == 1) ? "digit" : "digits");
        String zeroStr = (zeros == 1) ? " zero" : " zeros";

        System.out.println(number + " has " + evens + evenStr + ", " + odds + oddStr + ", and "
                + zeros + zeroStr + ".");
    }

}

The remaining 2 / 3 are up to you, good luck :)


Parallelizing upload I needed smth that could send (scp/rsync) many files in parallel but that would not overload machine, firewalls on the way by starting e.g. 600 simultaneous uploads, so upload should be done in batches.

Most of the utilities like aria2c are download managers and I needed something to send many files in parallel from a machine behind NAT to a machine on the internet (so no dl from internet possible).

Pls keep in mind that it's the draft that I quote below, I do not need details on general good practices such as setting paths and counters using variables and not literal values.

Any better approach? Problems? I've noticed that I have to sleep 1 between starting scp cmds in background or else target server refuses some connections.

#!/bin/bash

FLIST=file_paths_list.txt

COUNTER=0
PIDS=()

cat ${FLIST} | while read F; do
                COUNTER=$((COUNTER + 1))
                echo COUNTER $COUNTER
                sleep 1
                scp ${F} root@host:/data/tmp &
                PID=$!
                echo Adding PID $PID
                PIDS+=($PID)
                if [ $COUNTER -lt 20 ]; then
                        continue
                fi
                # wait for uploads batch to complete
                for PID in ${PIDS[@]}; do
                        echo waiting for PID $PID
                        wait $PID
                done
                # reset PIDS array
                PIDS=()
                COUNTER=0
done

It is very, very unlikely that running multiple files in parallel will go any faster than running them all sequentially (your bottleneck will be 'internet' bandwidth regardless).

Is there something wrong with doing something like:

#!/bin/bash

SCPCMD="scp \"\$@\" root@host:/data/tmp"
# echo The scp command is: $SCPCMD

cat ${FLIST} | tr '\n' '\000' | xargs --null bash -c "${SCPCMD}"

The above script will convert newlines to null characters in the input list, then copy the files to the server.

You can add the -n 10 argument to xargs to do 10 files are a time, or whatever works for you. I like the -t argument as well which echos the xarg command to stderr before it runs it.

The above will copy only one stream at a time, but, it will do the files in bulk operations, and you will likely be limited by your network bandwidth, not the parallelism of your copies.

EDIT:

If you want to run the scp's in parallel, you can add the -P xx (--max-procs)argument to xargs, which will run as many as xx scp's in parallel for you. So, for example, if you have hundreds of files, you can scp them in 5 parallel streams, 5 files at a time, with:

cat ${FLIST} | tr '\n' '\000' | xargs --null -P 5 -n 5 bash -c "${SCPCMD}"


Authorization - Create and unlock a User I just wrote this, but I am not realy happy with that. I think there is an easier way to do this. First I am checking if a user has an ID (from a portal we are using, it is in the header). After that I have to check if the user exists in my application, if not a user will be created, if the user exists, the datetime lastlogon will be updated.
(here is my first problem, CheckIfUserExists always returns true, except on an Exception, is this ok?). After that I am checking if userExists is true. If a user is created it has to be unblocked by an admin. If this happened the user can access the page.
If the user is in Role Admin, Viewbag is true otherwise false. If userExists is false, he is getting the default error.

I think my chain of thought is realy complicated.

public ActionResult Index()
{
    bool userExists = false;
    if (HelperClass.UserId != "")
        userExists = CheckIfUserExists(HelperClass.UserId);
    else
        return RedirectToAction("AccessDenied", "Error");

    if (userExists)
    {
        bool unblocked = CheckIfUnblocked(HelperClass.UserId);
        if (unblocked)
        {
            if (User.IsInRole("Admin"))
                ViewBag.Admin = true;
            else
                ViewBag.Admin = false;
            return View("Index");
        }
        return RedirectToAction("AccessDenied", "Error");
    }
    else
    {
        return RedirectToAction("Index", "Error");
    }
}

EDIT: Adding Code

public bool CheckIfUserExists(string u_gvid)
{
    try
    {
        using (NpgsqlConnection con = new NpgsqlConnection(_Connection))
        {
            con.Open();
            using (NpgsqlCommand cmd = new NpgsqlCommand())
            {
                cmd.Connection = con;
                //qry check if user exists
                var result = cmd.ExecuteScalar().ToString().ToLower();
                if (result == "false")
                {
                    return CreateUser(HelperClass.UserId, HelperClass.Name);
                }
                else
                {
                    //Update lastlogonn time
                    //qry
                    cmd.ExecuteScalar();
                    return true;
                }
            }
        }
    }
    catch (NpgsqlException ex)
    {
        //log
        RedirectToAction("DatabaseError", "Error");
    }
    catch (Exception ex)
    {
        //log
        RedirectToAction("Index", "Error");
    }
    return false;
}

public bool CheckIfUnblocked(string u_gvid)
{
    try
    {
        using (NpgsqlConnection con = new NpgsqlConnection(_Connection))
        {
            con.Open();
            using (NpgsqlCommand cmd = new NpgsqlCommand())
            {
                //qry "SELECT unblocked FROM tbl.users WHERE uid = @uid";
                var result = (bool)cmd.ExecuteScalar();

                if (result)
                    return true;
                else
                    return false;
            }
        }
    }
    catch (NpgsqlException ex)
    {
        //log
        RedirectToAction("DatabaseError", "Error");
    }
    catch (Exception ex)
    {
        //log
        RedirectToAction("Index", "Error");
    }
    return false;
}

Some quick remarks:

Don't name things "class", e.g. HelperClass.
Don't use Hungarian notation and give your variables meaningful names: u_gvid is a complete mystery to me.
I don't get why CheckIfUserExists and CheckIfUnblocked are public, and even less why they'd use RedirectToAction. I expect these methods to return a boolean, not to execute redirects etc.
NpgsqlConnection and NpgsqlCommand don't follow the Microsoft naming guidelines: they are compound words, so the "s" in "sql" should be capitalized.
Don't write ADO.NET code. You're using 15+ lines to write something that could be expressed much simpler by using Dapper. The var result = cmd.ExecuteScalar().ToString().ToLower(); and if (result == "false") bit is even worse. You don't provide us with the relevant SQL, so perhaps there is a good reason to return a boolean-like string, but I wouldn't be surprised if the query logic could be much simplified.
CheckIfUserExists and CheckIfUnblocked are IMHO bad method names. To me they should be something like DoesUserExist and IsUserUnblocked, which are the kind of methods I'd expect to return a boolean.
CheckIfUserExists should not execute CreateUser, unless you rename the method. And I wouldn't expect a method called CreateUser to return a boolean. Avoid this temptation by moving these methods to a service etc. instead of them being part of the controller. Keep your controllers as light as possible, things like data processing etc. should be handled by dedicated classes.
In CheckIfUnblocked you have a boolean: var result = (bool)cmd.ExecuteScalar();. Why then not simply return this instead of doing the whole if (result) return true; / else return false; dance? You spend six lines on something that could be expressed by a single one.


Secure session handling in PHP I tried to search and read for 10-12 hours on how to have a secure session, and this is the (simplified version of the) code that I came up with. (found no good book or article with complete guide about PHP Sessions, each had something and missed something else)

Could you kindly please check if I have taken the necessary steps or if I've made a mistake somewhere?

Additional Info: PHP 7.3.x will be used, Webserver is nginx on Ubuntu 18.04, will have a maximum of ~500 visitors per second, usually ~50-100, sessions are written on disk (default of PHP)

function secure_session_start($domain, $ip, $useragent)
{
    // Change PHPSESSID for better security, remove this if set in php.ini
    session_name('app_session');

    // Secure session_set_cookie_params
    session_set_cookie_params(0, '/', $domain, true, true);

    // Don't show any output if session_start fails, die immediately (will add log_error later)
    @session_start() or die();

    // Keep session alive (as suggested by comment on https://www.php.net/manual/en/function.session-start.php#121209)
    $_SESSION['time'] = time();

    // Hash Useragent to be safe from storing malicious useragent text as session data on server
    $useragent_hash = hash('sha256', $useragent);

    // Make sure we have a canary set
    if ( ! isset($_SESSION['canary']))
    {
        // Regenerate & delete old session as well
        session_regenerate_id(true);

        $_SESSION['canary'] = [
            'birth' => time(),
            'ip' => $ip,
            'useragent_hash' => $useragent_hash
        ];
    }

    // Regenerate session ID every 5 minutes:
    if ($_SESSION['canary']['birth'] < time() - 300)
    {
        // Regenerate & delete old session as well
        session_regenerate_id(true);
        $_SESSION['canary']['birth'] = time();
    }

    // If user is logged in, log out user if IP or Useragent is changed (this is intentional, I know users behind load-balancers etc will have issues)
    if (isset($_SESSION['username']) && ($_SESSION['canary']['ip'] !== $ip OR $_SESSION['canary']['useragent_hash'] !== $useragent_hash))
    {
        // Destroy cookie
        setcookie (session_name(), "", time() - 3600, '/', $domain, true, true);

        // Destroy session
        session_unset();
        session_destroy();

        // Redirect (avoid loop by checcking ip_browser_changed)
        if( ! isset($_GET['ip_browser_changed']))
        {
            header('Location: '.URL.'login/?ip_browser_changed');
            exit('IP Address or Browser has been changed, please login again!');
        }
    }
}

Then I will simply do secure_session_start() in my code after validating user IP Address with filter_var($ip, FILTER_VALIDATE_IP)

Edit: I've always relied on PHP Frameworks and their Auth library to handle the sessions etc for me and never worried about them, this time because of too many visitors, I had to skip using frameworks.

I have to make sure users are kept logged in with secure sessions as long as their Browser or IP hasn't changed. Now I'm trying to learn how to securely add the Remember Me feature, just needed to make sure I'm on the right track (with your help)

As an overarching rule, I never write scripts with the "stfu operator" (@). It looks like you plan to refine the session starting line, so I won't dwell.
You are calling time() 5 separate times in your function.  Because there is no benefit in recording their differences in terms of microseconds, I recommend that you call time() once, and cache the value in a variable/constant to be used in all processes in the custom function.
I tend to never use OR or AND in my php conditions (only in my SQL) as a matter of consistency.  This also prevents unintended hiccups regarding precedence.  Separately, I never use or die() in my scripts ...trying not to dwell.
Because you are generating a hash for non-cryptographic use, you may enjoy the advice/discussions about performance comparisons between different hash generators: Fastest hash for non-cryptographic uses?


Show with delay directive I'm pretty new to AngularJS and have made my first directive. Is this a good or a bad way to do this?  It works, but can't figure out if this is clever or not.

.directive('showWithDelay', function($timeout){
    return {
        restrict: 'A',
        link: function($scope, $element, attrs){
            $element.addClass("ng-hide");
            $timeout(function() {
                $element.removeClass("ng-hide");
            },attrs.showWithDelay);
        }
    }
})

Usage in template

CLICK ME

button will be displayed after 6 sec.

Looks fine to me, I would add some small changes:

If you create a directive/service/controller/.. in Angular it's a best practice to create them with the [ ] notation to make sure minifying your code won't break:

.directive('name', [ '$timeout', function ($timeout) {

} ] );

In your example a minifier would change the $timeout variable to something as 'a' which will result in an error because 'a' is not registered and thereby not known by Angular's dependency injection system.

In my example if a minifier would change $timeout it doesn't matter at all since a minifier doesn't change string values. A rule of thumb, always ngminify your code before jsminify.

And as last I would add a default value for the timeout function:

 $timeout(function () {
     $element.removeClass("ng-hide");
 }, attrs.showWithDelay || 5000);

This way if someone does not provide an attribute it will take the default value of 5000.


Closing InputStream of a running process Let's say you want to extract the output of a console application till you've found a certain keyword. If you have found the keyword the started console process still should be running, however the function which has found the keyword should return.

Here is a minimal application which simply creates an output of n lines (n is the first argument).

LineOutput.java

public class LineOutput {
    public static void main(String[] args) {
        int iterations = 10;
        if (args.length > 0) {
            iterations = Integer.valueOf(args[0]);
        }
        for (int i=0; i < iterations; i++) {
            System.out.println("Line " + i);
        }
    }
}

And here is the main application with the readStreamUntilFound method, which returns as soon as the keyword was found:

ReadTest.java

package org.test;

import java.io.BufferedInputStream;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Arrays;
import java.util.List;

public class ReadTest {
    public static void main(String[] args) {
        System.out.println("Result " + readStreamUntilFound(Arrays.asList("java", "-jar", "test.jar", "10000"), "Line 99"));
    }

    public static boolean readStreamUntilFound(List<String> cmd, String keyword) {
        boolean keywordFound = false;
        ProcessBuilder pb = new ProcessBuilder();
        pb.command(cmd);
        pb.redirectErrorStream(true);
        Process p;
        try {
            p = pb.start();

            try {
                BufferedInputStream bis = new BufferedInputStream(p.getInputStream());
                BufferedReader r = new BufferedReader(
                        new InputStreamReader(bis));

                String line;
                while ((line = r.readLine()) != null) {
                    System.out.println(line);
                    if (line.equals(keyword)) {
                        keywordFound = true;
                        break;
                    }
                }
            } finally {
                p.getInputStream().close();
            }
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

        return keywordFound;
    }
}

I know that the InputStream of a started process must be read, otherwise the process might be blocked. Therefore I am wondering if it is problematic to close the InputStream although the console application still is running (especially in the case where a Java Process object is kept as reference).

To add to @ChrisWue's answer I would pull the Process away from the actual processing of it's data. IE: Make a separate class that takes a stream and does processing. Think about it this way: what is your code doing? It is taking data off of some InputStream and checking for a specific keyword. Does that processing code need a Process instance to do that? Nope! You could pass your code any InputStream and it would be able to look for a keyword. Making sure your methods are only given exactly what it needs dramatically increases testability, flexibility, and re-usability.

For the second part actually using the Process' Input/Output/Error streams. If you read the JavaDocs you will find this:

  By default, the created subprocess does not have its own terminal or
  console. All its standard I/O (i.e. stdin, stdout, stderr) operations
  will be redirected to the parent process, where they can be accessed
  via the streams obtained using the methods getOutputStream(),
  getInputStream(), and getErrorStream(). The parent process uses these
  streams to feed input to and get output from the subprocess. Because
  some native platforms only provide limited buffer size for standard
  input and output streams, failure to promptly write the input stream
  or read the output stream of the subprocess may cause the subprocess
  to block, or even deadlock.

Basically, what this is telling me is that you need to make sure you are managing all of these streams appropriately. If you close one of the processes streams prematurely you have to be sure that the Process can handle that case. Most applications will crash or exit with an error code. Unless you know you won't break the Process you're trying to run be sure to process all the data from these streams until that process is done. You could use a simple thread which just reads and discards the data if you don't need it.

Just a thought but if you are controlling the application you are running as a Process you could possibly tell the application to close gracefully by sending it something using the OutputStream.


Counter with increment and decrement buttons For my first Kotlin project, I'm implementing the Redux pattern with simple Increment and Decrement buttons and a text view to display the current value.

My main questions have to do with Kotlin and Android idioms and how I structured my code. Is the below a radical departure to how Kotlin is normally written? For example the when statement... Would you put an else clause in it instead of the return at the bottom of the function?

Does it look right to override the onStart and onStop or would it make sense to move the store.subscribe code into the onCreate? If I did that, would the GC successfully collect the activity and the text field? (The GC in general has me nervous.) Is it weird to be using so many function objects or is that acceptable in Kotlin? Any constructive criticism is also wanted.

package com.myapplication

import android.support.v7.app.AppCompatActivity
import android.os.Bundle
import android.view.View
import android.widget.Button
import android.widget.TextView

class MainActivity : AppCompatActivity() {

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(R.layout.activity_main)

        textView = findViewById(R.id.textView) as TextView

        val incrementButton = findViewById(R.id.increment_button) as Button
        incrementButton.setOnClickListener { store.dispatch(Action.INCREMENT) }

        val decrementButton = findViewById(R.id.decrement_button) as Button
        decrementButton.setOnClickListener { store.dispatch(Action.DECREMENT) }

    }

    override fun onStart() {
        super.onStart()
        unsubscriber = store.subscribe { state ->
            textView?.text = state.count.toString()
        }
    }

    override fun onStop() {
        unsubscriber?.invoke()
        super.onStop()
    }

    private var unsubscriber: (() -> Unit)? = null
    private var textView: TextView? = null
}

package com.myapplication

import com.redux.Store

enum class Action { INCREMENT, DECREMENT }

data class State(val count: Int = 0) { }

fun reducer(action: Action, state: State): State {
    when (action) {
        Action.INCREMENT -> return state.copy(count = state.count + 1)
        Action.DECREMENT -> return state.copy(count = state.count - 1)
    }
    return state
}

val store = Store<Action, State>(State(), ::reducer)

package com.redux

class Store<Action, State>(initialState: State, reducer: (action: Action, state: State) -> State) {

    fun dispatch(action: Action) {
        if (dispatching) { throw Error("Can't dispatch in the middle of a dispatch.") }
        dispatching = true
        currentState = reducer(action, currentState)
        notifySubscribers()
        dispatching = false
    }

    fun subscribe(subscriber: (state: State) -> Unit): () -> Unit {
        val id = uniqueID
        uniqueID += 1
        subscribers[id] = subscriber
        val dispose: () -> Unit = {
            subscribers.remove(id)
        }
        subscriber(currentState)
        return dispose
    }

    private var currentState = initialState
    private val reducer = reducer
    private var dispatching = false
    private var subscribers: MutableMap<Int, (state: State) -> Unit> = mutableMapOf()
    private var uniqueID = 0

    private fun notifySubscribers() {
        for (subscriber in subscribers.values) {
            subscriber(currentState)
        }
    }
}

Preface: I haven't worked in the Android ecosystem, so this review is mostly looking at the Kotlin side of things. I've also had minimal exposure to the Redux pattern.

Some low hanging fruit that IntelliJ IDEA points out:

return state is unreachable code in fun reducer. Control flow will always exit as action can only be .INCREMENT or .DECREMENT. I would use an else -> throw IllegalStateException("Invalid Action") in the when, to keep safety in the case of expanding the enum class Action in the future. As a bonus, this is not marked as unreachable code, as IntelliJ recognizes it as a fallback case.
Explicit type arguments in val store = Store<Action, State>... are not required: just val store = Store(State(), ::reducer) works fine and Kotlin infers the types. This is Kotlin's biggest advantage over Java.
class Store<Action, State> can have declaration site variance. Read the link for information about what this means; it is written class Store<in Action, out State> and does not change any semantics of the present code.
private val reducer can be declared directly in the constructor. It's personal preference whether you declare it there or not, but if you do, I would probably also declare currentState there as well (instead of initialState).

Some other points:

I wouldn't use the package com.myapplication; it's a placeholder. Instead, give it something that identifies this package as yours. Oracle's Java tutorial recommends using a domain you own. Even though I do own http://cad97.com, rather than use package com.cad97.project I usually use just cad97.project, as a holdover from before I bought the domain. For you, this pattern would suggest a base package of daniel_t. If you do not own the domain, DO NOT use a com (or any other common TLD) base package.
Personally, I typically don't specify argument labels on function types. (This refers to the types for parameters to the Store constructor, the MutableMap types, the subscribe method.)
Java (and in my adventures so far, Kotlin) typically uses one-letter types for generic types, to distinguish them from regular types. This is compounded by the fact that your generic Action is named the same as your concrete Action. How you resolve this is up to the programmer, but in an ideal world you would not have this conflict of names.
You can return dispose directly: just write return { subscribers.remove(id) }
Store::notifySubscribers can be written as a single-expression function: private fun notifySubscribers() = subscribers.values.forEach { it(currentState) }
subscribers can be declared as private var subscribers = mutableMapOf<Int, (State) -> Unit>()
Rather than return Error from Store::dispatch, use IllegalStateException, as it is more descriptive of the reason for the error.
By the format of Store::dispatch, it looks like what you're trying to do is use a Lock. Kotlin provides special support for this, which would be written something like this:

private val dispatchLock = ReentrantLock() // replaces dispatching

fun dispatch(action: Action) = dispatchLock.withLock {
    currentState = reducer(action, currentState)
    notifySubscribers()
}


Multi-tiered sorting using custom IComparer I have this custom IComparer I use to sort a list of SurveyResponse objects, but it seems really inefficient, both in terms of runtime performance, and code elegance and maintainability. I thought of implementing IComparable for each of these components, and each CompareTo knowing its "parent" to compare, but the sort order for this report I'm making won't necessarily be the same as other sort orders, so that may not work.

It looks like the sort of problem that should be solvable using recursion, but maybe I'm just over-analyzing it?

Are there any best practices that would make this faster and more maintainable?

private class CommentComparer : IComparer<SurveyResponse>
{
  public int Compare(SurveyResponse x, SurveyResponse y)
  {
    // Sort by Application Name...
    int result = x.Question.Survey.Feature.Application.Name.CompareTo(
      y.Question.Survey.Feature.Application.Name);
    if (result != 0) { return result; }
    // ...then Feature Name...
    result = x.Question.Survey.Feature.Name.CompareTo(
      y.Question.Survey.Feature.Name);
    if (result != 0) { return result; }
    // ...then SurveyTime...
    result = x.Question.Survey.TimeTaken.CompareTo(y.Question.Survey.TimeTaken);
    if (result != 0) { return result; }
    // ...then ID...
    result = x.Question.Survey.ID.CompareTo(y.Question.Survey.ID);
    if (result != 0) { return result; }
    // ...then Position.
    return x.Question.Position.CompareTo(y.Question.Position);
  }
}

You can use types from ComparerExtensions to make your code more maintainable:

IComparer<SurveyResponse> comparer = KeyComparer<SurveyResponse>
    .OrderBy(sr => sr.Question.Survey.Feature.Application.Name)
    .ThenBy(sr => sr.Question.Survey.Feature.Name)
    .ThenBy(sr => sr.Question.Survey.TimeTaken)
    .ThenBy(sr => sr.Question.Survey.ID)
    .ThenBy(sr => sr.Question.Position);

But I don't think you can actually make your comparer more efficient, it should be already very fast (it's just a few simple instructions). Are you sure your performance problem is caused by this comparer?


All integer sequences given amount of integers and length of sequence Given two integers, X and Y, you must create all possible integer sequences of length Y using the numbers from 1 to X.

For certain reasons, the function must return a deque<deque<int> > representing such (the primary deque holds all my sequences - each of then is a deque of ints).

This is not a permutation generator.

Example Input

X = 3, Y = 3

This means that I require sequences of length 3 each one. To compose the sequences, I use the integers 1, 2, and 3.

Example Output

The previous input should produce:

1 1 1
1 1 2
1 1 3
1 2 1
1 2 2
1 2 3
1 3 1
1 3 2
1 3 3
2 1 1
2 1 2
2 1 3
2 2 1
2 2 2
2 2 3
2 3 1
2 3 2
2 3 3
3 1 1
3 1 2
3 1 3
3 2 1
3 2 2
3 2 3
3 3 1
3 3 2
3 3 3

My Idea

I'm not very good with this stuff. My approach is:

Create a start and end integer

Start is 1 repeated Y times. So in the previous example it is 111
End is X repeated Y times. So in the previous example it is 333

I count i from start to end.

If i contains an invalid digit (like 4, 5,...), abort this sequence.
If i is valid, then this sequence is valid, thus add it to my resulting deque.
When a sequence is valid, due to my needs, I split the integer and make a deque of each of its digits.

Why I need a review

I think it is pretty clear that this method doesn't sound very efficient. So many wasted iterations! Can you help me improve this code, and, perhaps, shorten it?

My Attempt

Note, you may notice that instead of "sequences" I call then "procedures".

#include <iostream>
#include <sstream>
#include <deque>

using namespace std;

// Converts a value to any type
template <typename T,typename S>
T convert(S original) {
    stringstream ss; ss << original;
    T result; ss >> result;
    return result;
}

// The actual function
deque<deque<int> > allPossibleProcedures(int values, int depth) {
    deque<deque<int> > result;
    // I create a start point and an end point for my counting
    string minString; for (int d = 0; d < depth; ++d) { minString.append("1"); }
    string limitString; for (int d = 0; d < depth; ++d) { limitString.append(convert<string>(values)); }
    int start = convert<int>(minString);
    int limit = convert<int>(limitString);
    // I begin counting
    for (int i = start; i <= limit; ++i) {
        deque<int> procedure;
        string text = convert<string>(i);
        bool ok = true;
        for (int c = 0; c < text.length() && ok; ++c) {
            int x = convert<int>(text.at(c));
            if (x <= values && x > 0) {
                // This is a valid character. Add to procedure.
                procedure.push_back(x);
            }else{
                // Not a valid character. Abort this procedure.
                ok = false;
                break;
            }
        }
        if (ok) {
            result.push_back(procedure);
        }
    }
    return result;
}

int main(int argc, const char * argv[]) {
    deque<deque<int> >procedures = allPossibleProcedures(3,3);
    for (int i = 0; i < procedures.size(); ++i) {
        for (int j = 0; j < procedures[i].size(); ++j) {
            cout << procedures[i][j] << " ";
        }
        cout << endl;
    }
    return 0;
}

I don't have time to do a style review at the moment, so until I can expand on this later, I'll just suggest a different algorithm.

Rather than depending on string manipulation and generating more items than you need, just take an arithmetic approach.

In particular, you can take a deque of ints and add 1 to it until it is equal to the max value.

std::deque< std::deque<int> > results;
std::deque<int> min(Y, 1);
std::deque<int> max(Y, X);
for (; min != max; min = increment(min, Y)) {
    results.push_back(min);
}
results.push_back(max);

As a bit more explanation, a std::deque<unsigned int> is frequently used as a make-shift arbitrary precision integer. The way this is done is to have a sign flag, and then to see each element of the deque as part of a radix = std::numeric_limits<unsigned int>::max() number.

As a concrete example, a std::deque<unsigned char> d = {95, 14, 230} would represent 95 * 255^2 + 14 * 255^1 + 230 * 255^0 in the same way common base ten 134 = 1 * 10^2 + 3 * 10^2 + 4 * 10^0.

Anyway, I'm getting carried away with this.

What you're in need of is a much more specific case of this. Rather than making a generic adder, all you have to do is just add 1, and if that causes the lowest digit to overflow, push it back down to a 1 and carry the add up the chain.

std::deque<int> increment(std::deque<int> d, int Y)
{
    bool carry = true;
    for (std::deque<int>::reverse_iterator it = d.rbegin(), end = d.rend(); it != end; ++it) {
        *it += 1;
        if (*it > Y) {
            *it = 1;
        } else {
            carry = false;
        }
    }
    if (carry) {
        d.push_front(1);
    }
    return d;
}

The above code is untested and unoptimized. It can probably be written more compactly, but it's been a long time since I've done anything like this, and I'm just trying to illustrate how it can be done arithmetically.

Also, if you're concerned about performance, you might want to have increment mutate d rather than return a modified copy.


Calculate distance using speed of sound I've been working on learning Java and this was a challenge as part of a chapter on data types:

  Create a program that computes how far away, in feet, a listener is
  from a sound. Sound travels approximately 1,100 feet per second
  through air.

  You can also compute the distance to a large object, such as a rock
  wall, by timing the echo. For example, if you clap your hands and time
  how long it takes for you to hear the echo, then you know the total
  round-trip time.

  Java: A Beginner's Guide, Sixth Edition - by Herbert Schildt · McGraw-Hill Education (Chapter 2)

I'm super-new to OOP and am trying to nip bad habits in the bud, so-to-speak. I've divided the logic in one class as much as I think is possible (at my current skill level).

SoundSpeed.java

public class SoundSpeed {
    private final double SOUND_TRAVEL_SPEED = 1100;
    public double timeInSeconds;
    public boolean isEcho;
    private int distanceDivisor;

    double getResult() {
        if(isEcho) {
            distanceDivisor = 2;
        } else {
            distanceDivisor = 1;
        }
        return (timeInSeconds * SOUND_TRAVEL_SPEED) / distanceDivisor;
    }
}

SoundSpeedCalc.java

class SoundSpeedCalc {
    public static void main(String args[]) {
        SoundSpeed soundSpeed = new SoundSpeed();
        soundSpeed.timeInSeconds = 35.079;
        soundSpeed.isEcho = true;
        String verb;

        if(soundSpeed.isEcho) {
            verb = "echo";
        } else {
            verb = "travel";
        }
        System.out.println("The sound took " + soundSpeed.timeInSeconds + " seconds to "
        + verb + " and thus the distance is " + soundSpeed.getResult() + " feet away.");
    }
}

Example output (using above parameters in main):

  The sound took 35.079 seconds to echo and thus the distance is 19293.45 feet away.

Example output if I set soundSpeed.isEcho to false:

  The sound took 35.079 seconds to travel and thus the distance is 38586.9 feet away.

What you want to do is simply

public static double distanceBySounding(double seconds, boolean isEcho) {
    return (isEcho ? 0.5 : 1.0) * seconds * 1100 /* ft. per second */;
}

The question is, how should this function be expressed in an object-oriented way?  It's pretty safe to say that this…

SoundSpeed soundSpeed = new SoundSpeed();
soundSpeed.timeInSeconds = 35.079;
soundSpeed.isEcho = true;
System.out.println(soundSpeed.getResult());

… is an unpalatable interface.  Manipulating public instance variables like that is not only cumbersome, it is an unsound practice (pardon the pun).  Given a choice between the two options above, I would much prefer the simpler function to the contorted object.  And that's fine.  Java allows you to write code that doesn't fully conform to OOP.

What if you wanted an object-oriented design?  The key to object-oriented design is that you want to model objects.  That means that you have to find plausible objects to model — the more realistic, the better.  What's a SoundSpeedCalc?  I don't know; you can't buy one.  But you can sure buy a sonar!  It makes sense to instantiate a Sonar with its configuration parameters (the sound medium and its active/passive mode), then you can start "aiming" it at stuff by calling .distance().

public class Sonar {
    public enum Medium {
        AIR(1100);

        private final double ftPerSec;

        Medium(double ftPerSec) {
            this.ftPerSec = ftPerSec;
        }
    }

    private final Medium medium;
    private final boolean isEcho;

    public Sonar(Medium medium, boolean isEcho) {
        this.medium = medium;
        this.isEcho = isEcho;
    }

    public double distance(double seconds) {
        return medium.ftPerSec * seconds / (isEcho ? 2 : 1);
    }
}

Here's how you can use it effectively:

public static void main(String[] args) {
    double seconds = 35.079;
    boolean isEcho = true;

    Sonar sonar = new Sonar(Sonar.Medium.AIR, isEcho);
    String verb = isEcho ? "echo" : "travel";
    System.out.printf("The sound took %s seconds to %s and thus the distance is %s feet.\n",
                      seconds, verb, sonar.distance(seconds));
}

Note the use of System.out.printf() as a nicer alternative to string concatenation.  Also, I've used conditional expressions — it's a perfect way to assign one of two values depending on a boolean switch.


Passing many configuration options to Java code formatter I use the DefaultCodeFormatter of the core Java package to prettify my code. Unfortunately I want to look my code very different from the default settings. That why, I have to explicitly define a lot of values:

public static String makeSourceCodeLookingMoreSexy(String source) {
        Map<String, String> options = DefaultCodeFormatterConstants.getEclipseDefaultSettings();

        options.put(JavaCore.COMPILER_COMPLIANCE, JavaCore.VERSION_1_6);
        options.put(JavaCore.COMPILER_CODEGEN_TARGET_PLATFORM, JavaCore.VERSION_1_6);
        options.put(JavaCore.COMPILER_SOURCE, JavaCore.VERSION_1_6);
        options.put(DefaultCodeFormatterConstants.FORMATTER_ALIGNMENT_FOR_ENUM_CONSTANTS, DefaultCodeFormatterConstants.createAlignmentValue(true, DefaultCodeFormatterConstants.WRAP_ONE_PER_LINE, DefaultCodeFormatterConstants.INDENT_ON_COLUMN));
        options.put(DefaultCodeFormatterConstants.FORMATTER_ALIGNMENT_FOR_PARAMETERS_IN_METHOD_DECLARATION, DefaultCodeFormatterConstants.createAlignmentValue(false, DefaultCodeFormatterConstants.WRAP_NO_SPLIT, DefaultCodeFormatterConstants.INDENT_DEFAULT));
        options.put(DefaultCodeFormatterConstants.FORMATTER_ALIGNMENT_FOR_PARAMETERS_IN_METHOD_DECLARATION, DefaultCodeFormatterConstants.createAlignmentValue(false, DefaultCodeFormatterConstants.WRAP_NO_SPLIT, DefaultCodeFormatterConstants.INDENT_DEFAULT));
        options.put(DefaultCodeFormatterConstants.FORMATTER_ALIGNMENT_FOR_THROWS_CLAUSE_IN_METHOD_DECLARATION, DefaultCodeFormatterConstants.createAlignmentValue(false, DefaultCodeFormatterConstants.WRAP_NO_SPLIT, DefaultCodeFormatterConstants.INDENT_DEFAULT));

        final CodeFormatter codeFormatter = ToolFactory.createCodeFormatter(options);

        final TextEdit edit = codeFormatter.format(CodeFormatter.K_COMPILATION_UNIT, source, 0, source.length(), 0, System.getProperty("line.separator"));

        IDocument document = new Document(source);
        try {
            edit.apply(document);
        } catch (MalformedTreeException e) {
            e.printStackTrace();
        } catch (BadLocationException e) {
            e.printStackTrace();
        } catch (NullPointerException e) {
            e.printStackTrace();
        }

        return document.get();
    }

That are not even all options that I want to set. Actually it is way more, maybe about 50. As you can imagine the code gets very long. So is there a better way to do this than I'm actually doing it?

I'd extract it out to a helper method:

private static Map<String, String> getOptionsMap() {
    final Map<String, String> options = DefaultCodeFormatterConstants.getEclipseDefaultSettings();

    options.put(JavaCore.COMPILER_COMPLIANCE, JavaCore.VERSION_1_6);
    options.put(JavaCore.COMPILER_CODEGEN_TARGET_PLATFORM, JavaCore.VERSION_1_6);
    options.put(JavaCore.COMPILER_SOURCE, JavaCore.VERSION_1_6);

    // TODO: rename the variable
    final String aligment0 = DefaultCodeFormatterConstants.createAlignmentValue(true,
            DefaultCodeFormatterConstants.WRAP_ONE_PER_LINE,
            DefaultCodeFormatterConstants.INDENT_ON_COLUMN);
    options.put(DefaultCodeFormatterConstants.FORMATTER_ALIGNMENT_FOR_ENUM_CONSTANTS, aligment0);

    // TODO: rename the variable
    final String aligment1 = DefaultCodeFormatterConstants.createAlignmentValue(false,
            DefaultCodeFormatterConstants.WRAP_NO_SPLIT,
            DefaultCodeFormatterConstants.INDENT_DEFAULT);
    options.put(DefaultCodeFormatterConstants.FORMATTER_ALIGNMENT_FOR_PARAMETERS_IN_METHOD_DECLARATION, aligment1);
    options.put(DefaultCodeFormatterConstants.FORMATTER_ALIGNMENT_FOR_PARAMETERS_IN_METHOD_DECLARATION, aligment1);
    options.put(DefaultCodeFormatterConstants.FORMATTER_ALIGNMENT_FOR_THROWS_CLAUSE_IN_METHOD_DECLARATION, aligment1);
    return Collections.unmodifiableMap(options);
}

Maybe you can split it up to two or more methods. The aligment0 and aligment1 variables should be renamed to a proper name. Note that you can reuse the Map, since Collections.unmodifiableMap() ensures that it cannot be changed by clients.


Perl CGI script to serve a PDF file In a different post I showed the following Perl CGI script. Someone in the comments said that

  "You're using a lot of very unperlish syntax, and I'd like to give you some feedback".

So, here it is, and tell me how to make it more Perlish?  I came from a C background so I'm all ears (or eyes)!

#!/usr/bin/perl -T

use strict;
use warnings;
use CGI qw/:all/;

my $basepath = '/some/path/to';
my $f = param("f") || '';

($f eq '') && die;

my $path = "$basepath/$f.pdf";

open(PATH, "<$path") || die;
binmode PATH;

print header(-type => "application/pdf",
         -target => "$f");
print;

while (<PATH>) {
    print($_);
}
close(PATH);

I will go through your code line by line and give feedback. We will skip the general advice on don't use CGI as it's actually suited for what you are trying to do here.

I wrote this answer in two stages, so some of the things here might have been said by other people. I'm attempting to provide full feedback, and I'm not copying from others. If several of us give the same feedback, it must probably make some sense.

#!/usr/bin/perl -T

The -T flag turns on taint mode. I am not sure you intended to do this, or just copied it from somewhere else. Taint mode essentially makes sure that data coming into your program from the outside must be validated before it's being used. But you are not untainting the variable $f. That could bite you in the back.

use strict;
use warnings;

This is good! Well done. Your code should always start with strict and warnings.

use CGI qw/:all/;

You are importing a lot of stuff into your namespace, but all you use is param. Instead, consider importing only :cgi, which will give you less stuff, or only import param and header. That also makes it easier to remember where that function came from.

use CGI qw(param header);

my $f = param("f") || '';

$f is not the greatest variable name. Use descriptive variable names, like $filename.

If the file is called zero 0, or 0000, this will fail. Read more about that at the end of this post.

($f eq '') && die;

While some people like to use logical operators for flow control, this is really weird to read. If you want concise syntax without blocks, use a post-fix if or unless.

Also, provide an error message as to why this failed. It's CGI, so this will show up in your server log. You don't want it to say Died at line 10, do you?

I prefer to use the q{} quoting operator for empty strings, as it shows the intent of giving an empty string more clearly.

die "No filename given" if $f eq q{}.

open(PATH, "<$path") || die;

Don't use a GLOB as a file handle. It's global to all of your program, including other modules and namespaces. If another module was to use the same PATH name for a handle, this would conflict. Instead, use a lexical file handle. The convention is to name file handles $fh.

This has the added benefit that Perl will take care of closing the handle for you as soon as the variable goes out of scope. In this case, that's when the program ends. So you don't have to use close explicitly.

Use the three-argument form of open, where the second argument is the mode. In your case, that's < for reading. This makes file operations more secure, as your $path could for example start with a pipe |, which would change the meaning. Especially if part of your file name comes from the user (and you didn't even untaint it), this is crucial advice.

Don't use binary or || for flow control as it has tight binding. Use or instead, which binds very loosely, so you don't need to put the parentheses after open. It will also read more like actual English.

Include a meaningful error message with your die, especially the error $! that was returned from the open call.

open my $fh, '<', $path or die "Error opening '$path': $!";
binmode $fh;

print header(-type => "application/pdf",
         -target => "$f");

Indent your code properly. This looks weird. You have indented the second line 9 spaces. Looks like one full tab in vim, and one space. Instead, break it around properly, and align everything. On average, code is read ten times more than written. Prepare for that.

Don't quote variables if there is no interpolation needed. If it's only one var, leave out the quotes.

print header(
    -type   => "application/pdf",
    -target => $filename,
);

print;

This line will print the current value of $_. I am not sure what that is at this time in the execution of the program. It's probably not intended. Get rid of it.

while (<PATH>) {
    print($_);
}

If you intend to use the loop variable, name it. The topic $_ is really useful for short one-liners, map and such, but a while loop is rarely such a case.

while ( my $buffer = <$fh> ) {
    print $buffer;
}

You can also use the aforementioned post-fix notation for that. Again, you don't need to use the topic, because it's implicit.

print while <$fh>;

But you can also just get rid of that completely, and simply use the fact that print is list context, and the diamond operator <> returns the full file as an array in list context.

print <$fh>;

I said earlier that your check for $filename with || will fail if the name is a false value. Remember that Perl has no actual true and false, just values that evaluate truthy, and values that don't. There are very few things that are false in Perl. 0, undef, the empty string q{} and the empty list () are such cases.

my $filename = param('f');
die unless $filename;

This would be similar, but again, 0 would be cast out.

my $filename = param('f');
die unless defined $filename;

If there was no param f, the param function returns undef. So we can avoid that by saying unless defined. That lets 0.pdf through. But what about the empty string?

Essentially you need to decide whether a file named $basepath/.pdf is valid. That's up to you. If you don't want that, you have to do additional checks.

You could also check if that file exists in general, with -e.

die "No such file" unless -e $path;

However, open will check that anyway, so it's not needed.


Semantically correct html5 markup I'm writing the markup for Corpora - A Business Theme:

And started from the header section:

Here is my markup for it:

<address>Phone: 1.800.corp</address>

<address>Email: office@corpora.com</address>

<address>Follow Us: </address>

        Home
go to start
        Services
what we do
        Gallery
our best products
        Our Clients
what we've done for others
        About Us
who we are
        Contact Us
let's keep in touch

Awesome business card design

Lorem ipsum dolor sit amet, consectetur adipisicing elit. Curabitur eget metus ac diam laoreet luctus rhoncus non nisi. Aenean at lobortis augue.

                Buy it now or Find Out More

Awesome business card design

Lorem ipsum dolor sit amet, consectetur adipisicing elit. Curabitur eget metus ac diam laoreet luctus rhoncus non nisi. Aenean at lobortis augue.

                Buy it now or Find Out More

Awesome business card design

Lorem ipsum dolor sit amet, consectetur adipisicing elit. Curabitur eget metus ac diam laoreet luctus rhoncus non nisi. Aenean at lobortis augue.

                Buy it now or Find Out More

Does it semantically correct? Special attention for tags address, em, span, section.  Many thanks to you all.

address

I'd enclose the ul.primaryContacts in one address element (not every item separately):

<address>

Phone: 1.800.corp

Email: office@corpora.com

Follow Us:

</address>

links without content

Your follow links don't contain any content:

Screenreader users wouldn't be able to make sense of these links.

Either add the text to it (and visually hide it, and display the icons via CSS), or use img here (together with the alt attribute, of course).

Same problem with the two slideButtons ().

em

I don't think the em element is correct in these cases:

1.800.corp
office@corpora.com

http://www.w3.org/TR/html5/text-level-semantics.html#the-em-element:

  The em element represents stress emphasis of its contents.

And, important:

  The placement of stress emphasis changes the meaning of the sentence.

In your cases the em doesn't change any meaning. You wouldn't stress the phone number or the email address while reading.

contact URIs

You could link your contact details:

1.800.corp (with the tel URI scheme)
office@corpora.com (with the mailto URI scheme)

br

You use br in the navigation (Home
go to start), but this is not a correct use:

  br elements must be used only for line breaks that are actually part of the content, as in poems or addresses.

site heading

Your page should probably have a site heading. Currently your outline would be:

Untitled Section
    Untitled Section
    Awesome business card design
    Awesome business card design
    Awesome business card design

In your case, the logo (resp. the alt value) is the site/page heading, so you'd have:

slider

It depends on content and context of your page if the slider should be part of header or not. Is the slider present on all pages? Then it's probably correct to place it in the header. But if it would be part of the main content of a page, it shouldn't be in the header.

I think you should enclose the whole slider in a section (resp. aside); if possible, find a heading for it. As soon as you use such a sectioning element for the slide, you can enclose the two slide buttons in a nav element (as they are the main navigation for that sectioning element). Also, each slide could be an article (instead of a section).

Then you'd get the following outline (together with the site heading mentionend before):

Corpora - A Business Theme
  Untitled Section (this is your site nav)
  Untitled Section (this is the slider heading)
    Untitled Section (this is your slider nav)
    Awesome business card design
    Awesome business card design
    Awesome business card design


Find a file in current or parent directories As a Rust beginner, I would like to know how I could improve the following points considering the function below:

Handling Path and PathBuf types correctly;
Performance;
Rust writing style in general.

Here is the function, decorated with a full working example:

use std::env;
use std::path::{Path, PathBuf};

const RUSV_FILENAME: &str = ".rusv.json";

/**
 * Find a rusv file in the current or parent directories of the given directory.
 */
fn find_rusv_file(starting_directory: &Path) -> Option<PathBuf> {
    let mut directory = starting_directory;
    let rusv_filename = Path::new(&RUSV_FILENAME);

    loop {
        let filepath: PathBuf = [
            directory,
            rusv_filename
        ].iter().collect();

        if filepath.is_file() {
            return Some(filepath);
        }

        match directory.parent() {
            Some(parent) => directory = parent,
            None => return None,
        }
    }
}

fn main() -> std::io::Result<()> {
    let path = env::current_dir()?;

    match find_rusv_file(&path) {
        Some(filepath) => println!("Rusv file was found: {:?}", filepath),
        None => println!("No rusv file was found."),
    };

    Ok(())
}

My question is not about:

Creating a more generic function (e.g. by giving the filename as an argument);
Anything related to the main() function.

Handling Path and PathBuf types correctly;

My concern is that you recreate PathBuf every time, I don't see the point to have a buffer if you don't use it.

  Performance;

I think it's ok, you search one file in particular the time you take creating the string is cheap compare to system call. So I don't think read_dir is better on your case. This could be benchmarked.

  Rust writing style in general.

You are using return, on your case maybe use break to make your function SESE (single entry single exit).

Path::new(&RUSV_FILENAME); the reference is not needed Path::new(RUSV_FILENAME);.

fn find_rusv_file(starting_directory: &Path) -> Option<PathBuf> {
    let mut path: PathBuf = starting_directory.into();
    let file = Path::new(RUSV_FILENAME);

    loop {
        path.push(file);

        if path.is_file() {
            break Some(path);
        }

        if !(path.pop() && path.pop()) { // remove file && remove parent
            break None;
        }
    }
}