Java FileInputStream exercise I've just learned about custom wrapper classes and I'm a bit confused about a couple of things.
First, what's the utility of such tool? From what I understand you can mimic the copied class behaviour but with slight modifications. Is this tool used often?

Also, could you please have a look at my code? I know I'm suppose to close streams with "try with resources", but I'm not sure where it fits inside a constructor. Any other suggestions about the quality of my code are appreciated, please tear it apart!

/*
UnsupportedFileName
Change the TxtInputStream class so that it only works with txt files (* .txt).
For example, first.txt or name.1.part3.txt.
If a non-txt file is passed (e.g. file.txt.exe), then the constructor should throw
an UnsupportedFileNameException.
Think about what else you need to do if an exception is thrown.

Requirements:
x 1. The TxtInputStream class must inherit the FileInputStream class.
2. If a txt file is passed to the constructor, TxtInputStream should behave like a regular FileInputStream.
3. If a non-txt file is passed to the constructor, an UnsupportedFileNameException should be thrown.
4. If an exception is thrown, then you must also call super.close().

*/

public class TxtInputStream extends FileInputStream {
    private FileInputStream original;
    public static String fileName;

    public TxtInputStream(String fileName) throws IOException, UnsupportedFileNameException {
        super(fileName);
        if(fileName.substring(fileName.length()-4).equals(".txt")) {
            this.fileName = fileName;
        } else {
            super.close();
            throw new UnsupportedFileNameException();

        }
    }

    public static void main(String[] args) {
    }
}

The cleanest way to solve this would probably be to check the argument before calling the super constructor. This allows the code to fail fast and prevents it from opening the file, rendering the close() call unnecessary:
public class TxtInputStream extends FileInputStream {
    public TextInputStream(String fileName) {
        // super call must be first statement so have to call validation method
        // as argument to super call
        super(checkFileName(fileName));
    }

    private static String checkFileName(String fileName) {
        if (!fileName.endsWith(".txt")) {
            throw new UnsupportedFileNameException();
        }
        return fileName;
    }
}

However, this is a pretty bad example for wrappers since the TxtInputStream does not add any functionality at all.
If you want to see some good examples have at look at which JDK classes implement InputStream:

BufferedInputStream: Buffers data, this can be useful when you are only consuming small amounts of data at a time, but reading such small amounts of data would be inefficient, e.g. for a network connection. Instead it reads larger chunks from the wrapped InputStream and when you then consume data it first serves it from the buffer.
CipherInputStream: Decrypts the data
InflaterInputStream: Uncompresses data

So assuming you read encrypted, compressed data over the network you could have:
InputStream networkInputStream = ...
BufferedInputStream bufferedInputStream = new BufferedInputStream(networkInputStream);
CipherInputStream cipherInputStream = new CipherInputStream(bufferedInputStream, cipher);
InflaterInputStream inflaterInputStream = new InflaterInputStream(cipherInputStream);

Then you can simply use inflaterInputStream like any other InputStream and do not even have to worry about all the previous processing steps which are taking place.


Simple wrapper for member function pointers with known signature I always wanted to know how to pass member functions as arguments, and then I stumbled across templates that could automatically deduce types, and so I rejoiced! This is used in a Publisher-Subscriber system for a game server. I wanted the subscribing to individual packet 'types' (i.e. by enum) to be simple at the calling site.

/**
* Wrapper struct for callbacks fired when RakNet::Packet*s arrive.
* TargetObj is used to identify this callback by which object it is bound to.
*/
struct PacketCallback
{
    template <typename Obj>
    PacketCallback(void(Obj::*mf)(const RakNet::Packet*), Obj* obj)
        : TargetObj(obj)
    {
        Func = std::bind(mf, obj, std::placeholders::_1);
    }

    std::function<void(const RakNet::Packet*)> Func;
    void* TargetObj = nullptr;
};

Used thus:

class ClientManagementTest : public INetworkSubscriber
{
public:

    virtual void Subscribe(NetworkPublisher* a_networkPublisher) override
    {
        a_networkPublisher->Register(ID_NEW_INCOMING_CONNECTION,
            PacketCallback(&ClientManagement::OnClientConnected, this));

        a_networkPublisher->Register(ID_CONNECTION_LOST,
            PacketCallback(&ClientManagement::OnClientDisconnected, this));

        a_networkPublisher->Register(ID_DISCONNECTION_NOTIFICATION,
            PacketCallback(&ClientManagement::OnClientDisconnected, this));
    }

    virtual void Unsubscribe(NetworkPublisher* a_networkPublisher) override
    {
        a_networkPublisher->UnregisterAll(this);
    }

    void OnClientConnected(const RakNet::Packet* a_packet)
    {
        printf("Someone connected\n");
    }

    void OnClientDisconnected(const RakNet::Packet* a_packet)
    {
        printf("Someone disconnected\n");
    }

};

Are there any glaring flaws or possible improvements, here? Perhaps something that's incredibly unsafe?

For context, the function pointer is invoked thus, where the publisher stores the callbacks in the form std::map<int, std::vector<PacketCallback>> m_subscribers;:

void NetworkPublisher::NotifySubscribers(int a_packetType, RakNet::Packet* a_packet)
{
    auto it = m_subscribers.find(a_packetType);
    if (it != m_subscribers.end())
    {
        for (int i = 0; i < it->second.size(); ++i)
        {
            it->second[i].Func(a_packet);
        }
    }
}

I'm really happy with it, but would appreciate criticism.

Thanks!

Sorry for the brief answer, typing on phone.

Your NetworkPublisher should have Register(int signal, std::function<void()> callback)

And then simply do:

publisher.Register(WHATEVER_SIGNAL, std::bind(&MyClass::MySignalCallback, this));

This way you dont need the PacketCallback class and your subscriber interface is more generally usable as you can now pass lamdas and free functions as well. Further more your NotifySubscribers will simplify a bit.

Side note: You might want to consider using smart pointers instead of raw pointers. Or references if null is not an allowed argument.

Edit: In reply to comments, this is one way I would consider of doing it:

enum class SignalType{
   NewConnection,
   ConnectionLost
};

const std::array<SignalType,2> AllSignalTypes = {
    SignalType::NewConnection,
    SignalType::ConnectionLost};

class ISubscriber{
public:
    virtual bool supportsSignal(SignalType signalType) const = 0;
    virtual void signal(SignalType signalType, const RakNet::Packet& packet) = 0;
}

class Publisher{
public:
    void registerSubscriber(const std::shared_ptr<ISubscriber>& subscriber){
        for(auto& type : AllSignalTypes){
            if(subscriber->supportsSignal(type)){
                // TODO: Check for duplicates or change to std::unordered_set<weak_ptr>
                subscribers[type].emplace_back(subscriber);
            }
        }
    }

    void signal(SignalType signalType, const RakNet::Packet& packet){
        forEachLiveSubscriber(signalType, [&packet](const std::shared_ptr<ISubscriber>& subscriber){
            subscriber->signal(signalType, packet);
            return true;
        });
    }

    void unregisterAll(const ISubscriber& subscriber){
        // Technically not necessary but for good measure
        for(auto& type : AllSignalTypes){
            forEachLiveSubscriber(signalType, [&packet](const std::shared_ptr<ISubscriber>& sharedSubscriber){
                return sharedSubscriber.get() != &subscriber;
            });
        }
    }

private:
    std::unordered_map<SignalType, std::vector<std::weak_ptr<ISubscriber>>> subscribers;

    template<typename Callable>
    void forEachLiveSubscriber(SignalType signalType, Callable&& callback){
        auto& signalSubscribers = subscribers[signalType];

        auto it = signalSubscribers.begin();
        while(it != signalSubscribers.end()){
            auto& strongPtr = it->lock();
            if(strongPtr && callback(strongPtr)){
                it++;
            }
            else{
                // The subscriber has destructed without unregistering,
                // Or the callback requested us to remove it.
                it = signalSubscribers.erase(it);
            }
        }
    }
}

class MySubscriber : public ISubscriber{
    bool supportsSignal(SignalType signalType) const override{
        return signalType == NewConnection || signalType == ConnectionLost;
    }

    void signal(SignalType signalType, const RakNet::Packet& packet) override{
        switch(signalType){
            case NewConnection:
                std::cout<<"Someone connected"<<std::endl;
                break;
            case ConnectionLost:
                std::cout<<"Someone lost connection"<<std::endl;
                break;
        }
    }
}

Note that by using shared_ptr we get access to weak_ptr which in turn allows lazy garbage collection of the subscribers. In turn this means that a subscriber doesn't have to unregister if all shared pointers to it just simply die.

The above code is untested pseudocode. Some assembly may be required :)


General FizzBuzz in Scala I wanted to create a FizzBuzz implementation that both showcases the strengths of Scala (Functional paradigm and collections) and is general enough to anticipate the typical interview follow-up questions. i.e. What if fizz was for factors of 4 instead? What if we wanted to replace factors of 7 with "foo"?

object FizzBuzz extends App {

  def fizzbuzz(factors: Map[Int, String])(currentNumber: Int): String = {
    val words = factors.filter(currentNumber % _._1 == 0).values
    if (words.isEmpty) currentNumber.toString else words.mkString("")
  }

  private final val Factors = Map(3 -> "fizz", 5 -> "buzz")
  private final val Range = 1 to 100

  Range.map(fizzbuzz(Factors)).foreach(println)
}

I would appreciate any feedback on how I can make my Scala more idiomatic.

Map is not guaranteed to impose or preserve any order for its entries.  For example, if I change Factors to …

private final val Factors = Map(3 -> "a", 5 -> "b", 6 -> "c", 7 -> "d", 8 -> "e", 9 -> "f")

… I get:

⁞
89
bcfa
d
92
a
94
b
cae
97
d
fa
b

So, the fact that you get "fizzbuzz" rather than "buzzfizz" is based on luck, not skill.  If you want to guarantee "fizzbuzz", you need a scala.collection.immutable.SortedMap.


Functionality to read large files (> 3 GB) in chunks Here is a function I wrote that can read chunks of large files (> 3 GB). It's designed to be used contentiously so that one can use it in a while loop until it returns EOF.

It's an early prototype and is only written to work under 32-bit Linux.

I'm okay with feedback on readability, maintainability, or anything else.

/**
 * Reads a chunk of given size into memory (heap) from file. Chunk size is
 * measured in amount of pages.
 *
 * The chunk will be allocated before reading starts. So no matter what
 * the return code you always need to decref the chunk.
 *
 * Parameters:
 * - pages    : The amount of pages to read to memory. The size of a page
 *              a page differs from different systems, but it is usually 4 KiB.
 *              See getpagesize or _SC_PAGESIZE for sysconf in unistd.h for
 *              details.
 * - start_pos: The position in the file to start reading from
 * - *end_pos : Pointer to the location where the end position of the
 *              reading shall be placed. This will be the byte after the last
 *              read byte, so that read_chunk can be called with this value
 *              as start_pos directly.
 * - **chunk  : Pointer to the pointer of an byte array where the result
 *              will be placed in.
 *
 * Return values:
 * - Returns 0 on success
 * - Returns -1 if chunk size was above 2^31 bits (~ 2 GB)
 * - Returns -2 if an fseek error occured, see errno for details
 * - Returns -3 if an fread error occured, see errno for details
 * - Returns -4 on EOF
 */

