Reading lines of a file into a vector of strings I have to read from a file and store the data inside an std::vector container. The procedure of opening the file, reading and writing onto the container are enclosed in a routine: readData.

Reading and writing include the use of several std functions that may fail.
Instead of putting a try catch for each of those std function calls, I have put a try catch-all block to collect all the possible errors.
Is this a bad practice?

#include "global.h

void readData( std::vector<std::vector<std::string> >&,
                    const std::string& );

int main()
{

    std::vector<std::vector<std::string> >& table;
    std::string path = "myDataFile";

    try
    {
        readData( table, path );
        std::cout << "Done" << std::endl;
    }
    catch( const char* error)
    {
        std::cout << error << std::cout;
        return 0;
    }

    // do something with the data ...

    return 0;
}

Here is the readData implementation:

void readData( std::vector<std::vector<std::string> >& table,
                    const std::string& path)
{

    std::ifstream ifs(path);
    if (  !ifs )
        throw "Error opening file.";

    try
    {
        while ( true ){
            std::string line;
            std::getline(ifs, line);

            if(!ifs)
                break;

            std::vector<std::string> row;
            std::istringstream iss(line);
            std::copy(std::istream_iterator<std::string>(iss),
                 std::istream_iterator<std::string>(),
                 std::back_inserter(row));
            table.push_back(row);
        }
    }
    catch(...)
    {
        throw "Error reading the content of the file";
    }

}

General Comment

  Instead of putting a try catch for each of those std function calls, I have put a try catch-all block to collect all the possible errors. Is this a bad practice?

No that is completely fine. The general principle is that if you are not going to handle the error then you should not catch it. Logging is an exception to the rule; where you catch log and then re-throw.

BUT: Most C++ io functions do not throw exception on error (you can make them do so, but it is not normal). Instead a an error causes the flags on the stream to be set to indicate one of the failure states. Now the stream object itself can be used in a boolean context to check the state of these flags (stream will convert to true if it is in a good usable state and false if any of the error flags are set).

Code Review

Avoid output parameters

I hate return parameters. They are non logical and make the flow of the code hard to read. I prefer to return all the results (even if there is more than one).

void readData( std::vector<std::vector<std::string> >&,
                    const std::string& );

Now you are probably thinking that this is inefficient as this requires a copy of the object out of the function. Which is technically true. But the compiler writers put a lot of work into making sure that this copy does not actually happen. RVO and NRVO (optimizations) basically make sure that the object is built in place at the destination (no copy required).

Also in C++11 we introduced move semantics to the language. So on the rare occasion that RVO and NRVO fail a return value will be moved (not copied) from a function. moving a vector simply involves swapping three pointers (or something very close to this) so it is not very expensive.

Prefer "\n" over std::endl

The difference is that std::endl flushes the stream.

        std::cout << "Done" << std::endl;

The stream already flush themselves at the most appropriate time to make themselves efficient; manually forcing a flush is more likely to cause inefficiencies.

Return a non zero value to indicate failure

The OS uses the return value of main to detect if the program ran correctly.

    catch( const char* error)
    {
        std::cout << error << std::cout;
        return 0;
    }

Return a non zero value to indicate failure.

Catch all exceptions in main.

If you throw an exception and it is not caught the program will exit. BUT it is implementation defined if this will force the stack to unwind. If you catch in main() this will force the stack to unwind all the way to the top (thus making sure that all the appropriate destructors are called).

Note: You should re-throw the exception potentially after logging. This is because most OS detect applications that exit via an exception and provide further facilities to the user when this occurs.

int main()
{
    try
    {
         // STUFF
    }
    catch(...)
    {
         std::cout << "Exception Reached main()\n";
         throw;
    }
}

Use an exception derived from a std::exception

When you throw an exception you should throw an exception derived from std::exception. If you don't throw a specialized exception your default should probably be std::runtime_error.

    if (  !ifs )
        // this throws an object of type `char const*`
        throw "Error opening file.";
    // Try this
    if (  !ifs )
    {
        throw std::runtime_error("Error opening file.");
    }

STandard pattern for reading a file.

Your code actually works correctly

        while ( true ){
            std::string line;
            std::getline(ifs, line);

            if(!ifs)    // Most people forget this check.
                break;

            // STUFF
        }

But can be made more succinct. If you do the read as part of the test you can do the read and test as a single line.

        // This is more the standard pattern for reading a file.
        std::string line;
        while ( std::getline(ifs, line) )
        {
            // STUFF
        }

Note: std::getline() returns a reference to the input stream. Which is used in the boolean context checks the state of the error flags. So the loop is only entered if the read succeedes.

Vector can be constructed with iterators.

            std::istringstream iss(line);
            std::vector<std::string> row;
            std::copy(std::istream_iterator<std::string>(iss),
                 std::istream_iterator<std::string>(),
                 std::back_inserter(row));

Very good. But this can be simplified to:

            std::istringstream iss(line);
            std::vector<std::string> row{std::istream_iterator<std::string>(iss),
                                         std::istream_iterator<std::string>()};

Don't generalize an exception

Here you are catching a specific exception that could contain lots of information and replacing it with a string.

    catch(...)
    {
        throw "Error reading the content of the file";
    }

This looses all the relevant information about the original exception and replaces it with a string. Loosing information is never good.

Answer to Comments

I would refactor like this:

#include <iostream>
#include <fstream>
#include <sstream>
#include <iterator>
#include <stdexcept>
#include <vector>
#include <string>

using StringTable = std::vector<std::vector<std::string>>;

StringTable readData(std::string const& path)
{
    std::ifstream ifs(path);
    if (  !ifs )
    {
        throw std::runtime_error("Error opening file.");
    }

    StringTable table;
    std::string line;
    while (std::getline(ifs, line))
    {
        std::istringstream iss(line);
        table.emplace_back(std::istream_iterator<std::string>(iss),
                           std::istream_iterator<std::string>());
    }
    return table;
}

void doMain()
{
    std::string path  = "myDataFile";
    StringTable table = readData(path);
    std::cout << "Done" << "\n";

    // do something with the data ...
}

int main()
{
    try
    {
         doMain();
    }
    catch(std::exception const& e)
    {
        std::cout << "Exception: " << e.what() << "\n";
        throw;
    }
    catch(...)
    {
        std::cout << "Exception: Unknown\n";
        throw;
    }
    return 0;
}


A lightweight CRUD, based on the Table Data Gateway pattern In order to reduce the boilerplate code, I decided to write a lightweight database helper, based on the  Table Data Gateway pattern (or at least the way I understand it) with the aim of using it for a simple CRUD application.
Writing INSERT and UPDATE queries is always a nuisance, repeating all those ranks of column names, parameter names, binding parameters and such. So I decided to write a simple class that does all the job under the hood, and accepts/returns simple arrays, without going down the rabbit hole of creating a full featured ORM.
My aim was also to make this class as secure as possible, hence the table and field names are always hardcoded.
It consists of one prototype class,
abstract class BasicTableGateway
{
    protected $db;
    protected $table;
    protected $fields;
    protected $primary = 'id';

    public function __construct(\PDO $db)
    {
        $this->db = $db;
    }
    public function listBySQL(string $sql, array $params): array
    {
        $stmt = $this->db->prepare($sql);
        $stmt->execute($params);
        return $stmt->fetchAll();
    }
    public function list(int $page = 1, int $limit = 50): array
    {
        $page--;
        $offset = $page * $limit;
        $stmt = $this->db->prepare("SELECT * FROM `$this->table` LIMIT ?,?");
        $stmt->execute([$offset,$limit]);
        return $stmt->fetchAll();
    }
    public function read($id): ?array
    {
        $ret = $this->listBySQL("SELECT * FROM `$this->table` WHERE `$this->primary`=?", [$id]);
        return $ret ? reset($ret) : null;
    }
    public function update(array $data, int $id): void
    {
        $this->validate($data);

        $params = [];
        $set = "";
        foreach($data as $key => $value)
        {
            $set .= "`$key` = ?,";
            $params[] = $value;
        }
        $set = rtrim($set, ",");
        $params[] = $id;
        $sql = "UPDATE `$this->table` SET $set WHERE `$this->primary`=?";
        $this->db->prepare($sql)->execute($params);
    }
    public function insert($data): int
    {
        $this->validate($data);

        $fields = '`'.implode("`,`", array_keys($data)).'`';
        $placeholders = str_repeat('?,', count($data) - 1) . '?';

        $sql = "INSERT INTO `$this->table` ($fields) VALUES ($placeholders)";
        $this->db->prepare($sql)->execute(array_values($data));

        return $this->db->lastInsertId();

    }
    public function delete($id)
    {
        $sql = "DELETE FROM `$this->table` WHERE `$this->primary`=?";
        $this->db->prepare($sql)->execute([$id]);
    }
    protected function validate($data)
    {
        $diff = array_diff(array_keys($data), $this->fields);
        if ($diff) {
            throw new \InvalidArgumentException("Unknown field(s): ". implode($diff));
        }
    }
}

and children classes where related properties are set to operate certain tables:
class UserGateway extends BasicTableGateway {
    protected $table = 'users';
    protected $fields = ['email', 'password', 'name', 'birthday'];
}

which is then used like this
$data = [
    'email' => 'foo@bar.com',
    'password' => 'hash',
    'name' => 'Fooster',
];
$id = $userGateway->insert($data);
$user = $userGateway->read($id);
echo json_encode($user),PHP_EOL;

$userGateway->update(['name' => 'Wooster'], $id);
$user = $userGateway->read($id);
echo json_encode($user),PHP_EOL;

$users = $userGateway->list();
echo json_encode($users),PHP_EOL;

$userGateway->delete($id);

The code works all right for a simple CRUD but I'd like to know whether it can be improved or may be there are some critical faults I don't realize.

In list(), because $page is a single-use variable, I don't know that it is worth decrementing.  Perhaps write just one line as:
$offset = ($page - 1) * $limit;

