When to use a new variable vs string interpolation? I wrote a script that I decided to refactor so I could add functionality to it as my coworkers think of it. I only saved four lines in the effort, but the main change is I removed both methods and reduced the number of called variables in favor of string interpolation/manipulation. Is there a preference for this? Is it better to declare a new variable just to use once, or is it more DRY to just make minor tweaks to the string when you need to use it? For example here is the original code:

$EMBED_HTML = ""

$BASE_URL = ""

$SAMPLE_FLV = ""

def normalize_directory(dir)
  normalized = dir.gsub('\\', '/')
  normalized += '/' unless normalized[-1] == '/'
end

def validate_directory(dir)
  puts "Enter the full directory path of the flv files." unless dir
  dir =  dir || gets.chomp
  dir = normalize_directory(dir)
  until File.directory?(dir) && Dir.glob("#{dir}*.flv") != []
    puts "That directory either doesn't exist or contains no .flv files. \nEnter the full directory path of the flv files."
    dir = $stdin.gets.chomp
    dir = normalize_directory(dir)
  end

  dir
end

def output_html_wrapper(flv_filename, output_folder)
  html_filename = flv_filename.gsub(".flv", ".html")

  html_body = $EMBED_HTML.gsub($SAMPLE_FLV, flv_filename.sub(/[a-z][:]/, ''))
  html_output = File.open(html_filename, "w")
  html_output.write(html_body)
  html_output.close

  link = flv_filename.sub(/[a-z][:]/, '').gsub(".flv", ".html")
  link_list = File.open("#{output_folder}List of Links.txt", "a")
  link_list.write($BASE_URL + link + "\n")
  link_list.close

  puts "#{html_filename} created successfully." if File.exists?(html_filename)
end

folder = ARGV[0].dup unless ARGV.empty?
folder = validate_directory(folder)
flvs = Dir.glob("#{folder}*.flv")
File.delete("#{folder}List of Links.txt") if File.exists?("#{folder}List of Links.txt")
flvs.each { |flv| output_html_wrapper(flv, folder) }

And the new stuff:

flash_folder = ARGV[0].dup unless ARGV.empty?

if !flash_folder
  puts "Enter the full directory path of the flv files."
  flash_folder = gets.chomp
end

flash_folder.gsub!('\\', '/')
flash_folder += '/' unless flash_folder[-1..-1] == '/'
until File.directory?(flash_folder) && Dir.glob("#{flash_folder}*.flv") != []
  puts "That directory either doesn't exist or contains no .flv files. \nEnter the full directory path of the flv files."
  flash_folder = $stdin.gets.chomp
  flash_folder.gsub!('\\', '/')
  flash_folder += '/' unless flash_folder[-1..-1] == '/'
end

flash_files = Dir.glob("#{flash_folder}*.flv")

File.delete("#{flash_folder}List of Links.txt") if File.exists?("#{flash_folder}List of Links.txt")

flash_files.each do |flv|
  html_output = File.open("#{flv.gsub(".flv", ".html")}", "w")
    html_output.write("#{embed_code.gsub("sample.flv", flv.slice(7..flv.length))}")
  html_output.close
  link_list = File.open("#{flash_folder}List of Links.txt", "a")
    link_list.write("#{flash_url}#{flv.slice(2..flv.length).gsub(".flv", ".html")}\n")
  link_list.close
end

puts "Finished."

Your refactor seems to have reduced the readability of your code, which is usually the opposite of your intent when refactoring. The code may be terser, and string interpolation and re-use is a part of how you've achieved this, but it doesn't make the code easy to follow. That will make it harder for you and your colleagues to modify it over time.

I actually prefer your original code a lot. You've pulled two methods out of your main execution flow, so it's possible to see at a glance what the code is doing - validate a directory, find all the files in it and do some form of outputting on each. That clarity is missing from the newer code because it is all written at the same level.

Your code makes use of #dup and #clone a lot. These methods are not needed in many of the places you use them, so they add noise to the code. For example files = folder.clone + "*.flv" does not need the #clone call because the + method will create a new String from the content of folder.clone and *.flv. Writing files = folder + "*.flv" would work just fine.

Here is a take on your original code that adds some readability by avoiding repetition and giving fuller names to variables and methods.

    def normalize_directory(dir)
        normalized = dir.gsub('\\', '/')
        normalized += '/' unless normalized.[-1,1] == '/'
    end

    def validate_directory(dir)
        if dir.empty?
            puts "Enter the full directory path of the flv files."
            dir = gets.chomp
        end
        dir = normalize_directory(dir)

        until File.directory?(dir) && !Dir.glob("#{dir}*.flv").empty?
            puts "That directory either doesn't exist or contains no .flv files. \nEnter the full directory path of the flv files."
            dir = gets.chomp
            dir = normalize_directory(dir)
        end

        dir
    end

    def output_html_wrapper(flv_filename, output_folder)
        html_filename = flv_filename.gsub(".flv", ".html")
        vid = flv_filename.slice(0..6)

        html_body = $EMBED.gsub("sample.flv", vid)
        html_output = File.open(html_filename, "w")
        html_output.write(html_body)
        html_output.close

        link = vid.gsub(".flv", ".html")
        link_list = File.open("#{output_folder}List of Links.txt", "a")
        link_list.write($BASE + link + "\n")
        link_list.close

        puts "Files created successfully."
    end

    folder = validate_directory(ARGV[0])
    flvs = Dir.glob("#{folder}*.flv")
    File.delete("#{folder}List of Links.txt") if File.exists?("#{folder}List of Links.txt")
    flvs.each { |flv| output_html_wrapper(flv, folder) }

Being DRY is about keeping a piece of knowledge in one place, whether that's a file name, the way you clean up some input or anything similar. The normalize_directory method I added DRYs up your code by putting in one place the logic for making a valid directory name out of the user's input.

Declaring a variable for a single use is extremely useful if it helps describe what the value is being used for. The vid variable in your code is still a mystery to me regarding what it actually contains. This is a great place to give something a name, and in doing so document your code without comments.


Raw filter with ternary operator for html class attribute I've been trying to figure out how to stop Twig escape using the following code, which I feel is a little nicer with the succinct ternary operator.

I hope someone can confirm I'm using the correct syntax and if there are any performance issues doing it like this (as well as alternative methods, if any):

<body{{ bodyClass is defined ? (' class="' ~ bodyClass ~ '"')|raw : '' }}>

It seems to work and is the only way I've managed to get proper output so far!