int8_t read_fachunk(
        FILE *f,
        uint16_t pages,
        uint64_t start_pos,
        uint64_t *end_pos,
        struct byte_array **chunk)
{
        assert(f != NULL);
        assert(end_pos != NULL);
        assert(chunk != NULL);
        assert(pages != 0);

        uint64_t chunk_b = pages * sysconf(_SC_PAGESIZE);

        if(chunk_b > MAX_CHUNK_SIZE) {
            return -1;
        }

        uint64_t curr_pos = 0;
        int64_t err = lseek64(fileno(f), start_pos, SEEK_SET);

        *chunk = byte_array_create(chunk_b);

        if (err == -1) return -2;

        curr_pos = fread((*chunk)->bytes, sizeof(uint8_t), chunk_b, f);
        (*chunk)->curr_i = curr_pos;

        if (ferror(f) != 0) {
            err = -3;
            goto complete;
        }

        if (feof(f) != 0) {
            err = -4;
            goto complete;
        }

        err = 0;

        complete:
    *end_pos = start_pos + curr_pos;
    return err;
}

Here are a few comments on your function.

Firstly taking the prototype, why return an int8_t. A simple int is better
as uint8_t will probably be slower.  Also returning several types of error
is unusual in UNIX functions like this.  I'd expect errno to tell what
failed and suggest you return the number of bytes read, 0 on EOF and -1 on
error (like read(2)).

And again on the prototype, why are you passing a FILE instead of a
descriptor?  Passing a FILE and then messing around with the raw descriptor
makes me uneasy.

You allocate a strange structure in the function.  This is odd for a low level
utility.  It is unnecessary and arguably wrong, as it means the caller cannot
avoid an allocation even if the buffer from the last call can be reused or a
static buffer is available.  Just pass a buffer and its size (as a size_t)
into the call.

And it would be more normal to pass in the size to read (as a size_t)
instead of the number of pages. If you pass a buffer and its size as I
suggested above, you don't need an extra size to say how much to read.

You are mixing use of buffered fread (operating on a FILE) and lseek (on
an fd).  This is a bad idea as you are messing with something the FILE has
reason to call its own.  If you are going to seek, then use read to read the
data.

And in your use of fread, the call returns the number of items read (number
of bytes in this case) so assigning that to a variable called curr_pos is
not sensible. The name nread would make more sense.  And keeping that value
in the byte_array structure with a different, equally meaningless name,
curr_i, is unnecessary.

