Reader-writers problem using semaphores in Java I have written my own solution to the Reader-Writers problems using semaphores based on the psuedocode from Wikipedia.  I would like to gauge the correctness and quality of the code.

import java.util.concurrent.Semaphore;

class ReaderWritersProblem {

    static Semaphore readLock = new Semaphore(1);
    static Semaphore writeLock = new Semaphore(1);
    static int readCount = 0;

    static class Read implements Runnable {
        @Override
        public void run() {
            try {
                //Acquire Section
                readLock.acquire();
                readCount++;
                if (readCount == 1) {
                    writeLock.acquire();
                }
                readLock.release();

                //Reading section
                System.out.println("Thread "+Thread.currentThread().getName() + " is READING");
                Thread.sleep(1500);
                System.out.println("Thread "+Thread.currentThread().getName() + " has FINISHED READING");

                //Releasing section
                readLock.acquire();
                readCount--;
                if(readCount == 0) {
                    writeLock.release();
                }
                readLock.release();
            } catch (InterruptedException e) {
                System.out.println(e.getMessage());
            }
        }
    }

    static class Write implements Runnable {
        @Override
        public void run() {
            try {
                writeLock.acquire();
                System.out.println("Thread "+Thread.currentThread().getName() + " is WRITING");
                Thread.sleep(2500);
                System.out.println("Thread "+Thread.currentThread().getName() + " has finished WRITING");
                writeLock.release();
            } catch (InterruptedException e) {
                System.out.println(e.getMessage());
            }
        }
    }

    public static void main(String[] args) throws Exception {
        Read read = new Read();
        Write write = new Write();
        Thread t1 = new Thread(read);
        t1.setName("thread1");
        Thread t2 = new Thread(read);
        t2.setName("thread2");
        Thread t3 = new Thread(write);
        t3.setName("thread3");
        Thread t4 = new Thread(read);
        t4.setName("thread4");
        t1.start();
        t3.start();
        t2.start();
        t4.start();
    }
}

The logic looks good in this program, but I am observing that there is a problem with readCount. Suppose if two threads (say t1 and t2) reach readCount++ at the same time. Initially, both will read the readCount value to be 0, and after being incremented by both, the value will be corrupted i.e., it may be 2 and the same will happen with readCount--.

The problem is that, according to the Java memory model, if this read and write for readCount are not atomic, we can declare the readCount as volatile:

volatile static int readCount = 0;

Will volatile resolve the problem:

Again, by declaring the readCount as volatile, it now makes the read and write operations atomic, meaning that every thread will read the updated value of readCount as there will be no optimizations by the compiler (as volatile declared). There will be only one copy of the variable and that will be in main memory, so every thread will read/write the value in main memory only. Now there will no variable copies in thread caches.

But still, this is not the full-proof solution, because increment/decrement operations on any variable are not atomic operations. readCount++ (or readCount--) is not a single operation and consists of these three operations:

Read readCount
Add one to readCount, i.e. readCount + 1, (readCount - 1, in case of readCount--)
Write the value (from step 2) again in readCount

Note: There are lots of links on Google about volatile, such as this one.

In order to solve this problem, I can think of two approaches:

synchronized block

We can write both increment (readCount++) and decrement (readCount--) operations in a synchronized block, because readCount is static. Will use a ReaderWritersProblem.class object for the synchronized block. Now, any thread that wants to enter the block first has to acquire the lock on the ReaderWritersProblem.class object. Then only it can proceed, otherwise it will wait. This way, we can make the increment/decrement operations atomic (which is actually composed of three operations, explained as above):

Replace readCount++:

synchronized(ReaderWritersProblem.class) {
    readCount++;
}

Replace readCount--:

synchronized(ReaderWritersProblem.class) {
    readCount--;
}

AtomicInteger class

We can use Java in-built atomic concurrent APIs i.e. classes in package java.util.concurrent.atomic. As readCount is of type Integer, we can go for an AtomicInteger class. This class provides methods for increment and decrement operations and guarantees the atomicity of these operations.

We can modify the code as such:

Replace the readCount declaration:

static AtomicInteger readCount = new AtomicInteger(0);

Replace readCount++:

readCount.getAndIncrement();

Replace readCount--:

readCount.getAndDecrement();

You can check here for more APIs in the AtomicInteger class.


Bash function that allows running user aliases with sudo on Ubuntu I wrote this little Bash function to replace my sudo command.

It checks whether I have an alias for the given command and runs the aliased command instead of the literal one in that case.

As this is going to be used with sudo, I want to be sure there are no critical mistakes in it which generate security or safety issues. Please check whether everything is okay with it, like quoting issues or improvements to the sed pattern maybe. Thanks.

Here is my function:

sudo() { if alias "$1" &> /dev/null ; then $(type "$1" | sed -E 's/^.*`(.*).$/\1/') "${@:2}" ; else command sudo $@ ; fi }

Or nicely formatted:

sudo() {
    # check if the command passed as argument is an alias
    if alias "$1" &> /dev/null ; then
        # extract the aliased command from the output of 'type'
        # and run it with the remaining arguments
        $(type "$1" | sed -E 's/^.*`(.*).$/\1/') "${@:2}"
    else
        # run the original 'sudo' command with all arguments
        command sudo $@
    fi
}

First of all, you need to quote $@:

command sudo "$@"

If you don't your function will break if the command you pass to sudo contains a space in its path.

Then, this will break if your input is aliased to something which contains a backtick. For example:

$ alias foo='ls `which sudo`'
$ type foo
foo is aliased to `ls `which sudo`'

Now, the output of your sed command is not what you expect:

$ type foo | sed -E 's/^.*`(.*).$/\1/'
$