An alternative version can be seen here, using string interpolation (#{} is my current preferred method):

" class=\"#{bodyClass}\""|raw

Does this work:

<body {{ bodyClass ? class='{bodyClass}'|raw : ''}}>

I don't think you need the brackets and tilde signs...


Using homogeneous coordinates to rotate an image Currently, I have this code snippet, but it's a little bit slow:

% input image
input_matrix = imread('placa.bmp');
[rows cols] = size(input_matrix);

% rotation
degree = 30;
radians = (pi * degree) / 180;
theta = radians;

% output matrix
t_matrix = input_matrix;
t_matrix(t_matrix == input_matrix) = NaN;

% transformation matrix
T = [cos(theta) -sin(theta) 0; sin(theta) cos(theta) 1; 0 0 1];

% loop over each input_matrix coordinate
for n = 1:numel(input_matrix)

    % current coordinate
    [x y] = ind2sub([rows cols], n);

    % transpose
    v = [x y 1]';

    % homogeneous coordinate
    v = T*v;

    % transponse again
    v = v';

    % only integer values
    a = floor(v(1));
    b = floor(v(2));

    if a > 0 && b > 0

        % replace in t_matrix
        t_matrix(a,b) = input_matrix(x,y);
    end
end

% get only a part of t_matrix
t_matrix = t_matrix(1:rows, 1:cols);

figure; imshow(input_matrix); % original image
figure; imshow(t_matrix)

How can I improve it?

Condition: keep using this matrix:

% transformation matrix
T = [cos(theta) -sin(theta) 0; sin(theta) cos(theta) 1; 0 0 1];

Rather than looping over all x and y values you could put them into a vector, that should speed things up considerably.

y=repmat(1:cols,rows,1)
x=repmat((1:rows)',cols,1)

M=[x(:) y(:) ones(rows*cols,1)];

Now you just need to multiply M or M' with T or T' and you have subsitituted that part of the code.


Project Euler 8: find the maximum product of 13 consecutive digits I'm doing Project Euler problems as a learning platform for Forth. Currently I'm solving Project Euler Problem 8 which involves a 1000-long string, which I entered directly in the source code.
My questions are:

What options are common for dealing with input?
If I put the locals on the stack, won't that make the loop too full of (otherwise unneeded) stack manipulation words?
Suggestions, criticism, nitpicks are welcome...

: e008-multNdigits ( a n -- p )
    1 swap 0 do swap dup i + c@ [char] 0 - rot * loop nip ;

: euler008
    0 13 locals| length maxproduct |
    s" 731671765313306249192...450"
    ( a 1000 )
    length - 0 do
        dup i + length e008-multNdigits
        dup maxproduct > if to maxproduct else drop then
    loop maxproduct . ;

Disclaimer: my Forth is extremely rusty.

length does not need to be local; is not a variable, it is a constant. Declare it as such:
13 constant length

Dealing with input. The stack annotation ( a 1000 ) strongly hints that what follows wants to be the word on its own. Indeed, logic should be separated from IO. Consider, for example, something along the lines of
: e008 ( a n -- p)
    ....
;

s" 731671765313306249192...450"
euler008
.

Once the logic and IO are separated, you may use open-file and read-file if you wish.

I do not endorse one-liners, especially if they involve loop. Consider
: e008-multNdigits ( a n -- p )
    1 swap 0
    do
        swap
        dup i +
        c@ [char] 0 -
        rot *
    loop
    nip ;

As a side note, nip is very rarely useful, and usually it is an indication of the suboptimal design. Try to get rid of it. The nipped value, if I am not mistaken, is a base address of the array. I have an impression that its only purpose is to undo a dup in the caller. Try to get rid of both.

The line
  dup maxproduct > if to maxproduct else drop then

is a long way to say
  maxproduct max to maxproduct

Consider having max product at TOS prior to setting up a call to e008-multNdigits. In this case,
  length - 0 do
      dup i + length e008-multNdigits
      max

would suffice, and eliminate the need for the local.

Not Forth-related issues:

The algorithm performs 13000 multiplications. A sliding window approach lets you get away with 1000 multiplications and 1000 divisions. Of course an extra care should be taken when 0 is encountered.

The product of 13 digits may take as much as 42 bits. A naive multiplication fails on a 32-bit cells.

Finally, Project Euler is not about programming. It is about math. To hone your Forth skills, consider implementing classical algorithms, and benchmark them against conventional implementations.


Towers of Hanoi redux Inspired by Mayur Kulkarni's question this morning, I decided to post some code for the classic Towers of Hanoi problem, but this time in C++. Since this is fairly "textbook" kind of task, all suggestions are welcome.

#include <iostream>
#include <vector>
#include <string>
#include <iterator>

class Hanoi {
    typedef std::vector<int> Tower;
    Tower a, b, c;

    void show(std::string const &label, Tower c) {
        std::cout << label;
        std::copy(c.cbegin(), c.cend(), std::ostream_iterator<int>(std::cout, "\t"));
        std::cout << "\n";
    }

    void dump() {
        show("A: ", a);
        show("B: ", b);
        show("C: ", c);
        std::cout << "\n";
    }

    void MoveDisc(Tower &a, Tower &b) {
        b.push_back(a.back());
        a.pop_back();
    }

    void MoveStack(Tower &a, Tower &b, Tower &c, size_t size) {
        if (size == 0)
            return;
        MoveStack(a, c, b, size - 1);
        MoveDisc(a, b);
        dump();
        MoveStack(c, b, a, size - 1);
    }
public:
    Hanoi(std::initializer_list<int> input) : a(input) { dump(); }

    void operator()() { MoveStack(a, c, b, a.size()); }
};

int main() {
    Hanoi h { 30, 25, 20, 15, 10, 5 };
    h();
}

Here are just a few observations that may help you improve your code.  In all, it is quite clean and easy to read and understand.

Use const where practical

The show() and dump() function don't (and shouldn't) alter the underlying object, so they should be declared const:

void show(std::string const &label, Tower c) const;
void dump() const;

Use reserve() to prevent reallocations

The constructor can easily determine the maximum size of the vectors.  If there were much larger ones than this, one could avoid potentially costly memory reallocations by using Tower.reserve() within the constructor.

Prefer flexible output

This is admittedly a very simple program, but generally it's nice to allow for printing to any std::ostream rather than having std::cout hardcoded into the class.  Perhaps it could be a constructor argument.

Use all required #includes

The code makes use of std::initializer_list but doesn't have this line:

#include <initializer_list>

However, as @Morwenn points out in a comment, it's not needed to include it explicitly since std::vector must necessarily already include it.


Billboard challenge algorithm This is a problem from Interview Street in Dynamic Programming section.

  Billboards (20 points)

  ADZEN is a very popular advertising firm in your city. In every road
  you can see their advertising billboards. Recently they are facing a
  serious challenge , MG Road the most used and beautiful road in your
  city has been almost filled by the billboards and this is having a
  negative effect on the natural view.

  On people's demand ADZEN has decided to remove some of the billboards
  in such a way that there are no more than K billboards standing
  together in any part of the road.

  You may assume the MG Road to be a straight line with N
  billboards.Initially there is no gap between any two adjecent
  billboards.

  ADZEN's primary income comes from these billboards so the billboard
  removing process has to be done in such a way that the billboards
  remaining at end should give maximum possible profit among all
  possible final configurations.Total profit of a configuration is the
  sum of the profit values of all billboards present in that
  configuration.

  Given N,K and the profit value of each of the N billboards, output the
  maximum profit that can be obtained from the remaining billboards
  under the conditions given.

  Input description

  1st line contain two space separated integers N and K. Then follow N
  lines describing the profit value of each billboard i.e ith line
  contains the profit value of ith billboard.

  Sample Input

6 2
1
2
3
1
6
10

  Sample Output

21

  Explanation

  In given input there are 6 billboards and after the process no more
  than 2 should be together.

  So remove 1st and 4th billboards giving a configuration _ 2 3 _ 6 10
  having a profit of 21. No other configuration has a profit more than
  21. So the answer is 21.

  Constraints

1 <= N <= 100,000 (10^5)
1 <= K <= N
0 <= profit value of any billboard <= 2,000,000,000 (2 * 10^9)

My solution (psuedocode):

Let Profit[i] denote the Profit from ith billboard.
(i, j) denotes the range of billboards
MaxProfit(i, j) for all (i, j) such that i<=j and i-j+1 <= K is:
    MaxProfit(i, j) = Profit[i] + Profit[i+1] + ... + Profit[j];

For other (i,j) MaxProfit equals,

MaxProfit(i, j)
{
    max = 0;
    for all k such that i<=k<=j // k denotes that, that position has no   billboard
    {
        temp = MaxProfit(i, k-1) + MaxProfit(k+1, j);
        if(temp > max)
        max = temp;
    }
    return max;
}

Code:

#include<stdio.h>
#include<stdlib.h>

long **DP;
long returnValue(long i, long j);
int main()
{
    long N, K;
    scanf("%ld", &N);
    scanf("%ld" ,&K);

    DP = (long**) malloc(sizeof(long *) * N);

    long ARR[N];
    long j, z, i=0;
    for(i=0;i<N;i++)
    {
        DP[i] = (long*)malloc(sizeof(long) * N);
        scanf("%ld", &ARR[i]);
    }

    for(i=0;i<N;i++)
    {
        long sum=0;
        for(j=1;j<=K && (i+j-1)<N;j++)
        {
            sum += ARR[i+j-1];
            DP[i][i+j-1] = sum;
        }
    }

    for(j=K+1;j<=N;j++)
    {
        for(i=0;i<=N-j;i++)
        {
            long max = 0;
            for(z=i;z<=i+j-1;z++)
            {
                long temp = returnValue(i, z-1) +     returnValue(z+1, i+j-1);
                if(temp > max)
                    max= temp;
            }
            DP[i][i+j-1] = max;
        }
    }
    printf("%ld\n", DP[0][N-1]);
    for(i=0;i<N;i++)
    {
            free(DP[i]);
    }
    free(DP);
}

long returnValue(long i, long j)
{
    if(i<=j)
    {
        //printf("%d\n", DP[i][j]);
        return DP[i][j];
    }
    else
        return 0;
}

My solution is of order O(N2). So I get TLE and Segmentation fault for larger N. I have already passed 6/10 test cases. I need to pass remaining 4.

The inputs have to be limited with constraints you have listed, then

use a TreeSet to store the N objects in it
remove the K first
print the Set

few lines in Java (no malloc problem, memory managed by JVM):

read the first input
use String.split() the line,
use Integer.valueOf(String) to convert
compare N,K and stop if not correct or if negative
use a while(<N) for input,
test limits of input whith if and negative values
you can use toString() to print the Set, but you have to use a
foreach to sum the result
print result


Creating ShortCut (RightClick) Menus in Access I'm writing a tool to create shortcut menus for an application that I maintain built on the Access runtime. I compile this app into an *.accde file, so much to my chagrin, all right click menus are disabled.

After some research, I've been able to develop this tool to add the menus to the database. This code is called during development, not at runtime.

I'm already seeing a lot of duplication, and am not sure what to do about it. So, I figured I'd stop and get some feedback before I started building all of the various menus that I'm going to need.

Note: There is not a built in Enumeration of all of the different button types. There are thousands of them and I'm adding them to a self defined enum as I need them.

Option Explicit

Private Enum ButtonControls
    bcCopy = 19
    bcCut = 21
    bcPaste = 22
    bcSortSmallToLarge = 210
    bcSortSortLargeToSmall = 211
    bcDelete = 478
    bcNewRecord = 539
    bcRowHeight = 541
    bcDeleteRecord = 644
End Enum

Private Sub CreateFormDatasheetClipboard()
    Dim menu As Office.CommandBar
    Set menu = CommandBars.Add("FrmDsClipboard", msoBarPopup, False)

    With menu.Controls
        .Add msoControlButton, bcCut
        .Add msoControlButton, bcCopy
        .Add msoControlButton, bcPaste
    End With
    Set menu = Nothing
End Sub

Private Sub CreateFormDataSheetCopyOnly()
    Dim menu As Office.CommandBar
    Set menu = CommandBars.Add("FrmDsCopyOnly", msoBarPopup, False)

    menu.Controls.Add msoControlButton, bcCopy

    Set menu = Nothing
End Sub

Private Sub CreateFormDataSheetRow()
    Dim menu As Office.CommandBar
    Set menu = CommandBars.Add("FrmDsRow", msoBarPopup, False)

    With menu.Controls
        .Add msoControlButton, bcNewRecord
        .Add msoControlButton, bcDeleteRecord
        .Add msoControlButton, bcCut
        .Add msoControlButton, bcCopy
        .Add msoControlButton, bcPaste
        .Add msoControlButton, bcRowHeight
    End With

    Set menu = Nothing
End Sub

Private Sub CreateFormDataSheetRowClipboardOnly()
    Dim menu As Office.CommandBar
    Set menu = CommandBars.Add("FrmDsRowClipboardOnly", msoBarPopup, False)

    With menu.Controls
        .Add msoControlButton, bcCut
        .Add msoControlButton, bcCopy
        .Add msoControlButton, bcPaste
    End With

    Set menu = Nothing
End Sub

Private Sub RemoveAllCustomMenus()
    Dim cbar As CommandBar
    For Each cbar In CommandBars
        If Not cbar.BuiltIn Then
            cbar.Delete
        End If
    Next
End Sub

Private Sub CreateAllCustomMenus()
    RemoveAllCustomMenus
    CreateFormDatasheetClipboard
    CreateFormDataSheetCopyOnly
    CreateFormDataSheetRow
    CreateFormDataSheetRowClipboardOnly
End Sub

As you already noticed, all the CreateSomething methods have the same repeating pattern. We can extract this pattern and encapsulate it in a method for reuse :

Private Sub CreateCommandBar(name As String, ParamArray buttonIDs() As Variant)
    Dim menu As Office.CommandBar
    Set menu = CommandBars.Add(name, msoBarPopup, False)

    For Each ID In buttonIDs
        menu.Controls.Add msoControlButton, i
    Next

    Set menu = Nothing
End Sub

And, CreateFormDataSheetRowClipboardOnly could be written as :

Private Sub CreateFormDataSheetRowClipboardOnly()
    CreateCommandBar "asd", bcCut, bcCopy, bcPaste
End Sub


Loop through folder names on a server directory I have the following code to loop through a directory of folders and print all folders names with their paths in a worksheet. This is a follow up question to this one: Excel VBA - Get Folder Names
Looking for code optimization for faster and better performance.

Sub PrintFolders()

    Dim wb As Workbook
    Dim ws As Worksheet
    Dim objFSO As Object
    Dim objFolder As Object
    Dim objSubFolder As Object
    Dim i As Integer
    Dim Folder_Name As String

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual
    Application.StatusBar = ""

    Set wb = ThisWorkbook
    Set wsControl = wb.Sheets("Control"): Set wsOutput = wb.Sheets("Output")
    Folder_Name = wsControl.Cells(1, 2)
    If Folder_Name = "" Then
        MsgBox "Path location is not entered. Please enter path"
        wsControl.Cells(1, 2).Select
        End
    End If
    Set objFSO = CreateObject("Scripting.FileSystemObject")
    Set objFolder = objFSO.GetFolder(Folder_Name)

    i = 1
    Dim MyArr() As Variant
    ReDim MyArr(1 To i, 1 To 2)
    On Error GoTo CleanFail

    Application.EnableCancelKey = xlErrorHandler
    Const IterationsToUpdate As Integer = 10
    For Each objSubFolder In objFolder.subfolders
        MyArr(i, 1) = objSubFolder.Name
        MyArr(i, 2) = objSubFolder.Path
        i = i + 1
            MyArr = Application.Transpose(MyArr)
            ReDim Preserve MyArr(1 To 2, 1 To i)
            MyArr = Application.Transpose(MyArr)
        If i Mod IterationsToUpdate = 0 Then
            Application.StatusBar = objSubFolder.Path & " " & objSubFolder.Name
            DoEvents
        End If
    Next objSubFolder
    Application.StatusBar = ""

    wsOutput.Rows("2:1048576").Delete
    Dim Destination As Range
    Set Destination = wsOutput.Range("A2")
    Destination.Resize(UBound(MyArr, 1), UBound(MyArr, 2)).Value = MyArr
    wsOutput.Columns.EntireColumn.AutoFit: wsOutput.UsedRange.HorizontalAlignment = xlCenter
    wsOutput.Activate

    MsgBox ("Done")

    CleanExit:
    Application.StatusBar = False
    Application.StatusBar = ""
    Application.Cursor = xlDefault
    Application.ScreenUpdating = True
    Application.Calculation = xlCalculationAutomatic
    Exit Sub

    CleanFail:
    Const MsgTitle As String = "Operation not completed"
    If Err.Number = 18 Then
        MsgBox "Operation was cancelled.", vbInformation, MsgTitle
    Else
        MsgBox "An error has occurred: " & Err.Description, vbCritical, MsgTitle
    End If
    Resume CleanExit

End Sub

Performance

If you really want to squeeze out every ounce of performance, I recommend early binding the FileSystemObject and related classes. From the "What form of binding should I use" from this MSDN article:

  Early binding is the preferred method. It is the best performer
  because your application binds directly to the address of the function
  being called and there is no extra overhead in doing a run-time
  lookup. In terms of overall execution speed, it is at least twice as
  fast as late binding.

Beyond that, there's not much more you can do in the way of performance. Most of the time here is going to talking to the remote file system. You could remove the ability to cancel or not update the status bar, but it's my belief the UX would suffer too much.

One thing you could do though is alter how often you update the status bar based on how many subfolders are found. Instead of having IterationsToUpdate be a constant, make it a variable and figure out a way to calculate how often the status bar should be updated using the SubFolder's Count property. Perhaps update at each 5%.

    IterationsToUpdate = CInt(Subfolders.Count * 0.05)

Update:

I changed my mind. There is something more you can do to make the performance better. Several things actually, but I'll let you decide if the second one is really worth it or not.

Stop Re-dimensioning the array. You already know how big it should be, so Dim it once and only once.

Dim MyArr(1 To objFolder.Subfolders.Count, 1 To 2) As Variant

This removes a lot of overhead inside of the loop. When you ReDim Preserve, you're effectively making a copy of the array at each iteration. There's no reason to do this when you know how big the array should be up front.

2. You could unroll the loop. Be aware, that doing this will cause a maintenance headache, but it *could significantly speed up the loop through the subfolders. This works by setting multiple "positions" of your array during each iteration.

*I do mean could. There's no way of knowing if it will actually perform better or not until it is tried and benchmarked.

Unfortunately, you can't access items in the Folders collection by integral index, only by key, so unrolling the loop is not an option.

Misc

Why delete the entire range here? Are all of those rows always filled? Could there ever be more?

wsOutput.Rows("2:1048576").Delete

It would be better to find the last non-empty row.
Why are you activating here? Smells like an unintended side effect to me.

 wsOutput.Activate

Perhaps you meant to select the range for the user? If so, the correct way to do that is this.

 wsOutput.Select

You don't need parenthesis here.

MsgBox ("Done")

You're wasting at least a few cycles telling the runtime to evaluate the default property of... a literal. I recommend reading over this StackOverflow Q & A that details the rules about using parenthesis in a routine call for more information.

It would also be nice to display the "Information" icon on the message box. It's a nicer UX.

 MsgBox "Done", vbInformation

Consider using another constant here instead of the literal 18.

If Err.Number = 18 Then

It's pretty clear what's going on from the context, but if I want to know exactly what that error is, I'd have to look it up. A (well named) constant keeps the maintainer in the IDE as it removes any ambiguity.


Building a parent/child tree relationship I have a problem: build a tree relationship parent/child like this one:

Can you review this piece of code please since I am convinced it could be improved?

struct item {
    char value;
    std::list<item*> children;
};

I write a comparison function for a sort :

bool compareChild(const item *first, const item *second)
{
    return ( first->value < second->value );
}

a function to get the parent:

item *getTopParent(const char src_data[][2], int len)
{
    for (int i = 0; i < len; ++i)
    {
        if (src_data[i][0] == '\0')
        {
            item* head = new item;
            head->value = src_data[i][1];
            return head;
        }
    }
    return NULL;
}

Two functions to push the children:

bool PushChildren(const char src_data[][2], int len, item *parent)
{
    bool parentGotChildren = false;

    for (int i = 0; i < len; ++i)
    {
        if (src_data[i][0] == parent->value)
        {
            item* child = new item;
            child->value = src_data[i][1];
            parent->children.push_back(child);
            parentGotChildren = true;
        }
    }
    parent->children.sort(compareChild);

    return parentGotChildren;
}

bool PushChildrenforAParent(const char src_data[][2], item *parent, int len)
{
    bool hasGotAChild = true;
    std::list<item*>::iterator itchild = parent->children.begin();
    std::list<item*>::iterator childLast = parent->children.end();

    while (hasGotAChild && (itchild != childLast))
    {
        hasGotAChild = PushChildren( src_data, len, *itchild);
        if (hasGotAChild)
        {
            hasGotAChild = PushChildrenforAParent( src_data, *itchild, len);
        }
        ++itchild;
    }
    return hasGotAChild;
}

A function to initialise the "tree":

item* initialize_tree(const char src_data[][2], int len)
{
    bool hasTopParentGotAChild = false;
    //initialisation of the head
    item *head = getTopParent( src_data, len);

    if (head != NULL)
    {
        hasTopParentGotAChild = PushChildren( src_data, len, head);

        if (head != NULL)
        {
            if (hasTopParentGotAChild)
            {
                PushChildrenforAParent(src_data, head, len);
            }
            return head;
        }
        else
        {
            return NULL;
        }
    }
    return NULL;
}

Some quick comments:

Your code style is all over the place. You have three different styles for function names: initialize_tree, PushChildren and getTopParent, pick one and stick to it. I would recommend the last one.
Your code looks more like C and not C++ with what I presume to be free functions.
item is an inappropriate name, a more typical name is Node.
You really should push the functionality to modify the nodes, or tree in general onto the item type and turn it into a class using member functions (methods).
You are allocating memory with new but never freeing it with delete thus you are leaking memory like it's 1995.
You are handling raw pointers, this is not recommended. Prefer to use the std containers or smart pointers for managing memory.
If you feel that you need to explain something to the reader, that's usually a hint that your code needs better structure. hint
std::list is slow, prefer to use std::vector on modern CPUs. Vector is \$\mathcal{O}(n)\$ insertion and removal at the front while list is \$\mathcal{O}(1)\$. However the cost of poor locality of reference on the list means that the cross-over point when the list becomes faster than the vector for just about anything at all is when you have about 100-1000 elements (depending on size) and are hitting the worst case for vector. Which I doubt you won't reach in your program. So unless you have a very specific reason to use std::list prefer a std::vector.


C# with Oracle ODP.NET int number;
DateTime dateTime;

using (var oracleConnection = new OracleConnection(ConnectionString))
using (var oracleCommand = oracleConnection.CreateCommand())
{
    oracleConnection.Open();

    oracleCommand.CommandText = "SELECT * FROM FROM MY_TABLE";

    using (var reader = oracleCommand.ExecuteReader())
    {
       while (reader.Read())
       {
          number = reader.GetInt32(0);
          dateTime = reader.GetDateTime(1);
       }
    }
}

Which is a better way to avoid the calls such as reader.GetInt32(0)?

This way is too hard to read.  A better way would be something like reader.GetInt32("ID") or reader.GetDateTime("Begin"), where ID and Begin are column names.

Or should I use enums?

A few comments:
Exceptions:
Keep in mind, even with using-statements, exceptions can happen. Make sure to catch them. I'd suggest, you wrap the whole snippet in a try-catch-block if that didn't happen already.
Types:
Why are you using the dynamyic type? Make it easier for readers and yourself by using the correct type. For yourself? Yes, if you use the explicit type, you get way better IntelliSense  proposals.
Accessing:
reading a bit through the msdn documentation for OracleDataReader I found, that there is a property for the reader, named Item with 2 overloads. One being Item[String].
It seems they return the Column Value "in it's native format".
using (OracleDataReader reader = oracleCommand.ExecuteReader())
{
   while (reader.Read())
   {
      number = reader.Item["ID"];
      dateTime = reader.Item["Begin"];
   }
}

Update:
@Sandeep claims that the return type is in fact Object. If that is the case, you will have to run a conversion: number = Convert.ToInt32(reader.Item["ID"]);
Furthermore it seems that reader.Item[index] can actually be replaced by reader[index].


Largest number order in string Given a string, assuming the string is numbers only, rearrange the string to that it is the greatest possible number.

Below is my solution to the problem above. I'd like feedback mainly on the algorithm, as I wrote this up very quickly. I used comprehension syntax to find the largest num, which I'm trying to use more. Any other feedback is appreciated as well.

def largest_number(string: str) -> str:
    """
    Given a string, organize the numbers such as the
    rearrangement is now the largest possible number
    """
    string = list(string)
    largest = ""
    for _ in string:
        largest_num = str(max([int(num) for num in string]))
        largest += largest_num
        string[string.index(largest_num)] = -1
    return largest

if __name__ == '__main__':

    # Test Cases #

    assert largest_number("12345") == "54321"
    assert largest_number("4689123") == "9864321"
    assert largest_number("9") == "9"
    assert largest_number("1923") == "9321"

This is an improvement on the answer by @coderodde. Just like they said, you can do this in \$\mathcal{O}(n)\$ time by counting how often each digit appears and then using a fixed output format. However, I would use collections.Counter from the standard library and string multiplication to achieve this goal:

from collections import Counter

def largest_number(string: str) -> str:
    counters = Counter(map(int, string))
    return "".join(str(d) * counters[d] for d in range(9, -1, -1))

Compared to sorting, as presented in the answer by @bullseye, and the original answer, this compares OK. It has a slight overhead for small strings due to the Counter object, but in the end it is faster than hardcoding character values and on the same level as sorting, but not better.

However, all three are vastly better than your \$\mathcal{O}(n^2)\$ algorithm (note the vastly different x limits):

As soon as you have three or more testcases, it might make sense to not repeat the testing code. Just put the input and expected output into a data structure (a list of tuples would do here), and iterate over it:

if __name__ == '__main__':
    test_cases = [("12345", "54321"), ("4689123", "9864321"), ("9", "9"), ("", "")]
    for string, expected in test_cases:
        output = largest_number(string)
        if output != expected:
            raise AssertionError(f"{string!r} gave back {output!r}"
                                 f" instead of {expected!r}")

For more complicated programs you might want to look at a testing framework like unittest.


Simple interface and class for holding text to be analyzed I am making a sentiment analysis library targeted first at social media text (like Twitter) and then I will expand it for more general use.

For the first step I am making a simple interface and a class that is going to be used for storing some data about the text to be analyzed like the text itself, it's manually annotated sentiment and the calculated sentiment that must be set only once (that's my current design anyway).