Also, reading up to end-of-file might return less than requested.  That is
not an error from the perspective of this function (it might be from an
application perspective if the file size is expected to be multiples of page
size, but the handling of such an error doesn't belong here).  So calling
feof and treating end-of-file as an error is wrong.  There is only a
possible error it nread != chunk_b.  But you should be using read(2)...

Some other points:

your goto statements are unnecessary.  Occasionally gotos are useful in
simplifying error recovery, but in this case they simplify nothing.
your end_pos is unnecessary.  The file descriptor knows its current
offset, which is available with fseek(fd, 0, SEEK_CUR)
chunk_b is a bad name for a size, don't you think?
test the return from lseek immediately. If you have a stderr, use perror
to print the reason for failure (perhaps best in the calling functions).
use of sizeof(uint8_t) is silly - just use 1

EDIT

FILEs and fopen, fwrite, fread, fprintf, fgets etc are provided by stdio for use as buffered IO in which the standard library maintains a buffer through which your requests are served.  When you use fread to read (say) 10 bytes, the library might read a whole page or disc sector or line of input from the terminal (on the assumption that you'll want the rest later) and give you just the amount you asked for, saving the rest in the buffer.  The "file descriptor" (fd), which is returned by fileno, is the lower level interface to the actual file returned by open (type man 2 open on UNIX/Linux/MacOS) - open is used by fopen to open the actual file/device.

If you are doing unbuffered or 'raw' IO, you can use open/write/read/lseek etc to save the overhead of the stdio and to give you more control. Since you are explicitly seeking to the location you want to read, you are certainly doing raw (unbuffered) IO and the lower-level interface makes more sense.

On the interaction between buffered and unbuffered IO on the same underlying file descriptor, I've never read anything that suggests this is forbidden.  But intuitively it seems that using buffered IO and then moving the file's position by seeking behind stdio's back is asking for trouble. Also, if you are on a 32-bit system, stdio probably does not support files bigger than 2GB (or perhaps 4).

I just would never mix calls to the FILE and the fd.  Hopefully someone else with deeper knowledge will shed some more light on this.


Submit form data to MYSQL table with PHP execute I have written code to send a simple email submission form to a sql table so that I can manage the data. I would like feedback on whether or not the code that I have written is efficient and secure. I have a config file that defines constants for database credentials.

HTML FORM

<form method="post" action="index.php">
    <input class="bt-email" type="email" name="email" placeholder="enter email address">
    <input class="bt-submit" type="submit" value="let's get moving!">
</form>

PHP Database Code File

try {
    $db = new PDO("mysql:host=" . DB_HOST . ";dbname=" . DB_NAME . ";port=" . DB_PORT,DB_USER,DB_PASS);
    $db->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);
    $db->exec("SET NAMES 'utf8'");
} catch (Exception $e) {
    echo "Could not connect to the database.";
    exit;
}

PHP/SQL Submission Code

<?php
require_once("inc/config.php");
if ($_SERVER["REQUEST_METHOD"] == "POST") {
    $email = $_POST["email"];
    require(ROOT_PATH . "inc/database.php");
    try {
        $db->exec("REPLACE INTO launch_email VALUES ('$email')");
    } catch (Exception $e) {
        echo "Data could not be submitted to the database.";
        exit;
    }
    header("Location: index.php?status=thanks");
    exit;
}
?>

You could possibly move the database include before the server checks and sets the $_POST variable, as it would not be needed if the database is unable to connect.

I notice there is no sanitization of the email variable. This may be helpful.


Number of paths in a grid (lattice) graph I am trying to solve a variant of the very popular question in which we find the number of distinct valid paths on a directed graph. The graph's nodes make up a m x n lattice (grid), and the edges only lead down and right. The goal is to find how many distinct paths start at the top left corner of and end in the bottom right.

So one possible path might be down m, right n.

In this variant of the problem, there is a complication. A rectangular area inside the lattice is blocked -- no paths can go through it. (The edges that would normally lead to these nodes don't exist)

I am using Dynamic Programming to solve with a time complexity of \$O(m*n)\$.

It works pretty good with small value for rows and columns but for bigger values it throws OutOfMemoryError.

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;

public class GeekForGeeks{

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

    BufferedReader inp = new BufferedReader(
            new InputStreamReader(System.in));

                    String[] s1 = inp.readLine().split(" ");
        int row = Integer.parseInt(s1[0]);
        int col = Integer.parseInt(s1[1]); // Input for the value of row &
                                            // col

        String[] s2 = inp.readLine().split(" ");
        int r1 = Integer.parseInt(s2[0]);
        int c1 = Integer.parseInt(s2[1]);
        int r2 = Integer.parseInt(s2[2]);
        int c2 = Integer.parseInt(s2[3]); // Input for the value of row &
                                            // column of blocked path

        int[][] A = new int[row][col];

        for (int i = 0; i < row; i++)
            for (int j = 0; j < col; j++)
                A[i][j] = 1;

        for (int i = r1 - 1; i < r2; i++)
            for (int j = c1 - 1; j < c2; j++)
                A[i][j] = 0;

        int answer = numOfPaths(A, row, col);
        System.out.println(answer);
    }

}

private static int numOfPaths(int A[][], int row, int col) {

    long mod = (long) Math.pow(10, 9) + 7;
    int[][] B = new int[row][col]; // new matrix to store number of paths
    // if starting point blocked then no path is there to reach right down
    if (A[0][0] == 0) {
        return 0;
    } else {
        int i, j;
        for (i = 0; i < col; i++) {
            if (A[0][i] == 0)
                break; // exit from the for loop, and set remaining elements
                        // the first row set to ZERO
            B[0][i] = A[0][i];
        }

        while (i < col)
            B[0][i++] = 0; // When any starting row is blocked then all the
                            // row down will also be blocked and
                            // have no path to be reached from starting
                            // point.

        for (i = 1; i < row; i++) {
            if (A[i][0] == 0)
                break;// exit from the for loop, and set remaining elements
                        // in the first column set to ZERO
            B[i][0] = A[i][0];
        }
        while (i < row)
            B[i++][0] = 0; // When any column is blocked all starting column
                            // towards right will be blocked

        for (i = 1; i < row; i++)
            for (j = 1; j < col; j++)
                if (A[i][j] != 0)
                    B[i][j] = (int) (((B[i - 1][j] % mod) + (B[i][j - 1] % mod)) % mod);
        // otherwise, we enter the sum of the value above and left

        return B[row - 1][col - 1]; // return the last column and row.
    }
}

It works pretty good with small value for rows and columns but for bigger values it throws OutOfMemoryError.

That's hard to believe as I can't see any allocation there besides new int[row][col], which is no problem unless the dimensions get much bigger than a few thousands. I guess they do and you'll need a smarter algorithm. As the whole computation runs top-down, you don't have to store the whole matrix. A single row should do.

// TODO Auto-generated method stub

Are you sure you need it???

String[] s1 = inp.readLine().split(" ");
int row = Integer.parseInt(s1[0]);
int col = Integer.parseInt(s1[1]); //Input for the value of row & col

Inline comments are fine as long as they belong to the command in their line. But this is a block comment and should precede the block. It could be a also a comment for s1, but not col.

int r1 = Integer.parseInt(s2[0]);

Are we supposed to guess what r1 means? What about naming it topLeft?

private static int numOfPaths(int A[][], int row,int col)
{

Java uses "Egyptian brackets" (no line break before the opening brace). There should be a space after every comma. Passing row and col is redundant as they're obtainable from A.

if( A[0][0] == 0)

Spacing! I doubt that this special case handling is necessary.

{
    return 0;
}
else
{

The nice thing about "early return" is that it allows you to handle special cases upfront without increasing the nesting depth. Your "else" after "return" is wrong.

int i,j;
for(i=0; i<col; i++)

Use for (int i=...) instead, so that the scope of every variable is as small as possible.

if( A[0][i] == 0 )
            break; // exit from the for loop, and set remaining elements the first row set to ZERO
        B[0][i] = A[0][i];

Spacing! So you first exit the loop and then (inside of the loop) set the remaining part to zero? This can't work.

But you're lucky as all int array elements are initialized to zero anyway.

I guess all what's needed is

for (i = 1; i < row; i++) {
    for (j = 1; j < col; j++) {
        if (A[i][j] != 0) {
            B[i][j] = (int) (((B[i - 1][j] % mod) + (B[i][j - 1] % mod)) % mod);
        }
    }
}

Note the formatting (done by Eclipse for free). Also note that it's recommended to always use braces (you know "goto-fail", right?).


Database update script Previous question:

Database input script

I'd like this new question reviewed.

    <?php
$mysqli =  new mysqli('localhost','root','','yaztor');
if($mysqli->connect_errno >0){
    die( "problem with the connection");

    }

function fetchTableFieldCount($mysqli , $databaseName , $tableName ) {
    $sql = "SELECT COUNT(`COLUMN_NAME`) AS FieldCount FROM `INFORMATION_SCHEMA`.`COLUMNS` WHERE `TABLE_SCHEMA`= '$databaseName'  AND `TABLE_NAME`= '$tableName' ";

    if ( ! ( $stmt = $mysqli->prepare($sql) ) ) {
      die("There is an error with mysql preparation statement".$mysqli->error);
    }

    if ( ! $stmt->execute() ) {
      echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
    }

    $res = $stmt->get_result();

    if ( $row = $res->fetch_array() ) {
      return $row['FieldCount'];
    }

    return 0;
  }

   function fetchTableFieldName($mysqli , $databaseName , $tableName ) {
    $sql = "SELECT `COLUMN_NAME`  FROM `INFORMATION_SCHEMA`.`COLUMNS` WHERE `TABLE_SCHEMA`= '$databaseName'  AND `TABLE_NAME`= '$tableName' ";

    if ( ! ( $stmt = $mysqli->prepare($sql) ) ) {
      die("There is an error with mysql preparation statement".$mysqli->error);
    }

    if ( ! $stmt->execute() ) {
      echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
    }

    $res = $stmt->get_result();

    while( $row = $res->fetch_array() ) {
     $array []= $row['COLUMN_NAME'];
    }
    return $array;
    return 0;
  }
    function updateDataIntoTable($mysqli, $tableName, $databaseName, $array ,$idFeildName , $idFeildValue) {

        if( ! $tableCoulmnsNames = fetchTableFieldName($mysqli , $databaseName , $tableName ) ){
            die("Error Fetching Table Information");
            }
        $tableColumnsCount = count($tableCoulmnsNames);

        $dataArrayCount    = count($array);

        if ( $tableColumnsCount != $dataArrayCount ) {
          die("Data array does not match table columns count");
        }

        if ( 0 == $dataArrayCount ) {
          return;
        }
          array_push($array,$idFeildValue);

          for ( $i = 0; $i < sizeof($array); $i++ ) {
          $string         =  gettype($array[$i]);
          //taking the first letter; example:  string = s, integer = i
          $a_param_type[] =  $string[0];
        }

        for($i=0;$i<$tableColumnsCount;$i++){
            $tableCoulmnsNames[$i] = "`" . $tableCoulmnsNames[$i] ."`";
        }

        $sql = " UPDATE `$databaseName`.`$tableName` SET ";
        $sql .= implode($tableCoulmnsNames , ' = ?  , ' );
        $sql .= ' = ? ';
        $sql .=" WHERE `$tableName`.`$idFeildName` = ? ";
        echo $sql;
         if ( ! $stmt = $mysqli->prepare($sql) ) {
          die("error in preparation " . $mysqli->error);
        }

         //puting data element type
         array_unshift($array, implode($a_param_type));

         //refrensing values
         for($i=0;$i<sizeof($array) ;$i++){
            $array[$i] = & $array[$i];
            }

        call_user_func_array(array($stmt, 'bind_param'), $array);

          if ( ! $stmt->execute() ) {
          die("Error in executing the query:  " . $mysqli->error);
        }
        return 1;
        }
      $array = array(10, 5 ,"name", "desc", "text", "12-1-2014 01:00", "1");
      updateDataIntoTable($mysqli, "table", "database", $array ,"idFeildname" , $idFeildValue)

    ?>

I think you must change your die/echo mechanism to something another. Like a throw exception or return a status from the function.

if ( ! $stmt->execute() ) {
  echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
}

And in this part you must not echo error, but return/throw it. Because if you just print it, your code will work further.

Also you must rewrite updateDataIntoTable like this:

function updateDataIntoTable($mysqli, $tableName, $databaseName, $array ,$idFeildName , $idFeildValue) {
  $dataArrayCount = count($array);
  if ( 0 == $dataArrayCount ) {
      return;
  }
  ...
}

This is called guard statement. Better to keep all returns at top or bottom function.


Repeated string concatenation I've been looking for a way to have a predicate that establishes that all elements of a list are substrings delimited by a given string. Analogues in other languages include: 'delimiter'.join(list) in Python, (mapconcat function sequence delimiter) in Emacs Lisp, array.join('delimiter') in JavaScript, and I'm sure there are more of the kind. Below are three different variants I came up with, but I've a strong feeling that I'm reinventing the wheel here:
join
join(_, [X], X) :- !.
join(Sep, [X | Xs], S) :-
    join(Sep, Xs, Sx),
    string_concat(Sep, Sx, Sy),
    string_concat(X, Sy, S).

Seems to be the simplest, but I don't like that it is not tail-recursive and that it has an obvious repetitive pattern.
interleave
interleave([X], _, X) :- !.
interleave([X | Xs], Glue, Result) :-
    interleave(Xs, Glue, Previous),
    format(atom(Result), "~p~p~p", [X, Glue, Previous]).

Still not tail-recursive, no repetition, but I'm not sure how bad is format if compared to plain string_concat.
mapconcat
reduce([], X, _, X) :- !.
reduce([X | Xs], Acc, Predicate, Result) :-
    call(Predicate, Acc, X, Interim),
    reduce(Xs, Interim, Predicate, Result).
reduce([X | Xs], Predicate, Result) :- reduce(Xs, X, Predicate, Result).

mapconcat_helper(Glue, X, Y, Z) :- format(atom(Z), '~p~p~p', [X, Glue, Y]).

mapconcat(List, Glue, Result) :-
    reduce(List, mapconcat_helper(Glue), Result).

To my surprise, there isn't a reduce-like predicate in the standard library, so I had to write my own. Finally, this won't explode the stack for longer lists.

So, which one, if any of these is best? Perhaps there are libraries that already do this better? I'm using SWI Prolog, and am doing this mostly for self-education, so I'm not yet concerned with portability etc. If it's SWI-specific, I think it'd still do.

A quick browse through the SWI-Prolog manual finds atomics_to_string/3:

atomics_to_string(+List, +Separator, -String)
Creates a string just like atomics_to_string/2, but inserts Separator between each pair of inputs. For example:
?- atomics_to_string([gnu, "gnat", 1], ', ', A).
A = "gnu, gnat, 1"


Checking for balanced brackets in JavaScript I'm wrote a simple function isBalanced which takes some code and returns true if the brackets in the code are balanced and false otherwise:

function isBalanced(code) {
    var length = code.length;
    var delimiter = '';
    var bracket = [];
    var matching = {
        ')': '(',
        ']': '[',
        '}': '{'
    };

    for (var i = 0; i < length; i++) {
        var char = code.charAt(i);

        switch (char) {
        case '"':
        case "'":
            if (delimiter)
                if (char === delimiter)
                    delimiter = '';
            else delimiter = char;
            break;
        case '/':
            var lookahead = code.charAt(++i);
            switch (lookahead) {
            case '/':
            case '*':
                delimiter = lookahead;
            }
            break;
        case '*':
            if (delimiter === '*' && code.charAt(++i) === '/') delimiter = '';
            break;
        case '\n':
            if (delimiter === '/') delimiter = '';
            break;
        case '\\':
            switch (delimiter) {
            case '"':
            case "'":
                i++;
            }
            break;
        case '(':
        case '[':
        case '{':
            if (!delimiter) bracket.push(char);
            break;
        case ')':
        case ']':
        case '}':
            if (!delimiter && bracket.length && matching[char] !== bracket.pop())
                return false;
        }
    }

    return bracket.length ? false : true;
}

The function must not operate on brackets inside strings and comments. I wanted to know if my current implementation will work correctly for all test cases. I also wanted to know whether brackets may be used in any other context beside strings and comments in a language like JavaScript (AFAIK this is not the case).

The function must not operate on brackets inside strings and comments.
If that's the case then why not just compare the number of opened vs closed symbols?
Example:
var haveSameLength = function(str, a, b){
    return (str.match(a) || [] ).length === (str.match(b) || [] ).length;
};
var isBalanced = function(str){
    var arr = [
        [ /\(/gm, /\)/gm ], [ /\{/gm, /\}/gm ], [ /\[/gm, /\]/gm ]
    ], i = arr.length, isClean = true;

    while( i-- && isClean ){
        isClean = haveSameLength( str, arr[i][0], arr[i][1] );
    }
    return isClean;
};

Simple Testcases.
console.log( isBalanced( "var a = function(){return 'b';}" ) === true );
console.log( isBalanced( "var a = function(){return 'b';" ) === false );
console.log( isBalanced( "/*Comment*/var a = function(){ \n // coment again \n return 'b';" ) === false );
console.log( isBalanced( "var a = function(){return 'b';" ) === false );

Here's a demo:
http://jsfiddle.net/9esyk/
Update
Your code is optimal if performance is the main consideration, but the complexity is too high.
Here are a few tips.
1)
Split up your function into smaller methods to reduce the complexity. One way to do this would be to have functions to filter your string so that you only analyze the meaningful characters.
2)
Avoid using the keyword char since it's a java reserved keyword.
Final Result:
var removeComments = function(str){
    var re_comment = /(\/[*][^*]*[*]\/)|(\/\/[^\n]*)/gm;
    return (""+str).replace( re_comment, "" );
};
var getOnlyBrackets = function(str){
    var re = /[^()\[\]{}]/g;
    return (""+str).replace(re, "");
};
var areBracketsInOrder = function(str){
    str = ""+str;
    var bracket = {
            "]": "[",
            "}": "{",
            ")": "("
        },
        openBrackets = [],
        isClean = true,
        i = 0,
        len = str.length;

    for(; isClean && i<len; i++ ){
        if( bracket[ str[ i ] ] ){
            isClean = ( openBrackets.pop() === bracket[ str[ i ] ] );
        }else{
            openBrackets.push( str[i] );
        }
    }
    return isClean && !openBrackets.length;
};
var isBalanced = function(str){
    str = removeComments(str);
    str = getOnlyBrackets(str);
    return areBracketsInOrder(str);
};

Testcases
test("test isBalanced for good values", function(){
    var func = isBalanced;
    ok(func( "" ));
    ok(func( "(function(){return [new Bears()]}());" ));
    ok(func( "var a = function(){return 'b';}" ));
    ok(func( "/*Comment: a = [} is bad */var a = function(){return 'b';}" ));
    ok(func( "/*[[[ */ function(){return {b:(function(x){ return x+1; })('c')}} /*_)(([}*/" ));
    ok(func( "//Complex object;\n a = [{a:1,b:2,c:[ new Car( 1, 'black' ) ]}]" ));
});
test("test isBalanced for bad values", function(){
    var func = isBalanced;
    ok(!func( "{" ));
    ok(!func( "{]" ));
    ok(!func( "{}(" ));
    ok(!func( "({)()()[][][}]" ));
    ok(!func( "[//]" ));
    ok(!func( "[/*]*/" ));
    ok(!func( "(function(){return [new Bears()}())];" ));
    ok(!func( "var a = [function(){return 'b';]}" ));
    ok(!func( "/*Comment: a = [} is bad */var a = function({)return 'b';}" ));
    ok(!func( "/*[[[ */ function(){return {b:(function(x){ return x+1; })'c')}} /*_)(([}*/" ));
    ok(!func( "//Complex object;\n a = [{a:1,b:2,c:[ new Car( 1, 'black' ) ]]" ));
});

Demo: http://jsfiddle.net/9esyk/3/


TDD workflow best-practices for .NET using NUnit I'm starting to dive into TDD with NUnit and despite I've enjoyed checking some resources I've found here at stackoverflow, I often find myself not gaining good traction.

So what I'm really trying to achieve is to acquire some sort of checklist/workflow —and here's where I need you guys to help me out— or "Test Plan" that will give me decent Code Coverage.

So let's assume an ideal scenario where we could start a project from scratch with let's say a Mailer helper class that would have the following code:

(I've created the class just for the sake of aiding the question with a code sample so any criticism or advice is encouraged and will be very welcome)

Mailer.cs

using System.Net.Mail;
using System;

namespace Dotnet.Samples.NUnit
{
    public class Mailer
    {
        readonly string from;
        public string From { get { return from; } }

        readonly string to;
        public string To { get { return to; } }

        readonly string subject;
        public string Subject { get { return subject; } }

        readonly string cc;
        public string Cc { get { return cc; } }

        readonly string bcc;
        public string BCc { get { return bcc; } }

        readonly string body;
        public string Body { get { return body; } }

        readonly string smtpHost;
        public string SmtpHost { get { return smtpHost; } }

        readonly string attach;
        public string Attach { get { return Attach; } }

        public Mailer(string from = null, string to = null, string body = null, string subject = null, string cc = null, string bcc = null, string smtpHost = "localhost", string attach = null)
        {
            this.from = from;
            this.to = to;
            this.subject = subject;
            this.body = body;
            this.cc = cc;
            this.bcc = bcc;
            this.smtpHost = smtpHost;
            this.attach = attach;
        }

        public void SendMail()
        {
            if (string.IsNullOrEmpty(From))
                throw new ArgumentNullException("Sender e-mail address cannot be null or empty.", from);

            SmtpClient smtp = new SmtpClient();
            MailMessage mail = new MailMessage();
            smtp.Send(mail);
        }
    }
}

MailerTests.cs

    using System;
    using NUnit.Framework;

    namespace Dotnet.Samples.NUnit
    {
        [TestFixture]
        public class MailerTests
        {
            [Test]
            public void SendMail_FromArgumentIsNotNull_ReturnsTrue()
            {
                // Arrange
                string argument = null;
                // Act
                Mailer mailer = new Mailer(from: argument);
                // Assert
                Assert.IsNotNull(mailer.From, "parameter 'from' cannot be null.");
            }

            [Test]
            public void SendMail_FromArgumentIsNotEmpty_ReturnsTrue()
            {
                // Arrange
                string argument = String.Empty;
                // Act
                Mailer mailer = new Mailer(from: argument);
                // Assert
                Assert.IsNotEmpty(mailer.From, "parameter 'from' cannot be empty.");
            }

            [Test]
            public void SendMail_FromArgumentIsNullOrEmpty_ThrowsException()
            {
                // Arrange
                dynamic argument = null;
                // Act
                Mailer mailer = new Mailer(from: argument);
                Action act = () => mailer.SendMail();
                // Assert
                Assert.Throws<ArgumentNullException>(new TestDelegate(act));
            }
        }
    }

So after having my first 2 failing tests the next obvious step would be implementing the functionality to make them pass, but, should I keep the failing tests and create new ones after implementing the code that will make those pass, or should I modify the existing ones after making them pass?

Any advice about this topic will really be enormously appreciated.

It is important to get one's mindset in a TDD mode beforehand. What I mean by this is

1) Identify the responsibility of a class - this helps in identifying the test cases.
2) Identify the interactions of the class with external dependencies - Unit testing shouldn't be testing the behaviour of the external dependency.

