Should I use module pattern for this notification controller? I have the following code, I use this pattern for the entire JavaScript code. I'm trying to re-factor my code again. Should I use the Module pattern? Revealing Module Pattern to be specific?

What's happening is, somewhere in my code I say Dnianas.Notification.init().

First I bind the events, and I will handle them one by one.

Is there a better way to do this?

Also I notice that I don't use the var keyword. because they depend on each other.

Dnianas.Notification = {
    init: function () {
        this.bindEvents();
        this.cache();
    },

    bindEvents: function () {
        $(document).on('click', '#opennotifii', this.markAsRead);
    },

    cache: function () {
        $notification = $('#opennotifii');
    },

    countNotifications: function () {
        var $notifications = $('.boxnotificationsusers').children().find('#boxsendnotifi');
        ids = [];

        // Add unread notifications to the ids array.
        $notifications.each(function () {
            if ($(this).data('read') === 0) {
                ids.push($(this).data('id'));
            }
        });

        return ids;
    },

    markAsRead: function () {
        self = Dnianas.Notification;
        ids = self.countNotifications();

        if (ids.length > 0) {
            var request = $.ajax({
                url: '/notifications/read',
                data: {
                    notifications: ids,
                    _token: token,
                }
            });

            request.done(function (data) {
                self.renderNotificationCount(data);
            });
        }
    },

    renderNotificationCount: function (data) {
        if (data.seen) {
            $notification.find('.not_nu1').fadeOut(200);
        }
    }
};

Here's a modified version that uses the original singleton declaration.

Description of changes:

Remove all undeclared and thus implicitly global variables.
Remove caching of jQuery selector object (don't see any need for it).
Remove several intermediate variables that are only used once
Add .bind() to event handler so it can use this directly
Change from .done() to .then() to code more closely to the promise standard.

Modified code:

Dnianas.Notification = {
    init: function () {
        this.bindEvents();
    },

    bindEvents: function () {
        $(document).on('click', '#opennotifii', this.markAsRead.bind(this));
    },

    countNotifications: function () {
        var ids = [];

        // Add unread notifications to the ids array.
        $('.boxnotificationsusers').children().find('#boxsendnotifi').each(function () {
            if ($(this).data('read') === 0) {
                ids.push($(this).data('id'));
            }
        });
        return ids;
    },

    markAsRead: function () {
        var self = this;
        var ids = this.countNotifications();

        if (ids.length > 0) {
            $.ajax({
                url: '/notifications/read',
                data: {
                    notifications: ids,
                    _token: token,
                }
            }).then(function (data) {
                self.renderNotificationCount(data);
            });
        }
    },

    renderNotificationCount: function (data) {
        if (data.seen) {
            $('#opennotifii .not_nu1').fadeOut(200);
        }
    }
};

Also, this piece of code looks suspicious:

$('.boxnotificationsusers').children().find('#boxsendnotifi').each(...)

because there should only ever be one '#boxsendnotifi' item in the entire document so if that was the case, you could just do this:

$('#boxsendnotifi').each(...)

or perhaps:

$('.boxnotificationsusers #boxsendnotifi').each(...)

if you want to limit what you find to a particular scope.

Or, perhaps you should just be using a class name if there are potentially multiple matches:

$('.boxnotificationsusers .boxsendnotifi').each(...)


Better way to create objects from strings I'm looking for a better way to create objects based off of a value of a string.

Consider the following:

Input file:

//each Vehicle type has a list of different options
//whose length varies, I provided a simple case
van*2006*dodge*grand caravan*green~
car*2010*toyota*corolla*red~
truck*2005*toyota*tundra*black~

Classes:

abstract class Vehicle: IVehicle{
    public int Year;
    public string Make;
    public string Model;

    ...
}

class Van : Vehicle
{
    ...
}

class Car : Vehicle
{
    ...
}

class Truck : Vehicle
{
    ...
}

Program:

static void Main(string[] args)
{
    char delimiter = '*';
    string fileToRead = "inventory.csv";
    string currentLine = string.Empty;
    List<IVehicle> inventory = new List<IVehicle>();

    //Open file
    using (StreamReader reader = new StreamReader(fileToRead))
    {
        //while you read each line
        while ((currentLine = reader.ReadLine()) != null)
        {
            IVehicle temp;

            //Tokenize the line
            string[] parts = currentLine.Split(delimiter);

            switch(parts[0])
            {
                case "van":
                    temp = new Van();
                    ...
                break;
                case "car"
                    temp = new Car();
                    ...
                break;
                case "truck"
                    temp = new Truck();
                    ...
                break;
            }

            inventory.add(temp);
        }
    }

    ...
}

My problem with the code is the switch statement. It feels messy and I'm looking for a better way.

In .Net laguages you can easily instatiate any object given only the full name of the class (=namespace+class name).

Here is a runnable example:

using System;
using System.Reflection;

namespace Test
{

    class MsgPrinter
    {
        public MsgPrinter()
        {
            System.Console.WriteLine("Hello!");
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            Assembly assembly = Assembly.GetExecutingAssembly();
            MsgPrinter printer = assembly.CreateInstance("Test.MsgPrinter") as MsgPrinter;
            Console.Read();
        }
    }
}

So in your case it would break down to something like (of course, the names in your parameter file would need to exactly match the class names in your application):

...
string[] parts = currentLine.Split(delimiter);
Vehicle vehicle = Assembly.GetExecutingAssembly().CreateInstance(namespace_name+"."+parts[0]) as Vehicle;
...


Slow anagrams detection This is my code for this question on hackerrank.
The idea is to find the amount of substrings of a given word that are anagrams of each other.

The code works fine, but it times out with longer strings. I've downloaded the test cases to make sure the result is correct and it is, it just takes too long.
I can do this in C and pass all tests, but I was wondering if there is a faster alternative in Perl.

I also tried using numbers, i.e. multiplying the ord()-97 of the characters in the two substrings, and then doing a single check on the number, to see if they are anagrams of each other.
That is much faster, but only works on small strings because it overflows quickly.

In this specific case, I'm not really that concerned about style, as much as performance. How can this be improved?

use strict;
use warnings;

use Data::Dumper;

sub get_all_substr {
    my ($input_string) = @_;

    my @results;
    push @results, $input_string =~ /(?=(.{$_}))/g for 1 .. length($input_string);

    return \@results;
}

sub is_anagram {
    my ($string_a, $string_b) = @_;

    my $len_a = length($string_a);

    return 0 if (length($string_b) != $len_a);

    my %vec_a;
    my %vec_b;

    foreach my $i (0..$len_a-1) {
        $vec_a{substr($string_a, $i, 1)}++;
        $vec_b{substr($string_b, $i, 1)}++;
    }

    return 0 if (scalar(keys(%vec_a)) != scalar(keys(%vec_b)));

    foreach my $key (keys(%vec_a)) {
        if (!defined($vec_b{$key}) || ($vec_a{$key} != $vec_b{$key})) {
            return 0;
        }
    }

    return 1;
}

my @results;

my $t = <STDIN>;
chomp $t;

while($t--) {
    my $input_string = <STDIN>;
    chomp $input_string;

    my $count = 0;
    my $strings;
    $strings = get_all_substr($input_string);

    my $max_size = scalar(@{$strings})-1;
    foreach my $i (0..$max_size) {
        foreach my $k ($i+1..$max_size) {
            if (is_anagram($strings->[$k], $strings->[$i])) {
                $count++;
            }
        }
    }
    push @results, $count;
}

print join("\n", @results);

Finally nailed it in Perl. In Python this same approach worked no problem, but clearly Perl (or at least the interpreter run on hackerrank) is much slower.

The "trick" is to extract all the substrings already in the form of an array, preferably already sorted, so that there is no need to do the loop another time inside of is_anagram.

Here is my code, admittedly not a beauty, but now it passes all tests.

use strict;
use warnings;

sub get_all_subarr {
    my ($input_string) = @_;

    my %results;

    foreach my $i (1..length($input_string)-1) {
        foreach my $j (0..length($input_string)-$i) {
            push @{$results{$i}}, [sort(split(//,substr($input_string, $j, $i)))];
        }
    }

    return \%results;
}

sub is_anagram {
    my ($arr_a, $arr_b) = @_;

    return 0 if (join('', @{$arr_a}) ne join('', @{$arr_b}));
    return 1;
}

my @results;

my $t = <STDIN>;
chomp $t;

while($t--) {
    my $input_string = <STDIN>;
    chomp $input_string;

    my $count = 0;
    my $strings_arr = get_all_subarr($input_string);
    foreach my $i (keys(%{$strings_arr})) {
        my $max_size = scalar(@{$strings_arr->{$i}})-1;
        foreach my $j (0..$max_size) {
            foreach my $k ($j+1..$max_size) {
                if (is_anagram($strings_arr->{$i}[$j], $strings_arr->{$i}[$k])) {
                    $count++;
                }
            }
        }
    }
    push @results, $count;
}

print join("\n", @results);


Checking for a logged user I have two classes with similar first checking codes, but different behaviors. I suppose there is a way to refactor the code, so I don't need to retype it every time.  Is it possible to refactor this, or I must retype this code every time?

# Get actual logged user
user, msg = checkLoggedUser()
if user:
    self.renderPage("customMessage.htm", custom_msg=msg)
else:

The code is simple. Forget about my way to render the page, is a method over my own PageHandler class that works.

# Check for actual logged user
def checkLoggedUser():
    # Get actual logged user
    user = users.get_current_user()

    # Don't allow to register for a logged user
    if user:
        return True, "You are already logged in."
    else:
        return False, None

# Get and post for the login page
class Login(custom.PageHandler):
    def get(self):
        # Get actual logged user
        user, msg = checkLoggedUser()
        if user:
            self.renderPage("customMessage.htm", custom_msg=msg)
        else:
            self.renderPage('login.htm')

    def post(self):
        # Get actual logged user
        user, msg = checkLoggedUser()
        if user:
            self.renderPage("customMessage.htm", custom_msg=msg)
        else:
            # Make the login
            # bla, bla bla... code for login the user.

# Get and post for the register page
class Register(custom.PageHandler):
    def get(self):
        # Get actual logged user
        user, msg = checkLoggedUser()
        if user:
            self.renderPage("customMessage.htm", custom_msg=msg)
        else:
            self.renderPage("registerUser.htm")

    def post(self):
        # Get actual logged user
        user, msg = checkLoggedUser()
        if user:
            self.renderPage("customMessage.htm", custom_msg=msg)
        else:
            # Store in vars the form values
            # bla, bla bla... code for register the user.

Create a class

class MyPageHandler(custom.PageHandler):
     def veryUserNotLoggedIn(self):
         if users.getCurrentUser():
             self.renderPage("customMessage.htm", custom_msg=msg)
             return False
         else:
             return True

Then you can write you class like

class Login(MyPageHandler):
    def get(self):
        if self.verifyUserNotLoggedIn():
             self.renderPage('login.htm')


Foolproof number validation I know there have been a lot of post, blog article answer on Stack Overflow (but the validation does return true on a tab character) about this. But I'm still looking for a neat way to validate if a value is a number.

I wrote the following. Does that cover the 99% of the cases? Is there a better way to do that?

function validateNumber(valueToCheck){
    var checkSum = 0;
    var isValid = true;

    if(typeof valueToCheck === "number"){
        return true;
    }
    if(typeof valueToCheck === "string"){
        if(valueToCheck.replace(/ /g,'') !== valueToCheck){
            return false;
        }

        if(valueToCheck.charAt(0) == '-'){
            valueToCheck = valueToCheck.substr(1); // remove minus sign
        }
        valueToCheck = valueToCheck.replace('.',''); // allow one decimal point

    }

    for(var i=0;i<valueToCheck.length; i++){
        if(!(valueToCheck.charCodeAt(i) >= 48 && valueToCheck.charCodeAt(i) <= 57)){
            isValid = false;
            break;
        }
    }

    return isValid;
}

Here is the jsfiddle with tests cases.

Personally I'm not sure why you're working so hard...

function validateNumber(valueToCheck) {
    return !isNaN(Number(valueToCheck));
}

or the more complete version from the question you've linked:

function validateNumber(n) {
    return !isNaN(parseFloat(n)) && isFinite(n);
}

JavaScript already has a number parser.

This will allow values like '42', '1.456', '1.456e3', [42], etc. All valid Numbers.

And it passes your test suite better than the original: http://jsfiddle.net/nL4L13k2/4/ and http://jsfiddle.net/nL4L13k2/5/


Improve speed of LINQ query How can I improve the speed of this LINQ query?

using (var txn = new TransactionScope(TransactionScopeOption.Required,
        new TransactionOptions { IsolationLevel = IsolationLevel.ReadUncommitted }))
{
     var myAlertsList = (from alerts in _db.Alerts.AsNoTracking()
                         where alerts.AlertID >= Id.AlertID && alerts.DataWystapienia >Id.DataWystapienia
                         select new MyAlerts { AlertID = alerts.AlertID, DataWystapienia = alerts.DataWystapienia, Message = alerts.Message })
                        .OrderByDescending(a => a.AlertID).Take(Id.NumberOfRows).Concat((
                         from subAlerts in _db.SubAlerts.AsNoTracking()
                         where subAlerts.AlertID >= Id.AlertID && subAlerts.DataWystapienia > Id.DataWystapienia
                         select new MyAlerts { AlertID = subAlerts.AlertID, DataWystapienia = subAlerts.DataWystapienia, Message = subAlerts.Message })
                        .OrderByDescending(c => c.AlertID).Take(Id.NumberOfRows)).OrderByDescending(a => a.AlertID).Take(Id.NumberOfRows);

     return myAlertsList;
}

It seems like the order by date is the longest process in this query. Is this correct?
The database has about a million records. NumberOfRows is max 200. Time of this query is 45ms (after using only one orderby). Is there something more that I can do to improve that LINQ query?

First lets split up the query so we can read it:

var alertList = from alerts in _db.Alerts.AsNoTracking()
                where alerts.AlertID >= Id.AlertID && alerts.DataWystapienia >Id.DataWystapienia
                select new MyAlerts { AlertID = alerts.AlertID, DataWystapienia = alerts.DataWystapienia, Message = alerts.Message };

var subAlertList = from subAlerts in _db.SubAlerts.AsNoTracking()
                   where subAlerts.AlertID >= Id.AlertID && subAlerts.DataWystapienia > Id.DataWystapienia
                   select new MyAlerts { AlertID = subAlerts.AlertID, DataWystapienia = subAlerts.DataWystapienia, Message = subAlerts.Message };

var myAlertsList = alertList.OrderByDescending(a => a.AlertID).Take(Id.NumberOfRows)
                   .Concat(subAlertList.OrderByDescending(a => a.AlertID).Take(Id.NumberOfRows))
                   .OrderByDescending(a => a.AlertID).Take(Id.NumberOfRows);

this is fully equivalent to your code I only extracted the 2 lists

the last operation of code has 3 OrderByDescending(a => a.AlertID).take(Id.NumberOfRows) while only 1 is needed

var myAlertsList = alertList.Concat(subAlertList).OrderByDescending(a => a.AlertID).Take(Id.NumberOfRows);


VHDL mux in need of generics I've built a multiplexer which takes 2 inputs: one array of std_logic_vector and one std_logic_vector to select the correct array. It should be written more generic, but I'm not sure how. The length of SEL grows if the length of X grows (4-bit if 16 inputs, 6-bit if 64, etc.). I'm currently using custom types as input, but this makes making the code more generic problematic.

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;

LIBRARY work;
USE work.type_arrays.ALL;

entity mux_16to1 is
    Port ( SEL : in  STD_LOGIC_VECTOR ( 3 downto 0);
           X   : in  A16_SLV16;
           Y   : out STD_LOGIC_VECTOR (15 downto 0));
end mux_16to1;

architecture sel of mux_16to1 is
begin
with SEL select
    Y <= X(0) when "0000",
         X(1) when "0001",
         X(2) when "0010",
         X(3) when "0011",
         X(4) when "0100",
         X(5) when "0101",
         X(6) when "0110",
         X(7) when "0111",
         X(8) when "1000",
         X(9) when "1001",
         X(10) when "1010",
         X(11) when "1011",
         X(12) when "1100",
         X(13) when "1101",
         X(14) when "1110",
         X(15) when "1111",
         "0000000000000000"  when others;
end sel;

With work.type_arrays consisting of:

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;

PACKAGE type_arrays IS
    SUBTYPE SLV16 IS std_logic_vector( 15 downto 0 );
    TYPE A16_SLV16 IS ARRAY ( 0 to 15 ) OF SLV16;
    TYPE A256_SLV16 IS ARRAY ( 0 to 255 ) OF SLV16;
END type_arrays;

Does anyone have a solution/idea to make it generic? I want to use this mux for 256*16-bit solutions and perhaps even 1024*24-bit as well. Writing the whole thing by hand can't be the VHDL way to do it.

The alternative is letting a generator write the code, but I'd like to prevent that if possible.

There are 4 ways to a solve this:

Solution 1:

You can define a fixed width STD_LOGIC_VECTOR (SLV), which can be used to construct other vectors (let's call them STD_LOGIC_VECTOR_VECTORs (SLVV).

subtype T_SLV_8  is STD_LOGIC_VECTOR(7 downto 0);        -- define a Byte
type    T_SLVV_8 is array(NATURAL range <>) of T_SLV_8;  -- define a new unconstrained vector of Bytes.

Now you can define your entity as follows:

entity mux_slvv_8 is
  generic (
    PORTS  : POSITIVE  := 4
  );
  port (
    sel  : in  STD_LOGIC_VECTOR(log2ceilnz(PORTS) - 1 downto 0);
    X    : in  T_SLVV_8(PORTS - 1 downto 0);
    Y    : out T_SLV_8
  );
end;

The internal logic can be reduced to:

architecture rtl of mux_slvv_8 is
begin
  Y <= X(to_integer(unsigned(sel)));
end;

This can be improved by extracting to_integer(unsigned(..)) into a function called to_index.

Solution 2:

The next step would be, to be also flexible in how many bits are multiplexed at once. So let's introduce a generic BITS to specify the data bits. VHDL has a problem to define array of unconstrained arrays (this is solved with VHDL-2008, but not all vendors support this feature).

So solution 2 uses a flat vector to pass all bits to the mux.

entity mux_flat is
  generic (
    PORTS  : POSITIVE  := 4;
    BITS   : POSITIVE  := 8
  );
  port (
    sel  : in  STD_LOGIC_VECTOR(log2ceilnz(PORTS) - 1 downto 0);
    X    : in  STD_LOGIC_VECTOR((BITS * PORTS) - 1 downto 0);
    Y    : out STD_LOGIC_VECTOR(BITS - 1 downto 0)
  );
end;

architecture rtl of mux_flat is
  type T_SLVV is array(NATURAL range <>) of STD_LOGIC_VECTOR(BITS - 1 downto 0);
  signal mux_in : T_SLVV(PORTS - 1 downto 0)
begin
  gen : for i in 0 to PORTS - 1 generate
    -- convert flat vector to slvv
    mux_in(i) <= X(((i + 1) * BITS) - 1 downto (i * BITS));
  end generate;
  Y <= mux_in(to_integer(unsigned(sel)));
end;

Solution 3:

Being flexible in PORTS and BITS can also be done by using a real 2-dimensional array -> STD_LOGIC_MATRIX (SLM).

type T_SLM is array(NATURAL range <>, NATURAL range <>) of STD_LOGIC;

This would be the entity declaration:

entity mux_slm is
  generic (
    PORTS  : POSITIVE  := 4;
    BITS   : POSITIVE  := 8
  );
  port (
    sel  : in  STD_LOGIC_VECTOR(log2ceilnz(PORTS) - 1 downto 0);
    X    : in  T_SLM(PORTS - 1 downto 0, BITS - 1 downto 0);
    Y    : out STD_LOGIC_VECTOR(BITS - 1 downto 0)
  );
end;

You could also write a generate loop to do some wiring or you can define some functions to do that.

-- get a matrix row
function get_row(slm : T_SLM; RowIndex : NATURAL) return STD_LOGIC_VECTOR is
  variable slv : STD_LOGIC_VECTOR(slm'high(2) downto slm'low(2)); -- Xilinx iSIM work-around, because 'range(2) = 'range(1); tested with ISE/iSIM 14.2
begin
  for i in slv'range loop
    slv(i) := slm(RowIndex, i);
  end loop;
  return slv;
end function;

And thats the corresponding architecture:

architecture rtl of mux_slm is
begin
  Y <= get_Row(X, to_integer(unsigned(sel)));
end;

Solution 4:

If your tool supports VHDL-2008 and arrays of unconstrained vectors, then you can use this type:

type T_SLVV is array(NATURAL range <>) of STD_LOGIC_VECTOR;

Type Conversion:

The following images illustrates all possible type conversions regarding: SL, SLV, SLVV and SLM.

If you are interested in more, I could upload my complete collection of vector, vector-vector and matrix types, functions and procedures. Please give me a hint.

Edit:

Here is the package PoC.common.vectors. It's included in my PicoBlaze Library which is currently in beta state. The release of PoC is also planned, but not so progressed. So I included some necessary packages and modules into this library. The source code license is 'Apache License 2.0'.


Reservation validation I got a Reservation model that has reservation_start and reservation_end.
Then I have the usual date validations (end date cannot be lesser than start date and so on).

The problem is with these two guys:

  def reservation_start_available
    reservations = Reservation.where(["transport_id =?", transport_id])
    date_ranges = reservations.map { |e| e.reservation_start..e.reservation_end }

    date_ranges.each do |range|
      if range.include? reservation_start
        errors.add(:reservation_start, "Start date not available")
      end
    end
  end

  def reservation_end_available
    reservations = Reservation.where(["transport_id =?", transport_id])
    date_ranges = reservations.map { |e| e.reservation_start..e.reservation_end }

    date_ranges.each do |range|
      if range.include? reservation_end
        errors.add(:reservation_end, "End date not available")
      end
    end
  end

This is not even close to the DRY we love so much. How can I pass a parameter to the methods like this?

Introducing custom validators!

Assuming you validate your dates with this "fresh" syntax:

validates :reservation_start,        # Note the comma!
  availability: true                 # ...and some other validations
validates :reservation_end,
  availability: {name: "End date"}   # same here

Define this validator separately, a common place is app/validators, in this case that file would be named availability_validator.rb

class AvailabilityValidator < ActiveModel::EachValidator

  # `each` stands for 'each attribute with a validation'
  def validate_each(record, attribute, value)
    # Args: a model instance, a symbol of attribute and a value it has
    # You also get a hash in `options`
    # If you specified validation as `availability: true`, you wouldn't get it

    reservations = Reservation.where(["transport_id =?", record.transport_id])
    date_ranges = reservations.map { |e| e.reservation_start..e.reservation_end }

    date_ranges.each do |range|
      if range.include? value
        errors.add(attribute, "#{options[:name] || 'Date'} not available")
      end
    end

  end
end

Sure this example needs some refinement, but it demonstrates most tools you have at hand.

The first validation gets no options, because there's true specified, we avoid errors by using a default name with ||. Not the best practice (merging with a separate hash with defaults is), but when you don't have many parameters and places, that will do.
Extraction of transport_id into options could come in handy if validation like this is broadly used.
"Availability" is a bit too common term, you might need to rename this class.
Stuff like error messages and attribute names is better be put into a locale file and fetched using I18n.t 'some.key.name'.
You're doing too much on Ruby side. When armed with a relational database, you can do stuff like this:

Reservation.where(transport_id: transport_id). # period tells there are further calls
  where("reservation_start <= :date AND reservation_end >= :date", date: value).
  exists? # returns true if there's a reservation with our value in range

If a lower bound is smaller and the upper bound is bigger than our balue, then our value is in the range of that entry. SQL can handle it.
There is a possible error: the validation (even yours) will false-accept the range if the submitted range encloses one existing range completely (start and end are both outside bounds of any other range). I'm not sure if that's what you want, so be advised. This can be fixed by an extra validation, I'm leaving this up to you.


List of lists of arrays, zipping, modifying in place I came back to some old code today and freaked out.  I was like "How does this even work, and if it works it can't do what I intended."  I ran a few quick tests and confirmed it did actually do what was intended.  I know the long term solution is to avoid this situation by having good unit tests -- then I don't need to freak out, I know the code is doing what I expect (New years resolution right there.)

But it wasn't that the code didn't do what it was supposed to, it did.  What it is doing is kind of complex, and it doesn't make it clear.

orig is a List of list of numpy arrays.  All the inner lists have the same lengths.

The information in orig is known as knowledge.  In particular each matrix inner list has 1-3 piece of knowledge, these knowledge are in have names the first is weight, the second is upwards-bias and the third downwards-bias

Each inner list comes is the knowledge of some particular object called a layer.  When those objects are collected to make a more knowledgeable object, then that object also has knowledge with is the list of the knowledge of its component parts.  So the outer list is a List of knowledge of layers. And is the overall knowledge

Sometimes I would like to destroy some of that knowledge by replacing all the matrices in a layer with a randomly valued matrices of the same size and I would like to specify that with a parameter called layer_nums_to_reset.

This parameter is a list of Booleans; it is equal in length to the outer list.

def rand_mat(rows,cols=None):
    if cols==None:
        ret =np.random.normal(0,0.01,(rows,))
    else:
        ret = np.random.normal(0,0.01,(rows,cols))
    return ret

def reset_layers(layer_nums_to_reset,*orig):
    assert(len(layer_nums_to_reset)  == len (orig[0]))
    def random_like(mat):
        return rand_mat(*mat.shape)

    news = [ [] for _ in orig]

    for layer_info in zip(layer_nums_to_reset, *orig):
        reset = layer_info[0]
        knows = layer_info[1:]
        for new_know_list, old_know in zip (news,knows):
            new_know = random_like(old_know) if reset else old_know
            new_know_list.append(new_know)

    return news

Since this code scares me mere months later, it needs to be improved.  It is a complicated idea, so it is not surprising the implementation is complicated.

1. Code review

There's no documentation! To make your code maintainable, it's vital to explain the purpose of each function. What does it do, what arguments does it take, and what does it return?
The function rand_mat seems useless. It's a very thin wrapper around numpy.random.normal so instead of writing:

random_like(old_know)

write:

np.random.normal(0, 0.01, old_know.shape)

This would allow you to get rid of the rand_mat and random_like functions, which would be two fewer things to understand and maintain.
The numbers 0 and 0.01 should have names. They should perhaps also be parameters to the reset_layers function.
The name layer_nums_to_reset is misleading as it is list of Booleans, not a list of layer numbers. A name like layers_to_reset or maybe just reset would be better.
The assert only checks the length of layer_nums_to_reset against orig[0], but you wrote, "All the inner lists have the same lengths." It would be clearer to assert the stronger condition.
The reset_layers function does not really need to know what is in the layers (whether knowledge or whatever does not matter). In fact, it is confusing to bring this irrelevant detail into the function. Separation of concerns is an important technique for making maintainable code.
The arguments to reset_layers do not correspond to the return value. The caller must pass the layers as individual arguments, but gets back a list of layers. It would be clearer to pass a list of layers and return a list of layers.
You go to some effort to process the layers in an unnatural order: that is, to process the first element of each layer, then the second element of each layer, and so on. I guess you do this so that you only have to iterate over layer_nums_to_reset once. But this seems like a false economy if it makes the function more complex and harder to understand. Keep things simple!

2. Revised code

import numpy as np

def reset_layers(reset, layers, mean=0, std=0.01):
    """Return a list of lists of arrays that contains the same arrays as
    layers but with some replaced by arrays of the same shape
    containing normally distributed random numbers.

    layers -- a list of lists of arrays. The inner lists all have the
              same length, n.
    reset -- an array of Booleans of length n. The replaced arrays are
             at the positions given by the True values in this array.
    mean, std -- the mean and standard deviation of the random numbers.

    """
    assert(all(len(reset) == len(layer) for layer in layers))

    def reset_layer(layer):
        """Reset arrays in a single layer."""
        for res, old in zip(reset, layer):
            if res:
                yield np.random.normal(mean, std, old.shape)
            else:
                yield old

    return [list(reset_layer(layer)) for layer in layers]


Find out the number under 1 million which produces the largest Collatz Sequence So, I have written a code which calculates the number under n( 1 million in this case) which produces the largest Collatz Sequence in that range().

The problem is given in detail below:

The following iterative sequence is defined for the set of positive integers:

n → n/2 (n is even)
n → 3n + 1 (n is odd)

Using the rule above and starting with 13, we generate the following sequence:

13 → 40 → 20 → 10 → 5 → 16 → 8 → 4 → 2 → 1
It can be seen that this sequence (starting at 13 and finishing at 1) contains 10 terms. Although it has not been proved yet (Collatz Problem), it is thought that all starting numbers finish at 1.

Which starting number, under one million, produces the longest chain?

NOTE: Once the chain starts the terms are allowed to go above one million.

My code:

def main(n):
    list = [1,2]
    for i in range(3,n):
        count = 0
        while i > len(list):
            if i % 2 == 0:
                i = i/2
            else:
                i = 3*i+1
            count += 1
        list.append(count + list[int(i)-1])
    return list.index(max(list))+1

print(main(1000000))

So, what are the ways in which I can make my code more efficient since it takes quite a lot of time to execute?

I changed your main function a little bit to LargestCollatz which runs a bit faster on my machine. I'm not sure but I think the biggest improvement is in comparing two integers (c < i) instead of the len function (i > len(list)). Futhermore I used the impopular break statement and used // (which might be faster) instead of / so i could eliminate an int function.

def LargestCollatz(n):

    # Collatz lengths, first entries are 'hand calculated'
    cl = [0, 1, 2]

    for i in range(3, n):
        c = i
        length = 0
        while True:
            if c % 2 == 0:
                c = c//2
            else:
                c = 3*c+1
            length += 1

            if c < i:
                cl.append(length + cl[c])
                break

    return cl.index(max(cl))


Estimate  using Machin's formula Overview
Below is a simple program to estimate pi to the full precision of the provided type. It uses
Machin's formula, but this question is about coding style / choices. It compares the result to M_PI from <cmath> - best we have in C++17.
Questions

Is my use of enable_if_t appropriate and idiomatic?
Is the use of static_cast<T> for one_over_5 and one_over_239 the "best" solution here? Neccesary, and appropriate?
How about the use of decltype, ::epsilon and ::digits10?
Accuracy: How about sign =* -1 and den += 2.0 and ... = sign * ...? These are integer operations but use the floating point type.
My choice to force the user of pi<T>() to specify a T?

Note: I know the templates should probably be in header file, but all in one here for simplicity on CR.
#include <cmath>
#include <iomanip>
#include <iostream>
#include <type_traits>

template <typename T, std::enable_if_t<std::is_floating_point_v<T>, bool> = true>
T arctan(T tan) {
  // fabs(tan) > 1 will not converge
  T num  = tan;
  T den  = 1.0;
  T sign = 1.0;
  T term = num;
  T res  = num;
  while (std::fabs(term) > std::numeric_limits<T>::epsilon()) {
    num *= tan * tan;
    den += 2.0;
    sign *= -1.0;
    term = sign * num / den;
    res += term;
  }
  return res;
}

template <typename T, std::enable_if_t<std::is_floating_point_v<T>, bool> = true>
T pi() {
  T one_over_5   = static_cast<T>(1.0) / static_cast<T>(5.0);
  T one_over_239 = static_cast<T>(1.0) / static_cast<T>(239.0);
  return (static_cast<T>(4.0) * arctan<T>(one_over_5) - arctan<T>(one_over_239)) *
         static_cast<T>(4.0);
}

int main() {
  auto pi_float = pi<float>();
  auto pi_double = pi<double>();
  auto pi_long_double = pi<long double>();
  std::cout << std::setprecision(std::numeric_limits<decltype(pi_float)>::digits10)
            << pi_float << '\n'
            << std::setprecision(std::numeric_limits<decltype(pi_double)>::digits10)
            << pi_double << '\n'
            << std::setprecision(std::numeric_limits<decltype(pi_long_double)>::digits10)
            << pi_long_double << '\n'
            << M_PI << '\n';
  return EXIT_SUCCESS;
}

Output - demonstrating that M_PI from <cmath> is actually incorrect at long double precision in the last 2 digits (the correct value is 3.1415926535897932384626433...)
Note (unrelated to question, but for future reference): It turns out that the reason M_PI is "wrong" is because the M_PI macro does not specify a literal L for long double, and hence the value is rounded to double precision before being displayed. If you put a literal 3.14159265358979323846L then it is accurate. Usually that is provided as the macro M_PIl.
3.14159
3.14159265358979
3.14159265358979324
3.14159265358979312

Answers to your questions

Is my use of enable_if_t appropriate and idiomatic?

Yes, it is appropriate, and I believe idiomatic for C++17 as well. In C++20 the idiomatic way would be to use concepts of course.

Is the use of static_cast for one_over_5 and one_over_239 the "best" solution here? Neccesary, and appropriate?

It's necessary to cast at least a few constants to ensure the expression has the right type. However, in this case I would use the constructor syntax instead, and only do it for the minimum number of constants necessary, to keep the expression concise and legible:
return (4 * arctan(T{1} / 5) - arctan(T{1} / 239)) * 4;

Also note that you don't need to specify <T> here, it can be automatically deduced, and since this is so short there is no need to declare temporary variables one_over_5 and one_over_239.

How about the use of decltype, ::epsilon and ::digits10?

The use of T::digits10 is fine here. However, using T::epsilon() is always very dangerous. It works here only because you are trying to calculate something close to 1, or if you wanted to calculate the arctangent from a very small number you have the advantage that \$\lim_{x\to 0} \arctan x = x\$. In general, if you want to calculate a value using some converging expansion and want to stop if further steps are no longer giving you any increase in precision, you can do that by checking if there is any change in the floating point value:
while (res + std::fabs(term) != res) {...}

Or perhaps using std::nextafter(), to keep in the spirit of T::epsilon():
while (res + std::fabs(term) > std::nextafter(res, res + 1)) {...}

Although for std::nextafter(), it might even be hard to come up with the correct second argument.
Again, your method works fine in this case, it produces the same output as both methods above.

Accuracy: How about sign =* -1 and den += 2.0 and ... = sign * ...? These are integer operations but use the floating point type.

Currently those are floating point operations because the left hand side is a floating point type, and if the other side is an integer it would be promoted to the floating point type before the operation is performed. Note that you can replace all the constants in arctan() with integers and the result will still be correct.
You could change the type of sign and den to be an integer type, but it is not necessary. Multiplying floating point sign by -1 will not make it drift away from 1/-1 over time, and since integers up to \$2^{24}\$ can be represented exactly by a float, you will not have any problems with den either, unless the loop would run for more than 8388607 iterations, which would mean you have bigger problems.

My choice to force the user of pi() to specify a T?

It forces the user to give the correct type, which might result in less errors and/or more optimal code. You could consider making it double by default:
template <typename T = double, ...>
T pi() {...}

Note that C++20 introduces std::pi_v<> which also doesn't not have a default type.
Make the functions constexpr
You can and should make the functions constexpr. Interestingly, the compiler would be free to calculate the results of pi_float, pi_double and pi_long_double at compile time regardless, however GCC and Clang don't without constexpr. With constexpr, GCC does calculate the result at compile time, but Clang still emits a lot of code.


As far as functional programming goes, this looks pretty nice to me,
clear and readable.

One performance drawback is the splitting to a string array.
The size of the array will be similar to the input,
and it can be much smaller if you apply a filter directly on the input.
Although strings don't have a filter method like arrays,
since the selectTwins function would work on strings,
you could apply the filter method of arrays on the input directly:

[].filter.apply(parsedInput, [selectTwins])

Another performance drawback is that the input is scanned twice:
once to replace whitespace characters, and once more for splitting.
We could use the same technique as earlier to apply a filter on the input directly instead of using a regex replacement.

Putting the above suggestions together:

const isDigit = x => /\d/.test(x);
const solution = [].filter.apply(INPUT, [isDigit])
  .filter(selectTwins)
  .map(toNumber)
  .reduce(sum, 0);


Converting multiple query to use parameters to avoid SQL injection I have some dropdownlist in my aspx page and I am using the choices from them in my SQL query:

query = "";
DataTable taskData = new DataTable();
connString = @""; //connection string
strClause = "";

if (!blOnLoad)
{
    if (ddlTaskName.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND CT.ATTR2739 LIKE '%" + ddlTaskName.SelectedItem.Text + "%'";
        //strClause += string.format();
    }
    else
    {
        strClause += " AND (CT.ATTR2739 LIKE '%' OR CT.ATTR2739 IS NULL)";
    }
    if (ddlService.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND SE.ATTR2821 LIKE '%" + ddlService.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (SE.ATTR2821 LIKE '%' OR SE.ATTR2821 IS NULL)";
    }
    if (ddlStatus.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND CT.ATTR2812 LIKE '%" + ddlStatus.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (CT.ATTR2812 LIKE '%' OR CT.ATTR2812 IS NULL)";
    }
    if (ddlDueDate.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND CONVERT(VARCHAR(14), CT.ATTR2752, 110) LIKE '%" + ddlDueDate.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (CONVERT(VARCHAR(14), CT.ATTR2752, 110) LIKE '%' OR CONVERT(VARCHAR(14), CT.ATTR2752, 110) IS NULL)";
    }
    if (ddlOwner.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND UA.REALNAME LIKE '%" + ddlOwner.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (UA.REALNAME LIKE '%' OR UA.REALNAME IS NULL)";
    }
    if (ddlClient.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND C.ATTR2815 LIKE '%" + ddlClient.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (C.ATTR2815 LIKE '%' OR C.ATTR2815 IS NULL)";
    }
    if (ddlSite.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND SI.ATTR2819 LIKE '%" + ddlSite.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (SI.ATTR2819 LIKE '%' OR SI.ATTR2819 IS NULL)";
    }
    if (ddlPractice.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND PR.ATTR2817 LIKE '%" + ddlPractice.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (PR.ATTR2817 LIKE '%' OR PR.ATTR2817 IS NULL)";
    }
    if (ddlProvider.SelectedIndex > 0) //dropdownlist
    {
        strClause += " AND P.ATTR2919 LIKE '%" + ddlProvider.SelectedItem.Text + "%'";
    }
    else
    {
        strClause += " AND (P.ATTR2919 LIKE '%' OR P.ATTR2919 IS NULL)";
    }

    if (ddlTaskName.SelectedIndex == 0 && ddlService.SelectedIndex == 0 && ddlStatus.SelectedIndex == 0 && ddlDueDate.SelectedIndex == 0 && ddlOwner.SelectedIndex == 0 && ddlClient.SelectedIndex == 0 && ddlSite.SelectedIndex == 0 && ddlPractice.SelectedIndex == 0 && ddlProvider.SelectedIndex == 0)
    {
        query = strMainQuery + " WHERE CT.ACTIVESTATUS = 0";
    }
    else
    {
        query = strMainQuery + " WHERE CT.ACTIVESTATUS = 0" + strClause;
    }
}
else
{
    query = strMainQuery + " WHERE CT.ACTIVESTATUS = 0";
}

using (SqlConnection conn = new SqlConnection(connString))
{
    try
    {
        SqlCommand cmd = new SqlCommand(query, conn);

        SqlDataAdapter da = new SqlDataAdapter(query, conn);

        myDataSet = new DataSet();
        da.Fill(myDataSet);

        myDataView = new DataView();
        myDataView = myDataSet.Tables[0].DefaultView;

        yourTasksGV.DataSource = myDataView;
        yourTasksGV.DataBind();
    }
}

I am using the dropdownlist text right inside my SQL query which I am sure is prone to SQL injection which I am trying to prevent. I was looking around and found out I can use string.format() along with .Parameters.AddWithValue(); to ensure there is no SQL injection.

I am not sure how to actually take my code above and change it entirety to use Parameters.

How can I achieve the use of parameters instead of taking the dropdownlist text?

The recommended way to avoid SQL injection attacks is to use parameters.
Also I can recommend that you create a stored procedure instead of using dynamic SQL.

You can pass your dropdown indexes as parameters to your SP and use the old trick of using it as conditional on the where clause.

Your select can end up like this:

select CT.columnA, CT.columnB
from tableA CT
join tableB SE on SE.idA = CT.id
where
      (@ddlTaskName_SelectedIndex > 0 and (CT.ATTR2739 LIKE '%' + @ddlTaskName_SelectedItemText))
-- or (@ddlTaskName_SelectedIndex = 0 and (CT.ATTR2739 LIKE '%' OR CT.ATTR2739 IS NULL))     -- if you think enough ill see this line is unnecessary
and
      (@ddlService_SelectedIndex > 0 and (SE.ATTR2821 LIKE '%' + @ddlService_SelectedItemText + '%')
-- This "else" line is unnecessary too, since we don't really filter it because eveything is "like %" or "null"

And so on ...

Those dropdown texts are still prone to SQL injection since a smart hacker may be able to change its value depending on your system.
You can fix it to just set variables inside the SP, this mean you will not pass the drop down text, just the indexes and do some swith case to set that varchar variables before the select.

That can add some maintenance issue, since your dropdown texts must match the same texts hardcoded on your SP. A better approach can be to create a domain table for each dropdown:

Create table DropDownClientValues
(
 index int
,text varchar(50)
)

And list your dropdown values directly from those domain tables; that way all your selects/SP can refer the exact same values.

This has the advantage of making adding/removing options from your drop downs very easy and with no impact in your code. Sadly this can be a bit onerous to refactor a big app that way.


Flask Framework - user sign in These functions are more or less the same. They are doing the sign in process for a flask application. A user is retrieved from a database (identified through email address) and the password is checked for validity (bcrypt). If either of those failed, an error should be displayed for the user in the template (signin.html).

What is considered to be more readable or elegant. Are there other, nicer solutions?

@app.route('/signin', methods=['POST'])
@public
def do_signin():
    user = get_user_by_email([request.form.get('email')])
    invalid = user is None or not bcrypt.checkpw(
        request.form.get('password').encode('utf-8'),
        user['password'].encode('utf-8'),
    )

    if invalid:
        error = 'Invalid Email or Password'
    else:
        session[SESSION_TOKEN] = add_token(user['id'])
        return redirect(url_for('index'))
    return render_template('signin.html', error=error or None)

or

@app.route('/signin', methods=['POST'])
@public
def do_signin():
    user = get_user_by_email([request.form.get('email')])
    error = 'Invalid Email or Password' if user is None or not bcrypt.checkpw(
        request.form.get('password').encode('utf-8'),
        user['password'].encode('utf-8'),
    ) else None

    if error is None:
        session[SESSION_TOKEN] = add_token(user['id'])
        return redirect(url_for('index'))
    return render_template('signin.html', error=error or None)

or

@app.route('/signin', methods=['POST'])
@public
def do_signin():
    user = get_user_by_email([request.form.get('email')])

    if user is None:
        error = 'Invalid Email or Password'
    elif not bcrypt.checkpw(
        request.form.get('password').encode('utf-8'),
        user['password'].encode('utf-8')
    ):
        error = 'Invalid Email or Password'
    else:
        session[SESSION_TOKEN] = add_token(user['id'])
        return redirect(url_for('index'))
    return render_template('signin.html', error=error or None)

There are a few things bothering me in one version or an other:

the error=error or None is unnecessary as error is either assigned 'Invalid Email or Password' or  None, stick to the simpler error=error;
the ): on its own line in the third version is more than disturbing, it feels like a syntax error and force the reader to go through the code again to understand what is going on. Instead, put it at the end of the previous line and include more indentation to distinguish the condition from the rest.;
each version seem to consider the success case as an error of the error case, that is the flow of reading the code does not feel as if it's a signin function (this seems to be the bad outcome) but rather a deny_access function;
the third version seems to accept invalid password and deny entry on successful passwords.

Instead, I would test each failing condition and bail out with a specific error message at that point rather than making the error case the "natural" exit point of the function:

@app.route('/signin', methods=['POST'])
@public
def do_signin():
    user = get_user_by_email([request.form.get('email')])
    if user is None:
        return render_template('signin.html', error='Invalid Email')

    if not bcrypt.checkpw(
            request.form.get('password').encode('utf-8'),
            user['password'].encode('utf-8')):
        return render_template('signin.html', error='Invalid Password')

    session[SESSION_TOKEN] = add_token(user['id'])
    return redirect(url_for('index'))


Leap year calculator I'm creating a program to determine whether or not a given year is a leap year. I just wanted to get anyone's feedback on it. I am aware of the calendar module in Python. I just wanted to try it from scratch.

year=int(raw_input("Input year:"))

if (year%4==0 and year%100==0 and year%400==0)or (year%4==0 and year%100 !=0 and year%400==0)or(year%4==0 and year%100 !=0 and year%400!=0):
  print str(year)+" is a leap year"
else:
    print str(year)+" is NOT a leap year"

Logic

Consider the if condition, which I have reproduced below with different alignment for ease of viewing…

if (year%4==0 and year%100==0 and year%400==0)or \
   (year%4==0 and year%100!=0 and year%400==0)or \
   (year%4==0 and year%100!=0 and year%400!=0):
    …

It's clear that the first two lines can be collapsed due to redundancy.  As for the third line, any number that is not divisible by 100 is automatically not divisible by 400 as well.

if (year%4==0 and year%400==0)or \
   (year%4==0 and year%100!=0):
    …

That can be further simplified to:

if (year%4==0 and (year%100!=0 or year%400==0)):
    …

Formatting

Please take care to follow Python formatting conventions, especially the consistent indentation by four spaces, especially since whitespace is significant in Python.

year = int(raw_input("Input year: "))

if year % 4 == 0 and (year % 100 != 0 or year % 400 == 0):
    print str(year) + " is a leap year"
else:
    print str(year) + " is NOT a leap year"


Union find using unordered map and templates in C++ I tried to implement union find (disjoint set data structure) algorithm in C++. There I used unordered_map data structure. Is there a better way of doing this using any other data structure.

While finding the root I have used the following way.

int root(int i){
        if(i != parent[i]) parent[i] = root(parent[i]);
        return parent[i];
    }

Also, I can implement it as

int root(int i){
        while (i != parent[i]){
            parent[i] = parent[parent[i]]);
            i = parent[i];
        }
        return i;
    }

It seems like both are same other than the (recursive and iterative) approach for me. Which is better here!

The following is my implementation.

#include<iostream>
#include<unordered_map>
#include<vector>
using namespace std;

class Disjoinset{

    private:

    unordered_map<int, int> parent, rank;
    // find the root
    int root(int i){
        if(i != parent[i]) parent[i] = root(parent[i]);
        return parent[i];
    }

    public:

    // initialize the parent map
    void makeSet(int i){
        parent[i] = i;
    }
    // check for the connectivity
    bool is_connected(int p, int q){
        return root(p) == root(q);
    }
    // make union of two sets
    void Union(int p, int q){

        int proot = root(p);
        int qroot = root(q);

        if(proot == qroot) return;

        if(rank[proot] > rank[qroot]) {
            parent[qroot] = proot;
        }
        else{
            parent[proot] = qroot;
            if(rank[proot] == rank[qroot]) rank[qroot]++;
        }
    }
};

Driver code

int main(){
    vector<int> arr = {1,2,9,8,6,5,7};
    Disjoinset dis; // create a disjoint set object
    for(int x: arr){
        dis.makeSet(x); // make the set
    }
    dis.Union(1,9); // create connections
    dis.Union(6,5);
    dis.Union(7,9);
    cout <<  dis.is_connected(1,7) << endl; // check the connectivity.
    cout <<  dis.is_connected(1,6) << endl;
    return 0;
}

Thereafter I tried to use generics and I came up with following. Can someone help me on this as I am new to C++. Is my implementation is correct or are there any better approach.

#include<iostream>
#include<unordered_map>
#include<vector>
#include<string>
using namespace std;

template<class T>
class Disjoinset{

    private:

    unordered_map< T, T> parent;
    unordered_map< T, int>rank;
    // find the root
    T root(T i){
        if(i != parent[i]) parent[i] = root(parent[i]);
        return parent[i];
    }

    public:

    // initialize the parent map
    void makeSet(T i){
        parent[i] = i;
    }
    // check for the connectivity
    bool is_connected(T p, T q){
        return root(p) == root(q);
    }
    // make union of two sets
    void Union(T p, T q){

        T proot = root(p);
        T qroot = root(q);

        if(proot == qroot) return;

        if(rank[proot] > rank[qroot]) {
            parent[qroot] = proot;
        }
        else{
            parent[proot] = qroot;
            if(rank[proot] == rank[qroot]) rank[qroot]++;
        }
    }
};

int main(){
    vector<string> arr = {"amal", "nimal", "kamal", "bimal", "saman"};
    Disjoinset <string>dis; // create a disjoint set object
    for(string x: arr){
        dis.makeSet(x); // make the set
    }
    dis.Union("amal", "kamal"); // create connections
    dis.Union("kamal", "nimal");
    cout <<  dis.is_connected("amal", "nimal") << endl; // check the connectivity.
    cout <<  dis.is_connected("bimal", "amal") << endl;
    return 0;
}

Thank you for every answer.

Overall, that looks good to me. A few nitpicks, however, follow:

Advice 1

#include<iostream>
#include<unordered_map>
#include<vector>
#include<string>

I would put a space between e and <. Also, I would sort the rows alphabetically, so that we get:

#include <iostream>
#include <string>
#include <unordered_map>
#include <vector>

Advice 2: One template per file

I would extract away the main and put the entire disjoint set template into its own file; call it DisjointSet.hpp to begin with.

Advice 3: Protect your header files from multiple inclusion

I would most definitely put the header guards in DisjointSet.hpp so that it looks like this:

#ifndef COM_STACKEXCHANGE_RUSIRU_UTIL_HPP
#define COM_STACKEXCHANGE_RUSIRU_UTIL_HPP
.
. Your funky DisjointSet.hpp code here. :-)
.
#endif // COM_STACKEXCHANGE_RUSIRU_UTIL_HPP

Advice 4: "Package" your code into namespaces

I have a habit of putting my data structures into relevant namespaces in order to avoid name collisions with other people's code:

namespace com::stackexchange::rusiru::util {
    template<class T>
    class DisjointSet {
        ...
    };
}

Advice 5: Remove minor noise

unordered_map< T, T> parent;
unordered_map< T, int>rank;

I would write:

unordered_map<T, T> parent;
unordered_map<T, int> rank;

Note the placement of spaces!

Advice 6: Arbitrary method naming scheme

Essentially, you have makeSet, is_connected, Union. That's three different method naming schemes right there; choose one and stick to it. For example, makeSet, isConnected, union.

Advice 7: Printing booleans to stdout:

You can do cout << std::boolalpha << ...; in order to print true/false.

Advice 8: Don't pollute your namespace

Generally speaking, using namespace std; is not what you may see in professional C++ code. Consider using individual uses such as:

using std::cout;
using std::endl;

Putting all together

Overall, I had this in mind:

DisjointSet.hpp

#ifndef COM_STACKEXCHANGE_RUSIRU_UTIL_HPP
#define COM_STACKEXCHANGE_RUSIRU_UTIL_HPP

#include <iostream>
#include <string>
#include <unordered_map>
#include <vector>

namespace com::stackexchange::rusiru::util {

    using std::boolalpha;
    using std::cout;
    using std::endl;
    using std::string;
    using std::unordered_map;
    using std::vector;

    template<typename T>
    class DisjointSet {

    private:

        unordered_map<T, T> parent;
        unordered_map<T, int> rank;

        // find the root
        T root(T i) {
            if (i != parent[i]) parent[i] = root(parent[i]);
            return parent[i];
        }

    public:

        // initialize the parent map
        void makeSet(T i) {
            parent[i] = i;
        }
        // check for the connectivity
        bool isConnected(T p, T q) {
            return root(p) == root(q);
        }
        // make union of two sets
        void union(T p, T q) {

            T proot = root(p);
            T qroot = root(q);

            if (proot == qroot) return;

            if (rank[proot] > rank[qroot]) {
                parent[qroot] = proot;
            }
            else {
                parent[proot] = qroot;
                if (rank[proot] == rank[qroot]) rank[qroot]++;
            }
        }
    };
}

#endif // COM_STACKEXCHANGE_RUSIRU_UTIL_HPP

main.cpp

#include "DisjointSet.hpp"
#include <iostream>
#include <string>
#include <vector>

using std::boolalpha;
using std::cout;
using std::endl;
using std::string;
using std::vector;
using com::stackexchange::rusiru::util::DisjointSet;

int main() {
    vector<string> arr = { "amal", "nimal", "kamal", "bimal", "saman" };
    DisjointSet<string> dis; // create a disjoint set object

    for (const string x : arr) {
        dis.makeSet(x); // make the set
    }

    dis.union("amal", "kamal"); // create connections
    dis.union("kamal", "nimal");

    cout << boolalpha << dis.isConnected("amal", "nimal") << endl; // check the connectivity.
    cout << dis.isConnected("bimal", "amal") << endl;
    return 0;
}


”check” function – OK to throw in destructor? My goal is to provide a function with the following functionality:

// Will throw iff a != 42.
check(a == 42) << a << " is not equal to 42!";

I have the following:

class check
{
public:
    check(bool everything_OK = false)
        : should_throw{!everything_OK}
    { }

    check(const check&) = delete;

    ~check() noexcept(false)
    {
        if (should_throw && !uncaught_exception()) {
            throw runtime_error(stream.str());
        }
    }

    template<typename T>
    check& operator << (T&& t)
    {
        if (!should_throw) {
            return *this;
        }

        stream << std::forward<T>(t);
        return *this;
    }

private:
    const bool should_throw;
    stringstream stream;
};

The thing is, all guidelines I have read forbid throwing in the destructor. I have also read experts saying they do not know of a good use of std:uncaught_exception. Can I still achieve what I want?

ADDITION:
Loki’s point below can be fixed by defining a macro:

#define my_check(expr) (expr) ? ((void)0) : check(false)

But once we allow ourselves to use macros, there are other solutions provided below.

Ignoring the throwing out of the destructor (which is a bad idea).

My main problem is with usage:

check(a == 42) << a << " is not equal to 42!";

Here you present a trivial case. But not all error checking is trivial. I think a more realistic case (that will happen more often than you think);

Universe  a;
// .... STUFF
check(a.theMeaningLTUAE(42)) << a << " The truth not reached " << a.checkInvariants();

Here we have calls to functions that will need to be evaluated even if they are never used (specifically checkInvariants() may be expensive (or even the streaming of a)). If the check passes you are still obliged to call it here but its value are simply discarded. I would prefer that the function is never called if no error message is going to be produced.

What you really want is a short cut operator that prevents subsequent operations from happening. Note overloading && and || on a class does not yield short-cut operators.

Or alternatively any functions should be passed as callbacks to operator<< and only evaluated if there output is required.


Container of objects with unique IDs This ObjectSet container adds new Objects with lowest possible unique ID. My implementation uses std::set for its sorting, but this convenience may not be required.

My two main questions are:

Is std::set generally a good and preferred solution for this problem?
In case of using std::set, can ObjectSet::minId be implemented with use of standard algorithms?

Code:

#include <iostream>
#include <set>
#include <string>

class Object
{
    std::uint32_t id_;

public:
    explicit Object(std::uint32_t id) : id_(id) {}

    std::uint32_t const& id() const
    {
        return id_;
    }

    std::string name() const
    {
        return "Object" + std::to_string(id_);
    }

    bool operator<(Object const& other) const
    {
        return id_ < other.id_;
    }
};

class ObjectSet : std::set<Object>
{
public:
    void addObject()
    {
        insert(Object(minId()));
    }

    void removeObject(std::uint32_t id)
    {
        erase(Object(id));
    }

    std::uint32_t minId() const
    {
        if(empty())
            return 1;

        for(auto it1 = begin(), it2 = it1++; it1 != end(); ++it1, ++it2)
            if(it1->id() != it2->id() + 1)
                return it2->id() + 1;

        return rbegin()->id() + 1;
    }

    using Base = std::set<Object>;

    using Base::begin;
    using Base::cbegin;
    using Base::rbegin;
    using Base::crbegin;

    using Base::end;
    using Base::cend;
    using Base::rend;
    using Base::crend;
};

int main()
{
    ObjectSet objects;
    objects.addObject();
    objects.addObject();
    objects.addObject();
    objects.removeObject(2);
    objects.addObject();

    for(auto const& object : objects)
        std::cout << object.name() << std::endl;
}

Looks pretty good to me. The only glaring stylistic issue I see is that you inherit from std::set instead of having a member of that type. Inheriting from classes whose innards you don't personally control usually leads to issues with slicing and/or expecting-polymorphism-and-not-getting-it. However, since you're doing private inheritance, this is probably okay.

There are a couple of functional improvements you might want to make (but then again might not, because they're subtle).

(1) Your Object type has an operator< that does nothing but compare ids. That's unlikely to be useful in practice. But you need to have that "compare ids" operation so that you can make your sorted set. The solution is to pass a comparator to your set, like so:

class ObjectSet {
    static bool id_less(const Object& a, const Object& b) {
        return a.id() < b.id();
    }
    std::set<Object, decltype(&id_less)> items_;

  public:
    ObjectSet() : items_(id_less) {}

    // ...
};

This allows you to reclaim Object::operator< for some more appropriate semantics.

(2) Continuing with comparators... Every time I call removeObject, you construct a whole Object to pass to erase. This seems wasteful (assuming Object is a heavyweight type), and also counter-intuitive: what do you mean, removeObject calls the constructor of Object?! :)

The solution requires C++14 or better, because it depends on a particular overload of std::set<T>::find() that first showed up in C++14.

class ObjectSet
{
    static bool id_less(const Object& a, const Object& b) { return a.id() < b.id(); }
    static bool id_less(const Object& a, std::uint32_t b) { return a.id() < b; }
    static bool id_less(std::uint32_t a, const Object& b) { return a < b.id(); }

    struct Comparator {
        template<class T, class U>
        bool operator()(const T& a, const U& b) { return id_less(a, b); }
        using is_transparent = void;
    };

    std::set<Object, Comparator> items_;

    // ...

    void removeObject(std::uint32_t id)
    {
        items_.erase(items_.find(id));
    }
};

(Wandbox link.)


2D array zeroing C++ style I have a valid code in C-style
int arr[10][10] = {0}; // all elements of array are 0

What is the optimal way to do the same for C++ style array?
My idea is
#include <array>

std::array<std::array<int, 10>, 10> arr = {0}; // is it correct?

I feel this question is more suited for stack overflow, but I'll answer it anyway. In c++ the preferred way to do this is:
std::array<std::array<int, 10>, 10> arr = {}; // Without the 0

Also remember that static variables are zero-initialised anyway, so you wouldn't need to do that.
Because this is code review, I would suggest using a typedef to replace std::array<std::array<int, 10>, 10> to make your code more readable.
EDIT: As pointed out in the comments, using is much more powerful than typedef and more modern-c++ ish and one should preferably use it rather than typedef. It supports type aliasing templates and all other cool stuff, which is why it is preferable to use. ( Though never use using namespace ...;)


Python program for fibonacci sequence using a recursive function A Fibonacci sequence is the integer sequence of 0, 1, 1, 2, 3, 5, 8.....

The first two terms are 0 and 1. All the other terms are obtained by adding the preceding two terms.

Here is my code:

def recur_fibonacci(n):
   if n <= 1:
       return n
   else:
       return(recur_fibonacci(n - 1) + recur_fibonacci(n - 2))

nterms = int(input("How many terms? "))

if nterms <= 0:
   print("Please enter a positive integer!")
else:
   print("Fibonacci sequence:")
   for i in range(nterms):
       print(recur_fibonacci(i))

So I would like to know whether I could make this program shorter and more efficient.

Any help would be highly appreciated.

Since other answers have focused on the code quality itself, I'll focus on performance.

Recursive Fibonacci by itself is \$O(2^n)\$ time.

Memoized fibonacci is linear time (check out functools.lru_cache for a quick and easy one). This is because fibonacci only sees a linear number of inputs, but each one gets seen many times, so caching old input/output pairs helps a lot.

Golden-ratio based solutions are approximately \$O(\log(n))\$, using \$\text{Fib}(n) = \frac{\phi^n - (1 - \phi)^n}{\sqrt 5}\$, where \$\phi\$ is the golden number. Note that without arbitrary precision numbers, this approach becomes inaccurate at large values of n. With increased precision, note that the cost of a multiplication increases as well, making the whole process a bit slower than log(n).


Evaluating Fibonacci-sequence-based recurrence relation (SPOJ Flibonakki) I have a function fib2 which returning two values as Tuple but I want to print the product of tuples returned by function fib2. I am new to Haskell so please explain it to me in a simple way.

This problem is from SPOJ which I have already solved using C and again I am trying it in Haskell:

  For each \$n\$, find \$G(n) \mod 1000000007\$, where
  $$\begin{align*}
G(0) &= 0 \\
G(n) &= G(n-1) + f(4n - 1), \qquad n > 0 \\
f(i) &= \mathrm{the\ i^{th}\ Fibobacci\ number}
\end{align*}$$

  The first line of input specifies the number of test cases.

Now my code is working fine but I'm getting TLE in this problem. Can someone show me how to optimize Fibonacci in Haskell?

import Data.List
import Data.Maybe
import Control.Monad
import qualified Data.ByteString.Char8 as C

fib2 0 = (1, 1)
fib2 1 = (1, 2)
fib2 n
 | even n    = ((a*a + b*b)`mod` 1000000007, (c*c - a*a)`mod` 1000000007)
 | otherwise = ((c*c - a*a)`mod` 1000000007, (b*b + c*c)`mod` 1000000007)
 where (a,b) = fib2 (n `div` 2 - 1)
       c     = a + b

solve n = (a*b)`mod`1000000007
    where (a,b) = fib2((2*n-1)`mod`2000000016)

main = C.getContents >>= putStrLn . unlines . map (show.solve.fromIntegral . fst . fromJust . C.readInteger) . tail . C.words

First of all, make sure to add type signatures and use the appropriate number type. All your numbers are fine with Int, since they're bounded by

2 * (1000000006 * 1000000006)

Therefore, you can get rid of Integer throughout your program and simply use Int:

fib2 :: Int -> (Int, Int)
fib2 0 = (1, 1)
fib2 1 = (1, 2)
fib2 n
 | even n    = ((a*a + b*b)`rem` 1000000007, (c*c - a*a)`rem` 1000000007)
 | otherwise = ((c*c - a*a)`rem` 1000000007, (b*b + c*c)`rem` 1000000007)
 where (a,b) = fib2 (n `quot` 2 - 1)
       c     = a + b

Next, you want to use rem and quot, since they're usually faster and provide the same result as mod and div on positive numbers.

We also add a type signature to solve:

solve :: Int -> Int
solve n = (a*b)`rem`1000000007
    where (a,b) = fib2((2*n-1)`rem`2000000016)

Last but not least, we now simplify your main:

main :: IO ()
main = C.getContents >>= putStrLn . unlines . map (show.solve. fst . fromJust . C.readInt) . tail . C.words

However, I don't think that the IO part is the bottle-neck, so your usual String methods should be fine for this challenge, e.g.

main = getContents >>= putStrLn . unlines . map (show . solve . read) . tail . words

Indeed, this version of your program gets accepted of SPOJ. But so did your one. I guess that they updated the GHC version* and used the appropriate optimization flags. But you can make sure that they use optimization via

{-# OPTIONS_GHC -O2 #-}

* I just rememered that there was a defect with even in GHC 7.10. If they used GHC 7.10 (which is unlikely), then you were affected by that defect.


Thread Safe Service to Perform Tasks in a Queue In answering a question on SO, I provided this example.  The intention was to provide a thread safe service that queues requests to make directories.  I figured I could definitely use some review on my use of threading, singletons and general Java skills.

package mkdir;

import java.io.File;
import java.util.concurrent.ConcurrentLinkedQueue;

public class MkDirService implements Runnable {

    private static MkDirService service;
    private static ConcurrentLinkedQueue<File> pendingDirs;

    private MkDirService() {
        pendingDirs = new ConcurrentLinkedQueue<File>();
    }

    public static MkDirService getService() {
        if (service == null) {
            service = new MkDirService();
            new Thread(service).start();
        }
        return service;

    }

    public void makeDir(File dir) {
        pendingDirs.add(dir);
    }

    @Override
    public void run() {
        while (true) {
            while (!pendingDirs.isEmpty())
            {
                File curDir = pendingDirs.poll();
                if (curDir !=null && !curDir.exists()) {
                    curDir.mkdir();
                }
            }
            try {
                Thread.sleep(100);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

    }

}

pendingDirs should not be static. It's one thing to maintain a static instance to implement a singleton, but that object should interact with its state using normal instance variables. And you may as well initialize it in its declaration instead of the constructor.

The service should create the thread itself instead of creating it in getService() to make it self-contained. This allows another implementation to use a thread pool for parallel directory creation and easier testing.

getService() is not thread-safe and could allow two or more instances to be created.

Use a blocking queue, so you don't have to poll and sleep. The thread will be put to sleep if the queue is empty and be awoken once a new directory is added to the queue. This will shrink your run loop to a few lines and allow you to rely on the well-tested thread-management in java.util.concurrent.


Auto move files to a sub folder with Ruby I started writing a script to handle my file downloads in osx because they quickly pile up and it gets time consuming to do this manually by drag and drop. I am looking forward to get some feedback.

require 'filewatcher'
require 'pathname'
require 'fileutils'

class FileAction

  def initialize(filename, event)
    @filename = filename
    @event = event
    @path = Pathname.new(@filename)
    @basename = @path.basename.to_s
    @base_path = "/Users/digerati"
    @dl_path = "#{@base_path}/Downloads"
    @dl_docs_path = "#{@base_path}/Downloads/Documents"
    create_folder_if_not_exist
    move_file
  end

  def create_folder_if_not_exist
    Dir.mkdir(@dl_docs_path) unless File.exists?(@dl_docs_path)
  end

  def move_file
    if(@event == :new)
      puts "Downloaded a document:" + @filename
      puts "Basename: " + @basename
      begin
        FileUtils.mv("#{@dl_path}/#{@basename}", "#{@dl_docs_path}/#{@basename}")
      rescue => e
        puts "+ Error: #{e.message}"
      end
    end
  end
end

@doc_files = ["pdf", "docx"].collect { |f| "~/Downloads/*.#{f}" }
FileWatcher.new(@doc_files).watch do |filename, event|
  FileAction.new(filename, event)
end

The constructor, initialize should only prepare the state of the object but not execute methods like move_file. You could call FileAction.new(filename, event).move_file instead.

You use a lot of instance variables that could replaced by local variables and method arguments. It follows the principle of minimum scope. That makes your code more functional. As result the methods become independ of the context of the class.

The class is not necessary since you have no real state to encapsulate.

FileWatcher.new(@doc_files).watch do |filename, event|
  move_file(filename) if event == :new
end

Hope it helps.


Is there a better version for verbosing the output of the euclidean method? Here is my implementation of the Euclidean Algorithm. My question is how to make it more "professional". It's working right, but isn't this too newbie?

public class Euklideszi {

    public int Euklideszi(int a, int b, boolean lepesrolLepesre) {
        int r = a % b;
        if (a < b) {
            r = a;
            a = b;
            b = r;
        }
        if (a % b == 0) {
            if (lepesrolLepesre) {
                System.out.println("A legnagyobb közös osztó: " + b);
            }
            return b;
        } else {
            while (r != 0) {
                r = a % b;
                if (lepesrolLepesre) {
                    System.out.println(a + "=" + a / b + "*" + b + "+" + r);
                }
                a = b;
                b = r;
            }
            if (lepesrolLepesre) {
                System.out.println("A legnagyobb közös osztó: " + a);
            }
            return a;
        }

    }

 }

There are a couple of "newbie" things here, yes.

Your method has an uppercase first character, it should be lower-case.
Your method is not static, which means that an instance of the class has to be created just to use your method. You can make the method static, and add a private constructor to the class so that it can't be initialized from outside.
As @skiwi says in a comment, please use English identifier names when you code.
boolean lepesrolLepesre is used to toggle whether or not the method should log anything to console. This is a bad thing. A method like this should not output anything to the console, it should simply return the result.
This code:

if (a < b) {
    r = a;
    a = b;
    b = r;
}

is used to switch so that a always is the largest variable, and it's using r as a temporary variable. Even though this works, I would use a temp variable here that's only visible in this scope.

if (a < b) {
    int temp = a;
    a = b;
    b = temp;
}
int r = a % b;

Note that here, I put the initial calculation of r after this switch has taken place.
There is no need to use the else as you return inside the if. You can remove else and reduce the indentation there.

Edit:

In the comments below you asked for a cleaner way for the user to see every step, you can accomplish this by using a callback interface.

Then you can modify your code to:

public static interface EuklidesStepCallback {
    void onStep(int a, int b, int r);
}
public static int gcd(int a, int b, EuklidesStepCallback callback) {
    ...
    while (r != 0) {
        r = a % b;
        if (callback != null) {
            callback.onStep(a, b, r);
        }
        a = b;
        b = r;
    }
    return a;
}

public static void main(String[] args) {
    EuklidesStepCallback callback = new EuklidesStepCallback() {
        @Override
        public void onStep(int a, int b, int r) {
            System.out.println(a + "=" + a / b + "*" + b + "+" + r);
        }
    };
    System.out.println(gcd(24, 16, callback));
}

If you're able to use Java 8, you can use a lambda for the callback:

public static void main(String[] args) {
    System.out.println(gcd(24, 16, (a, b, r) -> System.out.println(a + "=" + a / b + "*" + b + "+" + r)));
}

This callback system is more flexible in case you'd want to save each of the steps to a List or similar.


Alternative for two sequentially executed loops with identical ranges For the moment I'm executing my code as follows:

foreach (int data in dataList)
{
    PreProcess(data);
}
foreach (int data in dataList)
{
    Process(data);
}

First I have to loop over some data (from a list) and to do some pre-processing after which then I do have to loop again over the same data list and do some processing, but those loops have to be run one after another, and cannot be merged as follows...

foreach (int data in dataList)
{
    PreProcess(data);
    Process(data); // Cannot be run in the same loop with PreProcess(data);
}

...because PreProcess(data) and Process(data) are logging a lot of information which could be interlaced in case both are looping together.

The only alternative I've found will be using Linq, but still has the same duplicate ForEach:

dataList.ForEach(data => { PreProcess(data); });
dataList.ForEach(data => { Process(data); });

Is there any way to rewrite this in order to remove the redundancy for two loops with same range, but still looping sequentially first over PreProcess(data) and Process(data) afterwards?

You could also define something like

private void MyLoop<T>(Action<T> action, IEnumerable<T> dataList)
{
    foreach (var data in dataList)
    {
        action(data);
    }
}

and use it

MyLoop(Process, dataList);

or using Action

Action<Action<int>> myLoop= action =>
{
    foreach (var data in dataList)
    {
        action(data);
    }
};

and use it like this

myLoop(Process);


Square root function As a programming exercise I made a square root function in C# using recursion, here is the function:

static double root(double x, double increment, double num)
{
    num += increment;
    if ((num * num).ToString() == x.ToString())
        return num;
    else if (num * num < x)
        return root(x, Math.Abs(increment), num);
    else if (num * num > x)
        return root(x, Math.Abs(increment / 2) * -1, num);
    else
        return double.NaN;
}

static double sqroot(double x)
{
    return root(x, 10, 0);
}

It is able to do int.MaxValue without a problem, but my question is, how can this be improved?

A couple of things.

Is the string conversions really necessary just for comparing 2 doubles?

Figuring the squared value on the fly seems to lead to a bit of repetition

Also, optional parameters make the initial calling simpler.

static double root(double x, double increment = 10, double num = 0)
{
    num += increment;
    double sqrdValue = num * num;
    if (sqrdValue == x)
        return num;
    else if (sqrdValue < x)
        return root(x, Math.Abs(increment), num);
    else if (sqrdValue > x)
        return root(x, Math.Abs(increment / 2) * -1, num);
    else
        return double.NaN;
}

Since it appears that integers will introduce rounding errors, you filter for that by checking if the difference is less than or equal to 0.000000000000001, instead of just equal to, which gives the same degree of accuracy in the final answer as the built-in sqrt method of the Math class.

static double root(double x, double increment = 10, double num = 0)
{
    double maxError = 0.000000000000001;
    num += increment;
    double sqrdValue = num * num;
    if (Math.Abs(sqrdValue - x) <= maxError)
        return num;
    else if (sqrdValue < x)
        return root(x, Math.Abs(increment), num);
    else if (sqrdValue > x)
        return root(x, Math.Abs(increment / 2) * -1, num);
    else
        return double.NaN;
}


Segregate even and odd numbers in a given array Given an array A[], write a function that segregates even and odd
  numbers. The functions should put all even numbers first, and then odd
  numbers.

  Example:

  Input = {12, 34, 45, 9, 8, 90, 3}
  Output = {12, 34, 8, 90, 45, 9, 3}

  Input = {6, 4, 6, 8, 1, 1, 1}
  Output = {6, 4, 6, 8, 1, 1, 1}

public static void main (String args[]) {
    int []array = {6,1,6,8,2,2,3};
    evenOddFunction(array);
    System.out.println(Arrays.toString(array));
}

private static void evenOddFunction(int []data) {
    int leftSide = 0;
    int rightSide = data.length - 1;

    while(rightSide >= leftSide) {
        if(data[leftSide] % 2 != 0 && data[rightSide] % 2 == 0) {
             // swapping as soon as we find even and odd combination
             swappEvenOdd(data, leftSide, rightSide);
             leftSide++;
             rightSide--;
        } else {
            if(data[leftSide] % 2 == 0) {
                leftSide++;
            }
            if(data[rightSide] % 2 == 1) {
                rightSide--;
            }
        }
    }
}

private static void swappEvenOdd(int []data , int left, int right) {
    // swapping even and odd numbers
    int temp = data[left];
    data[left] = data[right];
    data[right] = temp;
}

I don't want to use another array for this so went with swap logic. Any chances of improvement/optimizations in this code?

What is the minimum space and time complexity we can achieve for this problem?

I strongly doubt that your program does exactly this:

Example:
  Input = {12, 34, 45, 9, 8, 90, 3}
  Output = {12, 34, 8, 90, 45, 9, 3}

AFAIK the output will be

  Output = {12, 34, 90, 8, ...}

i.e., the elements get reordered because of the swapping. But that's alright according to what's required.

 private static void evenOddFunction(int []data) {

It's not a function, it's a method. And calling something evenOddMethod wouldn't be any better as there's no information therein. something like evenOddSegregator sounds better, but method names should be verbs. So maybe segregateEvenOdd?

    if(data[leftSide] % 2 != 0 && data[rightSide] % 2 == 0) {
        ...
    } else {
        if(data[leftSide] % 2 == 0) {
            ...
        }
        if(data[rightSide] % 2 == 1) {
            ...
        }

I hate repeated conditions. What about

    if(data[leftSide] % 2 == 0) {
        ...
    } else if(data[rightSide] % 2 == 1) {
        ...
    } else {
        ...
    }

It's not exactly the same, but this doesn't matter.

private static void swappEvenOdd(int []data , int left, int right) {

This is misnomer (and a type, too). There's nothing even nor odd in this method.

  Any chances of improvement/optimizations in this code?

I guess your code is optimal, apart from this low-level optimization: Instead of (x % 2) use x & 1; it's much faster, works better for negative numbers, etc. But the JIT knows this trick too and there's not so much to gain.

An untested, stupid, but funny solution:

Comparator<Integer> comparator = new Comparator<>() {
    public int compare(Integer a, Integer b) {
        return (a & 1) - (b & 1);
    }
}
Collections.sort(Ints.asList(data), comparator);

The comparator claims that an even number is smaller than an odd number and sort does all the work. It's even stable, i.e., it doesn't reorder what it doesn't have to and produces the output from your example. Guava's Ints adapts the array as a List<Integer>.


CSS Ticker Animation I have never use css animations before, so I just want to know, is this the 'best' way to animate a Ticker? I 'best' in this case encompasses efficient and semantic use of html and css.

It's worth noting that I intend to add extra functionality to this, such as manual scrolling back and forth. Is this set up ok for extending with javascript or is something here too temperamental?

Basic functionality is, Tweets scroll from right to left. When you put your mouse on one the scrolling stops and the tweet expands, and the time posted appears.

JSFiddle to see it in action

HTML:

        test

                @Something
                3m

content

CSS:

#controller {
    background-color: blue;
    display: inline-block;
    position: absolute;
    width: 100%;
}
#controller:hover #ticker {
    -webkit-animation-play-state: paused
}
#ticker {
    -webkit-animation: move_eye 6s linear 0s infinite normal;

}
@-webkit-keyframes move_eye {
    from {
        margin-left:110%;
    }
    to {
        margin-left:-50%;
    }
    90% {
    }
}
#text{
    display: inline-block;
}
#tweet {
    box-sizing: border-box;
    background-color: white;
    display: inline-block;
    position: relative;
    border-radius:5px;
    margin: 0px 1px;
    padding: 0px 3px;
}
#tweet:hover {
    font-size: 120%;
}
#tweet:hover #extra_info {
    display: inline-block;
}
#tweet:hover img {
        width: 40px;
        height: 40px;
}
#extra_info {
    display: none;
}