ObjectForAnalysis.java interface:

package com.github.dkanellis.sa_for_socialmedia.sentiment.objects;

public interface ObjectForAnalysis {

    double getManuallyAnnotatedMeanSentiment();

    double getCalculatedMeanSentiment();

    boolean isCalculated();

    void setCalculatedMeanSentiment(double calculatedMeanSentiment);
}

TwitterStatus.java implementation:

package com.github.dkanellis.sa_for_socialmedia.sentiment.objects;

public class TwitterStatus implements ObjectForAnalysis {

    private final long id;
    private final String text;
    private final double manuallyAnnotatedMeanSentiment;
    private double calculatedMeanSentiment;
    private boolean calculated;

    public TwitterStatus(long id, String text, double manuallyAnnotatedMeanSentiment) {
        this.id = id;
        this.text = text;
        this.manuallyAnnotatedMeanSentiment = manuallyAnnotatedMeanSentiment;
        this.calculated = false;
    }

    @Override
    public double getManuallyAnnotatedMeanSentiment() {
        return manuallyAnnotatedMeanSentiment;
    }

    @Override
    public double getCalculatedMeanSentiment() {
        if (isCalculated()) {
            return calculatedMeanSentiment;
        } else {
            throw new IllegalStateException("Mean sentiment has not been calculated yet.");
        }
    }