In this case - you have written a wrapper class around SendMail whose responsibility is to "Collect and Despatch".

I'd say SmtpClient is a dependency - you should be able to Mock it's behaviour in your test.

Mailer.cs

using System.Net.Mail;
using System;

namespace Dotnet.Samples.NUnit
{
    public class Mailer
    {
        readonly string from;
        public string From { get { return from; } }

        readonly string to;
        public string To { get { return to; } }

        readonly string subject;
        public string Subject { get { return subject; } }

        readonly string cc;
        public string Cc { get { return cc; } }

        readonly string bcc;
        public string BCc { get { return bcc; } }

        readonly string body;
        public string Body { get { return body; } }

        readonly string smtpHost;
        public string SmtpHost { get { return smtpHost; } }

        readonly string attachment;
        public string Attachment { get { return Attachment; } }

        private SmtpClient smtpClient;

        public Mailer(string from = null, string to = null, string body = null, string subject = null, string cc = null, string bcc = null, string smtpHost = "localhost", string attachment = null, SmtpClient smtpClient= new SmtpClient())
        {
            this.from = from;
            this.to = to;
            this.subject = subject;
            this.body = body;
            this.cc = cc;
            this.bcc = bcc;
            this.smtpHost = smtpHost;
            this.attachment = attachment;
        }

        public void SendMail()
        {
            if (string.IsNullOrEmpty(From))
                throw new ArgumentNullException("Sender e-mail address cannot be null or empty.", from);

            MailMessage mail = new MailMessage();
            smtpClient.Send(mail);
        }
    }
}

Testing SendMail

[Test]
public voud ShouldCallSendOfSmtpClientWhenSendMailIsCalled
{
    //Stub & Setup
    var mockSmtpClient = MockRepository.GenerateMock<SmtpClient>();
    var mailer = new Mailer(smtpClient: mockSmtpClient);

    //Act
    var message = new MailMessage();
    mailer.SendMail(message);

    //Assert
    mockSmtpClient.AssertWasCalled(m=>m.Send(Arg<MailMessage>.Is.Anything));
}


Django functions to sanitize URL, return welcome message, redirect, compute HMAC, and manage profile picture I wrote this code to to perform the following:

I will remove the "spaces" within the url.
Return welcome message.
Redirect the user to different url.
Compute HMAC for the request.
Return and revoke the user profile picture.

Would someone please help me in checking if my code contains any security issues that I should avoid?

import base64
import mimetypes
import os
import hashlib
import hmac
import requests

from django.core.urlresolvers import reverse
from django.http import HttpResponse
from django.shortcuts import redirect, render
from django.views.decorators.csrf import csrf_exempt

def ordenary(s):
     return s.strip().replace(' ', '').lower()

def form_of_message(request):
     env = {'message': request.GET.get('message', 'hello')}
     response = render(request, 'forms/message_form.html', env)
     response.set_cookie(key='message_rendered_at', value=time.time())
     return response

 def proxy(request):
     url = request.GET.get('url')
     return redirect(url)

 def compute_hmac_signature(message, key):
     key = bytes(key, 'UTF-8')
     message = bytes(message, 'UTF-8')

     digest = hmac.new(key, message, hashlib.sha1).hexdigest()
     return "sha1={}".format(str(digest))

 def user_pic(request):
     """A view that returns the user's avatar image"""

     base_path = os.path.join(os.path.dirname(__file__), '../../images/avatars')
     filename = request.GET.get('u')

     try:
         data = open(os.path.join(base_path, filename), 'rb').read()
     except IOError:
         return render(request, 'templates/avatar.html')

     return HttpResponse(data, content_type=mimetypes.guess_type(filename)[0])

Some comments, with security issues highlighted:

Why would you remove whitespace from URLs? It is perfectly valid for URLs to contain whitespace, unencoded or encoded as + or %20. If your backend code can't handle whitespace there are much bigger fish to fry. In general:

Blacklisting is more dangerous than whitelisting, because it's easy to omit some values.
Whitelisting parts of values (such as characters in a URL) is more dangerous than whitelisting entire values, because clever hackers often find a way to escape the sandboxing attempt by combining parts. For example, depending on how you pass values to your backend, %20 in a URL may or may not be replaced with a space character.
Blanket removal of parts of the input limits your application severely while providing very little security (because of the above). A better solution is to escape the input and test the escaping using previously insecure values. This escaping should be provided by the official libraries (such as SQL or shell escaping).
An even better solution is to let the library do the escaping for you, for example with parametric SQL queries, because now you don't need to remember to escape every value yourself, only to use parameters rather than string concatenation.

Why would you lowercase the URL? There's no security issue with uppercase characters that I've ever heard of. And again, limiting the URL space like this is a bad trade-off.
Redirecting to a URL without checking it against a strict whitelist first is a well-known security issue.
Similarly, not validating the username parameter is a massive issue - what if I specify something like u=../../etc/passwd?
Why would you implement your own HMAC generator? It's built right into the standard library, even in Python 2.
What is the purpose of the message_rendered_at cookie?
Guessing the MIME type indicates that someone could send a JavaScript, PDF or other potentially harmful file as their avatar. If this is shown to other people that could lead to remote code injection. This has already happened on many sites with Bitcoin mining scripts. At best you should instead convert all avatars to a single format (PNG is ideal for this) and serve that. If you really need some flexibility then allow a small set of well-known formats and change the file extension when saving it. Then use a dict to map file extension to MIME type.
ordenary is not an English word as far as I know. ordinary would not be a good function name either, because it gives the programmer no idea what it actually does. One thing I've found useful to determine better names for things is to use type hints and validate them using mypy with strict settings (not allowing Any type). It takes some getting used to, but doing this properly should tell you exactly what goes into and comes out of any function. A function which takes a Request to /foo and returns a Response, for example, could be a respond_to_foo_request.
Some of the imports are unused.


Google FooBar Level 3 - Shortest path to 1 I recently found the Google Foobar challenges and am having a problem with level 3. The goal of the task is the find the minimum number of operations needed to transform a group of pellets down into 1, while following these rules:

  1) Add one fuel pellet

  2) Remove one fuel pellet

  3) Divide the entire
  group of fuel pellets by 2 (due to the destructive energy released
  when a quantum antimatter pellet is cut in half, the safety controls
  will only allow this to happen if there is an even number of pellets)

My code is as follows:

from math import log, ceil, floor
def answer(n):
    steps = 0
    steps_taken = " => {}"
    all_steps = "{}".format(n)
    n = int(n)
    if n < 1:
        return 0
    while n > 1:
        if n == 3:
            n -= 1
            all_steps += steps_taken.format(n)
            steps += 1
        elif n % 2 == 1:
            if n + 1 == 2**closest_power(n+1):
                n += 1
                all_steps += steps_taken.format(n)
            elif n - 1 == 2**closest_power(n-1):
                n -= 1
                all_steps += steps_taken.format(n)
            else:
                n -= 1
                all_steps += steps_taken.format(n)
            steps += 1
        else:
            n = n / 2
            all_steps += steps_taken.format(n)
            steps += 1

    print all_steps
    return steps

def closest_power(x):
    possible_results = floor(log(x, 2)), ceil(log(x, 2))
    return int(min(possible_results, key= lambda z: abs(x-2**z)))

This manages to perform the two test cases provided:

Inputs:

(string) n = "4"

Output:

(int) 2

Inputs:

(string) n = "15"

Output:

(int) 5

But it fails on others, that Google doesn't make available. Since I don't have access to the failing test cases, I'm looking for performance suggestions.

Right now the code is making the following assumptions:

If the number is even, divide it by two. This should reduce the amount of space left to reach 1
If a number is odd, check if the next and previous numbers are a power of 2. If so, add or subtract one to get to that number. Since powers of 2 can be divided repeatedly, there will be no more adding/subtracting of pellets
If neither neighbor is a power of two, subtract one. I am betting this is the assumption that is causing problems. I don't have the mathematical background to provide it, but my thought was that moving closer to 1 is better.
If the number is 3, subtract 1. Since both of its neighbors are a power of two, this is a special case. Additionally, moving from 3->2->1 is shorter than 3->4->2->1.

Your intuition is correct. A blind subtraction is the root cause of the problem.

The odd number has the least significant bit equals to 1. An even number has it equal to 0. The more zeroes a number has at the tail, the more divisions you can perform (this is what makes powers of 2 so special - they have all bits but most significant equal to 0). So what you really want is to compute number of trailing 0 bits in n + 1 and n - 1 and pick one which has more:

    tz0 = trailing_zero_bits(n + 1)
    tz1 = trailing_zero_bits(n - 1)
    if tz0 > tz1:
        n = n + 1
    else:
        n = n - 1

Notice how it eliminates computation of closest power of 2.

I leave the implementation of trailing_zero_bits for you. Hint: it is a one-line bit manipulation.


Concept based polymorphism I have a concept based polymorphism example listed below. I allow the user to provide any type that implements the draw method and then I add it into a vector of unique_ptr to concept base. If I have a pointer or reference I want to be able to deal with that even though the solution has value semantics in mind. I thus created a wrapper type which forwards to the correct implementation. I am hoping to get some feedback on if this is the best way to deal with the issue at hand.

#include <iostream>
#include <vector>
#include <memory>
#include <algorithm>

class drawable_concept{
public:
    drawable_concept() = default;
    virtual ~drawable_concept() = default;
    drawable_concept(drawable_concept const&) = delete;
    drawable_concept& operator = (drawable_concept const& ) = delete;
    virtual void draw() = 0;
};

template<class T>
class drawable_model : public drawable_concept{
public:
    typedef T model_type;
    drawable_model(T const& model) : model_(model){}
    void draw(){
        model_.draw();
    }
    ~drawable_model() = default;
private:
    T model_;
};

template<class T>
class drawable_forwarder{
public:

    drawable_forwarder(T const& item) : item_(item){}

    inline void draw(){
        item_->draw();
    }
private:
    T item_;
};

class graphics_surface{
public:
    void render(){
        std::for_each(std::begin(controls_), std::end(controls_), [] (std::unique_ptr<drawable_concept> const& control){
            control->draw();
        });
    }

    template<class T>
    void push_back(T control){
        auto t = new drawable_model<T>(std::move(control));
        controls_.push_back(std::unique_ptr<drawable_concept>(t));
    }
private:
    std::vector<std::unique_ptr<drawable_concept>> controls_;
};

struct triangle{
    void draw(){
        std::cout << "Triangle" << std::endl;
    }
};

struct square{
    void draw(){
        std::cout << "Square" << std::endl;
    }
};

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

    graphics_surface surface;
    surface.push_back(triangle());
    surface.push_back(square());

    std::shared_ptr<triangle> ptr(new triangle);
    drawable_forwarder<std::shared_ptr<triangle>> fwd(ptr);
    surface.push_back(fwd);

    surface.render();

    return 0;
}

There is not much to be said actually, the concept-based polymorphism seems to be well implemented. You can slightly improve your push_back method by having it emplace_back the std::unique_ptr. It will be a little bit less verbose:

template<class T>
void push_back(T control){
    auto t = new drawable_model<T>(std::move(control));
    controls_.emplace_back(t);
}

Also, I suppose that a draw shouldn't alter the object being drawn. Therefore, you better const-qualify all of your draw methods.

You could also be more consistent with the way you use inline: you inlined drawable_forwarder's draw method but not drawable_model while it basically does the same thing.

One fun addition would be to write a method that would create the drawable_model object in-place. That would require to add the following constructor to drawable_model:

template<typename... Args>
drawable_model(Args&&... args):
    model_(std::forward<Args>(args)...)
{}

And the following method to graphics_surface (not sure about the name):

template<typename T, typename... Args>
void emplace_back(Args&&... args)
{
    auto t = new drawable_model<T>(std::forward<Args>(args)...);
    controls_.emplace_back(t);
}

Then, you could use it this way:

int main()
{
    graphics_surface surface;

    // Assume triangle and rectangle have complete constructors
    surface.emplace_back<triangle>(3.5, 6.8, 4.8, 3.2);
    surface.emplace_back<square>(/* whatever */);

    surface.render();
}


Platform for creating data-oriented web apps In my web development company, we have many designers who have the following development knowledge:

Client-side web languages (HTML, CSS, JavaScript)
Basic database design (how to create tables, relationships, etc), and
Basic SQL

So we thought about giving them a simple platform to create data-oriented web apps.

What do you think of the following coding style for ASP.NET MVC?
How can we improve it?
Any ideas for a better SQL injection solution than the params array? Remember that the code should look clean and simple.

/apps/pages/index.html

@(DB.Select(table: "pages",
            where: "id = ? and date > ?",
            params: new[] { Request.QueryString["ID"], Request.QueryString["Date"] }))

    @foreach (var row in Model) {

@row.Name

    }

/apps/pages/create.html

@(DB.InsertOnPost(table: "pages",
                  fields: "name, description, date",
                  values: "?, ?, Now",
                  params: new[] { Request.Form["Name"], Request.Form["Description"] }))

@RedirectOnPost("/pages")

<form action="/pages/create" method=”post”>

        Name
        <input type="text" name="name" />

        Description
        <textarea name="description"></textarea>

</form>

/apps/pages/edit.html

@(DB.UpdateOnPost(table: "pages",
                  set: "name = ?, description = ?",
                  where: "id = ?",
                  params: new[] { Request.Form["Name"], Request.Form["Description"],
                                  Request.QueryString["ID"] }))

@RedirectOnPost("/pages")