but then again $offset is a single-use variable.  If you prefer the declarative coding style, leave it as is.  Alternatively, you could just write the arithmetic directly in the execute() call.  (either way, don't forget to add the space between offset and limit arguments)
$stmt->execute([($page - 1) * $limit, $limit]);

or because of precedence, you can pre-decrement to avoid parentheses:
$stmt->execute([--$page * $limit, $limit]);

or as stated in comments, "list can internally call listBySql" from @hjpotter92:
public function list(int $page = 1, int $limit = 50): array
{
    return listBySQL("SELECT * FROM `$this->table` LIMIT ?,?", [--$page * $limit, $limit]);
}

Please add the int type declaration to the $id argument in read().  I think that read() can be reduced to this:
return reset($this->listBySQL("SELECT * FROM `$this->table` WHERE `$this->primary`=?", [$id])) ?: null;

because if fetchAll() delivers an empty array, then reset() will return false in which case the Elvis operator can return the desired null value.  This is more of a style preference alternative rather than a functional gain -- the only real benefit is removing the $ret variable.

I would prefer that update() return int instead of void (even if your demo doesn't seek this information).  In my projects, I always return the number of affected rows.  This is integral in knowing if the query was actually successful in making changes or if there were no changes from the valid query.  I find this to be as important as insert() returning the new id.

Please add the array type declaration on $data in insert() and remove the blank line after returning the new id in insert().

Please add the int type declaration on $id in delete().

There is some inconsistency regarding when you declare a $sql variable and when you write the sql string directly into the prepare() call.  I think you should pick one style or the other.  My leanings, of course, bias toward removing single-use variables.

validate()'s incoming argument should have an array type declaration.  The implode() call in the thrown exception could use some glue -- a comma at minimum.


Input iterator in C++ For educational purposes and to understand how iterators are working in C++, I've create an input iterator and I would like others to look at and provide any feedback on my code.

template<typename T>
class iter_input {
 public:
  typedef T         value_type;
  typedef T&        reference_type;
  typedef const T&  const_reference_type;
  typedef T*        pointer_type;
  typedef const T*  const_pointer_type;

  iter_input(pointer_type& pointer) : pointer_(pointer) {}
  iter_input(const iter_input& other) : pointer_(other.pointer_) {}

  iter_input& operator= (const iter_input& rhs) {
    this->pointer_ = rhs.pointer_;
    return (*this);
  }

  iter_input& operator++ (void) {
    ++this->pointer_;
    return (*this);
  }

  iter_input operator++ (int) {
    iter_input temp(*this->pointer_);
    ++*this;
    return (temp);
  }

  bool operator== (const_reference_type rhs) {
    return (this->pointer_ == rhs.pointer_);
  }

  bool operator!= (const_reference_type rhs) {
    return (this->pointer_ != rhs.pointer_);
  }

 private:
  T* pointer_;
};

References:

apache.org
sgi.com

Correctness Issues

Needs dereferencing operator

There are two basic operations every iterator needs to support:

Incrementing the iterator
Dereferencing the iterator

Right now, you've covered incrementing, but not dereferencing. That means you can iterate over items, but you can't look at the items themselves, rendering the whole thing nearly useless (I would remove the "nearly", but you can compute the distance between two iterators, and once in a while that's all you really need).

reference_type operator *() { return *pointer_; }

const correctness

operator ==() and operator !=() should both be const operators.

Correct types

operator== and operator!= should take parameters of type iter_input as their parameters. When you do a comparison, you compare the iterator to another iterator, not the iterator to the type of object the iterator refers to.

These operators should look something like this:

bool operator== (iter_input rhs) const {
  return pointer_ == rhs.pointer_;
}

bool operator!= (iter_input rhs) const {
  return pointer_ != rhs.pointer_;
}

Constructor

The constructor that takes a pointer should take it by value rather than reference. This allows (for example) initializing an iterator from the name of an array:

int x[] = {1, 2, 3, 4};

iter_input<int> begin(x);
// or: iter_input<int> begin = std::begin(x);

You can normally take for granted that pointers are cheap to copy, so you don't normally want to pass pointers by reference for the sake of efficiency (but for things like linked lists, you do sometimes want to pass them by reference because you're going to modify the pointer that's passed).

Style Issues

this-> considered harmful

Using this-> for every reference to a member is (at least in my opinion) a poor idea. It adds visual noise, which hurts readability but provides no benefit.

return is an operator, not a function

When you return a value from a function, you don't need to enclose that expression in parentheses. Again, this simply adds visual noise and hurts readability without any benefit (and in a few cases that don't apply here can cause other problems as well).

Assignment operator

Unless you need compatibility with older (pre C++11) compilers I'd prefer to define the assignment operator as defaulted rather than including a function body for it.

iter_input& operator= (const iter_input& rhs) = default;

Use your own typedefs

Instead of T *pointer_;, I'd prefer pointer_type pointer_;.

Make it a proper header

If you want to do anything more than a minimal test of this code, you probably want it to be in a header. To facilitate that, you could add the (officially not exactly standard, but almost universally accepted) #pragma once at the beginning, or else add standard "header guards":

#ifndef ITER_INPUT_INCLUDED_
#define ITER_INPUT_INCLUDED_

// existing code here

#endif

Note that if you look at your standard headers, the name they use in place of ITER_INPUT_INCLUDED_ will typically start with an underscore. That gives a name that's reserved for the implementation, which means the people writing the compiler can (and in this case should) use that kind of name, but in normal code that's not part of the standard library, you should not use such a name.

Indentation

The 2-space indentation is too small for me to be certain what's supposed to line up with what. The access specifiers (public: and private:) being indented only one space makes this even worse. The days of 40-column displays are long gone. There's no need to skimp on indentation to this degree.


Open a text file and remove any blank lines I have the following functions which help me to open a text file and remove any blank (empty) lines:

def fileExists(file):
    try:
        f = open(file,'r')
        f.close()
    except FileNotFoundError:
        return False
    return True
def isLineEmpty(line):
    return len(line.strip()) < 1
def removeEmptyLines(file):
    lines = []
    if not fileExists(file):
        print ("{} does not exist ".format(file))
        return
    out = open(file,'r')
    lines = out.readlines()
    out.close()
    out = open(file,'w')
    t=[]
    for line in lines:
        if not isLineEmpty(line):
            t.append(line)
    out.writelines(t)
    out.close()

As you can see I open a file 2 times. One for reading and one for writing. Is there anything I can to to improve this code?

I see two good strategies to accomplish the task.

One solution is to read all of the text into a list, then rewind to the start of the file and write out the desired lines.

def remove_empty_lines(filename):
    """Overwrite the file, removing empty lines and lines that contain only whitespace."""
    with open(filename, 'r+') as f:
        lines = f.readlines()
        f.seek(0)
        f.writelines(line for line in lines if line.strip())
        f.truncate()

The disadvantage of that approach is that it scales poorly if the file is huge, because the entire contents have to be read into memory first.  The other approach would be to overwrite the file while reading from it.  This uses two file objects, each keeping track of its own position within the file.

def remove_empty_lines(filename):
    """Overwrite the file, removing empty lines and lines that contain only whitespace."""
    with open(filename) as in_file, open(filename, 'r+') as out_file:
        out_file.writelines(line for line in in_file if line.strip())
        out_file.truncate()

Note some other issues with your code.

First, PEP 8, the official Python style guide, recommends lower_case_with_underscore for function names, and two blank lines between functions.

The fileExists function does not just test for the existence of the file — it actually checks whether you can open it for reading, which is a more stringent condition, since it also involves file permissions.  But I don't see any reason to check specifically for file existence.  All kinds of I/O errors are possible, such as filesystem permission denial, read-only filesystem, disk quota, or hardware failure.  Furthermore, even if the file exists when you check, it could disappear during the split second between if not fileExists(…) and the real open(…) call.  On top of that, your rmoveEmptyLines function has no way of reporting failure to its caller.  (Printing an error message doesn't count!)  Therefore, the only reasonable approach is to Just Do It, and handle any exception that might occur.

Any open() call should be written using a with block, which will automatically close the file handle when exiting the block.


Bash script to install zsh on different distributions Extreme script newbie here, so please go easy on me!
I wrote a script to install zsh on a number of different Linux distributions. But since I'm so new at this, I'm piecing a lot of different things together and I doubt it's as efficient and tight as possible. Any suggestions are welcome. Thank you!
#!/bin/bash

export ZSHUSER=$USER

if [ -f /etc/os-release ]; then
    . /etc/os-release
    OS=$NAME
    VER=$VERSION_ID
elif type lsb_release >/dev/null 2>&1; then
    OS=$(lsb_release -si)
    VER=$(lsb_release -sr)
fi

echo "Creating fonts directory"
cd /tmp && mkdir fonts && cd $_

echo "Downloading fonts"
wget https://github.com/romkatv/powerlevel10k-media/raw/master/MesloLGS%20NF%20Regular.ttf
wget https://github.com/romkatv/powerlevel10k-media/raw/master/MesloLGS%20NF%20Bold.ttf
wget https://github.com/romkatv/powerlevel10k-media/raw/master/MesloLGS%20NF%20Italic.ttf
wget https://github.com/romkatv/powerlevel10k-media/raw/master/MesloLGS%20NF%20Bold%20Italic.ttf

echo "Entering sudo mode to copy fonts"

if [[ $NAME = "Fedora" ]]; then
sudo su - <<EOF
dnf install -y git zsh zsh-autosuggestions zsh-syntax-highlighting
cd /usr/share/fonts/ && mkdir MesloMGS
cp /tmp/fonts/*.ttf /usr/share/fonts/MesloMGS
fc-cache
rm -r /tmp/fonts
scp -v /mnt/E/Linux/zsh/$NAME/aliasrc $HOME
chown $USER:$USER $HOME/aliasrc
su $USER
EOF

elif [[ $NAME = "void" ]]; then
sudo su - <<EOF
xbps-install -y git zsh zsh-autosuggestions zsh-syntax-highlighting
cp /tmp/fonts/*.ttf /usr/share/fonts/TTF
fc-cache
rm -r /tmp/fonts
scp -v /mnt/E/Linux/zsh/$NAME/aliasrc $HOME
chown $USER:$USER $HOME/aliasrc
su $USER
EOF

elif [[ $NAME = "Debian" ]] || [[ $NAME = "Ubuntu" ]] || [[ $NAME = "Linux Mint" ]]; then
sudo su - <<EOF
apt install -y git zsh zsh-autosuggestions zsh-syntax-highlighting
cd /usr/share/fonts/truetype && mkdir MesloMGS
cp /tmp/fonts/*.ttf /usr/share/fonts/truetype/MesloMGS
fc-cache
rm -r /tmp/fonts
scp -v /mnt/E/Linux/zsh/Debian/aliasrc $HOME
chown $USER:$USER $HOME/aliasrc
su $USER
EOF

elif [[ $NAME = "CentOS Linux" ]]; then
sudo su - <<EOF
yum install -y git zsh zsh-syntax-highlighting
cd /usr/share/fonts/ && mkdir MesloMGS
cp /tmp/fonts/*.ttf /usr/share/fonts/MesloMGS
fc-cache
rm -r /tmp/fonts
scp -v /mnt/E/Linux/zsh/CentOS/aliasrc $HOME
chown $USER:$USER $HOME/aliasrc
git clone https://github.com/zsh-users/zsh-autosuggestions /usr/share/zsh-autosuggestions/zsh-autosuggestions.zsh
su $USER
EOF

else
echo "Operating system not identified"
pause
fi

mkdir ~/powerlevel10k && cd ~/powerlevel10k
git clone --depth=1 https://github.com/romkatv/powerlevel10k.git ~/powerlevel10k
echo 'source ~/powerlevel10k/powerlevel10k.zsh-theme' >> ~/.zshrc

echo "opening terminal to configure zsh"
if [[ $NAME = "Ubuntu" ]] || [[ $NAME = "CentOS Linux" ]] || [[ $NAME = "Linux Mint" ]]; then
    gnome-terminal -q -- zsh
    read -p "press enter to resume"
elif [[ $NAME = "Fedora" ]]; then
    mate-terminal -e zsh
elif [[ $NAME = void ]]; then
    qterminal -e zsh
elif [[ $NAME = Debian ]]; then
    xfce4-terminal -e zsh
fi

sudo su - <<EOF
echo "copying .zshrc file"
scp -v /mnt/E/Linux/zsh/.zshrc $HOME
chown $USER:$USER $HOME/.zshrc
su $USER
EOF

```

if [[ $NAME = "Fedora" ]]; then
elif [[ $NAME = "void" ]]; then
elif [[ $NAME = "Debian" ]] || [[ $NAME = "Ubuntu" ]] || [[ $NAME = "Linux Mint" ]]; then

That looks like a candidate for case "$NAME" in.  However, instead of changing that, let's look at what we're doing:

if [[ $NAME = "Fedora" ]]
then
   dnf install -y git zsh zsh-autosuggestions zsh-syntax-highlighting

Ah, so we're using that to determine which package manager is in use.  It might be better to just test that directly:

if [ -x /usr/bin/dnf ]
then
   dnf install

That has the added benefit that it's plain POSIX shell, not requiring Bash.

Hang on, where did that $NAME come from?  Let's look:

if [ -f /etc/os-release ]; then
    . /etc/os-release
    OS=$NAME
    VER=$VERSION_ID
elif type lsb_release >/dev/null 2>&1; then
    OS=$(lsb_release -si)
    VER=$(lsb_release -sr)
fi

Oh dear, it looks like NAME is assigned in only one of those two paths, and we actually wanted to be using OS (or assigning to NAME and VERSION_ID in the elif branch, and getting rid of OS and VER).
This is why it's a good idea to set -u, so that you get an error when expanding an undefined variable.

echo "Operating system not identified"
pause

That should be echo >&2, since it's an error message.  What's pause?  It's certainly not a common tool, and you probably just want to exit 1 at that point anyway.

Not a big fan of copying those font files into /usr/share (that should be only for files under control of the package manager).  That's what /usr/local is for.

Why are we using scp for a local copy?  Just use plain cp and reduce the dependencies.

Many platforms have a configurable preferred terminal emulator, so use that first if it exists.  For example /usr/bin/x-terminal-emulator on Debian.

On the use of sudo and su (not sure why you feel the need to double up like that!), I'd recommend turning this the other way around, and having a script that you must be root to run, but which drops privileges for the parts where that's possible.  That's certainly better than being repeatedly asked for passwords (in many sudo configurations), which isn't something you want to be training users to do.


Shell POSIX OpenSSL file decryption script follow-up #1 This question is the first follow-up of: Shell POSIX OpenSSL file decryption script

Please read it before you continue reading this. Thank you.

I only have one new problem solved and I would like you to have a look.

That problem being accepting one option -o output_directory.

I have little idea if I did this the best way, but it works.

The code follows:

#!/bin/sh

bold=$(tput bold)
red=$(tput setaf 1)
yellow=$(tput setaf 3)
nocolor=$(tput sgr0)

bold_red=${bold}${red}
bold_yellow=${bold}${yellow}

print_usage_and_exit()
{
    echo "Usage: $0 [-o output_directory] filename_to_decrypt"
    exit 1
}

print_error_and_exit()
{
    echo "${bold_red}$1 Exit code = $2.${nocolor}" 1>&2
    exit "$2"
}

while getopts ":o:" option
do
    case "${option}" in
        o)
          given_output_directory=${OPTARG}
          ;;
        h | *)
          print_usage_and_exit
          ;;
    esac
done

shift $((OPTIND - 1))

[ "$#" -eq 0 ] &&
print_usage_and_exit

[ "$#" -gt 1 ] &&
print_error_and_exit "Multiple files are not supported." 2

[ ! -f "$1" ] &&
print_error_and_exit "The given argument is not an existing file." 3

input_filename="$1"

[ ! -r "$input_filename" ] &&
print_error_and_exit "Input file is not readable by you." 4

input_filepath=$(dirname "$input_filename")

if [ -z ${given_output_directory} ]
then
    output_directory="$input_filepath"
else
    output_directory="$given_output_directory"
fi

[ ! -w "$output_directory" ] &&
print_error_and_exit "Destination directory is not writable by you." 5

filename_extracted_from_path=$(basename "$input_filename")
filename_without_enc_extension="${filename_extracted_from_path%.enc}"

if [ "$filename_extracted_from_path" = "$filename_without_enc_extension" ]
then
    # the file has a different than .enc extension or no extension at all
    # what we do now, is that we append .dec extention to the file name
    output_filename="$output_directory/$filename_extracted_from_path".dec
else
    # the file has the .enc extension
    # what we do now, is that we use the file name without .enc extension
    output_filename="$output_directory/$filename_without_enc_extension"
fi

[ -f "$output_filename" ] &&
print_error_and_exit "Destination file exists." 6

if ! pv -W "$input_filename" | openssl enc -aes-256-cbc -md sha256 -salt -out "$output_filename" -d 2> /dev/null
then
    [ -f "$output_filename" ] && rm "$output_filename"

    print_error_and_exit "Decryption failed." 7
else
    echo "${bold_yellow}Decryption successful.${nocolor}"

    exit 0
fi

Uploaded on GitHub: https://git.io/fxslm

Although print_error_and_exit correctly sends output to standard error and exits with a non-zero status, print_usage_and_exit outputs to standard out.  I recommend explicitly accepting -h (as your case implies, but getopts hasn't been instructed to do), and if the user asks for help, then writing to standard out and returning success, but if the help is provided due to error, then writing to standard error and returning failure:

print_usage()
{
    echo "Usage: $0 [-o output_directory] filename_to_decrypt"
}

while getopts ":ho:" option
do
    case "${option}" in
        o)
            given_output_directory=${OPTARG}
            ;;
        h)
            print_usage
            exit 0
            ;;
        '?')
            print_usage >&2
            exit 1
            ;;
    esac
done

shift $((OPTIND - 1))

[ "$#" -eq 0 ] &&
    print_usage && exit 1

While we're on error reporting, you might want to consider making the exit code be the first argument to print_error_and_exit rather than the last.  That can make the code easier to read, and it also allows you to pass multiple arguments, just like echo which it wraps:

print_error_and_exit()
{
    value=$1; shift
    echo "${bold_red}$* Exit code = $value.${nocolor}" 1>&2
    exit "$value"
}

It's good to see that you use tput to produce the terminal-specific escape codes (if they exist) for highlighting.  But it's probably not worth putting them into variables for a single use each.  I'd get rid of the variables and just write something like

print_error_and_exit()
{
    value=$1; shift
    exec >&2
    tput bold
    tput setaf 1   # red
    echo "$* Exit code = $value."
    tput sgr0
    exit "$value"
}

and (in the success branch)

else
    tput bold
    tput setaf 3   # yellow
    echo "Decryption successful."
    tput sgr0

    exit 0
fi

These tests are probably over-doing it:

[ ! -f "$1" ] &&
print_error_and_exit 3 "The given argument is not an existing file."

input_filename="$1"

[ ! -r "$input_filename" ] &&
print_error_and_exit 4 "Input file is not readable by you."

Unless there's a good reason we can't accept input from a pipe or other non-regular file, just skip the -f test: -r will fail if its argument doesn't exist.  It's also worth including the filename in the message - this is really useful when the user expands a variable into the call, and it doesn't expand to what she expects:

[ -r "$1" ] ||
    print_error_and_exit 3 "$1: not a readable file."

If you really feel you need different exit status for non-existent and existing-but-unreadable files (why?), then consider -e as an alternative to -f.

We should check whether the supplied output directory is actually a directory, before trying to use it:

[ -d "$output_directory" ] ||
    print_error_and_exit 4 "$output_directory: not a directory."
[ -w "$output_directory" ] ||
    print_error_and_exit 5 "Destination directory is not writeable by you."

A shorter way to write this:

if [ -z ${given_output_directory} ]
then
    output_directory="$input_filepath"
else
    output_directory="$given_output_directory"
fi

is to use parameter substitution with - modifier:

output_directory="${given_output_directory:-$input_filepath}"

I think a case helps with matching filename patterns here:

if [ "$filename_extracted_from_path" = "$filename_without_enc_extension" ]
then

I'd write that block as:

filename_extracted_from_path=$(basename "$input_filename")

# Strip .enc suffix from name if it has one, else add .dec suffix
case "$filename_extracted_from_path" in
    *.enc)
        output_filename="${filename_extracted_from_path%.enc}"
        ;;
    *)
        output_filename="$filename_extracted_from_path".dec
        ;;
esac
output_filename="$output_directory/$output_filename"

Note that there's no longer a need for $filename_without_enc_extension.

You definitely need to replace this -f with -e:

[ -f "$output_filename" ] &&
print_error_and_exit 6 "Destination file exists."


Implementing `dropJ` to Drop `JoinList` Elements In the same vein as the (+++) function on JoinList and indexJ on JoinList, I implemented dropJ to drop elements from a JoinList.

Here's my attempt:

dropJ ::(Sized b, Monoid b) => Int -> JoinList b a -> JoinList b a
dropJ _ Empty = Empty
dropJ i jl | i <= 0           = jl
dropJ _ (Single _ _)          = Empty
dropJ i (Append _ left right)
  | i >= leftHt               = dropJ (i-leftHt) right
  | otherwise                 = dropJ i left
    where leftHt = (getSize. size . tag) left

Testing

*JoinList>jlIndex2
Append (Size 3)
               (Single (Size 1) "foo")
               (Append (Size 2)
                   (Single (Size 1) "bar")
                   (Single (Size 1) "baz"))

*JoinList> dropJ 1 jlIndex2
Append (Size 2) (Single (Size 1) "bar") (Single (Size 1) "baz")

*JoinList> dropJ 2 jlIndex2
Single (Size 1) "baz"

*JoinList> dropJ 3 jlIndex2
Empty

*JoinList> dropJ 0 jlIndex2
Append (Size 3)
               (Single (Size 1) "foo")
               (Append (Size 2)
                   (Single (Size 1) "bar")
                   (Single (Size 1) "baz"))

It's fine, but you have a bug in the Append case, as you completely disregard the right part of the tree:

dropJ i (Append _ left right)
  | i >= leftHt               = dropJ (i-leftHt) right
  | otherwise                 = dropJ i left              -- where is right?
    where leftHt = (getSize. size . tag) left

You can fix this if you use +++ here again:

dropJ i (Append _ left right)
  | i >= leftHt               = dropJ (i-leftHt) right
  | otherwise                 = dropJ i left +++ right
    where leftHt = (getSize. size . tag) left

Other than that we could argue about formatting, but your code is fine. It works. Note that you should add more test cases though. A Arbitrary a => Gen (JoinList m a) generator might come in handy for QuickCheck, but that's left as an exercise.


Finding the average of each row in a CSV file I am getting back into python after a year of not doing it at all. I just wrote a bit of code to create a file that looks like this:

1,2
1,1,1,1
-1,0,1
42,17

Then I decided to take the average of each line and put it into a list. For this I wrote the following:

def line_averages():
    out_file = open("data.csv", "w") #create data file
    out_file.write("1,2\n1,1,1,1\n-1,0,1\n42,17") #write numbers into data file
    out_file.close()

    f = open("data.csv", "r") #read data file
    lines = f.readlines()
    avLines= [] #create a list to put averages into
    for line in lines:
        line2 = line.replace("\n", "") #remove /n from lines
        line2 = line2.split(",") #split the lines by commas
        Nlines = [] #create a list to put floats into
        for i in line2:
            i = float(i)
            Nlines.append(i) #put the floats into the list
        sums = sum(Nlines) #total up each line from the file
        avLines.append(sums/len(line2)) #put the averages of each line into a list
    return avLines

I'm pretty happy as this does exactly what I wanted but I can't help but wonder if it can't be shortened/simplified. It seems a bit inefficient to me to have lots of "placeholders" (like  Nlines or line2) which are used to do an operation rather than being able to do the operation directly on them. What would you guys do (if anything) to make this more compact? I'm sure it could be done more succinctly!

Firstly, note that you are not closing the file after opening it. A Better way to read from a file would be to use the with statement. Read What is the python “with” statement designed for? to know more about the with statement.

The code would be,

with open("data.csv") as filename:
    # Code Follows

Other things to note here are

The mode of the open statement is 'r' by default and hence, you don't need to explicitly mention it.
Use a meaningful variable name like filename. This is to help your future self from not asking What is the f?

Now coming to the core of the problem. You can use the fileobject directly to iterate over the lines, You do not need to read the entire file into a list. This is helpful for large files. The code will be

for line in filename:
     # Code follows

The line line2 = line.replace("\n", "") is quite inadequate as there might be other whitespace like a trailing space or a \r. To solve all these issues, you can use str.strip.

stripped_line = line.strip()

Again, Note the name of the variable. Coming to this code block

line2 = line2.split(",")
Nlines = []
for i in line2:
    i = float(i)
    Nlines.append(i)
sums = sum(Nlines)

You can solve this issue using split and a generator expression.

stripped_line  = stripped_line .split(',')
line_sum = sum(float(i) for i in stripped_line)

Alternatively you can use map

line_sum = sum(map(float,stripped_line))

(You can use literal_eval to directly do sum(literal_eval(stripped_line)), But the string functions are the right (and preffered way))

Hence the complete code will be reduced to (excluding the data creation part)

def line_averages():
    with open("test.csv", "r") as filename:
        avLines= []
        for line in filename:
            stripped_line  = line.strip()
            stripped_line  = stripped_line.split(',')
            line_sum = sum(float(i) for i in stripped_line )
            avLines.append(line_sum/len(stripped_line ))
        return avLines

Code Golfing the solution, We can use a List Comprehension. Read this to kow more about the list comprehensions.

def line_averages():
    with open("test.csv", "r") as filename:
        lines = [[float(i) for i in line.strip().split(',')] for line in filename]
        return [sum(i)/len(i) for i in lines]


Chemistry titration calculator This is a Perl program intended for my chemistry class. Since I am just learning to write in Perl, which is my first language, can you tell me if you think that this is well written? If not, could you tell me what is wrong with it or what could be improved?

## Name:Titration
## Beginning of Program

#Input Variables
system "clear";     # Clear the Screen

print "Milliliters Solute: ";
chomp ($milliliters_solute=<>);

print "Milliliters Solvent: ";
chomp ($milliliters_solvent=<>);

print "Molarity of Solvent: ";
chomp ($molarity_solvent=<>);

print "Moles of Solute: ";
chomp ($moles_solute=<>);

print "Moles of Solvent: ";
chomp ($moles_solvent=<>);

system "clear";     # Clear the Screen

#Calculate Answer
$liters_solute=$milliliters_solute/1000;
$liters_solvent=$milliliters_solvent/1000;
$molarity=($liters_solute/$liters_solvent)*($molarity_solvent/1)*($moles_solvent/$moles_solute);

#Print Answer
system "clear";     # Clear the Screen
print "Molarity is $molarity \n";

## End of Program

One suggestion, use

use warnings;

use strict;

Will force you to be a better perl programmer, by enforcing more rigidity to your scripts, and have the intepreter catch any nasty issues that might be otherwise hard to debug.


RSS feed class in C# How can I make this more readable?

public class RssFeed
{

    string url = "http://www.pwop.com/feed.aspx?show=dotnetrocks&filetype=master";

    public string responseBody;

    private string DATE_FORMAT = "ddd, dd MMM yyyy HH:mm:ss \\E\\D\\T";

    public Story[] Getstories()
    {
        getContent();
        var stories = Parse(responseBody);
        return GetMostRecent5(stories, new StoryComparer());
    }

    public void getContent()
    {
        HttpWebRequest req = WebRequest.Create(url) as HttpWebRequest;
        WebResponse response = req.GetResponse();
        StreamReader sr = new StreamReader(response.GetResponseStream());
        responseBody = sr.ReadToEnd();
    }

    static Story[] Parse(string content)
    {
        var items = new List<string>();
        int start = 0;
        while (true)
        {

            var nextItemStart = content.IndexOf("<item>", start);
            var nextItemEnd = content.IndexOf("</item>", nextItemStart);
            if (nextItemStart < 0 || nextItemEnd < 0) break;

            String nextItem = content.Substring(nextItemStart, nextItemEnd + 7 - nextItemStart);
            items.Add(nextItem);
            start = nextItemEnd;
        }

        var stories = new List<Story>();
        for (byte i = 0; i < items.Count; i++)
        {
            stories.Add(new Story() {
                title = Regex.Match(items[i], "(?<=<title>).*(?=</title>)").Value,
                link = Regex.Match(items[i], "(?<=<link>).*(?=</link>)").Value,
                date = Regex.Match(items[i], "(?<=<pubDate>).*(?=</pubdate>)").Value
            });
        }

        return stories.ToArray();
    }

    private static T[] GetMostRecent5<T>(T[] stories, IComparer<T> comparer)
    {
        List<T> recentStories = stories.Take(5).ToList();

        recentStories.Sort(comparer);

        var recentStoriesArray = new T[5];
        for (int i = 0; i <= 5; i++)
        {
            recentStoriesArray[i] = recentStories[i];
        }
        return recentStoriesArray;
    }

    public class Story
    {
        public string title;
        public string link;
        public string date;

        public override bool Equals(object obj)
        {
            return this.Equals(obj as Story);
        }

        public bool Equals(Story story)
        {
            return this.link == story.link;
        }
    }

    private class StoryComparer : IComparer<Story>
    {
        public int Compare(Story x, Story y)
        {
            if (x.Equals(y))
                return 0;
            //redundant else
            else
                return x.date.CompareTo(y.date);
        }
    }
}

Not that ugly. I've seen ugly code, and trust me, this is not ugly.

However, this is kind of a "God Class" that has more than a single responsibility, hence it violates SRP principle.

How to improve

Separate the class to:

ContentFetcher, which is responsible for fetching only

StoryParser, which parses the content and creates instances of your object model (Story[])

Both of the above should implement the Try.. pattern, so that the single method in the ContentFetcher has the following signature:

bool TryFetch(out string content){ /* code */ }