    @Override
    public boolean isCalculated() {
        return calculated;
    }

    @Override
    public void setCalculatedMeanSentiment(double calculatedMeanSentiment) {
        if (isCalculated()) {
            throw new IllegalStateException("Mean sentiment has already been calculated.");
        } else {
            this.calculatedMeanSentiment = calculatedMeanSentiment;
            calculated = true;
        }
    }

    @Override
    public String toString() {
        StringBuilder buffer = new StringBuilder();
        buffer.append("Tweet: ").append(id).append("\n");
        buffer.append("Text: ").append(text).append("\n");
        buffer.append("Manual Annotation: ").append(manuallyAnnotatedMeanSentiment).append("\n");
        buffer.append("Calculated Annotation: ")
                .append(isCalculated() ? "Not calculated" : calculatedMeanSentiment).append("\n");

        return buffer.toString();
    }

}

Feel free to critique anything from package naming to class naming to the way I implemented my constraint of having the mean sentiment set only once.

Your concrete TwitterStatus class would, in a single-threaded application, be OK for the purpose of having a single-set ("lazy set", or "set and freeze") mechanism. Two concurrent threads each setting the value, or one thread setting, and another thread getting the value may cause problems though.

The situation can be relatively easily resolved with an AtomicReference value though...

Consider the following value that replaces both the calculatedMeanSentiment and calculated fields:

private final AtomicReference<Double> calculatedMeanSentiment = new AtomicReference<>(null);

Then, in your methods, you can have:

@Override
public double getCalculatedMeanSentiment() {
    Double val = calculatedMeanSentiment.get();
    if (val == null) {
        throw new IllegalStateException("Mean sentiment has not been calculated yet.");
    }
    return val;
}

@Override
public void setCalculatedMeanSentiment(double calculatedMeanSentiment) {
    if (!this.calculatedMeanSentiment.compareAndSet(null, calculatedMeanSentiment)) {
        throw new IllegalStateException("Mean sentiment has already been calculated.");
    }
}

@Override
public boolean isCalculated() {
    return calculatedMeanSentiment.get() != null;
}

Notice also that when you throw, or return from the 'if' part of an if statement, there is no need to have the 'else' part. Specifically, you have structures like this:

    if (isCalculated()) {
        return calculatedMeanSentiment;
    } else {
        throw new IllegalStateException("Mean sentiment has not been calculated yet.");
    }

that return in the if. They can be easily rewritten as 'guard conditions':

    if (!isCalculated()) {
        throw new IllegalStateException("Mean sentiment has not been calculated yet.");
    }
    return calculatedMeanSentiment;


LinkedList implementation with Iterators in Java I am a newbie to Java. I will be glad to hear your opinion with regards to this LinkedList implementation. The Iterators are self implemented by my decision:

public interface Iterator {

    public boolean hasNext();
    public Object Next();

}

public class LinkedList {

    private Node mBase;

    private class ListIterator implements Iterator {

        private Node iterator;

        ListIterator(){
            iterator = mBase;
        }
        @Override
        public boolean hasNext(){
            return (iterator.getNext() != null);
        }
        @Override
        public Object Next() {
            Object userData = iterator.getData();
            iterator = iterator.getNext();
            return userData;
        }
    }

    static private class Node {

        private Node next;
        private Object data;

        Node(Object data, Node next) {
            this.data = data;
            this.next = next;
        }

        Object getData(){
            return data;
        }

        Node getNext(){
            return next;
        }

        boolean isNext(){
            if (next == null) return (false);

            return (true);
        }
    }

    public void push(Object data){
        Node newNode = new Node(data, mBase);
        mBase = newNode;
    }

    public Object pop(){

        Node baseNode = mBase;
        Object userdata = null;

        if (isEmpty())
        {
            return (null);
        }

        mBase = mBase.getNext();
        baseNode.next = null;
        userdata = baseNode.getData();
        baseNode = null;

        return (userdata);
    }

    public long getSize(){

        Node current = mBase;
        int counter = 0;

        if (isEmpty()){
            return (0);
        }

        while (current.isNext()){
            current = current.getNext();
            ++counter;
        }

        ++counter;

        return (counter);

    };

    public ListIterator getItr(){
        return new ListIterator();
    };

    public boolean isEmpty(){
        return (mBase == null);
    }

}

public class Main {

    public static void main(String[] args) {
        // TODO Auto-generated method stub

        LinkedList list = new LinkedList();

        System.out.print("isEmpty. Print true:");
        System.out.println(list.isEmpty());

        list.push(1);
        list.push(2);
        list.push(3);
        list.push("Hello");
        list.push("World");

        System.out.print("isEmpty. Print false:");
        System.out.println(list.isEmpty());

        System.out.print("getSize(). Should be 5: ");
        System.out.println(list.getSize());

        Iterator itr = list.getItr();

        System.out.print("itr.hasNext() (should be true)");
        System.out.println(itr.hasNext());

        while (itr.hasNext()) {
            System.out.print("itr.getNext()");
            System.out.println(itr.Next());
        }

        System.out.print("itr.getNext()");
        System.out.println(itr.Next());

        while(!list.isEmpty()){

            System.out.print("itr.list.pop()");
            System.out.println(list.pop());
        }

        System.out.print("getSize(). Should be 0: ");
        System.out.println(list.getSize());

    }

}

Use (at least parts of) the source, Luke

You mentioned that you decided to implement iterators on your own, but you could at least use the standard Iterator interface. It's almost the same as what you did, having only two methods, hasNext, and next. Not only it's standard so that it should be familiar to all readers, those are better names too, and follow the camelCase convention of the language.

What's in a name?

Using a common vocabulary helps programmers understand each other's code. It's OK to implement a common library class on your own for practice, but to facilitate communication, it's good to stick to the common vocabulary. So in the future I suggest to use the same names for methods used by the JDK, unless there is a good reason to deviate.

Inner classes

When possible, it's good to make inner classes static, so that instances of those classes don't hold a reference to the enclosing class. You could make ListIterator static by passing mBase to its constructor.

Performance

The getSize method had \$O(n)\$ time complexity, because it iterates over the elements every time it's called. (Btw, it's a bit ironic that the implementation doesn't use the iterator, what a missed opportunity!) You could do better, at the expense of an integer field, incremented when you add a new item, decremented when you remove an item. The there time complexity would become \$O(1)\$.

Simplify

This can be written simpler:

   boolean isNext(){
        if (next == null) return (false);
        return (true);
    }

Like this:

   boolean isNext() {
        return next != null;
    }


Project Euler #7 in C: find 10001st prime using something like Sieve of Eratosthenes I've gotten a solution for Project Euler #7 in C (find the 10,001st prime). I came up with the very simple algorithm myself (from what I can tell it's similar, if not identical, to the Sieve of Eratosthenes).

#include <stdio.h>

#include <stdlib.h>

long prime_finder(int amount_of_primes);

int main(void)
{
int amount_of_primes = 0;

printf("How many primes would you like?\n");
scanf("%d", &amount_of_primes);

printf("%d: %ld\n", amount_of_primes, prime_finder(amount_of_primes));

return 0;
}

long prime_finder(int amount_of_primes)
{
int total_primes_found = 0;

long * primes = malloc(sizeof(long) * amount_of_primes);

for(int i = 0;i < amount_of_primes; i++) //set everything to 0
{
    primes[i] = 0;
}

for(long i = 1; total_primes_found < amount_of_primes;i++) //find the primes
{
    if(i<14) //cheat a little for the first primes
    {
        switch (i)
        {
            case 1:
                break;

            case 2:
                primes[total_primes_found++] = 2;
                break;

            case 3:
                primes[total_primes_found++] = 3;
                break;

            case 5:
                primes[total_primes_found++] = 5;
                break;

            case 7:
                primes[total_primes_found++] = 7;
                break;

            case 11:
                primes[total_primes_found++] = 11;
                break;

            case 13:
                primes[total_primes_found++] = 13;
                break;

            default:
                break;
        }
    }

    else //if it is a larger number
    {
        if (i % 2 == 0 || i % 3 == 0 || i % 5 == 0
|| i % 7 == 0 || i % 11 == 0 || i % 13 == 0) /*makes the program a little quicker if the
*current number divides by a low number like 2 or 3*/
        {
            goto End;
        }

        else
        {
            for(int j = 0; j < total_primes_found; j++) //the brute force part of the program
            {
                if(i % primes[j] == 0)
                {
                    goto End;
                }
            }

            primes[total_primes_found++] = i;
        }

        End:;

    }
}

for(int i = 0 ; i < amount_of_primes - 1 ; i++)
    printf("%d: %ld\n", i+1, primes[i]);

return primes[amount_of_primes - 1];
}

Your code is using trial division (i % primes[j] == 0) and thus it is not at all similar to the Sieve of Eratosthenes - or any prime sieve, for that matter.

The core idea of prime sieves is that you have some sort of array with a flag for each of the candidate numbers - the sieve - and that all the non-primes in that array get struck off in some fashion so that whatever is left must be prime.

The hallmark of Eratosthenean sieving is the avoidance of multiplication and/or division during the striking-off process; multiples of a number are enumerated by repeated addition of that number. That's why it can be several orders of magnitude faster than the alternatives.

There's nothing wrong with trial division as such, as long as you find it fast enough for your purposes. Had the Euler problem description asked for the 100,000,001st prime then things would look different, though, and you'd need a lot of patience for approaches based on trial division. So much so that you'd be faster programming a nice sieve instead of waiting for the trial divider to finish...