@(DB.Select(table: "pages",
            where: "id = ?",
            params: new[] { Request.QueryString["ID"] })

<form action="/pages/edit" method="post">

        Name
        <input type="text" name="name" value="@Model.Name" />

        Description
        <textarea name="description">@Model.Description</textarea>

</form>

That coding style looks more like the WebMatrix Web Pages pattern rather than ASP.Net MVC. If this coding style is desired then it would be preferable to go ahead and use the data access library that is usually used with Web Pages, the WebMatrix.Data library. That will by default guard against SQL Injection as long as the convention of the parameterized queries is used, e.g.

    selectCommand = "SELECT URL, Name FROM Pages WHERE ID = @0 AND Date = @1";
    selectedData = db.Query(selectCommand, Request.QueryString["Id"], Request.QueryString["Date"]);

This should be OK for simple sites, but for larger or more complex sites you would be better to separate the data access from the presentation layer, using the full MVC pattern.  The downside of the pattern shown here is that is difficult to test or debug compared to MVC or WebForms and it tends to introduce duplication in the code.


Handling default value for int64_t representing an edge in an adjacency matrix I'm writing an adjacency matrix, in this moment I'm trying to figure out how to indicate a default numeric value for an unassigned edge. There is the macro _I64_MAX but it is said that it's a bad practice to use macros. I wish to know if there's other way to assign such value (as a keyword as in the case of nullptr instead of NULL) or if what I'm doing is OK.
Why don't I use 0? Because the distance for an edge which start and end vertex is the same is:
$$e=(v_i,v_i);~~~||e||=0$$
AdjacencyMatrix::AdjacencyMatrix(const SinglyLinkedList &vertices) : vertices_(vertices)
{
    this->edge_matrix_ = new int64_t *[this->vertices_.Size()];
    for (size_t i = 0; i < this->vertices_.Size(); i++)
    {
        this->edge_matrix_[i] = new int64_t[this->vertices_.Size()];
        for (size_t j = 0; j < this->vertices_.Size(); j++)
        {
            this->edge_matrix_[i][j] = _I64_MAX;
        }
    }
}

Use of macros
In this case, the macro you using is from a library header that was included with your compiler. This header (ultimately limits.h, although <climits> is usually used with C++ code) is also used by C code, so C++ specific language features cannot be used. These are OK to use.
The C++ way of referencing a minimum value for a type is to make use of std::numeric_limits (in the <limits>) header. In your case, you can replace _I64_MAX with std::numeric_limits<int64_t>::min().
Other notes
All of the references to this-> are unnecessary and can be removed from your code. You rarely need to specify this in C++ code (there a few specific cases where it is needed, like for variable name disambiguation or calling a function thru a member function pointer).
The for loop might be able to be rewritten using iterators or the range-for style if this is supported by the SinglyLinkedList type.


Split excel file with multiple sheets, manipulate the data and create final out file I have an excel file with 20+ separate sheets containing tables of data. My script iterates through each sheet, manipulates the data into the format I want it and then saves it to a final output file. I think it can be improved, and I've flagged sections in the code with "review this" which I think I've done more work than I've needed to. Any feedback or criticism would be awesome!

import openpyxl
import pandas as pd

path = 'C:/Desktop/Python/Excel Manipulation/'

wb = openpyxl.load_workbook(path + 'inputfile.xlsx')
sheets = wb.get_sheet_names()

CSVList = []

for sheet in sheets:

    #get the current active sheet
    active_sheet = wb.get_sheet_by_name(sheet)

    #count numbers of rows
    row_count = active_sheet.get_highest_row() - 1
    #count number of columns
    column_count = active_sheet.get_highest_column()

    count = 0
    values = []

    #write each row to a list, stop when reached max rows (REVIEW THIS - would have thought there was a better way than using a counter)
    while count <= row_count:
        for i in active_sheet.rows[count]:
            values.append(i.value)
        count = count + 1

    #split values list into tuples based on number of columns
    split_rows = zip(*[iter(values)]*column_count)

    #convert list of tuples to list of lists  (REVIEW THIS - creating a tuple and then converting to list seems like extra work?!?)
    rows = [list(elem) for elem in split_rows]

    #get elements of file and store (REVIEW THIS - looks messy?)
    title = rows.pop(0)[0]
    headers = rows.pop(0)
    headers[1] = 'Last Year'
    rows.pop(0)

    #create pandas dataframe
    df = pd.DataFrame(rows, columns=headers)

    #take header_id and remove to normalise the data
    header_id = headers.pop(2)

    normalise_data = pd.melt(df, id_vars=header_id, value_vars=headers, var_name='Measure', value_name='Value')
    normalise_data.insert(0, 'Subject', title)
    CSVList.append(normalise_data)

frame = pd.concat(CSVList)
frame.to_csv(path + 'CSV Outputs/' + 'final.csv', sep=',', index=False)

Found a better way to iterate over the rows (although I still feel like I'm repeating myself!):

for idx, row in enumerate(active_sheet.rows):
    for i in active_sheet.rows[idx]:
        values.append(i.value)

And instead of converting the tuple to a list:

rows = [list(t) for t in zip(*[iter(values)]*column_count)]


Unity3D script for controlling a character This is a Unity3D script I wrote for my game to control a character. I don't consider myself a highly-skilled coder, so I come here to ask for suggestions. Is my code clean enough? How can I improve my coding style and pattern?

using UnityEngine;

public enum OnWallStatus
{
    None = 0,
    OnLeft = 1,
    OnRight = 2
}

public enum OnGroundStatus
{
    OnGround = 0,
    InAir = 1
}

public class MoveControl : MonoBehaviour {

    private const float minSpeed = 1f;
    private const float maxSpeed = 5f;
    private const float minJump = 1f;
    private const float maxJump = 10f;

    [SerializeField]
    private float speedModifier;
    [SerializeField]
    private float jumpModifier;

    private new Rigidbody2D rigidbody2D;
    private new Transform transform;

    private OnGroundStatus _onGroundStatus;
    private OnWallStatus _onWallStatus;

    public OnGroundStatus onGroundStatus
    {
        get { return _onGroundStatus; }
    }

    public OnWallStatus onWallStatus
    {
        get { return _onWallStatus; }
    }

    private bool isHorizontalStill
    {
        get { return (rigidbody2D.velocity.x == 0); }
    }

    private bool isVerticalStill
    {
        get { return (rigidbody2D.velocity.y == 0); }
    }

    void Awake ()
    {
        speedModifier = Mathf.Clamp(speedModifier, minSpeed, maxSpeed);
        jumpModifier = Mathf.Clamp(jumpModifier, minJump, maxJump);

        rigidbody2D = GetComponent<Rigidbody2D>();
        transform = GetComponent<Transform>();

        _onGroundStatus = OnGroundStatus.OnGround;
        _onWallStatus = OnWallStatus.None;
    }

    public void MoveHorizontal(float speed)
    {
        speed = Mathf.Clamp(speed, -1f, 1f);
        rigidbody2D.AddForce(new Vector2(speed * speedModifier * 10f, 0f));

        Vector2 velocity = rigidbody2D.velocity;
        velocity.x = Mathf.Clamp(velocity.x, -1f * speedModifier, 1f * speedModifier);
        rigidbody2D.velocity = velocity;
    }

    public void MoveJump(float speed)
    {
        if (_onWallStatus == OnWallStatus.None && _onGroundStatus == OnGroundStatus.InAir)
            return;

        speed = Mathf.Clamp(speed, 0f, 1f);

        if (_onGroundStatus == OnGroundStatus.OnGround)
        {
            rigidbody2D.velocity = new Vector2(rigidbody2D.velocity.x, speed * jumpModifier);
        }
        else if(_onWallStatus == OnWallStatus.OnLeft)
        {
            rigidbody2D.velocity = new Vector2(1f * speedModifier, speed * jumpModifier);
        }
        else if (_onWallStatus == OnWallStatus.OnRight)
        {
            rigidbody2D.velocity = new Vector2(-1f * speedModifier, speed * jumpModifier);
        }
    }

    public void MoveLand(float speed)
    {
        if (_onWallStatus != OnWallStatus.None || _onGroundStatus != OnGroundStatus.InAir)
            return;

        speed = Mathf.Clamp(speed, -1f, 0f);
        rigidbody2D.velocity = new Vector2(rigidbody2D.velocity.x, speed * jumpModifier);
    }

    void OnCollisionEnter2D(Collision2D collision)
    {
        if (collision.gameObject.tag != "Platform")
            return;

        if (collision.contacts[0].normal.y > 0)
        {
            _onGroundStatus = OnGroundStatus.OnGround;
        }
        else
        {
            if(collision.contacts[0].normal.x < 0)
            {
                _onWallStatus = OnWallStatus.OnRight;
            }
            else
            {
                _onWallStatus = OnWallStatus.OnLeft;
            }
        }
    }

    void OnCollisionExit2D(Collision2D collision)
    {
        if (collision.gameObject.tag != "Platform")
            return;

        if (collision.contacts[0].normal.y > 0)
        {
            _onGroundStatus = OnGroundStatus.InAir;
        }
        else
        {
            _onWallStatus = OnWallStatus.None;
        }
    }

}

You don't need this directive in the code you have given us.

using System.Collections;

It's conventional to use Pascal casing to name private constants in a file, so

private const float minSpeed = 1f;

private const float maxSpeed = 5f;

private const float minJump = 1f;

private const float maxJump = 10f;

should be:

private const float MinSpeed = 1f;

private const float MaxSpeed = 5f;

private const float MinJump = 1f;

private const float MaxJump = 10f;

In C# it's convention that private instance fields start with an underscore.

[SerializeField]

private float _speedModifier;

[SerializeField]

private float _jumpModifier;

I would also consider writing 'getter' and 'setter' methods for each field instead of serializing them.

I believe public properties also use Pascal case conventionally.

public OnGroundStatus onGroundStatus

{

    get { return _onGroundStatus; }

}

public OnWallStatus onWallStatus

{

    get { return _onWallStatus; }

}

If you preferred you could use auto-properties to set these instead.

public OnGroundStatus OnGroundStatus { get; private set; }

public OnWallStatus OnWallStatus { get; private set; }

I don't see a usage of these properties in your code.

private bool isHorizontalStill

{

    get { return (rigidbody2D.velocity.x == 0); }

}

private bool isVerticalStill

{

    get { return (rigidbody2D.velocity.y == 0); }

}


Any better way to solve Project Euler Problem #5? Here's my attempt at Project Euler Problem #5, which is looking quite clumsy when seen the first time. Is there any better way to solve this? Or any built-in library that already does some part of the problem?

'''
    Problem 5:
    2520 is the smallest number that can be divided by each of the numbers from 1 to 10

    What is the smallest number, that is evenly divisible by each of the numbers from
    1 to 20?
'''

from collections import defaultdict

def smallest_number_divisible(start, end):
    '''
        Function that calculates LCM of all the numbers from start to end
        It breaks each number into it's prime factorization,
        simultaneously keeping track of highest power of each prime number
    '''
    # Dictionary to store highest power of each prime number.
    prime_power = defaultdict(int)

    for num in xrange(start, end + 1):
        # Prime number generator to generate all primes till num
        prime_gen = (each_num for each_num in range(2, num + 1) if is_prime(each_num))

        # Iterate over all the prime numbers
        for prime in prime_gen:
            # initially quotient should be 0 for this prime numbers
            # Will be increased, if the num is divisible by the current prime
            quotient = 0

            # Iterate until num is still divisible by current prime
            while num % prime == 0:
                num = num / prime
                quotient += 1

            # If quotient of this priime in dictionary is less than new quotient,
            # update dictionary with new quotient
            if prime_power[prime] < quotient:
                prime_power[prime] = quotient

    # Time to get product of each prime raised to corresponding power
    product = 1

    # Get each prime number with power
    for prime, power in prime_power.iteritems():
        product *= prime ** power

    return product

def is_prime(num):
    '''
        Function that takes a `number` and checks whether it's prime or not
        Returns False if not prime
        Returns True if prime
    '''
    for i in xrange(2, int(num ** 0.5) + 1):
        if num % i == 0:
            return False
    return True

if __name__ == '__main__':
    print smallest_number_divisible(1, 20)

    import timeit
    t = timeit.timeit

    print t('smallest_number_divisible(1, 20)',
             setup = 'from __main__ import smallest_number_divisible',
             number = 100)

While I timed the code, and it came out with a somewhat ok result. The output came out to be:

0.0295362259729  # average 0.03

Any inputs?

You are recomputing the list of prime numbers for each iteration. Do it just once and reuse it. There are also better ways of computing them other than trial division, the sieve of Eratosthenes is very simple yet effective, and will get you a long way in Project Euler. Also, the factors of n are all smaller than n**0.5, so you can break out earlier from your checks.

So add this before the num for loop:

prime_numbers = list_of_primes(int(end**0.5))

And replace prime_gen with :

prime_gen =(each_prime for each_prime in prime_numbers if each_prime <= int(num**0.5))

The list_of_primes function could be like this using trial division :

def list_of_primes(n)
    """Returns a list of all the primes below n"""
    ret = []
    for j in xrange(2, n + 1) :
        for k in xrange(2, int(j**0.5) + 1) :
            if j % k == 0 :
                break
        else :
            ret.append(j)
    return ret

But you are better off with a very basic sieve of Erathostenes:

def list_of_primes(n) :
    sieve = [True for j in xrange(2, n + 1)]
    for j in xrange(2, int(sqrt(n)) + 1) :
        i = j - 2
        if sieve[j - 2]:
            for k in range(j * j, n + 1, j) :
                sieve[k - 2] = False
    return [j for j in xrange(2, n + 1) if sieve[j - 2]]

There is an alternative, better for most cases, definitely for Project Euler #5, way of going about calculating the least common multiple, using the greatest common divisor and Euclid's algorithm:

def gcd(a, b) :
    while b != 0 :
        a, b = b, a % b
    return a

def lcm(a, b) :
    return a // gcd(a, b) * b

reduce(lcm, xrange(start, end + 1))

On my netbook this gets Project Euler's correct result lightning fast:

In [2]: %timeit reduce(lcm, xrange(1, 21))
10000 loops, best of 3: 69.4 us per loop


Prime number generator in Python I have made a Python program that can perform some prime number functions. For example, it can produce an endless output of sequential primes. I am looking for ways to make it faster and cleaner. When I was making it, I had a huge amount of trouble using generators, I don't know why, and what the problems were, because I made it long ago, so I had to stick to for loops and class variables in the end.

class Maths():
    prime = [2]
    squares = []
    primenum = 3
    sqrnum = 1
    fermat_number_true = 0
    fermat_number_false = 1

    def prime_generator(self):
        while True:
            if self.is_prime(self.primenum):
                self.prime.append(self.primenum)
                self.primenum += 2
                return self.primenum - 2
            self.primenum += 2

    def is_prime(self, num):
        for divisor in self.prime:
            quotient = num / divisor
            if quotient == int(quotient):
                return False
        return True

    def square_generator(self):
        while True:
            self.sqrnum += 1
            return (self.sqrnum - 1) * (self.sqrnum - 1)

    def square_output(self):
        while True:
            print(self.square_generator())

    def prime_output(self):
        while True:
            print(self.prime_generator())

    def prime_on_enter(self):
        while True:
            input()
            print(self.next_prime())

    def square_on_enter(self):
        while True:
            input()
            print(self.next_square())

    def next_prime(self):
        return self.prime_generator()

    def next_square(self):
        return self.square_generator()

    def next_fermat(self):
        while True:
            prime = self.next_prime()
            try:
                while self.squares[-1] < prime:
                    self.squares.append(self.next_square())
            except IndexError: self.squares.append(self.next_square())
            if self.isfermat_alg2(prime):
                self.fermat_number_true += 1
                return True, prime
            else:
                self.fermat_number_false += 1
                return False, prime
    def next_fermatf(self):
        true, num = self.next_fermat()
        if true:
            print("%d\t\t%f" % (num, (self.fermat_number_true / (self.fermat_number_false + self.fermat_number_true)) * 100))
        else:
            print("%d\tX\t%f" % (num, (self.fermat_number_true / (self.fermat_number_false + self.fermat_number_true)) * 100))

    def fermat(self):
        while True: self.next_fermatf()

    def isfermat_alg1(self, num, squares):
        for square in squares:
            if num > square:
                difference = num - square
                if difference in squares:
                    return True
        else:
            return False

    def isfermat_alg2(self, num):
        return bool(num % 4 == 1)

def main(): mathsobj.prime_output()

mathsobj = Maths()

if __name__ == '__main__':
    main()

A few quick notes:

In case of square_generator() and next_fermat() you have unnecessary...

while True:

Anytime all paths through the loop encounter a return or break, the loop will never loop.

This:

 return bool(num % 4 == 1)

can be:

 return num % 4 == 1

since the == will always produce a bool.

I would code:

def next_fermatf(self):
    true, num = self.next_fermat()
    if true:
        print("%d\t\t%f" % (num, (self.fermat_number_true / (self.fermat_number_false + self.fermat_number_true)) * 100))
    else:
        print("%d\tX\t%f" % (num, (self.fermat_number_true / (self.fermat_number_false + self.fermat_number_true)) * 100))

more like:

def next_fermatf(self):
    is_next_fermat, num = self.next_fermat()

    msg = "" if is_next_fermat else "X"
    print("%d\t%s\t%.1f%%" % (num, msg,
        100.0 * self.fermat_number_true /
        (self.fermat_number_false + self.fermat_number_true))))

Changing true to is_next_fermat allows if is_next_fermat to be read, while if true reads more awkwardly.
Eliminate common code
Keep the code below the pep8 recommended 80 columns
Format the % with fixed decimal and show the units.


DisjointSet class in Python I would like to improve my DisjointSet class implementation in Python. I would be grateful for specific advice. I use it in online contests so it must be short, quick and reusable.

from copy import copy
class DisjointSet:

  def __init__(self, s=set()):
    self.__par = {x: x for x in s}
    self.__csz = dict.fromkeys(s, 1)
    self.__root = set(s)

  def root(self, x=None):
    if x is None:
      return(copy(self.__root))
    r = x
    while r != self.__par[r]:
      r = self.__par[r]
    while x != r:
      p = self.__par[x]
      self.__par[x] = r
      x = p
    return(r)

  def add(self, x):
    if x not in self.__par:
      self.__par[x] = x
      self.__csz[x] = 1
      self.__root.add(x)

  def connected(self, x, y):
    return(self.root(x) == self.root(y))

  def connect(self, x, y):
    rx, ry = self.root(x), self.root(y)
    if rx == ry:
      return(rx)
    if self.__csz[rx] < self.__csz[ry]:
      rx, ry = ry, rx
    self.__par[ry] = rx
    self.__csz[rx] += self.__csz[ry]
    self.__root.remove(ry)
    return(rx)

  def size(self, x=None):
    if x is None:
      return(len(self.__root))
    else:
      return(self.__csz[self.root(x)])

  def __len__(self):
    return(len(self.__par))

Simple test

from random import randint
#ds = DisjointSet(xrange(1000000))
ds = DisjointSet()
for x in xrange(1000000):
  ds.add(x)
print len(ds)
print ds.size()
for __ in xrange(1000000):
  ds.connect(randint(0, 999999), randint(0, 999999))
print ds.root(0)
print sum([ds.size(x) for x in ds.root()])
print ds.size()

I'll edit this once it's posted what the code is supposed to do.

The code is extremely cryptic. There are no comments; the variable & membervariable names are all abbreviations. Someone else looking at this code will have no idea what is supposed to be happening. At the very least there should be docstrings at the start of each function/method that describe what the function is used for and the purposes of each variable.

The only possible performance improvement I see is to combine the __par, __csz, and __root members since they all have the same keys. That way the same element doesn't need to be hashed multiple times. The data structure, instead of being split between three dictionaries each of which being a member variable/field, would just have one dictionary with appropriate keys and the values being a list of [parent, csz, root].

def __init__(self, s=set()):
  self.__data = {x: [x, 1, x] for x in s}

def add(self, x):
  if x not in self.__data:
    self.__data[x] = [x, 1, x]

If you need one of the members you just refer to self.__data[x][0] or whatever.


Spring service for playing Rock Paper Scissors I had created a service which does the actual play of Rock Paper Scissors.

Below is the service which takes a PlayRequest and updates the State of Player objects given by the PlayerService.

So the consumer of the API knows about winner, loser or tie by doing a GET on a Player.

One of the things I feel wrong NOW is that the method play takes a PlayerRequest but uses it only for two lines, all other work is done from the injected dependencies.

Would appreciate any comments related to State as well as SOLID principles.

@Service
public class GameplayService {

  private PlayerService playerService;

  private GameSessionService gameSessionService;

  public GameplayService(GameSessionService gameSessionService, PlayerService playerService) {
    this.gameSessionService = gameSessionService;
    this.playerService = playerService;
  }

  public void play(PlayRequest playRequest) throws RPSException {
    GameSession currentSession = gameSessionService.sessions().get(playRequest.getInviteCode());
    Player player = playerService.changePlayerState(playRequest.getPlayerName(), State.PLAYING);
    currentSession.changeStateTo(GameSession.State.PLAYING);
    try {
      Turn turn = new Turn(player, Enum.valueOf(Move.class, playRequest.getMove()));
      if (currentSession.rounds().isEmpty()) {
        createNewRound(turn, currentSession);
      } else if (!currentSession.rounds().isEmpty()) {
        Round latestRound = currentSession.latestRound();
        if (OVER.equals(latestRound.getState())) {
          createNewRound(turn, currentSession);
        } else if (PLAYING.equals(latestRound.getState())) {
          latestRound.pushLatestTurn(turn);
        }
      }
      Round latestRoundAfterAllUpdates = currentSession.latestRound();
      Optional<Result> resultOptional = latestRoundAfterAllUpdates.getResult();
      if (resultOptional.isPresent()) {
        currentSession.changeStateTo(GameSession.State.WAITING);
        Result result = resultOptional.get();
        if (result.isTie()) {
          currentSession.setTie(true);
        } else {
          currentSession.setWinner(result.getWinner());
        }
        currentSession.getFirstPlayer().changeStateTo(State.WAITING);
        currentSession.getSecondPlayer().changeStateTo(State.WAITING);
      }
    } catch (InvalidOperationException e) {
      throw new RPSException(e.getMessage());
    }
  }

  private void createNewRound(Turn turn, GameSession gameSession) {
    gameSession.addRound(new Round(turn));
  }

}

SOLID Principles

Single responsibility principle

  A class should have one, and only one, reason to change.

This class should take a PlayerRequest and update the state of the corresponding Players.

The public void play method does more than that, by changing the state of the current game, creating a new round or pushing the latest turn. It would be easier to read the code if the play method was just updating the states and no more. The management of the current Round and Turn should be delegated to a different dependency. Since the Responsibility of the Service class is to update the states, it would be acceptable to have another method wait changing the states to State.WAITING.

Creating unit tests for this class would be then much easier, as we would need only to test the expected change of states and everything else would be mocked.

Open-Closed Principle

  You should be able to extend a class’s behavior, without modifying it.

This block of code, even if moved to another class, could violate the Open-Closed Principle:

      Turn turn = new Turn(player, Enum.valueOf(Move.class, playRequest.getMove()));
      if (currentSession.rounds().isEmpty()) {
        createNewRound(turn, currentSession);
      } else if (!currentSession.rounds().isEmpty()) {
        Round latestRound = currentSession.latestRound();
        if (OVER.equals(latestRound.getState())) {
          createNewRound(turn, currentSession);
        } else if (PLAYING.equals(latestRound.getState())) {
          latestRound.pushLatestTurn(turn);
        }
      }

It is not clear from the code if there are more possible Round States than OVER and PLAYING, but if the Game was extended and there was a new State to be handled (e.g. an EXTRA round), this code block would need to be changed to address it. The solution is to depend upon an abstraction to handle the Round management. Concrete classes will then handle the implementation for the different possible Round States.

Liskov substitution principle

  Let \${\displaystyle \phi (x)}\$ be a property provable about objects \${\displaystyle x}\$ of type T. Then \${\displaystyle \phi (y)}\$ should be true for objects \${\displaystyle y}\$ of type S where S is a subtype of T.

Following this principle will be a consequence of the abstraction made for the previous principle. The derived classes will have to not alter the expected behavior of their parent classes. This principle will be ensured by adding the correct assertions (postconditions) in the unit tests.

Interface Segregation Principle

  Make fine grained interfaces that are client specific.

As @RoToRa suggested, we would need to see the injected interfaces. They should be fine grained in order that the clients (derived classes) will only implement the methods they really use.

Dependency Inversion Principle

  Depend on abstractions, not on concretions.

Here it is pretty straight-forward: the injected dependencies of the GameplayService class should be abstractions (i.e. abstract classes or interfaces). Then it would be easy to change the behavior of the class in case the rules of the Game are extended.

General comments

The best way to check if your code is clean and well-written is to cover it by Unit Tests, even better if it is done using TDD. The complexity of the entire class would be then reduced and the feel of adding collaborators and mocking their behavior would come easier.

Any possible null pointer exceptions, bugs or redundancies in the code would also be avoided during Unit Testing, like this unnecessary if condition of the else branch:

else if (!currentSession.rounds().isEmpty())

I would also use lambdas when working with Optionals and typical Java 8 features, e.g.:

resultOptional.ifPresent(result -> {...})


progress bar in python import sys
import time
import multiprocessing

def change_char_at(s: str, ch: str, i: int, j: int=None):
    '''strings are immutable so i made this handy-little function to 'mutate' strings
    similar to =>
    s[i] = ch    # if j is not specified
    s[i:j] = ch  # if j is specified

    *note*: negative indices does not work!
    '''
    if not j:
        j = i + 1
    return s[:i] + ch + s[j:]

def loader(width: int=20, bar_width: int=3):
    '''
    A simple loading bar which shows no information or whatever
    the function should be run in a different thread and must be killed.
    '''
    s = '[' + ' ' * (width - bar_width - 1) + ']'
    while 1:
        for i in range(1, width - bar_width - 1):
            sys.stdout.write('\r' + change_char_at(s, '===', i))
            sys.stdout.flush()
            time.sleep(0.1)
        for i in range(width - bar_width - 1, 0, -1):
            sys.stdout.write('\r' + change_char_at(s, '===', i))
            sys.stdout.flush()
            time.sleep(0.1)

def loader_with_wait(wait: int=None, width: int=20, bar_width: int=3):
    '''
    A simple loading bar which shows no information or whatever.
    it will quit after `wait` seconds or will *run forever* if wait is not specified

    param: wait: for how much time should the loading bar run.
    '''
    # Start the loader
    p = multiprocessing.Process(target=loader, name='loader', args=(width, bar_width))
    p.start()

    if wait:
        # let the loader run for `wait` seconds
        time.sleep(wait)
        # terminate the loader() function
        p.terminate()
        # Cleanup
        p.join()
        # Start from newline
        sys.stdout.write('\n')

if __name__ == '__main__':
    try:
        loader_with_wait(10)
    except KeyboardInterrupt:
        pass

Hey Guys!
I made a simple progress bar in python. The animation is cool!
it can wait for a few seconds or it can run forever, the choice is upto you.
it can be used in other programs and can run in a separate thread.
Implementing the wait function was the hardest thing to do. is there any other way to do it?

Any suggestions will be helpful...  :-)