img{
    width: 30px;
    height: 30px;
}

If you are targetting styles for more than one element, then consider using class instead of id.
Block elements cannot be inside inline elements.
 can't be inside
 is a self-closing tag. No need for a  though you need to end it with />
Put some padding on your containers. They look awful when they all stick to the edge.
I have read somewhere that increasing the size on hover is awful CSS. Consider putting focus by changing the background color, or changing the font color or putting an outline in a subtle way.
Constrain the slider by putting overflow:hidden on the container. That way you don't have that horrible scrollbar at the bottom.
Use transform instead of margin. As far as I know, the new CSS3 properties are hardware accelerated (or were those just the 3D), so they're smoother.
Also, transform's values are relative to the element itself and not the parent. While 100% margin (top or bottom) is equal to the parent container, 100% transform is the height (for y) and width (for x) of the element. This is useful especially when the slider is longer than the container.

Too long to list down what I did so here's the code instead:

HTML:

                @Something
                3m

content

CSS:

#controller {
    box-sizing: border-box;
    background-color: #06C;
    position: absolute;
    width: 100%;
    overflow:hidden;
}
#controller:hover #ticker {
    -webkit-animation-play-state: paused
}
#ticker {
    white-space: nowrap;
    -webkit-animation: move_eye 6s linear 0s infinite normal;
}
@-webkit-keyframes move_eye {
    from {
        -webkit-transform: translate(100%,0)
    }
    to {
        -webkit-transform: translate(-100%,0)
    }
    90% {
    }
}
.text {
    display:inline-block;
}
.tweet {
    box-sizing: border-box;
    background-color: #EEE;
    display: inline-block;
    border-radius:5px;
    padding: 5px 10px;;
    vertical-align: top;
    margin: 5px 2px;
    height: 100%;
}
.tweet:hover .extra_info {
    display: inline;
}
.extra_info {
    display: none;
}
img {
    display:inline-block;
    height: 30px;
}