Next time, start with the simplest, most straightforward implementation of your idea and leave out the supposed optimisations. Don't start 'optimising' until you have proof that it's necessary, and keep optimisations only if the measured improvement significantly outweighs the resulting uglification of your source code. If you do some measurements then you'll find that only appreciable effect of your current 'optimisations' is to make the code unreadable.

Starting with the simplest, cleanest rendering of your algorithm might help you avoid mistakes like trial dividing by the primes 2 .. 13 twice (first explicitly in a branch statement, then again implicitly when checking against the array of found primes).

Also, in order to prove the compositeness of a number you only need to check potential factors up to the square root of the number. You are checking all primes less than that number (i.e. all primes found so far), which is a lot more than necessary.

Note: explicitly trial-dividing by a handful of the smallest primes is indeed a valid optimisation that can also be found in industrial-strength code. It works because dividing by constants enables compilers to pick more efficient means than actual division, like using multiplication and shifting, or testing bit 0 for computing x % 2. However, like all optimisations it needs not only the proof of validity but also the proof of necessity in order to make the cut.


Simple form validation in javascript Used to use Laravel's easy to use form validation class, decided to mimic it in JS. I've been learning JS for 4 days now and decided to write my first piece of real code.

I'm pretty sure its full of bad practices, and a lot of it can be improved. I'm looking for some constructive criticism on what I can do better next time, thanks in advance.

Class:

class Validator {
    make(rules) {
        this.rules = rules;
    }

    valid() {
        if (!this.hasOwnProperty('rules')) {
            return true;
        }

        let errors = [];
        let checkFunction = this.checkRuleForElement; // Storing this here because it doesn't seem to work in its invoked scope.

        Object.keys(rules).forEach(function(key) {
            let formElement = document.getElementsByName(key)[0];

            if (formElement == undefined) {
                return;
            }

            let ruleSet = rules[key].split('|');

            ruleSet.forEach((rule) => {
                let result = checkFunction(rule, formElement)

                if (!result.valid && result.message != undefined) {
                    errors.push(result.message);
                }
            });
        });

        return {
            valid: errors.length == 0,
            errors: errors
        }
    }

    checkRuleForElement(rule, formElement) {
        let isValid = true;
        let error = "no error";

        switch (rule) {
            case "required":
                if (formElement.value.length <= 0) {
                    isValid = false;
                    error = `Form element ${formElement.getAttribute("name")} is required.`;
                }
                break;
             // todo: alpha => only alpha characters
             // todo: alphanum => Alphanumeric characters
             // todo: num => Numbers only
        }

        return {
            valid: isValid,
            message: error
        };
    }
}

Setup (usage):

let validator = new Validator();

function checkValidation() {
    validator.make({
        "username": "required",
        "password": "required",
    });

    let result = validator.valid();

    if (!result.valid) {
        alert('Something went wrong.');
        alert('Errors are: ' + result.errors[0]);
    }
    else {
        alert('Validation was a success.');
    }
}

I must admit this is pretty good code for someone that has just started, I like that you use the more "functional" style Javascript allow you to use . So good job :) But there are some design issues I see with the code.

1. Factory vs constructor

Your Validator has a make method which takes the rules in, It is basically used as a factory method (A factory is a design pattern if you are not familuar with it). But it has none of the benefits a factory gives you, since it is a instance method.

Having to instantiate a Validator object and then being forced to call make is cumbersome. Instead I suggest you make the make function static and add a constructor

class Validator
{
   static make(rules)
   {
        return new Validator(rules);
   }

   constructor(rules)
   {
       this.rules = rules;
   }
}

2. Checking class properties

You don't need to do this check in your valid function.

if (!this.hasOwnProperty('rules')) {
    return true;
}

Since it's practically not possible to not have rules with the proposed factory. I understand why this is here though. You wanted to make sure you can't forget the make call after instantiating a Validator object. But that's a wrong schematic. If the caller uses your code wrong it should fail, and not say "I don't care, but this is valid"

3. Naming

Methods should indicate that they are actions, and properties that they are values.

Your valid method sounds a lot like a property, like a value. I would call it validate since that is a verb and makes it clear that it does something and not "is" something. And I would rename the property valid that your return in your valid method to isValid

4. ES6

EDIT: I noticed to late that you already used an arrow function in your code. Ignore the explanation if you want and just replace the Object.keys().forEach(callback) callback with an arrow function. BTW you do not need the parentheses if you only have one parameter.

You can use a arrow function in your forEach(function) function. This is a es6 feature and a bit more advanced tho. Using functions is fine but it can solve the issue you have with scoping.

The function keyword does create a new scope and therefore changes the meaning of this. The arrow function on the other hand behaves exactly like a function but does not change the meaning of this

validate() {
    let errors = [];

    Object.keys(rules).forEach(key => {
        let formElement = document.getElementsByName(key)[0];

        if (formElement == undefined) {
            return;
        }

        let ruleSet = rules[key].split('|');

        ruleSet.forEach((rule) => {
            let result = this.checkRuleForElement(rule, formElement)

            if (!result.valid && result.message != undefined) {
                errors.push(result.message);
            }
        });
    });

    return {
        isValid: errors.length == 0,
        errors
    }
}

Another thing you can leave out is the
errors: errors part. You can shorten it to just errors since the name of the variable is the same as key of the object

5. Decouple things

Currently your "rule running" and "rule definitions" are all in the same class/method (in the switch in checkRuleForElement) So if you want to add a new rule you have to modify your Validator class which should only do one thing: Invoke the rules for a element. The rule definitions are a whole separate concern.

You can try to decouple it by using arrays.

A sample implementation may look like this:

// This is not good code, take it as an "idea" or psuedocode

class Validator
{
   static registeredRules = [];

   static registerRule(name, run)
   {
      Validator.registeredRules.push({ name, run });
   }

   checkRuleForElement(ruleName, formElement)
   {
      var rule = Validator.registeredRule.find(element => element.name === ruleName);
      return rule.run(formElement);
   }
}

6. Abstract things

A thing you may want to consider is that there may be multiple forms or multiple elements, with your current implementation you are relying on the fact that there are is a unique element foreach "rule". Also you are interacting with the Dom directly which makes things hard to change when that assumption changes.

You could solve this by, for example using Dom elements as an input:

Validator.make({
     document.querySelector("#user_input"): "required"
})

Bonus exercises

You can try to add a more advanced "rule parser" and add parameters for rules.

This could look like this:

Validator.make({
  "username": "required|min:5"
})

Another thing you could try is, skipping this whole "naming" thing and using just functions

For example

const required = element => element != undefined;

// If you do not understand this Syntax, it's basically a function that returns a function.
const min = count => element => element.length > count

Validator.make({
  "username": [required, min(5)]
});

This would give you several advantages. For example that your rules can be checked by a static analyzer, If you use strings there is always a chance you mistype something, like use "regired" instead of "required" and not even notice it, with functions on the other hand you will get an error if you use something that is undefined. Also you could reuse rules better.

Have fun :)

I wrote this from my mobile so if there are any mistakes let my know. I will most likely add more when I get my hands on a pc!


Project Euler # 53 Combinatoric selections in Python There are exactly ten ways of selecting three from five, 12345:

123, 124, 125, 134, 135, 145, 234, 235, 245, and 345

In combinatorics, we use the notation, 5C3=10.

In general, nCr = n! / r! (n−r)!, where r≤n, n! = n × (n−1) × ... × 3 × 2 × 1, and 0! = 1.

It is not until n = 23, that a value exceeds one-million: 23C10 = 1144066.

How many, not necessarily distinct, values of nCr for 1≤n≤100, are greater than one-million?

from time import time
from math import factorial

def combinations(n, r):
    """Assumes n, r a set of numbers and r number of numbers to choose from n.
    returns number of combinations."""
    return factorial(n) // (factorial(r) * factorial(n - r))

def generate_combinations(n_range, minimum):
    """generates non-distinct combinations in range n_range inclusive that exceed the minimum."""
    for n in range(1, n_range + 1):
        for r in range(1, n):
            combination = combinations(n, r)
            if combination > minimum:
                yield 1

if __name__ == '__main__':
    START_TIME = time()
    print(sum(generate_combinations(100, 10**6)))
    print(f'Time: {time() - START_TIME} seconds.')

just a little hack if you want a more fast solution,
can become faster if consider the similarity of the nCr and nC(n-r)

dic=[1]

for i in range(1,101):
    val =dic[-1]*i
    dic.append(val)

def choose(n,r):
    sol = dic[n]//(dic[n-r]*dic[r])
    return sol

def func(maxi, valu):
    count = 0
    for i in range(2,maxi+1):
        for j in range(1,i+1):
            val = choose(i,j)
            if  val>valu:
                count+=1
    return count

if __name__ == '__main__':
    from time import time
    START_TIME = time()
    print(func(100, 10**6))
    print(f'Time: {time() - START_TIME} seconds.')

just store all the value of factorial of a number upto 100 in dict and then process

update suggested by  @AJNeufeld , using list to store data than dictonary


Project Euler, Problem 273: finding perfect-square partitions Problem:

  Consider equations of the form: \$a^2 + b^2 = N; 0 \leq a \leq b; a, b, N \in \mathbb{N}\$.

  For \$N=65\$ there are two solutions:

  \$a=1, b=8\$ and \$a=4, b=7\$.

  We call \$S(N)\$ the sum of the values of \$a\$ of all solutions of \$a^2 + b^2 = N\$.

  Thus \$S(65) = 1 + 4 = 5\$.

  Find \$\sum S(N)\$, for all squarefree \$N\$ only divisible by primes of the form \$4k+1\$ with \$4k+1 < 150\$.

My solution is painfully slow:

import math
import itertools
import time

def candidate_range(n):
    cur = 5
    incr = 2
    while cur < n+1:
        yield cur
        cur += incr
        incr ^= 6 # or incr = 6-incr, or however
def sieve(end):
    prime_list = [2, 3]
    sieve_list = [True] * (end+1)
    for each_number in candidate_range(end):
        if sieve_list[each_number]:
            prime_list.append(each_number)
            for multiple in range(each_number*each_number, end+1, each_number):
                sieve_list[multiple] = False
    return prime_list

primes = sieve(150)
goodprimes = []
for prime in primes:
    if prime%(4)==1:
        goodprimes.append(prime)
sum=[]
start_time = time.time()
#get a number that works
print("-------Part 1------")
mi=0
for L in range(1, len(goodprimes)+1):
    sumf=0
    for subset in itertools.combinations(goodprimes, L):
        max=2**L/2
        n=1
        for x in subset:
            n*=x

        for b in range(math.floor(math.sqrt(n/2)), math.floor(math.sqrt(n)+1)):
            a=math.sqrt(n-b*b)
            if a.is_integer() and b>=a:
                sum.append(a)
                mi+=1
                if mi==max:
                    mi=0
                    break
    for num in sum:
        sumf+=num
    print(L,sumf, "--- %s seconds ---" % (time.time() - start_time))

                    #q+=1

print("--- %s seconds ---" % (time.time() - start_time))
sumf=0
for num in sum:
    sumf+=num
print(sumf)

The code

The formatting has a number of PEP8 violations.