see it in action: https://asciinema.org/a/323180

The variable names in change_char_at are terrible. s for the string, ch for the char? i for the start? j for the end.

Seriously just write out names.

def change_char_at(string: str, change: str, start: int, end: int = None):
    return string[:start] + change + string[end or start+1:]

Your code isn't fully typed:

change_char_at has no return type.
end in change_char_at and wait in loader_with_wait are assumed to be Optional but not specified to be optional.
loader and loader_with_wait have no return types should they be None or NoReturn?

The functionallity of change_char_at can be eclipsed by the format mini-language.

You have a standard structure where the bar is surrounded by [] and filled with the contents.

bar = '[{}]'

The content of your bar is left aligned by width.

bar = '[{: <{width}}]'

Each iteration you increase the left hand space by 1.

bar.format(' ' * i + '=' * bar_width, width=width)

bar_width doesn't actually change the size of the bar.
I would prefer the progress bar to be a generator function or list, so the rest of your code is simpler.

import itertools

def progress_bar(width: int = 20, bar_width: int = 3):
    bar = f'[{{: <{width}}}]'
    for i in itertools.chain(
        range(0, width - bar_width + 1),
        reversed(range(0, width - bar_width)),
    ):
        yield bar.format(' ' * i + '=' * bar_width)


BestHTTP asset download I'm using the asset BestHTTP in unity and I'm downloading a few image from amazonaws to use as texture. It used to be one image, but now I have a few.