Python prime number checker function I was bored, and I was watching a sci-fi show where people were starting out during a long systems check asking each other whether numbers were prime or not, sort of passing the time as they do their inspections.

The show is irrelevant, but I wondered if I could build a prime number checker in Python 3 that would be very quick.  I know this has been done before, but I was curious whether I could do it on my own.

Now, this works fine for smaller numbers, and is pretty darn quick, a second or two.  But for large numbers, it takes a really long time.

I'm not a coder by profession, though, so I'm pretty sure this could be improved.  Any speed improvements are welcome.

Required imports, and the 'test' function which takes input of a number and spits out the results is below,

Currently, the results spit out a tuple, the first value either being Boolean or the number '0' (because the number '1' is a unique case that is not prime but you can't just say 'False' for), and a short string description.

from typing import AnyStr, Optional, Union

def is_prime(num: int) -> (Optional[Union[bool, int]], Optional[AnyStr]):
    if num <= 1:
        return 0, "Neither prime nor composite."

    if num <= 3:
        return True, "Prime"

    for i in range(2, num):
        if (num % i) == 0:
            return False, "Not Prime"

    return True, "Prime"

if __name__ == "__main__":
    n = int(input("Value: "))
    print(is_prime(n))

I'm curious how I can improve the performance of this code for larger numbers so that it's a fairly fast function, rather than taking a very long time to iterate over values.  I know that the iterations in the for loop are taking the hugest amount of time, but I'm not a mathematician so I'm not sure the 'best' approach for speeding up this.  Don't yell at me too harshly, this was more a quick 'mind test' to see if I could come up with something that worked.

Disclaimer: I can read Python, but I'm not a Python developer.

First of all, your code follows PEP8, which is a plus (although a second empty line before is_prime is necessary). And it's great that you try static type hints.

However, there are two things amiss here. First of all, Optional isn't honest at that point. We always return a value. And we always return a str, not AnyStr. We should therefore change the type hints accordingly:

from typing import Union

def is_prime(num: int) -> (Union[bool, int], str):

If there was a path where we could accidentally return None, None or None, b"12343", a static checker should warn us. So no Optional or AnyStr.

Since the types are now out of a way, let's have a look at documentation before we head to algorithms. A docstring is a nice addition:

def is_prime(num: int) -> (Union[bool, int], str):
    """Checks whether the given number is prime."""

    if num <= 1:
    ...

You can probably come up with a more detailed one so that the code itself contains the meaning of True, False and 0.

Now to the algorithm. Your algorithm has \$\mathcal O(n)\$ worst time complexity. However, for any pair \$x\$,\$y\$, such that \$x < y\$ and \$ x y = n \$ we must have \$x \le \sqrt{n}\$. Therefore, we can stop at math.ceil(sqrt(n)), not n. This yields \$\mathcal O(\sqrt{n})\$ worst time complexity.

For very large numbers this will still be too slow. There are other algorithms for that, which are out of scope for a small review.


Making complex array initializations more readable I have a function in the C++ program, which does simple computations and writes result to two arrays - result[53] and var1_new[53]. It contains two code blocks:

The first one -

double result[53];

result[0] = ((var1[13] * 1) + (var2[7] * 2)) / var3[0];
result[1] = ((var1[21] * 1) + (var2[7] * 2)) / var3[1];
result[2] = ((var1[12] * 2) + (var2[7] * 3)) / var3[2];
result[3] = ((var1[25] * 2) + (var2[7] * 3)) / var3[3];
//And so on, up to result[52]

And the second one -

double var1_new[53];

for (int x = 0; x < 53; ++x)
{
    var1_new[x] = var1[x];
}

var1_new[13] += result[0];
var1_new[21] += result[1];
var1_new[12] += result[2];
var1_new[25] += result[3];
//And so on, up to var1_new[52]

The blocks above use equal array indexes pattern - 13, 21, 12, 15, etc. The pattern is hardcoded.

This code runs pretty fast, thus I don't want to increase performance, but I'm thinking about readability - the function that contain code blocks is very complex. How can I make it more readable? Is it even possible?

First, magic constants are bad, you should name them

int const N = 53;
double result[N];

Second, magic patterns are even worse, you should also name them along with documentation comments

// explain where the pattern comes from
int const pattern[] = { 13, 21, 12, 25, /* 49 more constants */ };

Third, manually loop unrolling can sometimes help, but only after you benchmarked

for (int i = 0; i < N; ++i) {
    result[i] = ((var1[pattern[i]] * (i+1)) + (var2[7] * (i+2))) / var3[i];
}

For your second part, rinse and repeat:

double var1_new[N];

Fourth, use the Standard Library for standardized algorithms

#include <algorithm>
std::copy_n(var1, N, var1_new);

Again, rinse and repeat the loop unrolling

for (int i = 0; i < N; ++i) {
    var1_new[pattern[i]] += result[i];
}

Finally, you might want to give your variables meaningful names: var1, var2, var3 do not explain their intended use very well. Think about what they mean in your application (it was unclear from the code fragment alone).


Excel instances: release and kill Please check my code below. I don't have any problems but I'm not aware how far the code will work, release and kill excel instances..

try
{
    wBook = xCel.Workbooks.Open(excelfilepath);
    xCel.Visible = false;
    this.xCel.DisplayAlerts = false;
    wSheet = (Excel.Worksheet)wBook.Worksheets.get_Item(1);
    wSheet.Copy(Type.Missing, Type.Missing);
    wSheet = (Excel.Worksheet)wBook.Sheets[1];
    wSheet.SaveAs("1.xls");
}
catch
{
}
finally
{
    if (wBook != null)
    {
        try
        {
            wBook.Close();
        }
        catch
        {
        }
        Thread.Sleep(500);
    }

    if (excelprocid > 0)
    {
        Process xcelp = Process.GetProcessById(excelprocid);
        xcelp.Kill();
    }

    try
    {
        GC.Collect();
        GC.WaitForPendingFinalizers();
        GC.Collect();
        GC.WaitForPendingFinalizers();
    }
    catch{}

    Marshal.FinalReleaseComObject(wSheet); wSheet = null;
    Marshal.FinalReleaseComObject(wBook); wBook = null;
}

Since Excel runs via COM, it won't be released from memory until you remove all references to it.  Your example (above) does a pretty good job, but after you say wBook.Close(), you should say wBook = null.  Likewise Excel won't gracefully close-down while your xCel object refers to an instance of Excel.
This article on CodeProject shows the recommended/industry-standard way of closing-down excel. ffwd down to Sections 13 and 14.
http://www.codeproject.com/Articles/404688/Word-Excel-ActiveX-Controls-in-ASP-NET