It's not obvious from the name what candidate_range does. It seems to be a wheel for the sieve. Normally that would be inlined in the sieve; even if you prefer not to do that, you could place the function inside sieve to make its scope clear.

I don't find sieve_list a very helpful name. In general for sieving I prefer is_composite, inverting the booleans from the way you've done it. Similarly for each_number: it reads well on the first line which uses it, but very oddly on the others.

goodprimes = []
for prime in primes:
    if prime%(4)==1:
        goodprimes.append(prime)

It's more Pythonic to use comprehensions:

goodprimes = [p for p in primes if p % 4 == 1]

#get a number that works

What does this mean? It looks more like noise than a useful comment to me.

for L in range(1, len(goodprimes)+1):
    sumf=0
    for subset in itertools.combinations(goodprimes, L):

I don't know why itertools doesn't have a function to give all subsets, but it seems like the kind of thing which is worth pulling out as a separate function, both for reuse and for readability.

        max=2**L/2

What does this do?

        n=1
        for x in subset:
            n*=x

Consider as an alternative

from functools import reduce
import operator

    n = reduce(operator.mul, subset, 1)

        for b in range(math.floor(math.sqrt(n/2)), math.floor(math.sqrt(n)+1)):
            a=math.sqrt(n-b*b)
            if a.is_integer() and b>=a:

Why floors rather than ceils?

Are you certain that math.sqrt on an integer is never out by 1ULP?