I was not able to shorten it. When I'm trying to do it in a foreach loop loop it is misbehaving.

Could someone help me to optimize this working code?

for (int i = 0; i < tzPlayInfo.Instance.bc_gametablelist.Count; i++)
{
    dealer_img += tzPlayInfo.Instance.bc_gametablelist[i].dlrimage;
    dealer_img += ",";
}
string[] newLinks = dealer_img.Split(',');

new BestHTTP.HTTPRequest(new System.Uri("**************************.amazonaws.com/resources/"
    + "dealer/pic/" + newLinks[0]),
    (BestHTTP.HTTPRequest req, BestHTTP.HTTPResponse res)
    =>
    {
        var tex = new Texture2D(20, 20);
        tex.LoadImage(res.Data);
        uitex[0].GetComponent<UITexture>().mainTexture = tex;
    }).Send();

new BestHTTP.HTTPRequest(new System.Uri("**************************.amazonaws.com/resources/"
    + "dealer/pic/" + newLinks[1]),
    (BestHTTP.HTTPRequest req, BestHTTP.HTTPResponse res)
    =>
    {
        var tex = new Texture2D(20, 20);
        tex.LoadImage(res.Data);
        uitex[1].GetComponent<UITexture>().mainTexture = tex;
    }).Send();

new BestHTTP.HTTPRequest(new System.Uri("**************************.amazonaws.com/resources/"
    + "dealer/pic/" + newLinks[2]),
    (BestHTTP.HTTPRequest req, BestHTTP.HTTPResponse res)
    =>
    {
        var tex = new Texture2D(20, 20);
        tex.LoadImage(res.Data);
        uitex[2].GetComponent<UITexture>().mainTexture = tex;
    }).Send();

new BestHTTP.HTTPRequest(new System.Uri("**************************.amazonaws.com/resources/"
    + "dealer/pic/" + newLinks[3]),
    (BestHTTP.HTTPRequest req, BestHTTP.HTTPResponse res)
    =>
    {
        var tex = new Texture2D(20, 20);
        tex.LoadImage(res.Data);
        uitex[3].GetComponent<UITexture>().mainTexture = tex;
    }).Send();

Instead of using a foreach here, you need to use a for-loop, since you have to know the index of the image to assign it to the correct texture.

// this line replaces the first for loop you have there
var textureLinks = tzPlayInfo.Instance.bcGameTables.Select(t => t.dlrimage).ToArray();
// verify we have enough uitex elements:
if (uitex.Length < textureLinks.Length)
{
    throw new RuntimeException("too many textures linked");
}
for (int i = 0; i < textureLinks.Length; i++)
{
    new BestHTTP.HTTPRequest(new System.Uri($"**.amazonaws.com/resources/dealer/pic/{textureLinks[i]}"),
        (req, res) => {
            var tex = new Texture2D(20, 20);
            tex.LoadImage(res.Data);
            uitex[i].GetComponent<UITexture>().mainTexture = tex;
        }).Send();
}

There's also some words to be lost on the naming convention you use.

dealer_img should be dealerImg, uitex should be uiTextures, bc_gametablelist should be bcGameTables, dlrimage should be dealerImage...

In general you seem to try to shorten your names and adhere to C++ conventions. That would be fine if you were programming the graphics in C++. But since this is C#, you should be following C# convention (namely camelCase)

In the code above I made use of interpolated strings for the Uri. I also left the variable types of the lambda up for inference by the compiler. It should be smart enough to do so, if BestHTTP properly specified the generics on HTTPRequest.


Calculation of trigonometric sine function I'm creating a client-side pie chart in XSLT 1.0, without extensions or external dependencies (i.e., cannot use FXSLT, EXSLT, JavaScript, etc.).

The code is slow; at least fifteen iterations are necessary to get the correct precision required for the angles used by the pie chart. (I'm looking for help that doesn't involve loop unrolling, if possible.)

How would you speed calculating the sine function?

<xsl:template name="power">
  <xsl:param name="n"/>
  <xsl:param name="exponent"/>
  <xsl:param name="result" select="1" />
  <xsl:choose>
    <xsl:when test="$exponent = 0">1</xsl:when>
    <xsl:when test="$exponent = 1">
      <xsl:value-of select="$result * $n"/>
    </xsl:when>
    <xsl:otherwise>
      <xsl:call-template name="power">
        <xsl:with-param name="n" select="$n"/>
        <xsl:with-param name="exponent" select="$exponent - 1"/>
        <xsl:with-param name="result" select="$result * $n"/>
      </xsl:call-template>
    </xsl:otherwise>
  </xsl:choose>
</xsl:template>

<xsl:template name="factorial">
  <xsl:param name="n" select="1" />

  <xsl:choose>
    <xsl:when test="$n &lt;= 1">1</xsl:when>
    <xsl:otherwise>
      <xsl:variable name="f">
        <xsl:call-template name="factorial">
           <xsl:with-param name="n" select="$n - 1" />
        </xsl:call-template>
      </xsl:variable>
      <xsl:value-of select="$n * $f" />
    </xsl:otherwise>
  </xsl:choose>
</xsl:template>

<!-- sin(x) = x - x^3/3! + x^5/5! - x^7/7! ... -->
<xsl:template name="sin">
  <xsl:param name="degrees" />
  <xsl:param name="radians" select="$degrees * $PI div 180" />
  <xsl:param name="iter" select="1" />
  <xsl:param name="maxiter" select="15" />
  <xsl:param name="result" select="0" />

  <xsl:variable name="taylor" select="($iter - 1) * 2 + 1" />
  <xsl:variable name="numerator">
    <xsl:call-template name="power">
      <xsl:with-param name="n" select="$radians" />
      <xsl:with-param name="exponent" select="$taylor" />
    </xsl:call-template>
  </xsl:variable>
  <xsl:variable name="denominator">
    <xsl:call-template name="factorial">
      <xsl:with-param name="n" select="$taylor" />
    </xsl:call-template>
  </xsl:variable>

  <xsl:variable name="term">
    <xsl:choose>
      <xsl:when test="$iter mod 2 = 1">
        <xsl:value-of select="$numerator div $denominator" />
      </xsl:when>
      <xsl:otherwise><xsl:value-of select="-1 * $numerator div $denominator" /></xsl:otherwise>
    </xsl:choose>
  </xsl:variable>

  <xsl:choose>
    <xsl:when test="$iter &lt; $maxiter">
      <xsl:call-template name="sin">
        <xsl:with-param name="radians" select="$radians" />
        <xsl:with-param name="iter" select="$iter + 1" />
        <xsl:with-param name="result" select="$result + $term" />
      </xsl:call-template>
    </xsl:when>
    <xsl:otherwise><xsl:value-of select="$result" /></xsl:otherwise>
  </xsl:choose>
</xsl:template>

A similar question exists for C, but loop unrolling is awkward in XSLT due to its immutable variables.

Do not use power and factorial. Notice that

  $$x - \frac{x^3}{3!} + \frac{x^5}{5!} - \frac{x^7}{7!} + \ldots = x(1 - \frac{x^2}{2*3}(1 - \frac{x^2}{4*5}(1 - \frac{x^2}{6*7}(1 - \ldots))) \ldots) $$

Now calculate it backwards from the innermost term:

  <!-- sin(x) = x(1 - (x^2/2*3)(1 - (x^2/4*5)(1 - (x^2/6*7)))) -->
  <xsl:template name="sin">
    <xsl:param name="degrees" />
    <xsl:param name="radians" select="$degrees * $PI div 180" />
    <xsl:param name="iter" select="31" />
    <xsl:param name="result" select="1" />

    <xsl:variable name="term" select="($radians * $radians) div (($iter - 1) * $iter)" />

    <xsl:choose>
      <xsl:when test="$iter &gt; 2">
        <xsl:call-template name="sin">
          <xsl:with-param name="radians" select="$radians" />
          <xsl:with-param name="iter" select="$iter - 2" />
          <xsl:with-param name="result" select="1 - ($term * $result)" />
        </xsl:call-template>
      </xsl:when>
      <xsl:otherwise><xsl:value-of select="$radians * $result" /></xsl:otherwise>
    </xsl:choose>
  </xsl:template>


A class which represents an SQL table I'm representing DB tables as classes (in this case PHP classes). This has the goal of modeling every section of the web page, where you will get inputs from the user.

My actual base design is:

One class per table and every row is represented by an Object of this class.
The constructors asks for what they need in order to get the table row which they will represent:

info = $this->getRowFromVideos($id);
    }
}

The way I access the DB is not important here.

Every field of the table is stored as an array in an $info variable. And I have a method to access for the whole row or just a field.

    function getInfo($key = false){
        if($key) return $this->info[$key];
        else return $this->info;
    }

For composed tables like Players who needs an User and a Game ID, the query would be like this:

SELECT * FROM players WHERE userID = 'xxxxxx' AND gameID = 'SC2'

And the constructor class asks for an User object and Game object:

class Player extends DB {
    private $info = array();
    private $user;
    private $game;

    public function __construct($user, $game){
        $this->user = $user;
        $this->game = $game;
        $this->info =
              $this->getRowFromPlayers($user->getInfo("userID"),$game->getInfo("gameID"));
    }
}

Of course, these classes are not designed to satisfy CRUD needs, like inserting a new user or deleting a video. Those responsibilities would be encapsulated in other classes.

I cannot see which problems may appear with this design in the future if the web page gets really big.

Take a look at
Object Relational Mapping,
especially
Object-relational impedance mismatch
on Wikipedia.

Basically there problems with this as soon as it gets a bit more
complicated and you need to query e.g. a lot of objects at once, or you
have joins, and so on.

Let's take $this->getRowFromPlayers.  I'm assuming, that that method
will fetch one row from the database.  So you already have a
bottleneck here because you'll have to run n separate queries for
each of your n IDs when you want to get like a list of objects by
their IDs, which is very inefficient.  So you'll have to construct
objects from the outside, mapping a result of multiple rows from the
database to newly constructed (or cached) objects.

Another problem often comes up in that your objects don't exactly are
the table rows, they are just mapped onto objects, so testing by
identity is mostly meaningless unless you keep track of objects and
return the same object for the same row of a table.  It gets complicated
very fast.

Obviously there are already solutions for this, so I suggest you take a
look at existing PHP libraries for this.