That will return nothing since the ^.*` means "match the longest possible string from the beginning of the string to a backtick" and, therefore, will match everything except the final '. A better approach would be to use a tool that has non-greedy matching like perl:

$ type foo | perl -pe 's/^.*?\`(.*).$/\1/'
ls `which sudo`

You will also need to eval it in order for it to run correctly:

$ "$(type foo | perl -pe 's/^.*?\`(.*).$/\1/')"
bash: ls `which sudo`: command not found
$ eval $(type foo | perl -pe 's/^.*?\`(.*).$/\1/')
/sbin/sudo

Putting all this together, gives:

sudo() {
    # check if the command passed as argument is an alias
    if alias "$1" &> /dev/null ; then
        # extract the aliased command from the output of 'type'
        # and run it with the remaining arguments
        eval $(type "$1" | perl -pe 's/^.*?\`(.*).$/\1/' "${@:2}")
    else
        # run the original 'sudo' command with all arguments
        command sudo "$@"
    fi
}

I can't guarantee that it will work for all cases though. All things considered, I wouldn't use this approach at all. I would instead use

alias sudo='sudo '

As explained here.


How to simplify the record syntax with pattern matching? I've just finished reading Learn You a Haskell and I've started to experiment on my own. I just wanted to create a very simple game system where player A can attack player B.

Now I have 2 questions:

My autoAttack function was very daunting to write. How could I improve it?
How would I format my autoAttack function correctly? I don't find it very pleasing to read.

main = print $ createActor `autoAttack` createActor
type Vector2= (Float,Float)
type Vector3 = (Float,Float,Float)
data StatsSystem = StatsSystem {physicalDamage :: Float,
                                spellPower     :: Float,
                                health         :: Float,
                                mana           :: Float,
                                attackRange    :: Float,
                                castRange      :: Float,
                                attackSpeed    :: Float
                               } deriving(Show)

data MovementSystem = MovementSystem {direction :: Vector2,
                                      speed     :: Float,
                                      position  :: Vector3
                                     } deriving(Show)
data Actor = Actor {stats :: StatsSystem,
                    ms    :: MovementSystem
                   } deriving(Show)
createActor :: Actor
createActor = Actor {stats = StatsSystem {physicalDamage = 1.0,
                                          spellPower = 1.0,
                                          health = 10.0,
                                          mana = 1.0,
                                          attackRange = 1.0,
                                          castRange = 1.0,
                                          attackSpeed = 1.0
                                         },
                     ms = MovementSystem{direction = (1.0,1.0),
                                         speed = 50.0,
                                         position = (0,0,0)
                                        }
                    }
autoAttack :: Actor -> Actor -> Actor
autoAttack Actor {stats = StatsSystem {physicalDamage = p,
                                       spellPower = _,
                                       health = _,
                                       mana = _,
                                       attackRange = _,
                                       castRange = _,
                                       attackSpeed = _
                                      },
                   ms = m
                 }
           Actor {stats = s2,
                   ms = m2
                 }
           = Actor {stats = StatsSystem {physicalDamage = physicalDamage s2,
                                         spellPower = spellPower s2,
                                         health = health s2 - p,
                                         mana = mana s2,
                                         attackRange = attackRange s2,
                                         castRange = castRange s2,
                                         attackSpeed = attackSpeed s2
                                        },
                    ms = m2
                   }

First, when you are not using fields in a record, you don't have to use _ to mark that they should be ignored;  you only need to include the fields in the pattern that you are actually using.

Second, it is possible to update a couple of fields in a record without respecifying the whole thing from scratch.  If x is the name of the old record and you want to set the field a to 42 and b to 24, then the expression x { a = 42, b = 24 } is equal to the record x with a set to 42 and b set to 24.

Putting these ideas together, autoAttack could be rewritten like so:

autoAttack :: Actor -> Actor -> Actor
autoAttack actor@(Actor {stats = stats@(StatsSystem {physicalDamage = p, health = h}})) =
    actor {stats = stats { health = h - p } }

Where we have used the syntax actor@(Actor ..) to bind the Actor to the name actor, which is a useful pattern for when you want to both pattern match on an argument and also have access to its value.

Just to be clear, it is worth mentioning that, because data is immutable, changing a couple of fields involves making a full copy of the data structure.  (Thanks to Daniel Wagner for bringing this up).


Is this Recursion + Linq example inefficient? I have a hierarchy of of groups, and I want to get a collection of all the lowest level groups (or leaves if we'll look at this as a tree).

I wrote the following code. Is it inefficient?

public static class FeatureWeightGroupExtensions
{
    public static IEnumerable<IFeatureWeightGroup> GetLeafGroups(this IFeatureWeightGroup featureWeightGroup)
    {
        return GetLeafGroupsRecursive(featureWeightGroup).ToList();
    }

    private static IEnumerable<IFeatureWeightGroup> GetLeafGroupsRecursive(IFeatureWeightGroup featureWeightGroup)
    {
        if (!featureWeightGroup.ChildGroups.Any())
            return Enumerable.Repeat(featureWeightGroup, 1);

        return featureWeightGroup.ChildGroups.Aggregate(Enumerable.Empty<IFeatureWeightGroup>(),
                                                        (allGroups, group) =>
                                                        allGroups.Concat(GetLeafGroupsRecursive(group)));
    }
}

return featureWeightGroup.ChildGroups.Aggregate(Enumerable.Empty<IFeatureWeightGroup>(),
                                                    (allGroups, group) =>
                                                    allGroups.Concat(GetLeafGroupsRecursive(group)));

It looks like it can be replaced with:

return featureWeightGroup.ChildGroups
           .SelectMany(g => GetLeafGroupsRecursive(group));

Which probably will not be much better from performance point of view, but looks cleaner.


Create a dictionary which saves its data to file Task:

The task requires creating a class (say, ConfigDict) which inherits from the dict class.

It is supposed to save its keys and values in a file.
If a file does not exist, the instance of ConfigDict should be able to create it.

Examples:

For instance, the following code:

fh=open("config.txt","w")
fh.write("key1 = 5\n")
fh.write("key2 = 3\n")
fh.close()
cd=ConfigDict("config.txt")
cd.items()

would generate an output dict_items([('key1', '5'), ('key2', '3')]).

If a file does not exists, it should be possible for the instance to create it and save its data inside it.

dd=ConfigDict("test.txt")
dd["keyA"] = 'testA'
dd["keyB"] = 'testB'
dd.items()

The above lines would produce dict_items([('keyA', 'testA'), ('keyB', 'testB')]) (and a print message).

My attempt:

I will greatly appreciate any comments.

class ConfigDict(dict):
    def __init__(self,file):
        self.file = file

        try: #check if file exists
            with open(self.file,"r+") as fh:
                for line in fh:
                    key, item = line.replace(" ", "").rstrip().split("=")
                    dict.__setitem__(self,key,item)

        except:
            print("creating "+file)
            fh=open(self.file,"w")
            fh.close()

    def __setitem__(self,key,item):
        dict.__setitem__(self,key,item) #to avoid infinite loop
        lines = open(self.file).read().splitlines()

        for index, line in enumerate(lines):
            if line.replace(" ", "").rstrip().split("=")[0] == key:
               lines[index] = str(key)+" = "+str(item)
            else:
                lines.append(str(key)+" = "+str(item))

        open(self.file,'w').write('\n'.join(lines))

Corrected version

import os

class ConfigDict(dict):
    def __init__(self,file):
        self.file = file
        if os.path.isfile(self.file):
            with open(self.file,"r") as fh:
                for line in fh:
                    key, item = line.replace(" ", "").rstrip().split("=")
                    dict.__setitem__(self,key,item)

    def __setitem__(self,key,item):
        dict.__setitem__(self,key,item)
        with open(self.file, 'w') as save_file:
            for key, value in self.items():
                save_file.write("{} = {}\n".format(key, value))

bugs:

Your implementation of __setitem__() does not to add a line
if the key does not exist in the file yet.

other issues:

There is no need to create the file if it does not exist in
__init__(), as open(..., 'w') in __setattr__() will create
it for you.
It is gennerally not a good idea to print from a data class
(i.e. print("creating "+file)) as this makes the class unuseable for
programs that use stdout.
Iterating over a file in python automatically splits on lines so

lines = open(self.file).read().splitlines()
for index, line in enumerate(lines):

could be written as

for index, line in enumerate(open(self.file)):

(like you did in __init__())
If you are going to completly rewrite the file on every
__setitem__(), there is no point in reading the file first, as this
takes more time and is harder to read. here is an example
__setitem__():

def __setitem__(self,key,item):
    dict.__setitem__(self,key,item) #to avoid infinite loop

    with open(self.file, 'w') as save_file:
        for key, value in self.items():
            save_file.write("{} = {}\n".format(key, value))


Return the top 5 of each kind of record I have this currently working code, but it's extremely slow. I'm talking almost an hour to execute, if it executes at all (our server isn't all that great to start with). Is there a better way to write this?

Basically, I am wanting to return the Top 5 parts, that match a distinct pattern code within the same table. So, if there were 100 distinct pattern codes in my [PartNumber] table, I would end up with 500 records.

ORIGINAL at time of question posting (Executes in 32 minutes):

DECLARE @PartInfo TABLE(
    [PartNumber] [varchar](64) NOT NULL,
    [PatternCode] [varchar](64) NOT NULL,
    [LanguageCode] [varchar](24) NOT NULL);

DECLARE @PatternCode nvarchar(64);
DECLARE pattern_Cursor CURSOR LOCAL SCROLL STATIC
    FOR
    SELECT TOP 1000 [PartNumber].[PatternCode]
    FROM [Web_Service].[dbo].[PartNumber] [PartNumber]
    WHERE PatternCode NOT LIKE 'NOT FOUND'
    GROUP BY [PartNumber].[PatternCode]

    OPEN pattern_Cursor;

    FETCH NEXT FROM pattern_Cursor
    INTO @PatternCode;

    WHILE @@FETCH_STATUS = 0
        BEGIN
            INSERT INTO @PartInfo
            SELECT TOP 5 [PartNumberInfo].*
            FROM [Web_Service].[dbo].[PartNumber] [PartNumber]
            WHERE [PartNumber].[PatternCode] = @PatternCode;

            FETCH NEXT FROM pattern_Cursor
            INTO @PatternCode;
        END
CLOSE pattern_Cursor;
DEALLOCATE pattern_Cursor;

SELECT *
FROM @PartInfo
ORDER BY [@PartInfo].[PatternCode]

First revision (Executes in 24 minutes):

CREATE TABLE #PartInfo ([PartNumber] [varchar](64) NOT NULL,
                        [PatternCode] [varchar](64) NOT NULL,
                        [LanguageCode] [varchar](24));

DECLARE @PatternCode nvarchar(64);

DECLARE pattern_Cursor CURSOR FAST_FORWARD
    FOR
    SELECT [PartNumber].[PatternCode]
    FROM [Web_Service].[dbo].[PartNumber] [PartNumber]
    WHERE PatternCode <> 'NOT FOUND'
    GROUP BY [PartNumber].[PatternCode]

    OPEN pattern_Cursor;

    FETCH NEXT FROM pattern_Cursor
    INTO @PatternCode;

    WHILE @@FETCH_STATUS = 0
        BEGIN
            INSERT INTO #PartInfo
            SELECT TOP 5 [PartNumber].*
            FROM [Web_Service].[dbo].[PartNumber] [PartNumber]
            WHERE [PartNumber].[PatternCode] = @PatternCode
            ORDER BY [PartNumber].[DateTimeValidated] DESC;

            FETCH NEXT FROM pattern_Cursor
            INTO @PatternCode;
        END

CLOSE pattern_Cursor;

DEALLOCATE pattern_Cursor;

SELECT *
FROM #PartInfo
ORDER BY [#PartInfo].[PatternCode]

FINAL COMPLETED AND WORKING CODE (executes in 15 seconds):

SET NOCOUNT ON
GO
SET ARITHABORT ON
GO
SET ANSI_PADDING ON
GO

DECLARE @TopCount int = 5;

SELECT [DistinctTop].[PartNumber]
      ,[DistinctTop].[PatternCode]
      ,[DistinctTop].[LanguageCode]

FROM (SELECT [PartNumber]
            ,[PatternCode]
            ,[LanguageCode]
            ,RANK() OVER
                (PARTITION BY [PatternCode] ORDER BY [PartNumber]) AS RowRank
     FROM PartNumberInfo) AS [DistinctTop]

WHERE [DistinctTop].[RowRank] <= @TopCount

ORDER BY [DistinctTop].[PatternCode];

You have three problems that I can see. Two performance, and one logic. In least-to-most significant order:

Performance: You should no use the NOT LIKE 'NOT FOUND' statement, it should be  <> 'NOT FOUND'
Potential logic problem: You should have an order-by clause on the inner select for the TOP 5 ... otherwise, what TOP 5 are you getting?
Performance: Using declare @table... syntax has different performacne to create table #table ... For example, this blog entry shows some real differences. I recommend trying the same query with a #temp table instead.

Also, read up on this SO Answer: SQL Server tables: what is the difference between @, # and ##?
Edit:
It was almost too obvious to ask... but, you have done the two most basic items... right?

an index on the [PartNumber].[PatternCode]
updated all table statistics

... right?


C++ parallel merge sort I'm a first year computer engineering student who's learning about algorithms and data structures. I've implemented a parallel merge sort algorithm in C++ and would like constructive criticism. This was done in Visual Studio on Windows.

Some thoughts:

portability isn't a concern I have atm.
merge_sort isn't using tail recursion, which I would like it to do

includes:
#include <thread>

function merge:

template<typename T>
void merge(T sequence[], int size)
{
    T* sorted = new T[size];
    int middle = size / 2;

    int index_left = 0;
    int index_right = middle;
    int index_sequence = 0;

    while (index_left < middle && index_right < size)
    {
        if (sequence[index_left] < sequence[index_right])
            sorted[index_sequence++] = sequence[index_left++];
        else
            sorted[index_sequence++] = sequence[index_right++];
    }

    while (index_left < middle)
        sorted[index_sequence++] = sequence[index_left++];

    while (index_right < size)
        sorted[index_sequence++] = sequence[index_right++];

    for (int i = 0; i < size; i++)
        sequence[i] = sorted[i];

    delete[] sorted;
}

function merge_sort:

template<typename T, int min_size_to_thread = 10000>
void merge_sort(T sequence[], int size)
{
    if (size > 1)
    {
        int middle = size / 2;
        if (size > min_size_to_thread)
        {
            std::thread left(merge_sort<T, min_size_to_thread>, &sequence[0], middle);
            std::thread right(merge_sort<T, min_size_to_thread>, &sequence[middle], size - middle);
            left.join();
            right.join();
        }
        else
        {
            merge_sort<T, min_size_to_thread>(&sequence[0], middle);
            merge_sort<T, min_size_to_thread>(&sequence[middle], size - middle);
        }
        merge<T>(sequence, size);
    }
}

For those who are interested: I Did some performance testing on a i5-2530M 2.50Ghz (2 cores).
The sequence to merge sort is int[100000]

Edit:

I made merge_sort and merge take a T tmp[]. And I made a "wrapper" function around merge_sort like this:

template<typename T>
void merge_sort(T* sequence, int size)
{
    T* tmp = new T[size];
    _merge_sort(sequence, tmp, size);
    delete[] tmp;
}

Now new is only called once. The "old" merge_sort is renamed to _merge_sort.

You would hava a) thread-pools so that the code won't spawn a new thread on each sub-sort, b) a limit on the number of sorts split into threads so that there isn't a new thread spawned for a low number of elements and c) a single array where all the sorting takes place and which is paritioned like a stack among the sort-threads (afaik the whole length should be two times the size of the array).

That would be a much of work to gain an efficient implementation.


Permutations with replacement in Python Many a times, I've had the need to use a permutations with replacement function.

So, I've written a function to do just that:

from sys import setrecursionlimit
setrecursionlimit(10 ** 9)

def permutations_with_replacement(n: int, m: int, cur=None):
    if cur is None:
        cur = []

    if n == 0:
        yield cur
        return

    for i in range(1, m + 1):
        yield from permutations_with_replacement(n - 1, m, cur + [i])

if __name__ == '__main__':
    n = int(input("Please enter 'N': "))
    m = int(input("Please enter 'M': "))

    for i in permutations_with_replacement(n, m):
        print(*i)

There's a better way to do this if we used itertools.product, but there's no fun in that!

from itertools import product

def permutations_with_replacement(n, m):
    for i in product(list(range(1, m + 1)), repeat=n):
        yield i

if __name__ == '__main__':
    n = int(input("Please enter 'N': "))
    m = int(input("Please enter 'M': "))

    for i in permutations_with_replacement(n, m):
        print(*i)

I don't like the way I've implemented cur in my code. Is there any better way to do that?

First of all, this is a very nicely written code.

Before writing about the cur implementation, a few notes:

I do not think n and m should be in main. It creates shadowing in the function.
I added type annotation to the return value of the function. Since it's a generator it should be: Iterator[List[int]].

So you said you do not like the way cur is implemented in your code, I thought of a way to take it out of the function declaration.
I created a sub function that is recursive taken out the cur variable. This is how it turned out:

def permutations_with_replacement(n: int, m: int) -> Iterator[List[int]]:
    cur = []

    def permutations_with_replacement_rec(n_rec: int, m_rec: int) -> Iterator[List[int]]:
        nonlocal cur

        if n_rec == 0:
            yield cur
            return

        for i in range(1, m_rec + 1):
            cur = cur + [i]
            yield from permutations_with_replacement_rec(n_rec - 1, m_rec)
            cur.pop()

    yield from permutations_with_replacement_rec(n, m)

Now notice that I needed to add a pop to the variable since now it keeps the elements after the call to the function.
Also I needed to use the nonlocal term so the function will know the cur variable.

The full code (with tests):

from itertools import product
from typing import Iterator, List

from sys import setrecursionlimit
setrecursionlimit(10 ** 9)

def permutations_with_replacement(n: int, m: int) -> Iterator[List[int]]:
    cur = []

    def permutations_with_replacement_rec(n_rec: int, m_rec: int) -> Iterator[List[int]]:
        nonlocal cur

        if n_rec == 0:
            yield cur
            return

        for i in range(1, m_rec + 1):
            cur = cur + [i]
            yield from permutations_with_replacement_rec(n_rec - 1, m_rec)
            cur.pop()

    yield from permutations_with_replacement_rec(n, m)

def test_permutations_with_replacement():
    n = 3
    m = 4
    assert set(product(list(range(1, m + 1)), repeat=n)) == set(tuple(i) for i in permutations_with_replacement(n, m))

def main():
    n = int(input("Please enter 'N': "))
    m = int(input("Please enter 'M': "))

    for i in permutations_with_replacement(n, m):
        print(*i)

if __name__ == '__main__':
    main()


Number of files with specific file size ranges I am trying to write a script that will traverse through my directory and sub directory and list the number of files in a specific size.

For example, 0kb-1kb: 3, 1kb-4kb:4, 4-16KB: 4, 16kb-64-kb:11. This goes on in multiples of 4.

I am able to get the list of file numbers, the size in human readable format and find the number of files in a size group. But I feel my code is very messy and not anywhere near the standard.

import os
suffixes = ['B', 'KB', 'MB', 'GB', 'TB', 'PB']
route = raw_input('Enter a location')
def human_Readable(nbytes):
    if nbytes == 0: return '0 B'
    i = 0
    while nbytes >= 1024 and i < len(suffixes)-1:
        nbytes /= 1024.
        i += 1
    f = ('%.2f' % nbytes).rstrip('0').rstrip('.')
    return '%s %s' % (f, suffixes[i])
def file_Dist(path, start,end):
    counter = 0
    for path, subdir, files in os.walk(path):
        for r in files:
            if os.path.getsize(os.path.join(path,r)) > start and os.path.getsize(os.path.join(path,r)) < end:
                counter += 1
        print "Number of files greater than %s less than %s:" %(human_Readable(start), human_Readable(end)),  counter
file_Dist(route, 0, 1024)
file_Dist(route,1024,4095)
file_Dist(route, 4096, 16383)
file_Dist(route, 16384, 65535)
file_Dist(route, 65536, 262143)
file_Dist(route, 262144, 1048576)
file_Dist(route, 1048577, 4194304)
file_Dist(route, 4194305, 16777216)

suffixes = ['B', 'KB', 'MB', 'GB', 'TB', 'PB'] you never change the value of suffixes, so it should be a constant, the standard naming of constant in Python is  ALL_UPPERCASE, rename it to SUFFIXES
You should name variables with snake_case.
You wrote for r in files: the details count: the standard one-letter identifier for file is f, use that.
Look at f = ('%.2f' % nbytes).rstrip('0').rstrip('.'), f is a meaningless one-letter name. You should use a longer and more meaningful name
You are checking and i < len(suffixes)-1 in the loop: it is not possible that the user has a file bigger than one Petabyte (10**15 bytes) so you can safely remove that check. and it is very good that you that, code that crashes when you find an unexpected condition should be avoided with error checking like this one. (Thanks to @Veedrac for correcting me)

f = ('%.2f' % nbytes).rstrip('0').rstrip('.')
return '%s %s' % (f, suffixes[i])

You are a C programmer, aren't you? :) Using "%s" string formatting in Python is considered obsolete and should be avoided, instead use string.format()

print "Number of files greater than %s less than %s:" %(human_Readable(start), human_Readable(end)),  counter

You are printing from inside a function this is not nice, you should return values, not print them.

It is common to take the parametres from the command line so:

route = raw_input('Enter a location')

should become:

import sys
route = sys.argv[1]

this also makes the code compatible with Python 3 if you add brackets around the print

You are repeating yourself so much:

file_Dist(route, 0, 1024)
file_Dist(route,1024,4095)
file_Dist(route, 4096, 16383)
file_Dist(route, 16384, 65535)
file_Dist(route, 65536, 262143)
file_Dist(route, 262144, 1048576)
file_Dist(route, 1048577, 4194304)
file_Dist(route, 4194305, 16777216)

It would be better to use a loop:

for start in [1024,4096,16384,262144,1048577,4194305]:
    end = start * 4
    file_Dist(route,start,end)

You may want to import this script, using if __name__ will allow you to run the tests only if this script is the main file

def test():
    for start in [1024,4096,16384,262144,1048577,4194305]:
        end = start * 4
        file_Dist(route,start,end)

if __name__ == "__main__":
    test()


Timer class for games I've tried to write a simple timer class that could later be used in games, such as for updating the screen every second. Problem is, I read gettimeofday() was UNIX-specific and unsafe to use as well (that last one I'm not sure about). I know C has a clock() function, but it only has second precision.

Is there a way to get time in a cross-platform way that has millisecond precision at the very least? I know Boost and other libraries exist, but I prefer not to use them for various reasons.

class Timer
{
private:
   bool m_isRunning;
   int m_startTime;
   int m_delayAmount;
   double m_initialDelay;

public:
   Timer(int delayAmount, double initialDelay = 0.0)
   {
      m_delayAmount = delayAmount * 1000;
      m_initialDelay = initialDelay/1000;
   }

   void start()
   {
      m_startTime = getMilliseconds();
      clock_t endwait = clock() + m_initialDelay * CLOCKS_PER_SEC;
      while (clock() < endwait);
      m_isRunning = true;
   }

   void stop()
   {
      m_isRunning = false;
   }

   bool isRunning()
   {
      return m_isRunning;
   }

   bool hasTicked()
   {
      if (getMilliseconds() - m_startTime > m_delayAmount)
      {
         m_startTime = getMilliseconds();
         return true;
      }
      else
      {
         return false;
      }

   }

   int getMilliseconds() const
   {
      timeval t;
      gettimeofday(&t, NULL);
      return (t.tv_sec * 1000000) + t.tv_usec;
   }
};

This is kinda how you would use it. The program just prints a message every second after an initial delay of 3 seconds.

int main (int argc, char * const argv[])
{
   Timer timer(1000, 3000);
   timer.start();
   int i = 1;
   while (timer.isRunning())
   {
      if (timer.hasTicked())
      {
         cout << "tick " << i++ << endl;
      }
   }
}

I think you should take a look at std::chrono and std::thread.

If you want to replicate, say, a VB timer or whatever (something that calls a function every second), you can do this:

template<typename Func>
void timer(Func func, unsigned interval)
{
    while(true)
    {
        std::this_thread

        std::this_thread::sleep_for(;
            std::chrono::milliseconds(interval));
        func();
    }
}

Of course, you can change that into a functor and give it a bool for start and stop.

if you just want to measure the elapsed time then just use chrono

std::chrono::system_clock::time_point start=std::chrono::system_clock::now();
//start timer
//do stuff
std::chrono::duration<double> time_taken=std::chrono::system_clock::now() - start;

Don't reinvent the wheel.  If you don't have access to C++11, then all of this functionality can be used from Boost.


Simple parsing for string formatting (version 2) This is my second attempt (version 1) at writing a string formatting utility which uses parsing. This time I more explicitly designed it as a finite state machine. I have very little experience with writing FSMs. As before, I'm not 100% sure that this code does not contain any critical flaws. In addition to pointing out any such oversights, I would appreciate advice on how this could be modified to make it either faster or more readable.

#include <boost/lexical_cast.hpp>
#include <iostream>
#include <string>
#include <stdexcept>
#include <utility>
#include <cctype>

using std::to_string;

template <class T>
std::string to_string(T&& item)
{
    return boost::lexical_cast<std::string>(std::forward<T>(item));
}

template <class... Args>
std::string format(const std::string& fmt, Args&&... args)
{
    std::string arg_strs[] = { to_string(std::forward<Args>(args))... };

    std::string output, index;
    output.reserve(fmt.length() * 2);

    enum { COPYING, OPEN_BRACE, READ_INDEX, CLOSE_BRACE } state = COPYING;

    for (const char c : fmt)
    {
        switch (state)
        {
            case COPYING:
                if (c == '{') state = OPEN_BRACE;
                else if (c == '}') state = CLOSE_BRACE;
                else output += c;
                break;

            case OPEN_BRACE:
                if (isdigit(c)) {
                    index += c;
                    state = READ_INDEX;
                }
                else if (c == '{') {
                    output += '{';
                    state = COPYING;
                }
                else goto fail;
                break;

            case READ_INDEX:
                if (isdigit(c)) index += c;
                else if (c == '}') {
                    size_t i = std::stoi(index);
                    if (i >= sizeof...(args)) {
                        throw std::out_of_range(
                            "argument index is out of range in format");
                    }
                    output += arg_strs[i];
                    index.clear();
                    state = COPYING;
                }
                else goto fail;
                break;

            case CLOSE_BRACE:
                if (c == '}') {
                    output += '}';
                    state = COPYING;
                }
                else goto fail;
                break;
        }
    }

    if (state != COPYING) goto fail;
    return output;

fail:
    throw std::invalid_argument("invalid format string");
}

template <class... Args>
void print(const std::string& fmt, Args&&... args)
{
    std::cout << format(fmt, std::forward<Args>(args)...);
}

int main()
{
    print("Hello, {0}! The answer is {1}.", "World", 42);
}

Here is a diagram of the FSM:

Now just for fun, I converted this to JavaScript with regex...

function format(fmt, ...args) {
    if (!fmt.match(/^(?:(?:(?:[^{}]|(?:\{\{)|(?:\}\}))+)|(?:\{[0-9]+\}))+$/)) {
        throw new Error('invalid format string.');
    }
    return fmt.replace(/((?:[^{}]|(?:\{\{)|(?:\}\}))+)|(?:\{([0-9]+)\})/g, (m, str, index) => {
        if (str) {
            return str.replace(/(?:{{)|(?:}})/g, m => m[0]);
        } else {
            if (index >= args.length) {
                throw new Error('argument index is out of range in format');
            }
            return args[index];
        }
    });
}

function print(fmt, ...args) {
    console.log(format(fmt, ...args));
}

print("Hello, {0}! The answer is {1}.", "World", 42);

I think CS subjects like automata are too much for this problem, thats more something you have to deal with when implementing a regular expressions engine. Here is some code in C# that I've written to replace tags in a string, its absolutely not optimal because the state is checked every iteration and each character is copied individually, but you'll get the my idea.

private string ReplaceTags(string text, Dictionary<string, string> tags)
{
    StringBuilder result = new StringBuilder();
    StringBuilder tag = new StringBuilder();

    int mode = 0;

    int len = text.Length - 1;

    for (int i = 0; i <= len; i++)
    {
        char c = text[i];

        if (mode == 0)
        {
            if (text[i] == '\\')
            {
                i++;

                if (i <= len)
                {
                    result.Append(text[i]);
                }
            }
            else if (text[i] == '[')
            {
                mode = 1;
            }
            else
            {
                result.Append(text[i]);
            }
        }
        else
        {
            if (text[i] == ']')
            {
                string _tag = tag.ToString().ToLowerInvariant();

                if (tags.ContainsKey(_tag))
                {
                    result.Append(tags[_tag]);
                }

                tag.Length = 0;
                mode = 0;
            }
            else
            {
                tag.Append(text[i]);
            }
        }
    }

    return result.ToString();
}


How can I ascertain and test SQL injection vunerabilities on this login code? I know about the "little bobby tables" scenario and was just wondering if this code is vulnerable to such SQL injections.

I am fairly new to PHP and am curious about this.

My code for a login script:

<?php
  # session
  session_start();

  # get the variables from login.php
  $userid = $_POST["userid"];
  $pword = $_POST["pword"];

  mysql_real_escape_string($userid);
  mysql_real_escape_string($pword);

  # query the DB
  $query = mysql_query ("
    SELECT username
    FROM login
    WHERE username = '$userid'
    AND pword = '$pword';
  ");

  if ($query === FALSE) {
    die('There has been an error.

 Please re-enter your Login Details on the Login Page.

');
  }

  $result = mysql_fetch_assoc($query);
  $record = $result['username'] ;

  # valid login
  # check if session is operational, if so redirect the user
  # to the correct page
  if ($record != null) {
    $_SESSION['login'] = true;
    header( 'Location: index.php' ) ;
  }

  # invalid login
  else if ($record == null) {
    echo "
    We cant find you on the system.

    Please return to the Login page and ensure that

    you have entered your details correctly.

    Warning: You willl be redirected  back to the Login Page

 in  10 Second(s)";
  }
?>

The main reason for asking this is that when I login inputting:

'user'); DROP TABLE Login;--'

it doesn’t drop the table.

My question is: Am I typing in the right SQL injection?

This is vulnerable:

mysql_real_escape_string($userid);
mysql_real_escape_string($pword);

does not return the variable. Use:

$userid = mysql_real_escape_string($userid);
$pword = mysql_real_escape_string($pword);

If you really want to take care of all vulnerable SQL injections try to move to PDO. Here is a nice tutorial


Select and delete objects from S3 container GitLab saves files with the pattern 1530410429_2018_07_01_11.0.1_gitlab_backup.tar (epoch, date, version, fixed string) to an S3 container.

I wanted to create a Python module to manage those backups, particularly to delete old backups, but preserving a specified number of files for each version).

To this end I:

read S3 bucket contents and populate a list of dictionaries containing file name and an extracted version
extract a set of versions from the above list
iterate over each version and create a list of files to delete
iterate over the above result and delete the files from the bucket

It works, but I feel the data structures I used are not appropriate and not pythonic.

How could I simplify the data structures and possibly minimize the number of function calls.

import boto3

def get_set_of_versions(table_of_backups):
    set_of_versions = set()
    for backup in table_of_backups:
        set_of_versions.add(backup['gitlab_version'])
    return set_of_versions

def create_table_of_backups(list_of_backup_files):
    table_of_backups = [
        {
            "s3_object_name": filename,
            "gitlab_version": filename.split('_')[4]
        } for filename in list_of_backup_files
    ]
    return table_of_backups

def extract_list_of_backups(list_of_objects):
    list_of_backups = [s3_object.key for s3_object in list_of_objects if 'gitlab_backup' in s3_object.key]
    return list_of_backups

class GitlabBackups:
    def __init__(self, bucket_name):
        self.bucket_name = bucket_name

        self.s3 = boto3.resource('s3')
        self.bucket = self.s3.Bucket(bucket_name)
        self.list_of_backup_files = extract_list_of_backups(self.bucket.objects.all())
        self.table_of_backups = create_table_of_backups(self.list_of_backup_files)
        self.set_of_versions = get_set_of_versions(self.table_of_backups)

    def return_candidates_for_deletion_within_version(self, version, generations_to_keep=1):
        if generations_to_keep == 0:
            raise ValueError
        list_of_versioned_backups = [
            backup for backup in self.table_of_backups if backup['gitlab_version'] == version
        ]
        return [backup['s3_object_name'] for backup in list_of_versioned_backups[:-generations_to_keep]]

    def return_candidates_for_deletion(self, generations_to_keep=2):
        resulting_list = list()
        for version in self.set_of_versions:
            resulting_list.extend(self.return_candidates_for_deletion_within_version(version, generations_to_keep))
        return resulting_list

    def delete_old_backups(self, generations_to_keep=2):
        list_to_delete = self.return_candidates_for_deletion(generations_to_keep)
        for key in list_to_delete:
            self.s3.Object(self.bucket_name, key).delete()

I think your function names could use some work. There is no need to specify that return_candidates_for_deletion actually returns those candidates (it is obvious). It is also a bit superfluous to know that extract_list_of_backups returns a list. It is enough to know that it returns a collection that is iterable (whether or not that is a tuple, list, set or even just a generator does not really matter for the rest of the code).

In addition, you can save a lot of code by changing your internal data structure a bit. The main thing your code does is sort the backups by the gitlab version and then keep only some of them. So you should store the files in a dictionary with the gitlab version as the key:

import boto3
from collections import defaultdict

class GitlabBackups:
    def __init__(self, bucket_name):
        self.bucket_name = bucket_name

        self.s3 = boto3.resource('s3')
        self.bucket = self.s3.Bucket(bucket_name)

    def backups(self):
        return (obj.key
                for obj in self.bucket.objects.all()
                if 'gitlab_backup' in obj.key)

    def group_by_version(self, files):
        backups = defaultdict(list)
        for file_name in files:
            gitlab_version = file_name.split('_')[4]
            backups[gitlab_version].append(file_name)
        return backups

    def candidates_for_deletion(self, generations_to_keep=2):
        for files in self.group_by_version(self.backups()).values():
            # files.sort()  # is this not needed?
            yield from files[:-generations_to_keep]

    def delete_old_backups(self, generations_to_keep=2):
        for key in self.candidates_for_deletion(generations_to_keep):
            self.s3.Object(self.bucket_name, key).delete()

Here I put all the related standalone functions into the class. It has now one generator that yields all backup files, one that creates the dictionary with the versions as keys, one generator that yields all files except for the generation to keep for each version and then the actual deleting method.

Note that with this, as with your code, it is not guaranteed that the latest generation is actually at the end of the list of files. It depends on the order of self.bucket.objects.all(), so on how S3 returns it (which is apparently alphabetical). But you might want to make sure from your side as well and sort each files list in candidates_for_deletion.

In order to make this a bit more reusable, you could then pull out a groupby method that is similar to itertools.groupby but different because it consumes the whole input and therefore does not require the input to be sorted:

def groupby(objects, key=lambda x: x):
    out = defaultdict(list)
    for obj in objects:
        out[key(obj)].append(obj)
    return out

class GitlabBackups:
    ...
    @staticmethod
    def gitlab_version(file_name):
        return file_name.split('_')[4]

    def candidates_for_deletion(self, generations_to_keep=2):
        for files in groupby(self.backups(), key=self.gitlab_version).values():
            yield from files[:-generations_to_keep]


Graph: Depth First Search (N citizens with pairs of friends) I have solved problem 10608 on UVA Online Judge using Python 3.5.1. My solution works, but it takes too long to run when the online judge evaluates it.
Problem
There is a town with N citizens. It is known that some pairs of people are friends. According to the famous saying that “The friends of my friends are my friends, too” it follows that if A and B are friends
and B and C are friends then A and C are friends, too.
Your task is to count how many people there are in the largest group of friends.
Input
Input consists of several datasets. The first line of the input consists of a line with the number of test
cases to follow.
The first line of each dataset contains tho numbers N and M, where N is the number of town’s
citizens (1 ≤ N ≤ 30000) and M is the number of pairs of people (0 ≤ M ≤ 500000), which are known
to be friends. Each of the following M lines consists of two integers A and B (1 ≤ A ≤ N, 1 ≤ B ≤ N,
A ̸= B) which describe that A and B are friends. There could be repetitions among the given pairs.
Output
The output for each test case should contain (on a line by itself) one number denoting how many people
there are in the largest group of friends on a line by itself.
Sample Input
2
3 2
1 2
2 3
10 12
1 2
3 1
3 4
5 4
3 5
4 6
5 2
2 1
7 1
1 2
9 10
8 9

Sample Output
3
7

testCases = int(input())

for x in range(testCases):
    temp = input().split()
    N = int(temp[0])
    M = int(temp[1])

    nodes = []
    edges = []
    for _ in range(M):
        temp = input().split()
        A = int(temp[0])
        B = int(temp[1])
        edges.append([A, B])

    counter = 0
    for y in range(N):
        counter += 1
        nodes.append(counter)

    hashmap = {}
    for h in range(len(nodes)):
        neighbours = []
        for j in range(len(edges)):
            if edges[j].__contains__(nodes[h]):
                index_of_node = edges[j].index(nodes[h])
                if index_of_node == 0:
                    neighbours.append(edges[j][1])
                    hashmap[h + 1] = neighbours
                else:
                    neighbours.append(edges[j][0])
                    hashmap[h + 1] = neighbours
    current_group = 0
    highest_group = 0

    def reset_array():
        visited = []
        for _ in range(1, N + 2):
            visited.append(False)
        return visited

    visited = reset_array()

    def dfs(at):
        if visited[at]:
            return
        else:
            visited[at] = True
            global current_group
            current_group += 1
        if at in hashmap:
            neighbours = hashmap[at]
            for next in neighbours:
                dfs(next)
        else:
            return

    counter = 0
    for i in range(len(nodes)):
        dfs(i + 1)
        if current_group > highest_group:
            highest_group = current_group
        visited = reset_array()
        current_group = 0

    print(highest_group)

Review of your code

You should mostly prefer the in operator instead __contains__. See @HeapOverflow's comment for more details.

According to PEP 8, you should not prefer CamelCase for variable names. Use snake_case instead.

temp = input().split()
N = int(temp[0])
M = int(temp[1])

Can be replaced with
M, N = [int(x) for x in input().split()]

And the same applies for a similar case.

  counter = 0
  for y in range(N):
      counter += 1
      nodes.append(counter)

nodes is just equal to the values from 1 to N, which is just equal to list(range(1, N+1))
Therefore, you can remove counter completely.

  if index_of_node == 0:
      neighbours.append(edges[j][1])
      hashmap[h+1] = neighbours
  else:
      neighbours.append(edges[j][0])
      hashmap[h+1] = neighbours

Since hashmap[h+1] = neighbours is executed regardless of the if statement, you can move it outside the scope.

def reset_array():
    visited = []
    for _ in range(1, N+2):
        visited.append(False)
    return visited

visited is basically just equal to [False] * (N+1).
The whole function can be replaced to return [False] * (N+1)
Also, this is a personal preference, but you don't have to use a function for this.

Function dfs

next is an inbuilt function's name, and therefore it should be avoided.
neighbours = hashmap[at] Since this is used only once, the assignment is unnecessary
else: return this is unnecessary, as the function does that anyway

Here's how dfs might look after applying the above changes:
def dfs(at):
    global current_group

    if visited[at]:
        return

    visited[at] = True
    current_group += 1

    if at in hashmap:
        for next_ in hashmap[at]:
            dfs(next_)

Faster Algorithm
The dfs will take O(N) time, and since it's executed N times, the time complexity will be O(N^2) which is clearly not feasible.
A disjoint set union on the other hand, will take a lot lesser time.
Here's my accepted code that uses DSU:
for _ in range(int(input())):
    n, m = map(int, input().split())
    dsu = [-1] * n

    for _ in range(m):
        u, v = map(int, input().split())
        u -= 1
        v -= 1

        while dsu[u] >= 0:
            u = dsu[u]

        while dsu[v] >= 0:
            v = dsu[v]

        if u == v:
            continue

        if u > v:
            u, v = v, u

        dsu[u] = dsu[u] + dsu[v]
        dsu[v] = u

    print(-min(dsu))

In case you have any queries about the above code, do ask me in the comments.

Also, I guess you're switching to python from java judging by the fact you named a variable hashmap. If that's the case, welcome to world of python!


Mars rover movement and position I have written a solution of a Mars Rover problem in C++ using OOP, but I am not fully satisfied with the design.
What changes can I make to improve the design?

Problem description is:

  A rover’s position and location is represented by a combination of x and y co-ordinates and a letter representing one of the four cardinal compass points. The plateau is divided up into a grid to simplify navigation. An example position might be 0, 0, N, which means the rover is in the bottom left corner and facing North.

  In order to control a rover , NASA sends a simple string of letters. The possible letters are ‘L’, ‘R’ and ‘M’. ‘L’ and ‘R’ makes the rover spin 90 degrees left or right respectively, without moving from its current spot. ‘M’ means move forward one grid point, and maintain the same heading.

#include<iostream>
#include<string>
#include<fstream>
using namespace std;

class Rover{

int x;
int y;
char orientation ;

public :

Rover();
Rover( int , int , char );
void rotateLeft();
void rotateRight();
void movePosition();
void displayPosition();

};

Rover :: Rover()
{
x =0;
y =0;
orientation = 'N';

}

Rover :: Rover ( int positionX , int positionY , char Orientation )
{
x= positionX;
y= positionY;

orientation = Orientation ;
}

void Rover::  displayPosition()
{
cout<<x<<" "<<y<<" "<<orientation;
}

void Rover :: rotateLeft()
{
 if(orientation=='N')
 orientation= 'W';
 else if(orientation=='W')
 orientation='S';
 else if(orientation=='S')
 orientation='E';
 else if(orientation=='E')
 orientation='N';

}

void Rover :: rotateRight()
{
 if(orientation=='N')
 orientation= 'E';
 else if(orientation=='W')
 orientation='N';
 else if(orientation=='S')
 orientation='W';
 else if(orientation=='E')
 orientation='S';

}

void Rover ::movePosition()
{
 if(orientation=='N')
    y++;
 else if(orientation=='W')
    x--;
 else if(orientation=='S')
    y--;
 else if(orientation=='E')
    x++;
}

int main()
{

int x,y; char orient ;
cin>>x>>y>>orient;
Rover firstRover(x,y,orient);

firstRover.displayPosition();

string roverMovement;
cin>>roverMovement;

for(int i=0 ; i < roverMovement.size() ; i++  )
{

     if(roverMovement[i]=='L')
     firstRover.rotateLeft();

     else if(roverMovement[i]=='R')
     firstRover.rotateRight();

     else
     firstRover.movePosition();
}

firstRover.displayPosition();

return 0;
}

How the design of this code can be improved?

The design is fine. You modeled the rover as a class, its actions with methods, and its state with member variables. This is natural, intuitive and simple, good this way.

On top of the issues Jerry already pointed out in his answer, I have a few additional comments about the implementation technique.

The rotation methods are a bit tedious, and tedious tasks are sometimes error prone. I would recommend a different approach for changing the orientation:

Create a simple string with the orientations in clockwise order, that is "NESW"
Forget about storing the name of the current orientation, it's enough to store its index in the orientations string. In all actions of the rover, we can work with this index and the string
When rotating right,  the index of the target orientation is the index of the current +1 modulo length(orientations)
When rotating left, the index of the target orientation is the index of the current + length(orientations) -1 modulo length(orientations)

For moving forward, you could do similarly, by using arrays for the x and y movements depending on the index of the current orientation, so for x the content will be {0, 1, 0, -1}, and for y it will be {1, 0, -1, 0}.

The benefit of this approach will be faster actions, as there will be no more conditional statements (also called a table driven approach).


Convert "String" sqrt(#) into numerical value I had to come up with a way to convert sqrt(#) into its actual numeric value because before I would have an array containing elements such as:

[sqrt(3), -sqrt(3), 1]

If I tried to multiply, I would get the error:

  Argument "-sqrt(3)" isn't numeric in multiplication (*)

Here is my work-around, but I think there's a better way. Any suggestions?

#!/usr/bin/perl
#perl Solver.pl

use strict;
use warnings;

my $roots = "Sqrt[3] ||-Sqrt[3] ||1";
my @rootList = split(/ \|\|/, $roots); # fill array with string's values separated by " ||"

# Convert any Sqrt numbers to numerical value
# (ex.) Sqrt[3] -> sqrt(3)
for $_ ( @rootList ){
    $_ =~ s/Sqrt\[/sqrt(/g; # Sqrt[ -> sqrt(
    $_ =~ s/\]/)/g; # ] -> )

    if( $_ =~ /-sqrt/ ){ # replace string of negative sqrt()
        my $temp = substr( $_, 6 ); # take "#)"
        $temp =~ s/\)//g; # remove ")"
        $_ = -1*sqrt( $temp );
    }
    elsif( $_ =~ /sqrt/ ){ # replace positive sqrt()
        my $temp = substr( $_, 5 );
        $temp =~ s/\)//g;
        $_ = sqrt( $temp );
    }
}

print "[@rootList]\n";

I found the use of Sqrt[...] to be cumbersome, why have the case in that format, and why have the [] style braces?

I would normally not recommend this, but a simple solution, if you trust, or can sanitize your inputs, is to just use the eval function.

for $_ ( @rootList ){
    $_ =~ s/Sqrt\[(.+)\]/sqrt($1)/;
    $_ = eval($_);
    $@ and die $@;
}

That does it..... all of it. Convert Sqrt[...] to sqrt(...), and eval the result.


Adding a class to a root element I have written some ES2015 JavaScript that adds a class to a root element when one of it's immediate children is focused.

Here is the markup for the code to be run on:

    <input type="text">
    <button type="submit>

And the JS itself:

const rootElement = document.getElementById('my-root-element');

[...rootElement.children].forEach((element, _, children) => {
  const update = () => {
    const focused = children.indexOf(document.activeElement) !== -1;

    rootElement.classList[focused ? 'add' : 'remove']('focused');
  };

  [ 'focus', 'blur' ].forEach((eventName) => {
    element.addEventListener(eventName, () => setTimeout(update, 0));
  });
});

Are there any ways to "simplify" this code or generally make it smaller? I know it's short as it is currently but I understand that ES2015 has a lot of features that can help in writing short, concise code.

Here is an actual example of the code being used (focusing either the button or the text field should make the word "Focused!" appear):

const rootElement = document.getElementById('my-root-element');

[...rootElement.children].forEach((element, _, children) => {
  const update = () => {
    const focused = children.indexOf(document.activeElement) !== -1;

    rootElement.classList[focused ? 'add' : 'remove']('focused');
  };

  [ 'focus', 'blur' ].forEach((eventName) => {
    element.addEventListener(eventName, () => setTimeout(update, 0));
  });
});
.focused:after {
  content: 'Focused!';
}

    <input type="text">
    <button type="submit">Submit</button>

Well, for purpose of what you are trying to do I would say just CSS could do it.

Using the :focus selector and combining with a span element like this:

input:focus + span:after {
    content: 'Focused!';
}

/* just polishing the looks from here on */
button {
 display: block;
}
span {
  margin: 0 10px;
}

    <input type="text">
    <button type="submit">Submit</button>

About your JavaScript optimization question two things strikes me:

don't define functions inside loops if you don't need to.
I don't see why to use the setTimeout. Is there any browser mixing up the order of the events?

So a suggestion, without the function declaration inside the loop, using a closure and .toggle() instead of a ternary for add/remove:

const rootElement = document.getElementById('my-root-element');
((rootElement, children) => {
    const update = () => {
        rootElement.classList.toggle('focused', children.indexOf(document.activeElement) !== -1);
    };
    children.forEach(el => {
        ['focus', 'blur'].forEach((eventName) => {
            el.addEventListener(eventName, update);
        });
    });
})(rootElement, [...rootElement.children]);


Should jQuery DOM Ready code be in the page with the HTML or separate JavaScript file? I have an application with HTML, external JavaScript and a bit of jQuery on the page which is initializing specific stuff to that page. I try to keep most of the JavaScript in other files. However I'm unsure if stuff in the DOM Ready function should be split into a separate file as well which is specific to that page. I'm wondering the best way to structure this.

The code for the calculator.html page might look like this:

<!doctype html>
<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <link type="text/css" href="css/global.css" rel="stylesheet">
        <script type="text/javascript" src="js/jquery.js"></script>
        <script type="text/javascript" src="js/global.js"></script>
        <script type="text/javascript" src="js/calculator.js"></script>
        <script>
            $(function() {
                calc.initialize();
                calc.loadPreviousState();
            });
        </script>
    </head>
    <body>
        <!-- page HTML code here -->
    </body>
</html>

The global.js files would be common JavaScript functions used on most pages. The calc.js file is only used by the calculator.html page and might have 20 odd functions in it. Assume that the two function calls inside the DOM Ready event need to be done on DOM load.

My question is, should I move this DOM Ready code into the top of the calculator.js file for clean code and readability? That way all the JavaScript would be contained in it's own file. For example here's the revised calculator.js file:

$(function() {
    calc.initialize();
    calc.loadPreviousState();
});

var calc = {
    initialize: function() {
        // code here
        return true;
    },
    loadPreviousState: function() {
        // code here
        return true;
    }
};

What's best practice here?

Many thanks!

I don't know if it is good practice, but in my eyes it would be cleaner, since you have several JS-files at this point, to separate markup, layout and JS completely. So $() would reside in a .js-File.

In some cases, when you only have a few lines of JS or -on the other hand- if you have to care for every millisecond to deliver a good page experience, it would make sense to embed JS in the html-file and retrieve the non-critical JS at later time dynamically.

It depends on your usecase.


Overloading operator ->* for smart pointers I am implementing a smart pointer class template and I want to overload operator ->* (even if it’s rarely done). I came across Scott Meyer’s article Implementing operator ->* for Smart Pointers. The article is from 1999 so I decided to try to adapt the code for C++ 14 (using parameter packs and perfect forwarding) and to improve it by adding support for pointer to data member—Meyer’s implementation only supports pointer to member functions—(which is the first of the two remaining problems of Meyer’s implementation that are listed at the section “Loose Ends” of his article and that he gives the reader as exercises to solve).

I ended up with the following code that seems to work quite fine. Now I have two questions:

Is there anything that could be improved in my code?
How to solve the second remaining problem listed at the section “Loose Ends” of Meyer’s article? (“You can’t use user-defined pointers-to-members. If someone has overloaded operator ->* to take objects that act like member pointers, you may want to support such ‘smart pointers to members’ in your smart pointer class. Unfortunately, you need traits classes to get the result type of such overloaded operator ->*.”)

Here is the code:

#include<iostream>

template<typename T>
struct Member_function_type_traits { };

template<typename O, typename... A, typename R>
struct Member_function_type_traits<R (O::*)(A...)> {
  typedef O Object_T;
  typedef R Return_T;
};

template<typename O, typename... A, typename R>
struct Member_function_type_traits<R (O::*)(A...) const> {
  typedef O Object_T;
  typedef R Return_T;
};

template<typename O, typename... A, typename R>
struct Member_function_type_traits<R (O::*)(A...) volatile> {
  typedef O Object_T;
  typedef R Return_T;
};

template<typename O, typename... A, typename R>
struct Member_function_type_traits<R (O::*)(A...) const volatile> {
  typedef O Object_T;
  typedef R Return_T;
};

template<typename T>
class Pending_member_function_call {
  typedef typename Member_function_type_traits<T>::Object_T Object_T;
  typedef typename Member_function_type_traits<T>::Return_T Return_T;
  std::pair<Object_T*, T> operands;
  public:
    Pending_member_function_call(std::pair<Object_T*, T> opr): operands{opr} { }
    template<typename... U>
    Return_T operator ()(U&&... args) {
      return (operands.first->*operands.second)(std::forward<U>(args)...);
  }
};

template<typename T>
class Smart_pointer {
  T* ptr;
  public:
    Smart_pointer(T* ptr): ptr{ptr} { }
    ~Smart_pointer() { delete ptr; }
    // …
    // operator ->* overload for pointers to data member
    template<typename U, typename V>
    V operator ->*(V U::* pdm) const {
      return ptr->*pdm;
    }
    // operator ->* overload for pointers to member function
    template<typename U>
    Pending_member_function_call<U> operator ->*(U pmf) const {
      return std::make_pair(ptr, pmf);
    }
};

struct A {
  int dm{5};
  int mf(int x) { return 2 * x; }
};

int main() {
  Smart_pointer<A> p{new A};
  auto pdm = &A::dm;
  auto pmf = &A::mf;
  std::cout << "A::dm called: " << p->*pdm << '\n';
  std::cout << "A::mf called: " << (p->*pmf)(8) << '\n';
  return 0;
}

Is there anything that could be improved in my code?

I think you can accomplish the same thing in much less code with just a lambda:

template <class U>
auto operator->*(U pmf) const {
    return [=](auto&&... args){
        return (ptr->*pmf)(std::forward<decltype(args)>(args)...);
    };
}

Could have a static_assert on std::is_member_pointer for extra safety.

  How to solve the second remaining problem listed at the section “Loose Ends” of Meyer’s article?

I believe the lambda solves this problem as well.

Also you could consider const propagation. If the Smart_pointer is const, do you really want to allow calling non-const member functions? I don't know. If you decide you don't, you should provde both a const and non-const overload and forward to a helper:

template <class U>
auto operator->*(U pmf) {
    return pending_mem_fun(ptr, pmf);
}

template <class U>
auto operator->*(U pmf) const {
    return pending_mem_fun(const_cast<T const*>(ptr), pmf);
}

where pending_mem_fun doesn't actually have to be a member function.


Python Command Line text editor I want to make a command-line text editor in python. Is this good code?

from termcolor import *
import os

while True:
    files = os.listdir()
    bufname = input("File name: ")
    title = cprint("Ganesha Editor v1.0", "green")
    controls = cprint("Type $exit to exit text editor. It autosaves.", "blue")

    def write():
        print("Compose Something Below:\n\n")
        text = input()
        if text == "$exit":
            exit()

        else:
            if bufname in files:
                os.remove(bufname)
                create = open(bufname, "w")
                create.close()

                autosave = open(bufname, "a")
                autosave.write(text)
                autosave.write("\n")
                autosave.close()

            else:
                autosave = open(bufname, "a")
                autosave.write(text)
                autosave.write("\n")
                autosave.close()

    while True:
        write()

I prettified the code with 8 spaces for the indents.

Does anyone like this code? It's very minimal, and no one will like it, but, I'm trying to make a secondary text-editor.

The code's pretty straightforward, but it could still be simplified a bit:

There's no reason to have multiple levels of while loop; you already run your write() function in an inner loop until it's time to exit, so the outer loop only ever gets executed once.
The code to append to the file is duplicated.
The file deletion behavior is very strange and seems like a bug -- if the file exists, you delete it, but you do that after every line, making it impossible to write a multi-line file (but only in the case where the file existed when you started the editor).  Seems like you probably meant to only delete it once but forgot to update your files list?
The write() function doesn't necessarily always write something; sometimes it exits (which is generally considered bad form since it makes reuse and testing difficult).  It also has the extra responsibility of deleting the existing file, which is why that deletion is maybe happening more often than you meant it to.  Separating the setup from the core editing loop would make this much easier to reason about.
cprint doesn't return a value, so assigning it to title and controls (which you don't use anyway) is confusing.
When an if block breaks the control flow (e.g. by exit() or return or break) it's not necessary to have everything else under an else.  In some cases the code may be more clear with a redundant else (e.g. when the two branches are similar in length and/or when you want to make it more obvious that they're mutually exclusive), but when the bulk of your code is indented unnecessarily it tends to make it harder to follow rather than easier.
It's generally better to use file handles as context managers (with open ...).  It makes it impossible to forget to close the file, and it makes it very obvious what you're doing while the file is open.
This is more of a note on the UX than the code, but I suggest (A) telling the user when you're deleting their file (and only doing it on startup rather than after every autosave) and (B) not having so many blank/repeated lines in the interface (it makes it harder to see what you've already entered).

Below is the result of applying the above notes -- now there are two functions (init and run), with one in charge of everything that happens up front, and the other in charge of everything that happens on each subsequent input.

import os
from termcolor import cprint

def init() -> str:
    """
    Start the editor by prompting for a filename and printing
    usage information.  If the file already exists, remove it.
    Returns the filename.
    """
    filename = input("File name: ")
    if filename in os.listdir():
        print("Deleting existing file to give you a blank slate.")
        os.remove(filename)
    cprint("Ganesha Editor v1.0", "green")
    cprint("Type $exit to exit text editor. It autosaves.", "blue")
    print("Compose Something Below:")
    return filename

def run(filename: str) -> bool:
    """
    Prompts for input and writes it to the file.
    Returns True as long as it's still running,
    returns False when it's time to exit.
    """
    text = input()
    if text == "$exit":
        return False
    with open(filename, "a") as autosave:
        autosave.write(text)
        autosave.write("\n")
    return True

if __name__ == "__main__":
    filename = init()
    while run(filename):
        pass


Function to test for different conditions in input data I have the following function that checks a data record to see if it has met certain criteria specified separately in configuration settings. The configurations are set up as a list of dictionaries with certain trigger keywords that are used by the condition check function ('in','not in', etc).

The condition checker has a lot of duplicated code, but it's just different enough that a better setup is not obvious to me. I would appreciate any feedback.

Here is a minimal working example to illustrate:

def check_if_condition_was_met( row, condition ):
    condition_met = True
    for key, val in condition.iteritems():
        if key == 'in':
            for condition_key, condition_val in val.iteritems():
                if row[condition_key] in condition_val:  # IN
                    continue
                else:
                    condition_met = False
                    break
        elif key == 'not in':
            for condition_key, condition_val in val.iteritems():
                if row[condition_key] not in condition_val:  # NOT IN
                    continue
                else:
                    condition_met = False
                    break
        elif key == 'max':
            for condition_key, condition_val in val.iteritems():
                if not row[condition_key]:
                    condition_met = False
                    break
                if int(row[condition_key]) <= int(condition_val): # MAX (<=)
                    continue
                else:
                    condition_met = False
                    break
        elif key == 'min':
            for condition_key, condition_val in val.iteritems():
                if int(row[condition_key]) >= int(condition_val): # MIN (>=)
                    continue
                else:
                    condition_met = False
                    break
    return condition_met

if __name__ == '__main__':
    # data
    test_data = [
         {'Flag1':'Y', 'Flag2':'Canada','Number':35}
        ,{'Flag1':'Y', 'Flag2':'United States','Number':35}
        ,{'Flag1':'N', 'Flag2':'United States','Number':35}
        ,{'Flag1':'N', 'Flag2':'England','Number':35}
        ,{'Flag1':'N', 'Flag2':'Canada','Number':35}
        ,{'Flag1':'N', 'Flag2':'Canada','Number':5}
    ]

    # configuration
    test_conditions = [
         { 'in':{'Flag1':['N'], 'Flag2':['United States']} }
        ,{ 'in':{'Flag1':['Y'],'Flag2':['Canada']}, 'max':{'Number':7} }
        ,{ 'in':{'Flag1':['Y'],'Flag2':['Canada']}, 'min':{'Number':7} }
        ,{ 'not in':{'Flag1':['Y']}, 'min':{'Number':7} }
    ]

    for condition_id, condition in enumerate(test_conditions):
        print
        print 'now testing for condition %i' % condition_id
        for data_id, data in enumerate(test_data):
            print '%s | %s' % ( data_id, check_if_condition_was_met(data,condition) )

Similar to @Peilonrayz, I would map each of the comparators to a function, but I'd go further.

Function design: check_if_condition_was_met(…) is an awkward name, partly because of the unusual use of past tense.  But why make it so wordy?  Couldn't you just call it verify(…)?

Furthermore, it would be customary to put the condition parameter first, and the row second.  It would certainly read more naturally in English, especially after the function rename.  Also, based on the observation that verify(condition) could be considered as a test, it's a general functional-programming principle that the condition parameter should be considered more tightly associated with the verification process and should therefore be put first.
Use all(…):  You want to express the idea that a function should return True if all of the conditions must be met, and False if any condition fails.  You can do that using all() with a generator expression.   It's a lot less cumbersome than your condition_met, for, continue, and break.  The entire function can be simplified down to a single expression!
Naming:  I think that "key" and "val" are not quite descriptive enough.  I suggest the following renamings:

key → comparator
val → criteria
condition_key → key
condition_val → desired_val

I'm also skeptical about some of the behaviour when given anomalous input:

Why are there int(…) casts with 'min' and 'max'?  Is it for parsing strings as numbers?  None of your example cases needs such parsing, though.  Is it for truncating floats towards zero?  Probably not, but it might have that unintended effect, if the thresholds or data are already numeric.
What happens if the row is missing a key that is specified in the condition?  Maybe it's not a concern to you, but it might be more appropriate to have the function return False rather than raise a KeyError, as your code does?

Consider writing doctests to explain what the function does.  This is a situation where examples are more expressive than words.

Suggested solution

COMPARATORS = {
    'in':       lambda v, lst: v in lst,
    'not in':   lambda v, lst: v not in lst,
    'min':      lambda v, n: (v is not None) and (v >= n),
    'max':      lambda v, n: (v is not None) and (v <= n),
}

def verify(condition, row):
    """
    Verify that the specified criteria are all true for the given row.

    >>> rows = [
    ...     {'Flag1':'Y', 'Flag2':'Canada',        'Number':35},
    ...     {'Flag1':'Y', 'Flag2':'United States', 'Number':35},
    ...     {'Flag1':'N', 'Flag2':'United States', 'Number':35},
    ...     {'Flag1':'N', 'Flag2':'England',       'Number':35},
    ...     {'Flag1':'N', 'Flag2':'Canada',        'Number':35},
    ...     {'Flag1':'N', 'Flag2':'Canada',        'Number':5},
    ... ]

    >>> [verify({'in': {'Flag1': ['N'], 'Flag2': ['United States']}}, r)
    ...  for r in rows]
    [False, False, True, False, False, False]

    >>> [verify({'not in': {'Flag1': ['Y']}, 'min': {'Number': 7}}, r)
    ...  for r in rows]
    [False, False, True, True, True, False]

    >>> [verify({'not in': {'Blah': ['whatever']}}, r) for r in rows]
    [True, True, True, True, True, True]
    """
    return all(
        all(
            COMPARATORS[comparator](row.get(key), desired_val)
            for key, desired_val in criteria.iteritems()
        )
        for comparator, criteria in condition.iteritems()
    )


Transferring a CSV file to a database while merging some columns Say I am given a csv file that I want to import into an sqlite3 database.  The first two columns in the csv file contain unique, important information while the rest of the columns are really a list of things.  So the csv file with the header row could look like:

cucumber.csv

'important1', 'important2', 'listItem_1', 'listItem_2', 'listItem_3'
'123', '9876', 'apple', 'orange', 'banana'
'456', '7890', 'cabbage', 'onion', 'carrot'

So, when I import into the database, I want to shove all but the first two columns into one column.  The schema for the table in the database would then look like:

import csv
import sqlite3

def main():
    data_filename = 'cucumber.csv'
    db_filename = 'spam_mart.sqlite'
    SQL_create_table = """DROP TABLE IF EXISTS cucumber;
                          CREATE TABLE cucumber (
                          important1  NUMBER PRIMARY KEY,
                          important2  NUMBER,
                          item_list   TEXT
                       );
                       """
    SQL = """insert into cucumber(important1, important2, item_list)
             values (:important1, :important2, :item_list)
          """

    with open(data_filename) as f, sqlite3.connect(db_filename) as conn:
        conn.executescript(SQL_create_table)
        cursor = conn.cursor()
        reader_object = csv.reader(f)
        next(reader_object, None)      # Skip the headers
        row_count = 0
        for row in reader_object:
            row_count += 1
            row_dict = {}
            important1, important2, *item_list = row  # unpack the row
            row_dict['important1'] = important1
            row_dict['important2'] = important2
            row_dict['item_list'] = repr(item_list)  # convert list to string first
            cursor.execute(SQL, row_dict)

    print('Loaded {} of {} records'.format(str(row_count),
                                       str(reader_object.line_num - 1)))

if __name__ == '__main__':

    main()

I would normally use a csv.DictReader() object to transfer a csv file to a database, but since I was creating a list from some of the columns first, I am going with a normal csv.reader() object.

I used the repr() so that I can easily access the list again using the eval() if need be.  Script works as expected.  However, the whole technique seems a little clumsy to me.  All honest critiques welcomed.

csv.DictReader accepts 'restKey' 3rd parameter.

with open(data_filename) as f, sqlite3.connect(db_filename) as conn:
        conn.executescript(SQL_create_table)
        cursor = conn.cursor()
        next(f, None)      # Skip the headers
        reader_object = csv.DictReader(f,['important1','important2'],'item_list')
        row_count = 0
        for row_dict in reader_object:
            row_count += 1
            row_dict['item_list'] = repr(row_dict['item_list'])  # convert list to string first
            cursor.execute(SQL, row_dict)


Security of login form using prepared statements I've prepared a simple login form, using prepared statements to prevent SQL injections. How secure is this ?

    <title>Login</title>
        <?php
        session_start();

        // set up a token
        $form_token = md5(uniqid('auth', true));

        // session form token
        $_SESSION['form_token'] = $form_token;

        if (isset($_SESSION['user_id'])) {
            print "You're already logged in";
        }
    ?>

Login!

    <?php
    if (isset($_SESSION['user_id'])) {
        print "Log Out Here";
        exit;
    }
    ?>
    <form action="login.php" method="post">

Username :

        <input type="text" name="username" maxlength="20">

Password :

        <input type="text" name="pswd" maxlength="20">
        <input type="hidden" name="form_token" value="<?php echo $form_token; ?>" />

        <input type="submit" value="Login">
    </form>

Nothing too special on the form..
Now, the PHP that process the data:

    <title>Login</title>

    <?php
    session_start();
    $message = "";

    if (isset($_SESSION['user_id'])) {
        $message .= "You're already logged in
";
        print $message;
        print "Log Out Here";
        exit;
    }

    //
    // GET THE USER DATA
    //
    $username = $_POST['username'];
    $pswd = $_POST['pswd'];
    $form_token = $_POST['form_token'];

    //
    // CHECK THE DAATA
    //

    if (empty($username) || empty($pswd) || !isset($form_token)) {
        $message .= "Please enter a valid username/password
";
    }
    elseif ($form_token != $_SESSION['form_token']) {
        $message .= "Invalid Form Submission
";
    }
    elseif (strlen($username) > 20 || strlen($username) < 4) {
        $message .= "Invalid length for a username
";
    }
    elseif (strlen($pswd) > 20 || strlen($username) < 4) {
        $message .= "Invalid length for a username
";
    }
    else {

        // encrypting the password

    $pswd = sha1($pswd);

        // *********************
        // CONNECTING TO THE DATABASE
        // *********************

    $servername = "localhost";
    $usrname = "root";
    $password = "";
    $db_name = "auth";

    // connect to the database
    $conn = new mysqli($servername, $usrname, $password, $db_name);
    if ($conn->connect_error) {
        die("Connection failed".$conn->connect_error);
    }

    // statement
    $stmnt = $conn->prepare("SELECT username, pswd FROM users WHERE username = ? AND pswd = ?");
    $stmnt->bind_param('ss', $username, $pswd);

    $stmnt->execute();

    $result = $stmnt->get_result();
    if ($result->num_rows > 0) {
            $user_id = $result;
            $_SESSION['user_id'] = $result;
            $message .= "Succesfully Logged in!";
    }
    else {
        $message .= "Failed to Log In. Invalid Username or Password";
    }

    }

    ?>

<?php
    print $message;

?>

I'll point out a few things here:

$pswd = sha1($pswd); is cringeworthy. Please use password_hash() if you can. If you can't update your PHP version to comply, my next best suggestion is to use crypt().
Having all of your DB connection code on the page is generally considered poor coding. Place everything between $servername ... // statement in a separate file and require it.
In your select statement, there is no reason to retrieve the password from the DB alongside the username. The less you deal with the password, most likely will be safer. Also, you're putting the hashed password in the session, which I'm not sure is the best idea. A phishing attempt could potentially reveal this and a malicious user could take advantage of this information.
Your method of creating a session token is weak, and I suggest you check out this StackOverflow post for more info.
Format your code. You're missing a lot of indentation and general style rules aren't 100% met.


Python: Selection sort After implementing suggestions from an earlier, related question (Python: Insertion Sort), I have written this code. Could you help me to improve this code?

def get_input():
    input_str = input("Enter elements to be sorted: ")
    lst = list(map(int, input_str.split()))
    return lst

def selection_sort(thelist):
    for i in range(len(thelist)-1):
        min_idx = i

        for j in range(i+1, len(thelist)):
            if thelist[j] < thelist[min_idx]:
                 min_idx = j

        thelist[i], thelist[min_idx] = thelist[min_idx], thelist[i]

if __name__ == '__main__':
    input_list = get_input()
    selection_sort(input_list)
    print(*input_list, sep = ", ")

Regarding the code itself I think functions should usually return an output and then this should be printed. It is also worth introducing some way of alerting the user if there input causes an error. I entered
5, -3, 0 and this raised an error because your code splits on spaces not commas. Additionally, you are mapping the list to int so entering a character by mistake breaks the code

Hence saying something like

def get_input():
    input_str = input("Enter elements to be sorted: ")
    try:
        lst = list(map(int, input_str.split()))
    except:
        raise TypeError ("Please enter a list of integers only, separated by a space")
    return lst

def selection_sort(thelist):
    for i in range(len(thelist)-1):
        min_idx = i

        for j in range(i+1, len(thelist)):
            if thelist[j] < thelist[min_idx]:
                 min_idx = j

        thelist[i], thelist[min_idx] = thelist[min_idx], thelist[i]
    return thelist

if __name__ == '__main__':
    input_list = get_input()
    output = selection_sort(input_list)
    print(*output, sep = ", ")


Java Date class- switches and ifs in the constructor I'm a beginner i have an assignment to write a Date class (as part of a bigger project). in my question i focus on the constructor. here's some background: the given guidelines are that the date is not expected to be valid and the following instance variables expect this input range: day- integer 1-31 month- integer 1-12 year- integer 4 digits year.

now, if an invalid day/month/year or invalid date (such as 31.2.2010) is entered, the object will be created with the date of 1.1.2000.

this is the code I've come up with and it does compile and seem to work fine.

public class Date
{
    private int _day;
    private int _month;
    private int _year;

    public Date (int day, int month, int year)
    {
        switch (month)
        {
            case 1:
            case 3:
            case 5:
            case 7:
            case 8:
            case 10:
            case 12: if ((day>0 && day<32) && (year>999 && year<10000))
            {
                _day=day;
                _month=month;
                _year=year;
            }
            else
            {
                _day=1;
                _month=1;
                _year=2000;
            }
            break;

            case 4:
            case 6:
            case 9:
            case 11: if ((day>0 && day<31) && (year>999 && year<10000))
            {
                _day=day;
                _month=month;
                _year=year;
            }
            else
            {
                _day=1;
                _month=1;
                _year=2000;
            }
            break;

            case 2: if (leap(year))
            {
                if ((day>0 && day<30) && (year>999 && year<10000))
                {
                    _day=day;
                    _month=month;
                    _year=year;
                }
                else
                {
                    _day=1;
                    _month=1;
                    _year=2000;
                }
                break;
            }

            else
            {
                if ((day>0 && day<29) && (year>999 && year<10000))
                {
                    _day=day;
                    _month=month;
                    _year=year;
                }
                else
                {
                    _day=1;
                    _month=1;
                    _year=2000;
                }
                break;
            }
        }
    }

    /** check if leap year */
    private boolean leap (int y)
    {
        return (y % 4 == 0 && y % 100 != 0) || (y % 400 == 0);
    }
}

here are my questions:

Is it fine to put all that code in the constructor? will it greatly affect the processing time or cause an error? is there an alternative if its a problem?
Is any part of the code could be considered a bad practice? such as the switches and ifs? I'm not feeling to confident with this build despite it working fine...

sorry for indentation inconvenience and such, 1st post :(

Getting your class compiling and working is an achievement in itself. So don't let my comments discourage you, but see them as hints for the next learning steps.

Processing time

Don't worry about that until you really experience performance problems. Even expert programmers often fail when trying to predict the performance of their code. If you run into performance issues, then learn to use analysis tools like "profilers". But most probably, you won't need them for the next few years.

Code in constructor

It's good to do validity checks in constructors. But you're right, the constructor's readability suffers from its length. The key to readability is introducing good abstractions instead of writing lengthy code, as you already did with the leap() method. Writing a monthLength(int year, int month) method and using that in the constructor, you can get rid of most switch/if statements.

Invalid dates

You chose to create a 01.01.2000 date in case of invalid date elements. Professionals will instead throw an IllegalArgumentException.

As a Java beginner, it's quite likely that you haven't learnt about the concept of exceptions yet, but it's an important thing, an elegant way to tell someone who wants to construct e.g. a 31.2.2010 date that you can't give him that date as it doesn't exist. As a caller of a constructor or method, I always want to be informed about success or failure, and exceptions are a good way to do that.


Project Euler problem 12 in Ruby I've been trying to optimize this while making it readable. Thoughts?

  The sequence of triangle numbers is generated by adding the natural
  numbers.  So the 7th triangle number would be 1 + 2 + 3 + 4 + 5 + 6 +
  7 = 28.  The first ten terms would be:

1, 3, 6, 10, 15, 21, 28, 36, 45, 55, ...

  Let us list the factors of the first seven triangle numbers:

 1: 1
 3: 1,3
 6: 1,2,3,6
10: 1,2,5,10
15: 1,3,5,15
21: 1,3,7,21
28: 1,2,4,7,14,28

  We can see that 28 is the first triangle number to have over five
  divisors.

  What is the value of the first triangle number to have over five
  hundred divisors?

@triangle_number = 441
@factors = []
@right_of_tree = []
@left_of_tree = []

def find_triangle_number_with_500_factors
  natural_number = 1000
  while !over_500_factors?
    @factors = []
    @right_of_tree = []
    @left_of_tree = []
    triangle_number_generator(natural_number)
    find_factors_with_a_factor_tree
    natural_number += 1
  end
  p @triangle_number
end

def triangle_number_generator(natural_number)
  @triangle_number = natural_number*(natural_number+1) / 2
end

def find_factors_with_a_factor_tree
  create_prime_factor_tree(@triangle_number)
  find_combinations_in_left_of_tree
end

def find_combinations_in_left_of_tree
  last_prime_factor = @right_of_tree.last
  @left_of_tree.push(last_prime_factor)
  (1..@left_of_tree.length).each do |combination_size|
    cxget_combinations(@left_of_tree, combination_size)
    combinations.each do |arr|
      @factors << arr.inject(:*)
    end
  end
end

def get_combinations(arr, combination_size)
  arr.combination(combination_size).to_a.uniq
end

def create_prime_factor_tree(number)
  until number <= 1
    factor = get_initial_divisor(number)
    if factor
      @right_of_tree.push(number / factor)
      @left_of_tree.push(factor)
      number = number / factor
    else
      break
    end
  end
end

def get_initial_divisor(number)
  2.upto(number / 2) do |possible_factor|
    if number % possible_factor == 0
      return possible_factor
      break
    end
  end
  return nil
end

def over_500_factors?
  return true if @factors.size + @right_of_tree.size >= 498
end

Benchmark.bm do |bm|
  bm.report { find_triangle_number_with_500_factors}
end

Main method

You want your code to tell the story of how you are solving the problem, starting with the general picture and then working down to the details.

Your main method is find_triangle_number_with_500_factors. I suggest you change the name slightly:

drop "find" as being useless.
don't hardwire "500".

Not hardwiring "500" makes the method more general, facilitates testing and helps explain what the method does. In general, hardwiring is bad and doesn't buy you anything.

Perhaps we might call the method smallest_triangle_number(nbr_factors).

Make the main method simple, showing how the calculation is made is the simplest of terms:

def smallest_triangle_number(nbr_factors)
  n = 0
  nfacs = 0
  while nfacs < nbr_factors
    n += 1
    tnbr = triangle_number(n)
    nfacs = nbr_factors(tnbr)
  end
  tnbr
end

To my way of thinking, the shorter the method, the shorter the variable names can be.  If a method is short, and you understand what value a variable holds, you don't have to remember its name very long, so it can be short. Here I used n (and nbr) for "number" because I always use that convention. triangle_number(n) obviously is a method that calculates n's triangle number. I can use something short like tnbr because the expression says what it is.  Same with nbr_factors and nfacs.

If you want to get fancy you don't have to begin with the number 1. Instead, you could calculate a lower bound on numbers that have triangle numbers with the given number of factors.

Calculate triangle numbers

Next, in order, the method triangle_number(n). Your method is fine, except "generator" doesn't add anything to the name and there is no need for an instance variable. Just return the triangle number, which also helps with the self-documentation of the method:

def triangle_number(nbr)
  nbr * (nbr + 1) / 2
end

I suggest that you only introduce instance variables when you really need them.  For this problem, I don't think you need any.

This problem concerns natural numbers, so I think nbr is enough. (Nobody will ask, "What if nbr is negative, a float, a complex number or a Roman numeral"?) You could instead calculate each triangle number from the previous one, but this is a quick calculation relative to the other work that must be done, and stands alone, so I think that's fine.

Calculate factors

Next up, nbr_factors. You are doing that calculation in two steps, so again, keep the method short to give the reader the big picture:

def nbr_factors(nbr)
  pfacs = prime_factors(nbr)
  prime_factors_to_nbr_factors(pfacs)
end

Again, this is pretty much self-documenting. Notice that I've written the argument as nbr, rather than some variation of triangle_number. That's because the method applies to natural numbers generally, even though we will only use it for triangle numbers. You could of course also write this as:

def nbr_factors(nbr)
  prime_factors_to_nbr_factors(prime_factors(nbr))
end

That's up to you, but you probably want to use the first form at least while you are debugging and testing.

Calculate number of factors from prime factors

Now let's go out of order and look at the method     prime_factors_to_nbr_factors. This one is actually quite easy, as we can employ a version of the Tau function.

Suppose we have constructed prime_factors(n) to return the array:

pfacs = [[f1,n1], [f2,n2],..., [fm, nm]],

where:

fi is the ith prime factor
ni is the number of times fi is a factor

and:

n = f1**n1 * f2**n2 *...* fm**nm

The Tau function tells us the number of factors is:

(n1+1)*(n2+1)*...*(nm+1)

which in Ruby-speak is:

def prime_factors_to_nbr_factors(prime_factors)
  prime_factors.reduce(1) { |tot,(_,n)| tot *= (n+1) }
end

It makes sense that the number of factors should not depend on what the particular prime factors are, just how many there are and the number of times each occurs.

(Note that if you don't care about displaying the prime factors, you could have prime_factors--probably renamed--return only [n1, n2,..., nm].)

Suppose, for example:

tnbr = triangle_number(7)
  #=> 28

Then

pfacs = prime_factors(28)
  #=> [[2,2], [7,1]]

so

prime_factors_to_nbr_factors([[2,2], [7,1]])
  #=> (2+1)*(1+1) => 6

It remains to construct the method prime_factors to return an array similar to pfacs above. As I've rambled on awhile, I'll leave it to others to cover that.