The same goes for StoryParser.

As for the fetcher itself

You can choose if the URL is either injected in the constructor, or is part of the method signature. Modern OOP coding is biased towards the constructor technique.
The code should implement the using (...) { ... } pattern for classes that implement the IDisposable interface, and be wrapped in a try-catch what so ever.

As for the parser itself

since rss is a format on top of xml, I don't think it's a good idea to parse it using regular expressions. Let the framework do the parsing by using either XmlDocument or XDocument. Or use an external library to parse the string for you (hopefully it's implemented by the above techniques, and not by using Regexes).
a try-catch might be needed here, too

Putting it all together

So finally, the class you have should use the above two classes (a fetcher and a parser), and then, getting the stories from the feed should look like this:

public Story[] Getstories()
{
    string content;
    if (!this.fetcher.TryGetContent(out content)) return null;
    Story[] stories;
    if (!this.parser.TryParse(out stories)) return null;
    return GetMostRecent5(stories, new StoryComparer());
}

As you can see, null is a special value for failure. You can change that to TryGetStories which returns a boolean and and out parameter if you want to avoid the special value of null.

The actual instances of fetcher and parser can be created by the class itself (in the constructor) and/or be injected to the class by the constructor. Let's see the injection pattern:

public class RssFeed
{
    private readonly ContentFetcher fetcher;
    private readonly StoryParser parser;

    public RssFeed(ContentFetcher fetcher, StoryParser parser)
    {
        if (null == fetcher) throw new ArgumentNullException("fetcher");
        if (null == parser) throw new ArgumentNullException("parser");
        this.fetcher = fetcher;
        this.parser = parser;
    }
}

As a final step of refactoring, you can extract the preparing code to its own method, moving the parsed stories to be part of the class' state:

private Story[] stories = null;

private bool Prepare()
{
    if (this.stories != null) return true;
    string content;
    if (!this.fetcher.TryGetContent(out content)) return false;
    Story[] temp;
    if (!this.parser.TryParse(out temp)) return false;
    this.stories = temp;
    return true;
}

public Story[] Getstories()
{
    if (!Prepare()) return null;
    return GetMostRecent5(this.stories, new StoryComparer());
}

Good luck!


Bash function to dynamically move N directories above the current directory up moves N directories above the current directory (executes "cd ..""
N times)
When executed without input values it moves only one
directory above the current directory
When executed with input
values, up uses the first input value (checking to see if it is a
positive integer)to move above the current directory the specified
number of directories

up() {
    if [ -z "$1" ]
        then
            eval "cd .."
    else
        if [ "$1" -eq "$1" ]
            then
                if [ "$1" -gt "0" ]
                    then
                        for i in `seq 1 $1`;
                        do
                            eval "cd .."
                        done
                            eval "pwd"
                else
                    echo "First argument is not a positive integer"
                fi
        else
            echo "First argument is not an integer"
        fi
    fi
}

I did consider building a cd ../../.. etc., chain which would execute after the for loop builds the required string and would also preserve the usefulness of cd - but this version was slightly cleaner.

The [ "$1" -eq "$1" ] thing is one way to check if a value is an integer.

If you're writing this as a shell function, then you don't need to eval.

The indentation could be done more idiomatically.  By using elif, you can eliminate one level of indentation and make the code structure clearer.

up() {
    if [ -z "$1" ]; then
        cd ..
    elif [ "$1" -eq "$1" ]; then
        if [ "$1" -lt 0 ]; then
            echo "First argument is not a positive integer"
        else
            for i in `seq 1 $1`; do
                cd ..
            done
            pwd
        fi
    else
        echo "First argument is not an integer"
    fi
}

Your question is very similar to this one.  The main problem, as you have already noted in your own comment, is that by doing multiple cd .. in sequence instead of one cd ../../.., you would be inserting many hops into the directory history.  Then, cd - or referencing $OLDPWD wouldn't work as expected.


Fetch records that do not get matched in another table I'm trying to fetch records from table PaymentToken if its AccountNumber matched but AccountToken wasn't:

select DISTINCT DTR_PT.Id, DTR_PT.AccountNumber, DTR_PT.CardNumber, DTR_PT.ExpirationDate, DTR_PT.CardHolderName, DTR_PT.AccountToken, DTR_PT.CreatedDate, DTR_PT.Active
from Dev_NeoGateway_DTR.dbo.PaymentToken as DTR_PT
JOIN Dev_NeoGateway_DTA.dbo.DTA_Customer as DTA_C on DTA_C.Id = DTR_PT.AccountNumber
WHERE DTR_PT.AccountToken NOT IN (select DTA_PI.CardToken FROM  Dev_NeoGateway_DTA.dbo.DTA_PaymentInstrument as DTA_PI )

Because of the WHERE clause, this query takes a lot of time as it queries all the table again and again.

Linting & Naming:

Your query is overflowing the box to the right. That's not good because it means I (and you) have to scroll to see it completely. Additionally while you use aliases, the table aliases are unnecessarily shortened.

After renaming them a little and reformatting the query looks as follows:

SELECT DISTINCT
    PaymentToken.Id
    , PaymentToken.AccountNumber
    , PaymentToken.CardNumber
    , PaymentToken.ExpirationDate
    , PaymentToken.CardHolderName
    , PaymentToken.AccountToken
    , PaymentToken.CreatedDate
    , PaymentToken.Active
FROM Dev_NeoGateway_DTR.dbo.PaymentToken AS PaymentToken
JOIN Dev_NeoGateway_DTA.dbo.DTA_Customer AS Customer
    ON Customer.Id = PaymentToken.AccountNumber
WHERE PaymentToken.AccountToken NOT IN (
    SELECT PaymentInstrument.CardToken
    FROM Dev_NeoGateway_DTA.dbo.DTA_PaymentInstrument AS PaymentInstrument
)

Optimizing the query

From what I can see, the Customer table is not used in your query. Joining it only slows down your query.
Other than that, the query is very straightforward. If you could restrict the subselect's result set, you could achieve better performance.

Other than that, the only thing I'd expect to improve perfomance are indices on the involved columns


AggregateException and Flatten I have the following method that has been shortened for the sake of this review. The actual method has many more tasks. Essentially what I am trying to do is to create many tasks, fire them off, wait until they are all finished, and handle any exceptions generated. Does this look correct?

public static void RefreshCoaterCaches()
{
    try
    {
        CicApplication.CoaterDataLock.EnterWriteLock();

        List<Task> alarmTasks = new List<Task>();

        alarmTasks.Add(Task.Factory.StartNew(() =>
        {
            using (DistributorBackpressure44Logic logic = new DistributorBackpressure44Logic())
                logic.RefreshDistributorBackpressure44Cache();
        }));

        alarmTasks.Add(Task.Factory.StartNew(() =>
        {
            using (DeltaPressureLogic logic = new DeltaPressureLogic())
                logic.RefreshDeltaPressureCache();
        }));

        Task.WaitAll(alarmTasks.ToArray());
    }
    catch (System.AggregateException exception)
    {
        CicServerLogic.HandleUnexpectedAggregateException(exception.Flatten(), string.Empty);
    }
    catch (System.Exception exception)
    {
        CicServerLogic.HandleUnexpectedException(exception);
    }
    finally
    {
        CicApplication.CoaterDataLock.ExitWriteLock();
    }
}

I then have this method to handle any AggregateException thrown:

internal static void HandleUnexpectedAggregateException(AggregateException aggregateException)
{
    try
    {
        foreach (Exception exception in aggregateException.InnerExceptions)
        {
         ...
         ...
         Do some stuff with the current exception, like logging etc.
         ...
        }
    }
    catch (Exception)
    {
    }
}

I suggested to think about following:

Extract every step into the separate asynchronous method
Think about more careful locking technique. Asynchronous operations by its nature could be long running and using global write lock can degrade performance dramatically. Maybe its possible acquire locks only during processing results.
Global exception handling: in general global exception handling is a code smell. Its really hard to create generic logic for different operations. I suggested to use such techniques with some tools like MEF, but avoid using its manually.

Here slightly modified version of your code:

private static Task GetDistributorBackpressure44LogicAsync()
{
  Task.Factory.StartNew( () =>
  {
    using (DistributorBackpressure44Logic logic = new DistributorBackpressure44Logic())
                logic.RefreshDistributorBackpressure44Cache();
  }
}

private static Task DeltaPressureLogicAsync()
{
  Task.Factory.StartNew(() =>
  {
    using (DeltaPressureLogic logic = new DeltaPressureLogic())
                logic.RefreshDeltaPressureCache();
  }
}

public static void RefreshCoaterCaches()
{
    try
    {
        CicApplication.CoaterDataLock.EnterWriteLock();

        var t1 = GetDistributorBackpressure44LogicAsync();
        var t2 = DeltaPressureLogicAsync();

        // You can switch back to you list-based approach if you'll have
        // much more other operations
        Task.WaitAll(t1, t2);
    }
    catch (System.AggregateException exception)
    {
        // I'm really not sure that its a good idea to create "generic"
        // exception handling scheme. Maybe you should handle them manually right here.
        // And what is string.Empty is all about?
        CicServerLogic.HandleUnexpectedAggregateException(exception.Flatten(), string.Empty);
    }
    // You should not catch System.Exception thats why I removed it
    finally
    {
        CicApplication.CoaterDataLock.ExitWriteLock();
    }
}

Now, about your generic exception handler (I mentioned already that this technique can lead to errors hiding and complicated "generic" logic) (see my comments right in the code):

internal static void HandleUnexpectedAggregateException(AggregateException aggregateException)
{
    // What are we afraiding of, why we're using try/catch here?
    // We should now swallow unknown exception because it can lead to
    // hiding bugs in our code!
    try
    {
        // This technique is ok
        foreach (Exception exception in aggregateException.InnerExceptions)
        {
         ...
         ...
         Do some stuff with the current exception, like logging etc.
         ...
        }
    }
    catch (Exception)
    {
       // I strongly not recommended to use empty catch blocks!!
    }
}


Helper Method for Generating Stubbed ICommand in Rhino Mocks This helper method is designed to make stubbing ICommands simpler.

Do you see any problems with this implementation?

public static ICommand GenerateCommandStub(
    Action<object> executeAction = null,
    Func<object, bool> canExecuteFunc = null)
{
    var commandStub = MockRepository.GenerateStub<ICommand>();
    commandStub.Stub(command => command.Execute(Arg<object>.Is.Anything))
        .Do(executeAction ?? (_ => { }));
    commandStub.Stub(command => command.CanExecute(Arg<object>.Is.Anything))
        .Do(canExecuteFunc ?? (_ => true));
    return commandStub;
}

It looks fine to me, although I would personally prefer to have method chaining rather than optional parameters just for semantic purposes. Are your arguments meant to be weakly typed (using object)? If I could, I would change that to something strongly typed, but that is out of scope for this question.


Mouseover effects for social network icons Hey so I have written some code out in jQuery, but it seems to violate DRY. I'm not too familiar with JavaScript or jQuery, so I was just asking for some advice on how to optimize this code:

$("#twitter").rotate({
  bind:
  {
    mouseover : function() {
    $(this).rotate({animateTo:360});

    $("#twitter").fadeTo(function() {
        $(this).load(function() { $(this).fadeTo(); });
        $(this).attr("src", "images/logos/Twitter-Color.png");
    });
  },
  mouseout : function() {
    $(this).rotate({animateTo:0})
    $("#twitter").fadeTo(function() {
        $(this).load(function() { $(this).fadeTo(); });
        $(this).attr("src", "images/logos/Twitter-White.png");
    });
  }
  }
});

$("#linkedin").rotate({
  bind:
  {
    mouseover : function() {
    $(this).rotate({animateTo:360});

    $("#linkedin").fadeTo(function() {
        $(this).load(function() { $(this).fadeTo(); });
        $(this).attr("src", "images/logos/Linkedin-Color.png");
    });
  },
  mouseout : function() {
    $(this).rotate({animateTo:0})
    $("#linkedin").fadeTo(function() {
        $(this).load(function() { $(this).fadeTo(); });
        $(this).attr("src", "images/logos/Linkedin-White.png");
    });
  }
  }
});
$("#facebook").rotate({
  bind:
  {
    mouseover : function() {
    $(this).rotate({animateTo:360});

    $("#facebook").fadeTo(function() {
        $(this).load(function() { $(this).fadeTo(); });
        $(this).attr("src", "images/logos/Facebook-Color.png");
    });
  },
  mouseout : function() {
    $(this).rotate({animateTo:0})
    $("#facebook").fadeTo(function() {
        $(this).load(function() { $(this).fadeTo(); });
        $(this).attr("src", "images/logos/Facebook-White.png");
    });
  }
  }
});

Styling and readability

Your indentation is incorrect. This makes it harder to read, and makes people assume that "mouseout" belongs to a different Object that it actually does.

Whitespace is not always consistent (e.g. $(this).rotate({animateTo:360}); versus other uses of Objects.

DRY

As you correctly noted, this code repeats itself quite a bit.

The code seems to only differ in id and logo name. You have various options:

You create a function that sets this. I am not entirely sure what you would name it, but something like setRotationEffects( id, logoName ); might do the trick.
You can use named functions for mouseout and mouseover. This will leave you with a basic boilerplate for each id:

$("#linkedin").rotate({
  bind:
  {
    mouseover : whatEverYouCallIt,
    mouseout  : whatEverOtherThingYouCallIt
  }
});

This might be more optimal if you expect to have other similar constructs with different effects.

Similarly:

The id in the mouseover and mouseout handler is not needed. In the handler you can use $(this) instead. This leaves you with only the name of the logo.

You can consider pulling this name from the existing logo. After all, the logo was set to a particular name in the first place. This makes all these identical.
You can use an anonymous function containing a function call with parameters (function() { doTheThings( id, logoName ); }.

Note: It might be just me, but I link "optimize" to "performance", and DRY has nothing to do with performance. We use DRY to make maintenance easier, not to make code outperform "WET" code.


Wikipedia indexer and shortest link finder I have the following code, how can I make it more efficient? Also, it doesn't always find the shortest route. (See Cat -> Tree)

import urllib.request, re, json

try:
    file = open("urls.json", "r")
    urls = json.loads(file.read())
    file.close()

    file = open("links.json", "r")
    links = json.loads(file.read())
    file.close()
except:
    urls = ["http://en.wikipedia.org/wiki/Cat"]
    links = [[-1, 0]]

for url in urls:
    try:
        print("Trying: " + url)
        request = urllib.request.Request(url, headers = {"User-Agent" : "DanBot/2.0"})
        file = urllib.request.urlopen(request)
        source = file.read().decode("utf-8")
    except Exception as e:
        print("Failed: " + url)
        while url in urls:
            urls.remove(url)

    suburls = re.findall("href=[\"']?([^\"' >]+)", source)

    for suburl in suburls:
        if len(suburl) > 6 and suburl[:7] == "http://" or suburl[:8] == "https://":
            fullurl = suburl
        elif len(suburl) > 1 and suburl[:2] == "//":
            fullurl = "http:" + suburl
        elif len(suburl) > 0 and suburl[0] == "/":
            fullurl = "/".join(url.split("/")[:3]) + suburl
        else:
            fullurl = "/".join(url.split("/")[:-1]) + "/" + suburl

        if fullurl[:29].lower() == "http://en.wikipedia.org/wiki/" and "#" not in fullurl[29:] and ":" not in fullurl[29:]:
            if fullurl not in urls:
                urls.append(fullurl)
            if [urls.index(url), urls.index(fullurl)] not in links:
                links.append([urls.index(url), urls.index(fullurl)])
        else:
            continue

    file = open("urls.json", "w")
    file.write(json.dumps(urls))
    file.close()

    file = open("links.json", "w")
    file.write(json.dumps(links))
    file.close()

input()

That program creates the index, and this one finds the links:

import urllib.request, re, json

try:
    file = open("urls.json", "r")
    urls = json.loads(file.read())
    file.close()

    file = open("links.json", "r")
    links = json.loads(file.read())
    file.close()
except:
    print("Error: No link or url file found")
    input()
    exit()

starturl = "http://en.wikipedia.org/wiki/" + input("Start at: http://en.wikipedia.org/wiki/")
endurl = "http://en.wikipedia.org/wiki/" + input("End at: http://en.wikipedia.org/wiki/")

if starturl not in urls or endurl not in urls:
    print("Error: One or more urls not found in index")
    input()
    exit()

print("")

route = []
selection = [[-1, urls.index(starturl)]]

done = False
for link in selection:
    for link2 in links:
        if link[1] == link2[0]:
            selection.append(link2)
        if link2[1] == urls.index(endurl) and link2 in selection:
            done = True
            break
    if done:
        break

if not done:
    print("Error: No link found between pages")
    input()
    exit()

selection = list(reversed(selection))
route = [selection[0]]
for link in selection:
    if link[1] == route[-1][0]:
        route.append(link)
route = list(reversed(route))

print("Route:")
for link in route:
    print(urls[link[1]])

input()

** this won't make your code more efficient, but is still something you should consider doing

With all your file opening, you should do it as:

with open('filename','r') as fin:
    urls/links = json.loads(fin.read())

while you have the try/catch block, your exception handler is not checking and closing open file handles, which is what the with context does


Sorting algorithms - Relocation Counter I want to count the total relocations for each of these sorting algorithms. From the results it seems my code is correct, but I need someone to confirm it. Is the counter variable placed in the right position?

int bubblesort(int *p)
{
        int i,j,temp,relocations=0;
        for(i=1;i<N;i++)
                for(j=N-1;j>=i;j--)
                        if(p[j-1]>p[j])
                        {
                                temp=p[j-1];
                                p[j-1]=p[j];
                                p[j]=temp;
                                relocations++;
                        }
        return relocations;
}

int straightSelection(int *p)
{
        int k,i,min,j,relocations=0;
        for(i=0;i<N-1;i++)
        {
                k=i;
                min=p[i];
                for(j=i+1;j<N;j++)
                {
                        if(p[j]<min)
                        {
                                k=j;
                                min=p[j];
                        }
                }
                p[k]=p[i];
                p[i]=min;
                relocations++;
        }
        return relocations;
}

int straightInsertion(int *p)
{
        int x,i,j,relocations=0;
        for(i=2;i<=N;i++)
        {
                x=p[i];
                p[0]=x;
                j=i-1;
                while(x<p[j])
                {
                        p[j+1]=p[j];
                        j=j-1;
                        relocations++;
                }
                p[j+1]=x;
                relocations++;
        }
        return relocations;
}

int quicksort(int left, int right, int *p)
{
        int relocations=0;
        int i,j,mid,x,temp;

        if(left<right)
        {
                i=left;
                j=right;
                mid=(left+right)/2;
                x=p[mid];
                while(i<j)
                {
                        while(p[i]<x)
                                i++;
                        while(p[j]>x)
                                j--;
                        if(i<j)
                        {
                                if(p[i]==p[j])
                                {
                                        if(i<mid)
                                                i++;
                                        if(j>mid)
                                                j--;
                                }
                                else
                                {
                                        temp=p[i];
                                        p[i]=p[j];
                                        p[j]=temp;
                                        relocations++;
                                }
                        }
                }
                relocations += quicksort(left,j-1,p);
                relocations += quicksort(j+1,right,p);
        }
        return relocations;
}

Your quicksort has a bug
Your midpoint calculation can overflow for large arrays:
mid=(left+right)/2;

Consider the following example code (C++ but it applies to C as well):
int x = std::numeric_limits<int>::max()/2 + 1;
unsigned int y = (x + x)/2;
cout<<"x:"<<x<<" y:"<<y<<endl; // x:1073741824 y:3221225472

The correct way to calculate this is like this:
mid = left + (right - left)/2;

which will not overflow if right>left for any right and left.


Angular 2 / rxjs chaining HTTP calls I have a registration form. Step 1 user creates an account. As user creates an account I need to authenticate user with the created account.

in my account.service.ts I have following.

Is this the best way to handle this? and how can improve this to handle errors from 2 different http calls?

  public createAccount(reg: CreateUserRequestModel): Observable<any> {
    return this.apiService.post('api/register/account', reg)
      .flatMap(()=>{
        return this.authenticationService.authenticate(reg.emailAddress, reg.password)
      }).map((response)=>{
        return response.json()
      })
      .catch((error: any) => {
        return Observable.throw(error.json());
      });
  }

public createAccount(registrationRequest: CreateUserRequestModel): Observable<SomeType> {
  return this.apiService.post('api/register/account', registrationRequest)
    .flatMap(() => this.authenticationService.authenticate(registrationRequest.emailAddress, registrationRequest.password))
    .map(response => <SomeType>response.json())
    .catch(error => Observable.throw(error.json()));
}

@Tushar made a good point on code compactness, which can be applied to all arrow functions in the code.
TypeScript is about types. Try to not use any in return type definitions. Do define some return type and cast the result to it (see SomeType).
I'm not sure if you really want to do .catch(error => Observable.throw(error.json())) where it happens now or move it to the consumers side (e.g. .subscribe(() => {...}, error => this.handle(error))
reg is a bad name. Spell things out.


Counting numbers whose digits all go up or down I have created a program which takes a value x and uses this value to create a power of ten value like so 10 ** x.

This has then been used as the maximum number of a range 1..(10 ** x), which has been expanded using the splat * method to create an array of numbers, and then assigned to the variable num_array.

Using the values in this array, I want to find the total number of increasing and decreasing integers within this range. These are integers containing digits that either ascend or descend with each proceeding digit.

For example, if

x = 3

then

num_array = [*1..(10**3)] #=> num_array = [1, 2, 3, 4, .. 998, 999, 1000]

Then numbers like 234 which is ascending and 987 which is descending should be counted. Numbers like 555 fall into both categories so also should be counted but numbers like 482 should not because it doesn't follow either rule.
The each method is used to evaluate which integers follow this rule in the range.

My problem is as the powers of ten increase with each method call at the end of the program, it gets slower and slower to return the end result until it calls total_nums(6) which then takes too long.

Is there a general rule of thumb to help process really large arrays like this so it's much faster for your program to return the end result?
What would you say are your preferred methods for such an approach like this?
And would something like Memoization work here?

def total_nums(x)
   return 0 if x == nil
   return 1 if x == 0

   num = 10 ** x
   num_array = [*1..num]
   total = 0

   num_array.each do |i|
         if i == i.to_s.split("").sort.join.to_i || i == i.to_s.split("").sort.reverse.join.to_i
         total +=1
         end
   end

   return total

end

total_nums(0) #=> 1
total_nums(1) #=> 10
total_nums(2) #=> 100
total_nums(3) #=> 475
total_nums(4) #=> 1675
total_nums(5) #=> 4954
total_nums(6) #=> 12952

To make Ruby internally use a loop instead of an Array you may want to use 1.upto(10 ** x) that is an Enumerator.

But I doubt the "HUGE array" is the performance issue here.

First of all Ruby says to do total = .count{ ... } instead of total = 0; .each{ total += 1 if ... }
Then do this thing only once and store in a variable t = i.to_s.split("").sort -- x2 faster
Then use .chars instead of .split("") -- x4 faster

Then conversions between Integer and String seem to be unnecessary:

def total_nums x
  return 0 if x == nil
  return 1 if x == 0

  1.upto(10 ** x).count do |i|
    s = i.to_s.chars
    t = s.sort
    s == t || s == t.reverse
  end
end

Note: probably you want either return 0 if x == 0 or start the loop from 0, not 1.


I've finally found a satisfactory way to create classes on JavaScript. Are there any cons to it? Depending on external OO libraries is really bad, using prototypes/new has some limitations, using only hashes has others. Dealing with classes was always a pain in JavaScript and it didn't help that every library has it's own way to deal with ti. But since some time I've finally "set" with this design:

function my_class(extend,init){
    var self = extend || {};

    var private = 5;
    self.public = 5;
    self.get_private = function(){
        return private;
    };

    for (var key in init) self[key] = init[key];
    return self;
};

Tests:

var instance = my_class();

// publics and privates
console.log(instance.public);
console.log(instance.get_private());

// methods never lose context
(function(method){
    console.log(method());
})(instance.get_private);

// multiple inheritance trivial
var random_obj = {a:1,b:2,c:3};
my_class(random_obj); // now random_obj inherits my_class

// serializable, store in a DB and recover it with:
var serialized = JSON.stringify(instance);
var instance = my_class({},JSON.parse(serialized));
console.log("Instance: ",instance);

I'm surprised how this simple design solves all the problems of the other approaches. Prototypes make multiple inheritance hard, hashes won't allow privates, there's no more the "this" binding issue, instances are JSON-serializable, all that without depending on external libraries. Is there any cons to this method, or something that is hard to do with it - or can I just go and start preaching about it?

What about regular inheritance? Is extend supposed to be the superclass?

foo = { foo: "bar" };

a = my_class(foo);
b = my_class(foo);

a.foo = "baz";
console.log(b.foo);

--> "baz";

I would expect a and b to have their own state, yet they share a single foo property.


Lazy Resource Loading class in C++ Recently I was studying about the benefits and uses of std::optional in C++17 (Bartek's Blog) and there I saw a line that said "std::optional can be used for Lazy Loading of Resources" . Upon digging a little bit, I saw that C++ does not have native support for Lazy evaluation. So I just gave it a try and wrote the following code for lazy evaluation in C++.

Lazy.ixx - Visual Studio(Experimental :Modules)

To compile this (VS2019):

cl /experimental:module /EHsc /TP /MD /std:c++latest Lazy.ixx /module:interface /module:output Lazy.pcm /Fo: Lazy.obj /c

#include<optional>
#include<functional>
export module Lazy;

export namespace gstd
{
template<typename T>
class Lazy
{
   std::optional<T> m_resource;
   std::function<T()>m_ctor_func;
public:
   Lazy(std::function<T()>ctor_func)
   {
       m_ctor_func=ctor_func;
   }
   std::optional<T> operator ->()
   {
     //If resource is not initialized(i.e Ctor not invoked/ first time use)
    if(!m_resource)
    {
        m_resource=m_ctor_func();
    }
     return m_resource;
   }
};
}

Now sorry for using modules! (I just love them). To use this class it's pretty simple:

//main.cpp
#include<iostream>
import Lazy;

// A simple class called Resource
class Resource
{
  public:
  Resource()
  {
    std::cout<<"Welcome to new C++\n";
  }
  Resource(int a, int b)
  {
    std::cout<<"This also works : "<<a<<" "<<b<<"\n";
  }
  void func()
  {
    std::cout<<"Resource::func()\n";
  }

};

int main()
{
   gstd::Lazy<Resource>resx([](){ return Resource(4,5); });
   std::cout<<"Before construction\n";

   //Some code before using the resource

   resx->func();
   std::cin.get();
   return 0;
}

To compile this:

cl /experimental:module /module:reference Lazy.pcm /std:c++latest /TP /MD /EHsc /c /Fo: main.obj main.cpp

Get the executable by linking those two files:

cl main.obj Lazy.obj

I am pretty much new to programming so please bear me with the silly mistakes. One problem that can be seen at the first glance is this class (Lazy) is not thread-safe. Implementing that would be easy with mutexes. But, other than that, how can I improve my code?

Preface

I am using Apple Clang 10 and the following commands to build.

clang++ -std=c++17 -fmodules-ts --precompile Lazy.cppm -o Lazy.pcm
clang++ -std=c++17 -fmodules-ts -c Lazy.pcm -o Lazy.o
clang++ -std=c++17 -fmodules-ts -fprebuilt-module-path=. Lazy.o main.cpp

I'm using C++ 17 since that's the tag on the question, although I believe MSVC /std:c++latest corresponds to the parts of C++ 20 that are already implemented on MSVC.

I had one compilation error when I first tried this:

In file included from main.cpp:3:
Lazy.cppm:10:21: error: definition of 'optional' must be imported from module 'Lazy.<global>'
      before it is required
   std::optional<T> m_resource;
                    ^
main.cpp:26:24: note: in instantiation of template class 'gstd::Lazy<Resource>' requested here
   gstd::Lazy<Resource>resx([](){ return Resource(4,5); });

I solved it by adding #include <optional> in main.cpp, but maybe there's a better way.  Please let me know if there is a better solution.  I have only toyed around with modules so I'm far from an expert.

Fix your indentation

Minor nitpick: use consistent indentation.
You have 4 spaces for most of it (which is fine) but 1 space in a few spots.
And 0 spaces in a few spots too.  There are tools that can do this for you automatically (although it's pretty
easy to just do it by hand).

Consider using operator T

C++ can implicitly convert your object into a T:

template <typename T>
struct Lazy {
    operator const T&() {
        ...
    }
};

int main() {
    Lazy<int> n(...);
    return n;
}

Consider making access const

When you access an object, you don't expect to modify it.  In other words, you should be able to write:

Lazy<int> const n(...);
int x = n + 2;

This implies an object that looks something like

template <typename T>
struct Lazy {
    operator const T&() const {
        ...
    }
private:
    std::optional<T> mutable opt;
};

Think about what kind of functions you want to support

As you've written it now, you have two std::functions: the argument to the ctor and the one in the object.  You should at most have one.  You could use std::move.

Lazy(std::function<T()> ctor_func)
    : m_ctor_func(std::move(ctor_func)) // at least do this!
{}

At least use a member initializer list instead of initialization by assignment.

It may be preferable to get rid of std::function completely and instead use a template parameter.
This would allow you to get rid of all the copying.
You could even have non-copyable types in the function object.
You could even go overboard and deduce the stored type based on the templated function.  Then you could just write:

Lazy const n([nc=NonCopyable()]{ return 1; });

No template arguments!  It's up to you to decide whether this is a good idea, but it may be a good exercise if you
are new to templates.

Use optional::emplace

... instead of assignment.


Webscraping cryptocurrency In this class I made a webscraper for a cryptocurrency website to get the name and the price of each currency. There are maybe API's that I can also use, but I thought it is better to gain some knowledge doing it with scraping.

import requests
from bs4 import BeautifulSoup
import json

class Coins:

    def __init__(self):
        self.url = "https://www.coinbase.com/price"
        self.new_list = []

    # Check if I get a 200 status code and if so, create instance from bs4
    def get_response(self):
        self.response = requests.get(self.url)

        if self.response:
            print("Access granted")
            self.soup = BeautifulSoup(self.response.text, 'lxml')
        else:
            print("Error")

    # Select the class and get the data
    def get_data(self):
        info = self.soup.find_all(class_="Header__StyledHeader-sc-1q6y56a-0 hZxUBM TextElement__Spacer-sc-18l8wi5-0 hpeTzd")
        # Here, I only want to get the name and the price of the crypto, I don't want to get other information and thats why I am using the modulo operator.
        for x,e in enumerate(info):
            if x % 3 == 2:
                # print("I dont need this information")
                pass
            else:
                self.new_list.append(e.text)

        return self.new_list

    # From the information that I got and appended into a list, make a dict
    def make_dict(self):
        self.my_dict = {self.new_list[x]:self.new_list[x + 1] for x in range(0, len(self.new_list), 2)}
        return self.my_dict

    # Save the data into a json file
    def save_data(self):
        with open('data.json','w') as output:
            json.dump(self.my_dict, output, indent=2)

if __name__ == '__main__':
    test = Coins()
    test.get_response()
    test.get_data()
    test.make_dict()
    test.save_data()

One of my concern is: I try to access variables inside a method, is using self a good idea? For example in the method save_data(). I wanted to pass the function get_data() in json.dump() but it didn't work out. So I changed in make_dict() the variable to self.my_dict to access it in the method save_data() and it worked out, but is it a good practice or not at all?

This is my first time actually working with classes. I appreciate any feedback to improve myself.

Here is an alternate implementation:

#!/usr/bin/env python3

from bs4 import BeautifulSoup
from pprint import pprint
import re
import requests

class Coins:
    table_re = re.compile('^AssetTable__Table-')

    def __init__(self, url="https://www.coinbase.com/price"):
        self.url = url
        self.session = requests.Session()

    # Check if I get a 200 status code and if so, create instance from bs4
    def _get_response(self) -> BeautifulSoup:
        response = self.session.get(self.url)
        response.raise_for_status()
        return BeautifulSoup(response.text, 'lxml')

    # Select the class and get the data
    def get_data(self) -> dict:
        soup = self._get_response()
        table = soup.find('table', attrs={'class': self.table_re})
        prices = {}
        for row in table.tbody.find_all('tr'):
            cells = row.find_all('td')
            if len(cells) < 6:
                continue
            name = cells[1].find('h4').text
            price = cells[2].find('h4').text
            prices[name] = price
        return prices

Note:

Do not rely on long, terrible, possibly dynamically-generated CSS class names
If you want to reuse this class to repeatedly fetch prices, then the members should be the session and the URL, and the data should be restricted to the function
Don't write access granted. Other than it being corny, you should only throw an exception if there's a failure, not write to stdout if there's a success in the data retrieval method. Such progress indicators are best left to the calling function above.
Just index into the cells. Don't use the modulus.


Calculate mean and standard error of a generator in Python The goal of the following code is to calculate the mean and the standard error of vectors of randomly-generated numbers. I am looking for feedback both on the correctness of the calculation, and on its efficiency.

import numpy as np

def mean_and_stderr(num_of_iterations:int, instance_generator) -> (float,float):
    """
    Calculate the mean and standard error of the given generator.

    :param instance_generator: a function that accepts no parameters,
             and returns either a float or a numpy array.
    :param num_of_iterations: number of times to run the instance_generator.

    :return a tuple: (mean, standard_error)

    Test on a degenerate (constant) generator of numbers:
    >>> generator = lambda: 5
    >>> mean_and_stderr(100, generator)
    (5.0, 0.0)

    Test on a degenerate (constant) generator of vectors:
    >>> generator = lambda: np.array([1,2,3])
    >>> mean_and_stderr(100, generator)
    (array([ 1.,  2.,  3.]), array([ 0.,  0.,  0.]))
    """
    sum = sumSquares = None
    for i in range(num_of_iterations):
        x_i = generator()
        if sum is None:
            sum = x_i
            sumSquares = (x_i*x_i)
        else:
            sum += x_i
            sumSquares += (x_i * x_i)
    mean = sum / num_of_iterations
    variance = sumSquares / num_of_iterations - (mean*mean)
    stderr = np.sqrt(variance) / num_of_iterations
    return (mean,stderr)

if __name__=="__main__":
    generate_uniformly_random_number = np.random.random
    print(mean_and_stderr(10, generate_uniformly_random_number))
    # Typical output: (0.5863703739913031, 0.026898107452102943)
    print(mean_and_stderr(1000, generate_uniformly_random_number))
    # Typical output: (0.514204422858358, 0.0002934476865378269)

    generate_uniformly_random_vector = lambda: np.random.random(3)
    print(mean_and_stderr(10, generate_uniformly_random_vector))
    # Typical output: (array([ 0.53731682,  0.6284966 ,  0.48811251]), array([ 0.02897111,  0.0262977 ,  0.03192519]))
    print(mean_and_stderr(1000, generate_uniformly_random_vector))
    # Typical output: (array([ 0.50520085,  0.49944188,  0.50034895]), array([ 0.00028528,  0.00028707,  0.00029089]))

I'd want to address the generator, to begin. :param generator: accepts no parameters and returns a vector is false. The generator returns a number (with floating point or not). This is pretty confusing. When I read the code at first, I thought the generator would be an iterable that returns numbers (like range(0,10) for example).

In that case you wouldn't need to pass both the parameters num_of_iterations and generators. Otherwise, I don't think the parameter should be named generator or maybe your documentation should be stronger.

Next thing, you shouldn't initialize sum = sumSquares = None this way. They are numbers, initialize them at zero, that would give you the opportunity to remove your if/else.

sum = sumSquares = 0
for i in range(num_of_iterations):
    x_i = generator()
    sum += x_i
    sumSquares = (x_i*x_i)

Apart from that, the coding style is a little off. Sometimes you use camelCase and sometimes snake_case.

If we were to have an iterator instead of the generator, you could do something like this :

def mean_and_stderr(iterator) -> (float,float):
    sum = sumSquares = 0
    for x in iterator:
        sum = x
        sumSquares = (x*x)

    mean = sum / num_of_iterations
    variance = sumSquares / num_of_iterations - (mean*mean)
    stderr = np.sqrt(variance) / num_of_iterations
    return (mean,stderr)


Read JSON as a dictionary of country codes and names For a small system I need the Country Code mapped to the country name
(e.g. "GB"-> "United Kingdom"). To accomplish that I read a JSON file and save it to a dictionary using swiftyJSON. Is there a cleaner and better way than I did:

fileprivate func loadNames() {
    if let filePath = Bundle.main.path(forResource: "countryNames", ofType: "json") {
        if let jsonData = NSData(contentsOfFile: filePath) {
            let json = JSON(jsonData)
            countryNames = json.dictionaryObject! as NSDictionary
        }
    }
}

The JSON looks like this:

{
"BD": "Bangladesh",
"BE": "Belgium",
...
}

There's nothing wrong with what you currently have. However, a plist is another option, if this JSON is just static file.

The nice thing about the property list format is that Xcode has built-in support for editing it and Foundation has built-in support for reading it.

fileprivate func loadNames() {
    if let filePath = Bundle.main.path(forResource: "countryNames", ofType: "plist") {
        countryNames = NSDictionary(contentsOfFile: filePath)!
    }
}


Code Style and Efficiency of 3SUM I'm trying to implement a version of the 3SUM problem. Any help with code style and efficiency would be greatly appreciated. I'm trying to get it roughly \$O(n^2)\$. Also, there should be no duplicates. I specifically wanted to try and build upon 2SUM. I know there are other, maybe better, ways to do it.

def two_sum(nums, target):
    d = {}
    for i in range(len(nums)):
        key = target - nums[i]
        if key in d:
            return [nums[d[key]], nums[i]]
        else:
            d[nums[i]] = i
    return []

def three_sum(nums):
    ans = set()
    for n in nums:
        lst = two_sum(nums, -n)
        if lst:
            lst.append(n)
            ans.add(tuple(sorted(lst)))
    return ans

print(three_sum(nums))

1. Bug

The algorithm in three_sum is not correct (at least not in the usual version of the 3SUM problem):

>>> three_sum([1, -2])
{(-2, 1, 1)}

In the call two_sum(nums, -n) there's nothing to stop two_sum from using the number n in its pair, even though n is also needed to make up the triple.

2. Review

two_sum and three_sum have similar names, so you might guess that they do similar things, but two_sum return at most one pair adding up to an a given target whereas three_sum returns a list of triples adding up to zero. When writing a group of similar functions it's worth taking care that they have similar interfaces — this makes them easier to use reliably.

In this case the difference in implementations means that it's not clear exactly what three_sum does — we can't say that it returns all the triples adding up to zero, because when it calls two_sum it gets at most one result, and this might cause it to miss some triples. This is quite unsatisfactory — we can't use this function if we want all the triples, but if we only want one triple then we have to wait longer than we need to.

(This observation is similar in spirit to the zero one infinity rule — we often need functions that find one thing, or find all the things, but very rarely do we need functions that find some of the things.)
There are no docstrings. What do these functions do? What do they return? (I think that if you had written docstrings then you might have spotted the problem in point 1 above.)
In two_sum, it's not clear what the name d means. (I guess d stands for dictionary, but a dictionary containing what?) I would use a more descriptive name like seen. Similarly, instead of key I would use a name like remainder.


Getting a date range I'm just trying to be more pythonic in my coding style and was wondering if this is a good way to get a list of seven days leading up to and including endDate:

daysDelta   = 6
lastSeven   = [endDate - datetime.timedelta(days=daysDelta)]
for x in range(1,7):
    myOffset    = daysDelta - x
    lastSeven.append(endDate - datetime.timedelta(days=myOffset))

There is an easy way to clean up your loop:

lastSeven = [endDate]
for x in range(1, 7):
    lastSeven.append(endDate - datetime.timedelta(x))

Essentially, I am working backwards instead of forwards. This yields the correct dates, but in backwards order.

If you want them in chronological order do:

lastSeven.sort()

Also, just in case you don't like the sorting thing, here's a version which goes forwards:

lastSeven = [endDate - datetime.timedelta(7)]
for x in range(1, 7):
    lastSeven.append(lastSeven[x-1] + dateTime.timeDelta(1))

Lastly, there are some issues with your coding style:

Don't write code like this:

shortName    = 3
veryLongName = 4

Write code like this:

shortName = 3
veryLongName = 4

Also, put a space between arguments (so foo(x, y) is correct, while foo(x,y) is not).

This is in line with PEP8, the python style guide

Lastly, you can use a list comprehension for a bit of extra speed as one of the answers suggests, but it comes at the cost of readability, which is something that Python strives to maximize over speed (usually).

Here are two below:

# go backwards, then sort
lastSeven = [endDate - datetime.timedelta(x) for x in range(7)].sort()

# go to start, then iterate forwards
startDate = endDate - datetime.timedelta(7-1)
lastSeven = [startDate + datetime.timedelta(x) for x in range(7)]


How to create a simple stateless cache class in PHP i need to create a cache class to avoid making the same calculations more than needed. My idea was something really basic like

class transient_cache {
    static private $cache = array();
    static public function get_value( array $obj, $key ) {
        // $obj is an array( $object, $method )
        $classname = get_class( $obj[0] );
        $method = $obj[1];
        if( ! isset( self::$cache[$classname] ) ) {
            self::$cache[$classname] = array();
        }
        if( ! isset( self::$cache[$classname][$method] ) ) {
            self::$cache[$classname][$method] = array();
        }
        if( ! isset( self::$cache[$classname][$method][$key] ) ) {
            self::$cache[$classname][$method][$key] = call_user_func( $obj );
        }
        return  self::$cache[$classname][$method][$key];
    }
}

Do you think this approach is correct or would you suggest something better?

I would say that there isn't a correct or incorrect way of achieving this. Everyones requirements differ.

You probably want to expand this out, there's a lot going on in the one static method you have in that class. Whilst I don't have anything against mediator patterns, I would probably combine a number of patterns to build a powerful caching class, as well as utilising magic methods (I know some people are averse to this).

I need to note the below is a lot more complicated and would benefit from some refactoring to be stored against the actual method rather than a key.

class Cache {

    /**
     * Cached objects
     * @static array
     */
    private static $cache = array();

    /**
     * Per instance class name
     * @var string
     */
    private $call;

    /**
     * Per instance method name
     * @static array
     */
    private $method;

    /**
     * Construct the class and build cache array if not set
     * @param string $method
     * @param string $class
     * @param string $key
     */
    public function __construct($method, $class, $key)
    {
        if ( ! isset(static::$cache[$class] )
            static::$cache[$class] = array();

        if ( ! isset(static::$cache[$class][$method] )
            static::$cache[$class][$method] = array();

        // Set the class for this instance
        $this->call = $class;

        // Set the method for this instance
        $this->method= $method;

        // Set the key for the cache
        $this->key = $key;
    }

    /**
     * Get the cached value
     * @return mixed Cached value
     */
    public function get()
    {
        return $this->{$this->key};
    }

    /**
     * Get magic method to return the key
     * @param string $key
     */
    public function __get($key)
    {
        if (isset(static::$cache[$this->call][$this->method][$key]))
            return static::$cache[$this->call][$this->method][$key];

        return static::$cache[$this->call][$this->method][$key] = call_user_func(array($this->call, $this->method));
    }

    /**
     * Call static magic method, catches all statically called methods
     * and builds an instance of the cache class using a reflector
     * @param $method
     * @param $args
     */
    public static function __callStatic($method, $args)
    {
        $method = str_replace(__CLASS__ . '::', '', $method);

        $ref = new ReflectionClass(__CLASS__);

        $inst = $ref->newInstanceArgs($args);

        return $inst->get();
    }

}

Usage

$result = Cache::{method}({class}, {key};

To retrieve say a user object from the User class with method get_user;

$user = Cache::get_user('User', 'user_object');

The main advantage to this is that the class can be extended easily using other design patterns such as a Facade that further simplifies the class API. It can also be added to easily with further methods within the class body itself rather than by a pure static call. The class can also be instantiated with the new keyword providing the ability to create separate cache instances that utilise the same static properties and methods.

I need to add this class is very much a prototype that I thought of when I saw your class above. I love cacheing stuff in general and providing standardised interfaces for it is always a great thing to do across all applications you write.


Setting a query for MongoDB The following function sets a query for MongoDB, but that's not very important. It takes 2 arguments key can be either - which comes from an empty search by the user in the front end, or any other string. Select can be either sample or all
So it's a switch like this:
                            |
                          /   \
                         /\   /\

In my head at least.
But the code looks terribly bad, so something may be wrong, any tip or trick is welcome:
function setQuery(key, select){
  const project = {"$project":{"_id":0, "quote":1} }
  if (key==="-"){
   return select==="sample"
      ? [{"$sample":{"size":5}}, project ]
      : [{$match:{quote:{$ne:null}}}, project] } else {
        //if key is not - (is a word)
        const reg = new RegExp(key, 'i')
        const userText =  { "$match":{"quote":{ "$regex": reg } }}
      return select==="sample"
        ? [userText, {"$sample":{"size":5}}, project]
        : [userText, project]
    }
  }

I think the clearest way to approach this would be to define all the array items up front, to separate out the somewhat convoluted data boilerplate from the branch logic. Also, every query contains project at the end, so to decrease repetitiveness (and to increase the distinction between the different branches later), you might make a function for it.
function setQuery(key, select) {
  // Not sure what these should be called, there's probably a more precise name than `matchers`
  const matchers = {
    project: {$project:{_id:0, quote:1} },
    sample: {$sample:{size:5}},
    any: {$match:{quote:{$ne:null}}},
    text: { $match:{quote:{ $regex: new RegExp(key, 'i') } }},
  };
  const makeProjectQuery = (...initials) => [...initials, matchers.project];

  if (key === "-") {
    return makeProjectQuery(select === "sample" ? matchers.sample : matchers.any);
  } else {
    // Then key is a word; search for it
    return select === "sample"
      ? makeProjectQuery(matchers.text, matchers.sample)
      : makeProjectQuery(matchers.text);
  }
}

It's a few more lines, but I think it's easier to make sense of at a glance. Plus, if you ever needed to expand the functionality of this section, it would be a pretty natural extension to add something to matchers and add conditions for it in the lower body.
The code could be further condensed, but it would come at the sake of readability.
Review
Some suggestions regarding your original code:
Only quote property names when necessary - leaving out the "s results in less syntax noise and is a bit easier to read and write.
Indent code properly - two statements that are at the same level in a block should have the same indentation level - this makes the control flow clearer. Similarly, when ending a block, consider putting the } on the next line - being able to pick up on the locations where blocks begin and end at a glance is very important. Lots of IDEs can automatically indent code for you, they're well worth it.
Consider using spaces around operators, especially in complicated code. select === "sample" is easier to recognize at a glance than select==="sample" (especially if more operators become involved later)
When one branch differs from another only in a single expression, consider the conditional operator instead of repeating everything else around the expression. (eg, replace with something like select === "sample" ? matchers.sample : matchers.any as above) You don't have to use the conditional operator - sometimes it's clearer without it - but it's something to think about in that sort of situation.
Good comments use natural language that flows while reading the code out loud.