Why is b>=a necessary? (Obviously b==a is impossible, and isn't the point of the range chosen to force b > a?)

                sum.append(a)

Is this for debugging? I can't see why you wouldn't just add a to a total.

NB sum is aliasing the builtin function for adding up the values in a list.

                    #q+=1

??? I can't see any other mention of q.

The algorithm

There are a few Project Euler problems which fall to brute force, but in general you need to find the right mathematical insight. Given the way this question is structured, you probably need to figure out how to find \$S(n)\$ given the prime factorisation of \$n\$.


Optimize jQuery selectors code I have this fiddle http://jsfiddle.net/L7zuT/ where I'm working on a small part of a big event program. The functionality is ok, the part of the problem would be the large JavaScript lines needed with the full event program. Is there any way to implement it with DRY (don't repeat yourself) or a better approach for a simpler way?

    13:00-14:30
    18.AAL Solution Deployment Seminar
      9. Results of the AALIANCE Network: Roadmap and more
      7. From    actimetry to ADL ,in the framework of remote medical monitoring of elderly    from living labs to the nursing home
      8. Interoperability APIs and LivingLabs for AAL
      10. Large scale deployment of AAL solutions based on Open    Platform: Challenge and perspectives
    &nbsp;

    14:30-16:00
      1.    User Forum and workshop for exergames in AAL
    &nbsp;

        16:00-17.30
        19.Deployment event - matchmaking session
        &nbsp;

<input id="date_main01" name="date_main01" type="radio" value="01" />
<input id="date_main02" name="date_main01" type="radio" value="02" />
<input id="date_sub03" name="date_sub01" type="radio" value="03" />
<input id="date_sub04" name="date_sub02" type="radio" value="04" />
<input id="date_main05" name="date_main01" type="radio" value="05" />
<input id="date_main06" name="date_main01" type="radio" value="06" />
<input id="date_last07" name="date_last01" type="radio" value="07" />

CSS

tr {
    height: 1px;
}
td{
    height: 100%;
}
label{
    display: block;
    min-height: 100%; /* for the latest browsers which support min-height */
    height: auto !important; /* for newer IE versions */
    height: 100%; /* the only height-related attribute that IE6 does not ignore  */
}
label:hover {
    cursor:pointer;
}

JS

$(document).ready(function(){
    $("input[name='date_main01']").click(function() {
        var test = $(this).val();
        $("input[name='date_sub01']").prop('checked', false);
        $("input[name='date_sub02']").prop('checked', false);
        $("label[for*='date_main']").css("background", "none");
        $("label[for*='date_sub']").css("background", "none");
        $("label[for='date_main"+test+"']").css("background-color", "yellow");
    });
    $("input[name='date_sub01']").click(function() {
        var test = $(this).val();
        $("input[name$='date_main01']").prop('checked', false);
        $("label[for*='date_main']").css("background", "none");
        $("label[for='date_sub"+test+"']").css("background-color", "yellow");
    });
    $("input[name='date_sub02']").click(function() {
        var test = $(this).val();
        $("input[name$='date_main01']").prop('checked', false);
        $("label[for*='date_main']").css("background", "none");
        $("label[for='date_sub"+test+"']").css("background-color", "yellow");
    });
        $("input[name='date_last01']").click(function() {
        var test = $(this).val();
        $("label[for='date_last"+test+"']").css("background-color", "yellow");
    });
});

$("input[name='date_sub01']").prop('checked', false);
 $("input[name='date_sub02']").prop('checked', false);

Can be turned into:

$("input[name='date_sub01'], input[name='date_sub02']").prop('checked', false);

Also if you assign the selectors to a variable your code will be much faster:

var sub_01 = $("input[name='date_sub01']")

You never need to use the method .val().

If you do:

var test = this.value;

You will get the same results without accessing jQuery.

Since you are using background:none alot, you can keep it in an object:

noneBackground: {"background":"none"}

and pass it as an argument

  $("label[for*='date_main']").css(noneBackground);


Reversing a matrix in C# Implementation:

static void ReverseMatrix<T>(T[,] matrix)
{
    int width = matrix.GetLength(1);
    int height = matrix.GetLength(0);
    int startRow = 0;
    int startCol = 0;
    int endRow = height-1;
    int endCol = width-1;

    while (true)
    {
        if (startRow == height-1 || endRow == 0) return;

        // swap
        var temp = matrix[startRow, startCol];
        matrix[startRow, startCol] = matrix[endRow, endCol];
        matrix[endRow, endCol] = temp;

        startCol++;
        endCol--;
        if (startCol == width)
        {
            startRow++;
            startCol = width - 1;
        }
        if (endCol == -1)
        {
            endRow--;
            endCol = 0;
        }
    }
}

Test program:

static void Main()
{
    var matrix = new[,]
    {
        {1, 2, 3},
        {4, 5, 6},
        {7, 8, 9}
    };

    ReverseMatrix(matrix);

    PrintMatrix(matrix);
}

static void PrintMatrix(int[,] matrix)
{
    for (int row = 0; row < matrix.GetLength(0); row++)
    {
        for (int col = 0; col < matrix.GetLength(1); col++)
        {
            Console.Write("{0}\t",matrix[row,col]);
        }
        Console.WriteLine();
    }
}

Input:

1 2 3
4 5 6
7 8 9

Output:

9 8 7
6 5 4
3 2 1

How can I improve this implementation?

I also changed the Reversal code and got rid of all the variables.  from the looks of it you did what I did only you did it with out the use of for loops

I used one loop to go through the rows(or the first dimension of the multi-dimensional array) and then I nested a second loop inside the first to loop through the elements of each array.   now in doing this I needed some independent variables to be incremented for the output matrix so I made sure that I declared them in the right places and incremented them in the right places.   the output matrix was declared using the input matrix so that I didn't have to declare any magic numbers and so that I could minimize the number of variables that I had to declare as well.

enough chit chat here is the code.

    static int[,] ReverseMatrix(int[,] inputMatrix)
    {
        var outputMatrix = new int[inputMatrix.GetLength(0), inputMatrix.GetLength(1)];
        var x = 0;
        for (int i = inputMatrix.GetLength(0) - 1; i >= 0; i--)
        {
            var y = 0;
            for (int j = inputMatrix.GetLength(1) - 1; j >= 0; j--)
            {
                outputMatrix[x, y] = inputMatrix[i, j];
                y++;
            }
            x++;
        }
        return outputMatrix;
    }

Posted as a question also Reversing a Matrix

one thing that I did differently was that I used an input and an output for my method, I personally don't like the way that your code changes a variable outside of the scope like that, if the purpose were to change the original then I wouldn't change my code at all, I would just change how I call the code, I would do it like this

matrix = ReverseMatrix(matrix);

I don't like the way that you did it, I am sure there is a reason for it and a good and bad way to do it, but I don't think this is a good place to use code like this.


ODE45 solver implementation in Python I have a project where I need ODE solver without dependencies to libraries like Scipy. I decide to implement ODE45. According to tutorials from internet and from what I remember from classes I implement it somehow.

The code below contains also example function (pendulum) with exactly same values as in the scipy ODE tutorial https://docs.scipy.org/doc/scipy-0.18.1/reference/generated/scipy.integrate.odeint.html. The resulting plot from my code looks the same.

However, I am not really sure if I done it in the good way. Especially I would really appreciate any hints how to improve the functions ode45_step and ode45 and verification whether my implementation is correct. I do not need to check the input variables, it will be done somewhere else.

import numpy as np
import matplotlib.pylab as plt

def ode45_step(f, x, t, dt, *args):
    """
    One step of 4th Order Runge-Kutta method
    """
    k = dt
    k1 = k * f(t, x, *args)
    k2 = k * f(t + 0.5*k, x + 0.5*k1, *args)
    k3 = k * f(t + 0.5*k, x + 0.5*k2, *args)
    k4 = k * f(t + dt, x + k3, *args)
    return x + 1/6. * (k1 + k2 + k3 + k4)

def ode45(f, t, x0, *args):
    """
    4th Order Runge-Kutta method
    """
    n = len(t)
    x = np.zeros((n, len(x0)))
    x[0] = x0
    for i in range(n-1):
        dt = t[i+1] - t[i]
        x[i+1] = ode45_step(f, x[i], t[i], dt, *args)
    return x

def f(t, y, b, c):
    """
    Pendulum example function.
    """
    theta = y[0]
    omega = y[1]
    dydt = [omega, -b*omega - c*np.sin(theta)]
    return np.array(dydt)

b = 0.25
c = 5.0

N = 101

x0 = np.array([np.pi - 0.1, 0.0])
t = np.linspace(0, 10, N)

x = ode45(f, t, x0, b, c)

plt.plot(x)
plt.show()

The following line is incorrect:

return x + 1/6. * (k1 + k2 + k3 + k4)

Should be:

return x + 1/6. * (k1 + 2*k2 + 2*k3 + k4)

Cross reference:
This code


Trying to grasp OOP Rock, Paper, Scissors program I'm trying to wrap my head around OOP but so far I feel like I'm just moving functions into classes and calling them methods. I am trying to create a "Rock Paper Scissors" game using OOP best practices.

Yesterday I asked someone to review this code and was told that my use of OOP was akin to being handed an electric drill and using it to hammer nails so I tried again:

from random import randrange

class NewGame(object):
    def __init__(self, num_rounds):
        self.num_rounds = num_rounds
        self.current_round = 0

    def round(self):
        human_choice = human_player.choose()
        comp_choice = comp_player.choose()
        decide_winner()
        game.current_round += 1

class Player(object):
    def __init__(self):
        self.score = 0
        self.choice = None

    def choose(self):
        self.choice = int(raw_input("Rock [1] Paper [2] Scissors [3]"))
        return self.choice

    def win_round(self):
        self.score += 1

    def win_game(self):
        print("YOU WON!")

class ComputerPlayer(Player):

    def choose(self):
        self.comp_choice = randrange(1, 4)
        return self.comp_choice

    def win_game(self):
        print("THE COMPUTER WON!")

def who_won(score1, score2):
    human_player_score = score1
    comp_player_score = score2
    if human_player_score > comp_player_score:
        human_player.win_game()
    if human_player_score < comp_player_score:
        comp_player.win_game()
    if human_player_score == comp_player_score:
        print ("IT WAS A TIE!")

def decide_winner():

    if human_player.choice == comp_player.comp_choice:
        print("It was a tie.")
    elif human_player.choice == 3 and comp_player.comp_choice == 2:
        print("The computer has won. Scissors cut paper.")
        comp_player.win_round()
    elif human_player.choice == 2 and comp_player.comp_choice == 3:
        print ("You won. Scissors cut paper.")
        human_player.win_round()
    elif human_player.choice == 2 and comp_player.comp_choice == 1:
        print ("You won. Paper covers rock.")
        human_player.win_round()
    elif human_player.choice == 1 and comp_player.comp_choice == 2:
        print ("You lost to the computer. Paper covers rock.")
        comp_player.win_round()
    elif human_player.choice == 1 and comp_player.comp_choice == 3:
        print ("You won. Rock smashes scissors.")
        human_player.win_round()
    elif human_player.choice == 3 and comp_player.comp_choice == 1:
        print ("You lost. Rock smashes scissors.")
        comp_player.win_round()

def game_play():
    num_rounds = raw_input("How many rounds? ")
    global game
    game = NewGame(num_rounds)
    global human_player
    global comp_player
    human_player = Player()
    comp_player = ComputerPlayer()
    while game.current_round < int(num_rounds):
        game.round()
        print "COMPUTER: ", comp_player.score, "HUMAN: ",human_player.score
    who_won(human_player.score, comp_player.score)

if __name__ == '__main__':
    game_play()

While I feel like this is a step in the right direction I'm also pretty sure that I'm still not quite "getting it" and was hoping that you could offer suggestions on how to do better.

Also, I'm pretty sure I read that global variables should be avoided but I could not figure out how to get it to work without them. I understand that I needed to use them because of encapsulation and scope but I don't know of how to get around that without using global variables.

Your Player and ComputerPlayer are good, with a very sensible use of inheritance. The NewGame is a bit of a mess, though; you have a tight coupling between the class and some sort-of-separate functions, with global variables (traditionally a bad sign), and the players aren't really related to the game.

Here's one alternative:

from random import randrange

class Game(object):

    OUTCOMES = {(1, 1): 0, (2, 2): 0, (3, 3): 0, # tie
                (1, 2): -1, (2, 3): -1, (3, 1): -1, # p1 lose
                (2, 1): 1, (3, 2): 1, (1, 3): 1} # p1 win

    def __init__(self, num_rounds):
        self.num_rounds = num_rounds
        self.human_player = Player()
        self.comp_player = ComputerPlayer()

    def play(self):
        for _ in self.num_rounds:
            outcome = self.OUTCOMES((self.human_player.choose(),
                                     self.comp_player.choose()))
            if outcome == 1:
                self.human_player.score += 1
            elif outcome == -1:
                self.comp_player.score += 1
        if self.human_player.score > self.comp_player.score:
            print "You won."
        elif self.human_player.score < self.comp_player.score:
            print "The computer won."
        else:
            print "It was a tie."

class Player(object):

    def __init__(self):
        self.score = 0

    def choose(self):
        while True:
            try:
                i = int(raw_input("Rock [1] Paper [2] Scissors [3]"))
            except ValueError:
                print "Must be an integer."
            else:
                if i in range(1, 4):
                    return i
                print "Must be between one and three."

class ComputerPlayer(Player):

    def choose(self):
        return randrange(1, 4)

if __name__ == '__main__':
    while True:
        try:
            num_rounds = int(raw_input("How many rounds? "))
        except ValueError:
            print "Must be an integer."
        else:
            break
    Game(num_rounds).play()

Note the following:

No more global - we play the Game directly and both Players belong to it, so everything is accessible within the instance methods;
All Games use the same OUTCOMES, so I've made that a class attribute, and the dictionary lookup makes working out who won much easier;
Input validation for the number of rounds and the human player's choice, so the program doesn't crash if they type 'foo';
No need for special methods to increment a player's score, you can access it directly; and
All the logic for playing is now in the Game itself.

A few places for further improvement (so you can have some of the fun):

The choose instance methods don't use any class or instance attributes, so perhaps they should be static methods instead;
The Players now seem too closely coupled to the Game - if choose took a choices argument (e.g. {1: 'rock', 2: 'paper', 3: 'scissors'}, which could be another Game class attribute) the coupling would be reduced; and
It doesn't tell you much about the outcome now - if OUTCOMES included e.g. {(1, 2): (-1, "paper wraps rock"), ...} you could add more feedback to the user.

Once those changes are made, you should be able to define e.g.:

class HarderGame(Game):

    CHOICES = {1: 'rock', 2: 'paper', 3: 'scissors'
               4: 'lizard', 5: 'Spock'}

    OUTCOMES = {(4, 5): (1, "lizard poisons Spock"), ...}

with everything else inherited (if you don't know the outcomes, see e.g. Wikipedia), and play it exactly the same:

HarderGame(num_rounds).play()


Readable Caesar cipher I was trying to refactor the following code for Caesar cipher, but it feels overly complicated at the end of the refactoring. Could someone else take a look at it and let me know if I am just overdoing/making it complicated for someone trying to read the code?

My highest priority is to write readable code, so any pointers in that regard would be welcome.

module CASE_WRAP
  def cyclic_next!
    if self == 'z'
      replace 'a'
      return
    end

    if self == 'Z'
      replace 'A'
      return
    end

    self.next!
  end

  def shift(number)
    number.times { self.cyclic_next! }
    self
  end
end

class CaesarCipher
  def self.cipher(message, shift)
    CaesarCipher.new(shift: shift)
      .encrypt message
  end

  def initialize(shift: 0)
    @shift = shift
  end

  def encrypt(message)
    message.gsub(/\w/) do |character|
      character.extend(CASE_WRAP)
        .shift @shift
    end
  end
end

Find tests for this in my repo.

All the caesar cypher does is rotate an alphabet by n places. If we let the alphabet be an Array, we can use Array#rotate to do the rotating for us. To substitute single characters, there is String#tr. Its name and usage are a little odd, but you'll quickly get used to it.

Basically, it maps characters in a String to characters in another String by their index. So "abc".tr("abc", "123") maps "a" to "1", "b" to "2" and "c" to "3".It expands ranges such as "a-c" first, so you can can write "abc".tr("abc", "123") as "abc".tr("a-c","1-3").

Though the interface is different, here's a simple implementation of the Caesar cypher.

module Caesar
  def self.encrypt(message, shift = 3)
    message.tr("a-zA-Z", rotated_alphabet(shift))
  end

  def self.decrypt(message, shift = 3)
    encrypt(message, 26 - shift)
  end

  def self.rotated_alphabet(shift)
    lower = "a".upto("z").to_a.rotate(shift)
    upper = "A".upto("Z").to_a.rotate(shift)

    lower.concat(upper).join
  end
end

Caesar.decrypt Caesar.encrypt("hi")
# => "hi"

For efficiency, you could change how you calculate the rotated alphabets. Here's an efficient, but not exactly a readable implementation.

  def self.rotated_alphabet(shift)
    lower = (97 + shift).chr # 97 is the ASCII code for "a"
    upper = (65 + shift).chr # 65 is the ASCII code for "A"

    "#{lower}-za-#{lower}#{upper}-ZA-#{upper}"
  end


Style guide template engine I'm new to JS and am trying to write a perfectly clean source code. I'm using gulp to concatenate and minify my files, as well as removing comments. So the end result will always be the same in production environment: a single line without spaces. But for a dev environment, I'm trying to achieve clean code.

I'm building a small style guide template engine, based on Jekyll. But I need JS for several features, like dynamic styling of components. I might be overzealous but here is one of my modules with its source indentation and formatting:

$(function() {

    /* Dynamic Styling ------------------ */

    // Links
    $('.field-style a')
        .each(function() {
            var base =
                $(this)
                .next('pre')
                .children('code')
                .text();
            var replaced =
                base
                .replace(
                    /    |{|}|^a |\r?\n|\r/g,
                    '');
            $(this)
                .attr('style', replaced);
        });

    // Color box
    $('.field-style .colored')
        .each(function() {
            var base =
                $(this)
                .next('pre')
                .children('code')
                .text();
            var replaced =
                base
                .replace(
                    /\D+/g,
                    '');
            $(this)
                .css('background', '#' + replaced);
        });

    // LESS code blocks
    if ($('.field-style .less')
        .length) {
        $('.field-style .less')
            .html($('.field-style .less')
                .html()
                .replace(
                    /(none)|(decimal)|(inside)|(hidden)/g,
                    '$&'));

        $('.field-style .less')
            .html($('.field-style .less')
                .html()
                .replace(
                    /(em)|(px)/g,
                    '$&'));
    };

    /* Code blocks trigger ------------------ */

    // Button markup
    var button =
        '<button type="button" href="javascript:;"' +
        'class="btn btn-primary show-code">' +
        'Voir CSS</button>';

    // Button generator
    $('pre')
        .before(button);

    // Animation
    $('.show-code')
        .on('click', function() {
            $(this)
                .next('pre')
                .slideToggle();
            return false;
        });

});

As you can see, I indented every method as well as every var. I also break lines in replace method for each argument

What would you say about this indentation level?

While looking at the code, the first thing that poped on me was the indentation. It is too strict, too inflexible, context-less, width-less... It isn't a good indentation.

Right in the middle of your code, you have this:

// LESS code blocks
if ($('.field-style .less')
    .length) {

This is next to unreadable. I even though it had broken syntax. Here's how it should be:

// LESS code blocks
if ($('.field-style .less').length) {

The { on the same line is target of discussion. In my opinion, it should be in a line on it's own but most people prefer this way.

Other things like this:

$('.field-style a')
    .each(function() {

Which can and should be all in the same line. You should only split the methods in new lines if and only if they look too wide.

Now, lets split it up and analise:

// Links
$('.field-style a')
    .each(function() {
        var base =
            $(this)
            .next('pre')
            .children('code')
            .text();
        var replaced =
            base
            .replace(
                /    |{|}|^a |\r?\n|\r/g,
                '');
        $(this)
            .attr('style', replaced);
    });

You re-use the $(this) in there, and store the text on a variable. Your priorities are swapped! The slowest thing in that code is repeated, but you have to store a string in a new variable.

This is how I would write it:

$('.field-style a').each(function() {
    var $this = $(this);

    var replaced = $this
        .next('pre')
        .children('code')
        .text()
        .replace(/    |{|}|^a |\r?\n|\r/g, '');

    $this.attr('style', replaced);
});

You could even write it as:

$('.field-style a').each(function() {
    var $this = $(this);

    $this.attr(
        'style',
        $this
            .next('pre')
            .children('code')
            .text()
            .replace(/    |{|}|^a |\r?\n|\r/g, '')
    );
});

This part can also be used for the $('.field-style .colored') block.

But a major concerning area is that regular expression. It is made to match newlines, the first a and a space and {}. Here's my proposal:

/a |\s*(?:\r|\n|\r\n)\s*|[\{\}]/g

This goes a step further and also deletes any tab or any other whitespace after and before a newline.

An input like:

a {
    color: red;
    size: auto;
    width: 120px;
    white-space: break;
}

Will result in:

  color: red;size: auto;width: 120px;white-space: break;

While yours results in:

  <TAB>color: red;size: auto;<TAB>width: 120px; white-space: break;

(Due to limitations on the representation of whitespace, I've seen myself forced to replace all tabs with a <TAB> 'indicator')

Lets analise the .less block:

// LESS code blocks
if ($('.field-style .less')
    .length) {
    $('.field-style .less')
        .html($('.field-style .less')
            .html()
            .replace(
                /(none)|(decimal)|(inside)|(hidden)/g,
                '$&'));

    $('.field-style .less')
        .html($('.field-style .less')
            .html()
            .replace(
                /(em)|(px)/g,
                '$&'));
};

First, you have that dangling ;.

You are currently wasting too much CPU! You repeated $('.field-style .less') 5 times! You have no idea how slow that is. Store that in a new variable.

You could remove repetitions, like this:

var $less = $('.field-style .less');
if( $less.length ) {
    $less.html(
        $less
            .html()
            .replace(
                /(none)|(decimal)|(inside)|(hidden)/g,
                '$&'
            )
            .replace(
                /(em)|(px)/g,
                '$&'
            )
    );
}

But I'm still not happy. The regex don't need the () around every word. Or at all. Basically, you could write both as this:

/none|decimal|inside|hidden/g
/em|px/g

In the replacements, instead of $&, you use $0. This will replace everything that was matched.

You currently have the following code:

// Animation
$('.show-code')
    .on('click', function() {
        $(this)
            .next('pre')
            .slideToggle();
        return false;
    });

There's not much to say on this one. Using the .on() handler, you can take advantage of it by picking a common parent node. Then you set the 2nd argument to have a selector. Like this:

$('#fake-parent').on('click', '.show-code', function(){
    [...]
});

Also, notice that when you use return false; on an event handler, jQuery runs event.preventDefault() and event.preventPropagation(). Depending on what you are trying to do, you should be careful when returning anything in an event handler.


Expanding sizes between given values This a very simple function to expand the sizes between the given values.

For instance, if the input variable is "S-XL" then the function returns "S, M, L, XL".

Basically I'm not happy with this function.  I think it's too complex for its purpose.

function sizexpander($sizes) {
    $sizearray=Array("XS", "S", "M", "L", "XL", "XXL", "XXXL");
    $size1=array_search(strstrb($sizes, "-"), $sizearray);
    $size2=array_search(substr(strstr($sizes, "-"),1), $sizearray);
    while ($size1 <= $size2) {
        $prodsizes.= $sizearray[$size1].", ";
        $size1++;
    }
    return substr($prodsizes, 0, -2);
}

function strstrb($h,$n){
    return array_shift(explode($n,$h,2));
}

You can use the function array_slice to take the subarray which contains the sizes from $size1 to size2. And then use implode to turn this subarray into a comma-separated string. This will simplify your algorithm.

You can also just use explode instead of substr and strstr to split the range string into its parts.

In addition I would recommend changing the variable names of $size1 and $size2. To me the name $size1 suggests that the variable contains the actual size while in fact it contains its index. Something like index_of_size1 or index_from might be more appropriate.

If I were to write this function, it'd look something like this:

function sizexpander($size_range) {
    $sizearray=Array("XS", "S", "M", "L", "XL", "XXL", "XXXL");
    list($size_from, $size_to) = explode("-", $size_range);
    $index_from = array_search($size_from, $sizearray);
    $index_to = array_search($size_to, $sizearray);
    $subsizes = array_slice($sizearray, $index_from, $index_to-$index_from+1);
    return implode(", ", $subsizes);
}


Zig-zag child element with Flexbox and JavaScript Recently I found a question on StackOverflow that seemed very interesting: How to make HTML elements "zig zag" this way:

 -----------------
|A > B > C > D > E|
|J < I < H < G < F|
 -----------------

 ---
|A H|
|B G|
|C F|
|D E|
 ---

I was able to implement a simple solution for this problem with Flexbox and a bit of JavaScript (works for any even number of elements):

var reverseBoxes = function () {

  var flexItems = document.querySelectorAll(".child"),
      flexItemsCount = flexItems.length,
      reverseAt = flexItems.length / 2,
      breakPoint = 480;

  for (var i = reverseAt; i < flexItemsCount; i++) {
    flexItems[i].style.order = flexItemsCount - i;
  }

  for (var j = 0; j < flexItemsCount; j++) {
    if (window.innerWidth > breakPoint) {
      flexItems[j].style.width = (100 / flexItemsCount) * 2 - 2 + "%";
      flexItems[j].style.height = "auto";
    } else {
      flexItems[j].style.height = (100 / flexItemsCount) * 2 - 2 + "%";
      flexItems[j].style.width = "auto";
    }
  }

}

reverseBoxes();
window.addEventListener("resize", reverseBoxes);
body {
  font-family: Arial, sans-serif;
  font-size: 18px;
  margin: 0;
  padding: 0;
}

.parent {
  display: flex;
  flex-wrap: wrap;
  list-style-type: none;
  padding: 0;
  height: 100vh;
}

.child {
  margin: 1%;
  text-align: center;
  background: #069;
  color: #fff;
  display: flex;
  align-items: center;
  justify-content: center;
}

@media only screen and (max-width: 480px) {
  .parent {
    flex-direction: column;
  }
  .child {
    width: 48%;
  }
}

A

B

C

D

E

F

G

H

I

J

Tiny adjustment : you can update all your objects in your first loop and get rid of the second.

var reverseBoxes = function() {

    var flexItems = document.querySelectorAll(".child"),
        flexItemsCount = flexItems.length,
        reverseAt = flexItems.length / 2,
        breakPoint = 480;

    if (window.innerWidth > breakPoint) {
        for (var i = reverseAt; i < flexItemsCount; i++) {
            flexItems[i].style.order = flexItemsCount - i;
            // First half of items
            flexItems[flexItemsCount - i - 1].style.width = (100 / flexItemsCount) * 2 - 2 + "%";
            flexItems[flexItemsCount - i - 1].style.height = "auto";
            // Second half of items
            flexItems[i].style.width = (100 / flexItemsCount) * 2 - 2 + "%";
            flexItems[i].style.height = "auto";
        }
    } else {
        for (var i = reverseAt; i < flexItemsCount; i++) {
            flexItems[i].style.order = flexItemsCount - i;
            // First half of items
            flexItems[flexItemsCount - i - 1].style.height = (100 / flexItemsCount) * 2 - 2 + "%";
            flexItems[flexItemsCount - i - 1].style.width = "auto";
            // Second half of items
            flexItems[i].style.height = (100 / flexItemsCount) * 2 - 2 + "%";
            flexItems[i].style.width = "auto";
        }
    }
}

Edit : got the if out of the for loop


Web API 2 authentication with JWT Please review my code for bearer token (JWT) authentication of Web API 2 (Self Hosted using OWIN)

Are there any security issues in the implementation?

Quick overview:

Token creation and validation using JWT Handler
Symmetric key encryption
CORS support not yet checked for the authorization header
Web traffic will be on SSL.
The key cannot be auto-generated as it will break during a load balanced scenario. Can I save the key in config? Or switch to X509 certificates?

This is the main class to create and validate tokens:

public class TokenManager
{
    public static string CreateJwtToken(string userName, string role)
    {
        var claimList = new List<Claim>()
            {
                new Claim(ClaimTypes.Name, userName),
                new Claim(ClaimTypes.Role, role)     //Not sure what this is for
            };

        var tokenHandler = new JwtSecurityTokenHandler() { RequireExpirationTime = true };
        var sSKey = new InMemorySymmetricSecurityKey(SecurityConstants.KeyForHmacSha256);

        var jwtToken = tokenHandler.CreateToken(
            makeSecurityTokenDescriptor(sSKey, claimList));
        return tokenHandler.WriteToken(jwtToken);
    }

    public static ClaimsPrincipal ValidateJwtToken(string jwtToken)
    {
        var tokenHandler = new JwtSecurityTokenHandler() { RequireExpirationTime = true };

        // Parse JWT from the Base64UrlEncoded wire form
        //(<Base64UrlEncoded header>.<Base64UrlEncoded body>.<signature>)
        JwtSecurityToken parsedJwt = tokenHandler.ReadToken(jwtToken) as JwtSecurityToken;

        TokenValidationParameters validationParams =
            new TokenValidationParameters()
            {
                AllowedAudience = SecurityConstants.TokenAudience,
                ValidIssuer = SecurityConstants.TokenIssuer,
                ValidateIssuer = true,
                SigningToken = new BinarySecretSecurityToken(SecurityConstants.KeyForHmacSha256),
            };

        return tokenHandler.ValidateToken(parsedJwt, validationParams);
    }

    private static SecurityTokenDescriptor makeSecurityTokenDescriptor(
        InMemorySymmetricSecurityKey sSKey, List<Claim> claimList)
    {
        var now = DateTime.UtcNow;
        Claim[] claims = claimList.ToArray();
        return new SecurityTokenDescriptor
        {
            Subject = new ClaimsIdentity(claims),
            TokenIssuerName = SecurityConstants.TokenIssuer,
            AppliesToAddress = SecurityConstants.TokenAudience,
            Lifetime = new Lifetime(now, now.AddMinutes(SecurityConstants.TokenLifetimeMinutes)),
            SigningCredentials = new SigningCredentials(sSKey,
                "http://www.w3.org/2001/04/xmldsig-more#hmac-sha256",
                "http://www.w3.org/2001/04/xmlenc#sha256"),
        };
    }
}

I have a message handler to intercept the requests and I verify the validity of token except for the route for log in, using TokenManager.ValidateJwtToken() above.

To create the token, in the LoginController, I have the following code:

[Route("login")]
[HttpPost]
public HttpResponseMessage Login(LoginBindingModel login)
{
    if (login.Username == "admin" && login.Password == "password")  //Do real auth
    {
        string role = "Librarian";
        var jwtToken = TokenManager.CreateJwtToken(login.Username, role);

        return new HttpResponseMessage(HttpStatusCode.OK)
        {
            Content = new ObjectContent<object>(new
            {
                UserName = login.Username,
                Roles = role,
                AccessToken = jwtToken
            }, Configuration.Formatters.JsonFormatter)
        };
    }
    return new HttpResponseMessage(HttpStatusCode.BadRequest);
}

The full working code is available here and the instructions to run the sample are in the Wiki.

Instead of keeping KEY in config, I would keep it with user records. A unique key for each user.

I admit, I don't get why creating keys dynamically would break the load balancing scenario.  We can have a key created at the back-end where we have a single service serving all the load balances servers (such as a database).