Review of document registrar Would you be kind to make me a code review? It would be nice to have some comments, whether you would have done something differently and so on.  I am not sure about the way I handle the exception.  Is there a better way?

@Transactional
public classServiceImpl implements Service {

    private static final Logger LOG = Logger.getLogger(HermesServiceImpl.class);

    @Autowired(required = true)
    private Client client;

    @Autowired
    private SubmissionDAO submissionDAO;

    @Autowired
    private SubmissionSender hrsQueueSender;

    @Transactional
    @Override
    public void registerDocument(String submissionId) {

        final long start = System.currentTimeMillis();
        final List<HermesItem> items = submissionDAO.getSubmissionItems(submissionId);
        RegistrationSummary regSummary = null;

        if (!items.isEmpty()) {
            final HrsDocumentBuilder builder = new HrsDocumentBuilder();
            final Date docDate = new Date();
            builder.setTitle(submissionId);
            builder.setDocumentDate(docDate);
            builder.setSentDate(docDate);
            builder.setMailType(HermesMailType.INTERNAL);

            /* Document is constructed, try to invoke Hermes - upload annexes and register document. */
            try {
                regSummary = client.registerDocument(builder.buildDocument());
            } catch (final ClientException e) {
                LOG.error("Unable to register document with Submission Id= [" + submissionId + "], " + e.getMessage());
            }
            if (regSummary != null) {
                final String hermesDocId = regSummary.getHermesDocumentId();
                /* Update the DB with the documentID received from Hermes. */
                final int update = submissionDAO.updateSubmission(submissionId, hermesDocId, SubmissionStatus.HRS_REGISTERED.name());

                if (update < 1) {
                LOG.error("update error");
                }
            } else {
                submissionDAO.updateSubmission(submissionId, null, SubmissionStatus.HRS_FAILED.name());
                hrsQueueSender.sendMesage(submissionId);
            }

        } else {
            LOG.error("Not found");
            submissionDAO.updateSubmission(submissionId, null, SubmissionStatus.ANNEXES_NOT_FOUND.name());
        }
    }
}

Few points:

Try to avoid long or nested if/else
Sub-divide tasks in smaller functions(clean and more readable code + easier unit testing)
Minimize variable scope(try to declare variables in the scope where you really need them)
Think about Exception in context of your business domain, may be using builder pattern (like HrsDocumentBuilder) can be useful to have consistent logging message for same type of exception(override toString() method).

We can refactor the method registerDocument() like this:

You may replace

final long start = System.currentTimeMillis();

with StopWatch

@Transactional
@Override
public void registerDocument(String submissionId) {
final long start = System.currentTimeMillis();
final List<HermesItem> items = submissionDAO.getSubmissionItems(submissionId);

if (!hasItems(items)) //extract else to method
    return;

final HrsDocumentBuilder builder = ...builder.setMailType(HermesMailType.INTERNAL);//stays the same
try {
      // minimized the scope of local variable, you need regSummary in try block so declare it here.
        RegistrationSummary regSummary = client.registerDocument(builder.buildDocument()); // will rename buildDocument() -> build()
        update(regSummary);

} catch (final ClientException e) {

       //To standarized log message we can refactor it like :
        ClientExceptionBuilder b = new ClientExceptionBuilder(e).submissionId(submissionId).build();// override toString()
        LOG.error(b.getMessage());
}
}//function registerDocument() end here

Since all other method can be easily extracted from else blocks, i am posting only the update(regSummary) because this function can be further subdivide into two smaller functions like this:

void update(regSummary) {

    if(!hasSummary()) //extract else to method
        return

    final String hermesDocId = regSummary.getHermesDocumentId();
    /* Update the DB with the documentID received from Hermes. */
    final int update = submissionDAO.updateSubmission(submissionId, hermesDocId, SubmissionStatus.HRS_REGISTERED.name());

    if (update < 1)
        LOG.error("update error");

}


INI File configuration handler I am currently working on a game, and I get the configuration in an .ini file, and I get information with this library https://github.com/benhoyt/inih I would like to improve the readability / performance of this code, as I do not know much about good practices.

include "project.h"

// typedef struct s_server_conf {
//      const char              *ip;
//      unsigned int    port;
// }                            t_server_conf;

// typedef struct       s_db_conf {
//      const char              *host;
//      const char              *user;
//      const char              *passwd;
//      const char              *db;
//      unsigned int    port;
// }                            t_db_conf;

// typedef struct s_conf {
//      t_server_conf   server;
//      t_db_conf               conf;
// }                            t_conf;

static int      parse_server(t_conf *conf, const char *name, const char *value)
{
        # define MATCH(n) (strcmp(name, n) == 0)

        if (MATCH("ip"))
                conf->server->ip = strdup(value);
        else if (MATCH("port"))
                conf->server->port = atoi(value);
        else
                return (0);
        return (1);
}

static int      parse_database(t_conf *conf, const char *name, const char *value)
{
        # define MATCH_NAME(n) (strcmp(name, n) == 0)

        if (MATCH_NAME("host"))
                conf->db->host = (strdup(value));
        else if (MATCH_NAME("user"))
                conf->db->user = (strdup(value));
        else if (MATCH_NAME("passwd"))
                conf->db->passwd = (strdup(value));
        else
                return (0);
        return (1);
}

static int      handler(void *conf, const char *section, const char *name, const char *value)
{
        # define MATCH_SECTION(s) (strcmp(section, s) == 0)

        printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", name, value, section)
;

        if (MATCH_SECTION("database"))
                return (parse_database((t_conf*)conf, name, value));
        if (MATCH_SECTION("server"))
                return (parse_server((t_conf*)conf, name, value));
        return (0);
}

void            clear_conf(t_conf *conf)
{
        if (conf->db)
                free(conf->db);
        if (conf->server)
                free(conf->server);
        free(conf);
}

static  t_conf          *create_conf(void)
{
        t_conf  *conf;

        if (!(conf = (t_conf*)malloc(sizeof(t_conf))))
                return (NULL);
        memset(conf, '\0', sizeof(t_conf));
        if (!(conf->db = (t_db_conf*)malloc(sizeof(t_db_conf))))
        {
                clear_conf(conf);
                return (NULL);
        }
        conf->db->port = 3306;
        if (!(conf->server = (t_server_conf*)malloc(sizeof(t_server_conf))))
        {
                clear_conf(conf);
                return (NULL);
        }
        return (conf);
}
t_conf          *get_conf(void)
{
        t_conf  *conf;

        if (!(conf = create_conf()))
                return (NULL);
        if (ini_parse("settings.ini", handler, conf) < 0)
        {
                printf("Unable to load configuration\n");
                exit (1);
        }
        return (conf);
}

Design

Rather than bury a magic filename, consider passing it in.

//t_conf          *get_conf(void) {
//  ...
//    if (ini_parse("settings.ini", handler, conf) < 0)

t_conf *get_conf(const char *ini_file_name) {
  ...
  if (ini_parse(ini_file_name, handler, conf) < 0)

Maybe add before the if()

  if (ini_file_name == NULL) {
    ini_file_name = "settings.ini";
  }

Devoid of overall comments

Consider comments to at least convey the coding goals.

Allocate to the size of the de-referenced object

... rather than allocate to the size of the type - easier to code right, review and maintain.  Cast not needed.

//if (!(conf = (t_conf*)malloc(sizeof(t_conf))))
//            return (NULL);
conf = malloc(sizeof *conf);
if (conf == NULL) {
  return NULL;
}

// like-wise
// memset(conf, '\0', sizeof(t_conf));
memset(conf, 0, sizeof *conf);

Use auto formatting

Life is too short for manual formatting. Find and use an auto formatter - customized as needed.

Comments of typedefs not needed

As is now, this code has comments that is not required to match what is truly in "project.h" and as time goes on, can diverge.  For code posting here, post a portion of "project.h" instead.

Invalid code

include "project.h" should be #include "project.h"

Hiding objects here is bad

# define MATCH(n) (strcmp(name, n) == 0) brings in name, but not obvious from the call if (MATCH("ip")).

Alternative:

static bool streq(const char *a, const char *b) {
  return strcmp(a,b) == 0;
}
...
if (streq(name, "ip"))

Naked magic number

Buried in code is

conf->db->port = 3306;

Better to have

// near top of code
#define PORT_DEFAULT 3306

// later on
conf->db->port = PORT_DEFAULT;

Unneeded if

Just call free().  free(NULL) is OK.

//if (conf->db)
//            free(conf->db);
free(conf->db);

Like free(), I'd expect clear_conf(NULL) to not error

void clear_conf(t_conf *conf) {
  if (conf) { // add

() not needed in return and others

Many places

// return (conf);
return conf;

// conf->db->host = (strdup(value));
conf->db->host = strdup(value);

Non-standard C

Note: strdup() is not standard, yet ubiquitous.

Missing #include <>

Do not rely on "project.h" to provide them unless that is clearly a project rule.

NULL as name

Given the potential generic use of handler, I'd guard against NULL.

// printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", name, value, section);
if (name === NULL || value == NULL || section == NULL) {
  tbd();
} else {
  printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", name, value, section);
}

Further I'd bracket such strings to help detect leading/trailing white spaces in them

  printf("Le nom du champ est \"%s\", la valeur est \"%s\" (La section est \"%s\")\n",
      name, value, section);

Lack of error checking

// what if `value` is "", "abc", "123xyz", "12345678901234567890", ...?
conf->server->port = atoi(value);

// what if allocation returns NULL?
conf->server->ip = strdup(value);

Respect presentation width

        printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", name, value, section)
;

vs.

        printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n",
            name, value, section);

Another reason for an auto formatter.

Curious dual language

I'd expect one.

printf("Le nom du champ est %s, la valeur est %s (La section est %s)\n", ...
printf("Unable to load configuration\n");

Conversion changes sign-ness

unsigned port;
...
conf->server->port = atoi(value);

Perhaps a helper function

// return error status
bool to_unsigned(unsigned *u, const char *s) {
  if (s == NULL) {
    return true;
  }
  char *endptr;
  errno = 0;
  unsigned long = strtoul(s, &endptr, 10);
  if (errno || s == endptr || *endptr || value > UINT_MAX) {
    return true;
  }

  if (u) {
    *u = (unsigned) value;
  }
  return false;
}

if (to_unsigned(&conf->server->port, value)) {
  hanlde_oops();
}


Fast way to calculate Standing Wave modes in NumPy I am trying to plot the standing wave modes in a 2D box over time. The equation for this is:

$$ A(x,y,t) = \sum\limits_{n,m} sin \left( \frac{n \pi x}{L_x} \right) sin \left( \frac{m \pi y}{L_y} \right) cos \left( w_{n,m}t + \phi_{n,m} \right) $$ where $$ w_{n,m} = \sqrt{ {\left( \frac{n \pi}{L_x}\right)}^2 + {\left( \frac{m \pi}{L_y}\right)}^2 }$$

The code I have used is given below:

Nx = 384
Ny = 384
Nt = 360

# function to calculate standing wave
def s_wave(ll, xx, yy, tt, pp, max_modes):
   tot_amp = 0
   Mm = 1000 # a constant relevant to my problem
   k = 0
   for m in range (1,max_modes):
       for n in range (1,max_modes):
           omega = ((m*np.pi/(ll*Mm))**2 + (n*np.pi/(ll*Mm))**2)**0.5
           amp   = np.sin(m*np.pi*xx/ll)*np.sin(n*np.pi*yy/ll)*np.cos(omega*tt-pp[k])
           tot_amp = tot_amp + amp
           k = k + 1
   return (tot_amp)

# building grid points in x and y direction
L = 6.144 # Length of the Box along each axis
T = 10 # delta_T
x = np.linspace(-L/2, L/2, Nx)
y = np.linspace(-L/2, L/2, Ny)
t = np.linspace(0, (Nt-1)*T, Nt)

modes = 60 #number of standing wave modes
phase = np.random.uniform(0,2*np.pi,modes*modes) # to introduce random phase in each mode

ustand = s_wave(L,x[None,None,:],y[None,:,None],t[:,None,None],phase,modes)

This approach works quantitatively fine with my problem. But the execution time is pretty slow. Is there any way to accelerate the execution time?

Thanks in advance.

A few things to consider:

Loops in python are very inefficient. To optimize performance, when
it is possible to vectorize code, you should do so.
Avoid calculating the same thing multiple times. For example, the term $$ cos(\omega_{n,m}t + \phi_{n,m}) $$ does not depend on x and y, so there is no need to calculate it during every iteration of the loop.
The number of calculations you are performing is huge. Nx * Ny * Nt * n_max * m_max is roughly 191 billion iterations, and on each of those iterations, multiple calculations are performed. I am not sure about the reason for using so many points on your x,y,t grid, but for adequate visualization, much fewer are required. If the goal is numerical precision, I would recommend checking out SciPy's interp2D function. What you could do is calculate a smaller number of grid points, and if you need to sample a value that is not on the grid, you can use the 2d interpolation function.

I have altered your code to vectorize it as much as possible. I did not completely vectorize out the loops, because my laptop does not have RAM capacity in the triple digits of gigabytes.

Reducing the number of grid points in the X and Y directions results in a huge speedup, and the resulting plot is shown after the code.

import numpy as np

#Nx = 384
#Ny = 384
#Nt = 360

Nx = 38
Ny = 37
Nt = 360

#########################################################
# function to calculate standing wave (original code had error in amp line)
#########################################################
def s_wave(lx, ly, xx, yy, tt, pp, max_modes):
    tot_amp = 0    # Initialize total amplitude to 0
    Mm      = 1000 # a constant relevant to my problem
    p_idx   = 0    # The index for accessing the phase noise

    for m in range (0,max_modes):
        for n in range (0,max_modes):
            #############################
            # Calculating Omega
            #############################
            omega1 = n*np.pi/(lx*Mm)
            omega2 = m*np.pi/(ly*Mm)

            omega  = np.sqrt(omega1**2 + omega2**2)

            #############################
            # Calculating Amplitude
            #############################
            amp1   = np.sin(n*np.pi*xx/lx)
            amp2   = np.sin(m*np.pi*yy/ly)
            amp3   = np.cos(omega*tt - pp[p_idx])

            amp    = amp1 * amp2 * amp3

            #############################
            # Update total amplitude
            #############################
            tot_amp = tot_amp + amp
            p_idx   = p_idx + 1

    return tot_amp

def s_wave_opt(lx, ly, xx, yy, tt, pp, max_n, max_m):
    tot_amp = 0    # Initialize total amplitude to 0
    Mm      = 1000 # a constant relevant to my problem

    #############################
    # Initialize total amplitudes at all points to 0
    #############################
    A_total = np.zeros([len(xx), len(yy), len(tt)])

    #############################
    # Calculate omega
    #############################
    n,m     = np.mgrid[1:max_n+1:1, 1:max_m+1:1]
    omega_n = n * np.pi / ( lx * Mm )
    omega_m = m * np.pi / ( ly * Mm )
    omega   = np.sqrt(omega_n**2 + omega_m**2)

    #############################
    # Vactorize calculation of term3
    #############################
    term3 = np.multiply.outer(t,omega)
    term3 = np.add(pp,term3)
    term3 = np.cos(term3)

    #############################
    # Calculating A[x,y,:]
    #############################
    nr     = np.arange(1,max_n+1)
    term1  = np.multiply.outer(xx, nr*np.pi/(lx*Mm))
    term1  = np.sin(term1)

    mr     = np.arange(1,max_m+1)
    term2  = np.multiply.outer(yy, mr*np.pi/(ly*Mm))
    term2  = np.sin(term2)

    for y_idx in range(len(yy)):
        xy  = np.multiply.outer(term1, term2[y_idx])
        xyt = np.multiply(xy[:,None,:,:],term3[None,:,:,:])
        tot = np.sum(xyt, axis=(2,3))
        A_total[:,y_idx,:] = tot

    return A_total

# building grid points in x and y direction
Lx = 6.144 # Length of the Box along each axis
Ly = 6.144
T = 10 # delta_T
x = np.linspace(-Lx/2, Lx/2, Nx)
y = np.linspace(-Ly/2, Ly/2, Ny)
t = np.linspace(0, (Nt-1)*T, Nt)

modes_n = 60
modes_m = 61
phase2  = np.random.uniform(0,2*np.pi, [modes_n,modes_m])
ustand  = s_wave_opt(Lx, Ly, x, y, t, phase2, modes_n, modes_m)

The resulting plot was too large to attach as a gif, but attached is a screenshot of one frame:

The code to produce the gif plot:

##################################################
# Plotting the results
##################################################
from mpl_toolkits import mplot3d
import matplotlib.pyplot as plt

import os
if not os.path.isdir("./images"):
    os.system("mkdir images")

for i in range(Nt):
    fig = plt.figure()
    ax  = plt.axes(projection='3d')
    X,Y = np.meshgrid(y,x)
    ax.plot_surface(X, Y, ustand[:,:,i])

    ax.set_xlim3d(x.min(), x.max())
    ax.set_ylim3d(y.min(), y.max())
    ax.set_zlim3d(ustand.min(), ustand.max())
    ax.set_title("Standing Wave at Time {}".format(t[i]))
    plt.savefig("./images/frame{}.png".format(i))
    plt.close()

os.system("ffmpeg -i ./images/frame%d.png -vf palettegen -y paletter.png && ffmpeg -framerate 20 -loop 0 -i ./images/frame%d.png -i paletter.png -lavfi paletteuse -y plot.gif")

The code runs in about 4 minutes. Roughly 1 minute are the calculations, and the rest is time spent creating the graph.


Copying files locally with rsync and subprocess I have Python code to copy the Files locally on the Server from one Directory to another using rsync and subprocess module, this is just in continuation of the post from this post where I tried to get various insight but opted to use below code finally.

Please advise if this further can be improved or optimized in pythonic way and help to provide your esteemed reviews please.

#!/bin/python3
import os
import glob
import datetime
import subprocess

def Copy_Logs():
   # Variable Declaration to get the month and Curr_date_month
    Info_month = datetime.datetime.now().strftime("%B")
    Curr_date_month = datetime.datetime.now().strftime("%b_%d_%y")
    Sourcedir = "/data1/logs"
    Destdir = "/data2/logs/"
    ###### End of your variable section #######################
    # The result of the below glob _is_ a full path
    for filename in glob.glob("{2}/{0}/{1}/*.txt".format(Info_month, Curr_date_month, Sourcedir)):
        if os.path.getsize(filename) > 0:
            if not os.path.exists(Destdir + os.path.basename(filename)):
                subprocess.call(['rsync', '-avz', '--min-size=1', filename, Destdir ])

if __name__ == '__main__':
    Copy_Logs()

You're calling now() twice - what will happen if the month changes between those two calls?

Your code skips all files that exist, even though your linked question indicates that you need to update changed files.

Your code checks that file is non-empty, but rsync is already doing this for you with --min-size=1.

More generally, before you code up file-sync logic, it's best to read the rsync man page and see if your problem is already solved.   Syncing files is a task full of corner cases and gotchas; life is too short for you to find them all.  Just let rsync take care of it, whenever possible.

The code below will update changed files while skipping unchanged files (as in your problem description).  If you want to copy only new, non-existing files (as in your posted code), add --ignore-existing to the rsync options.

import datetime
import subprocess

def Copy_Logs():
    Sourcedir = datetime.datetime.now().strftime("/data1/logs/%B/%b_%d_%y/")
    Destdir = "/data2/logs/"
    subprocess.call(['rsync', '-avz', '--min-size=1', '--include=*.txt', '--exclude=*', Sourcedir, Destdir ])


Help improve my first PHP class I recently started to learn more about OOP in PHP and I created a website for testing. I have 4 classes: database, user, posts, comments.
This is a simple database.class.php:

class DB {

    protected $pdo;
    protected $engine    = 'mysql';
    protected $host      = 'localhost';
    protected $db_name   = 'test';
    protected $db_user   = 'root';
    protected $db_passwd = '';

    public function connect() {
        try {
            $this->pdo = new PDO($this->engine.':host='.$this->host.';dbname='.$this->db_name,$this->db_user,$this->db_passwd);
            $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            return $this->pdo;
        }   catch(PDOException $e) {
            die($e->getMessage());
        }
    }
}

Then, every other class starts like this:

class user {

    public $link;

    public function __construct() {
        $pdoObj = new DB();
        $this->link = $pdoObj->connect();
        return $this->link;
    }

}

I use $link as a reference to the pdo object so I can use all the methods associated with PDO.  My question is: is this wrong?

I saw a lot of source codes and most of them used 'static' and didn't instantiated the class with new Class(); but just ClassName::foo(); Is it wrong to instantiate every class and work with instances? I always do:

$obj = new user();
$obj->SelectAll();

// and so on..

If my way is wrong, how can I move my code to static?

Here is an example of a method:

public function register($username, $email, $passwd, $re_passwd) {
    # fix email and passwd regex
    $reg_error = array();
    $regex_username = '/[a-zA-Z0-9-._]{6,20}$/';
    $regex_email = '/[a-zA-Z0-9._-]{3,}@[a-zA-Z0-9]{3,}.[a-zA-Z.]{2,}$/';
    if (empty($username) || empty($email) || empty($passwd) || empty($re_passwd)) {
        $reg_error['fields'] = 'All fields are required';
    }   else {
        if (!preg_match($regex_username, $username)) {
                $reg_error['username'] = 'Username must be at least 6 characters';
        }   if (!preg_match($regex_email, $email)) {
                $reg_error['email'] = 'Invalid email';
        }   if(strlen($passwd) <= 6) {
                $reg_error['passwd'] = 'Password must be at least 6 characters long and must contain at least 1 uppercase, 1 lowercase and 1 digit value';
        }   if ($re_passwd !== $passwd) {
                $reg_error['match'] = 'Passwords do not match';
        }   if($this->usernameExists($username)) {
                $reg_error['username_exists'] = 'Username: ' .$username. ' already exists';
        }   if($this->emailExists($email)) {
                $reg_error['email_exists'] = 'Email: ' .$email. ' already exists in our database';
        }
    }
    if (!empty($reg_error)) {
        foreach($reg_error as $error => $er) {
            echo 'Error: '.$er.'<br />';
        }
    }   else {
        try {

            $query = $this->link->prepare("INSERT INTO users(username, email, password, ip_address, activation_code)
                                            VALUES(:username, :email, :passwd, :ip_addr, :code)");
            $passwd = hash('sha512', $passwd);
            $ip_addr = $_SERVER['REMOTE_ADDR'];
            $code = substr( hash('sha512', md5(rand(999, 9999))) , 15, 40);
            $params = array(':username' => $username,
                            ':email'    => $email,
                            ':passwd'   => $passwd,
                            ':ip_addr'  => $ip_addr,
                            ':code'     => $code);
            $query->execute($params);
            if ($query->rowCount() != 0) {
                redirect('index.php?lastid='.$this->lastId());
            }   else {
                    return 'There\'s been a tehnical problem during the registration process, please try again later.';
            }
        }   catch (PDOException $e) {
                die($e->getMessage());
        }
    }
}

What happens if you change your database credentials?

Right now your class is tied to a specific set of database credentials, and you'll need to change it if:

you want to use a different engine,
or a different host,
or a different user,
or a different password,
or any combination of the above.

This is why PDO requires you to pass the dsn and the credentials through its constructor, and you should do the same. Then of course your class wouldn't be much different than PDO itself, if you don't have any additional functionality, just forget about it and use PDO.

If you want to keep your class, just store your credentials in a configuration file. A simplistic solution would be an INI file, let's call it database.ini:

engine   = "mysql"
host     = "localhost"
name     = "test"
user     = "root"
password = ""

Your database class could then be:

class DB {
    private $pdo;
    private $engine;
    private $host;
    private $db_name;
    private $db_user;
    private $db_passwd;

    public function _construct($path) {
        if(!is_file($path)) {
            throw new InvalidArgumentException("Hey, that's not a file!");
        }

        $credentials = @parse_ini_file($path);

        if(empty($credentials)) {
            throw new InvalidArgumentException("Hey, that's a file, but it's not a valid INI file!");
        }

        $this->engine    = $credentials["engine"];
        $this->host      = $credentials["host"];
        $this->db_name   = $credentials["name"];
        $this->db_user   = $credentials["user"];
        $this->db_passwd = $credentials["password"];

        try {
            $this->pdo = new PDO($this->engine.':host='.$this->host.';dbname='.$this->db_name,$this->db_user,$this->db_passwd);
            $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            return $this->pdo;
        }   catch(PDOException $e) {
            throw new Exception("Boo, wrong credentials!");
        }
    }

}

Looks like a lot of work? Well, it is, and it's not finished, but changing any of your credentials is now as simple as changing its value in database.ini. All you need to do to instantiate your class is:

$db = new DB("database.ini");

Let's go quickly through the main parts of the class:

Don't protect what should be private

Don't declare your class properties and methods as protected if you don't need to. Protected means:

  Members declared protected can be accessed only within the class itself and by inherited and parent classes

If you are not going to extend the class, there's absolutely no reason to go with protected instead of private:

  Members declared as private may only be accessed by the class that defines the member.

This might seem extremely pedantic, the world won't stop spinning because your properties are protected, but since you're a fresher, better avoid this common bad habit.
Connect? Why, let's construct instead!

The database class is useless if we don't run connect(), calling the method is a prerequisite for the class to do anything else. When that happens, it's a good sign that our method should either be called in the class constructor, or be the constructor itself. If it's absolutely necessary for the method to be called, then it makes sense to be the one function that gets called by default when we create an instance of our class, doesn't it?
Check your arguments

I'm doing two checks on our $path argument, first if it's a valid file and then if parse_ini_file returned an array (that would signify that the INI file was succesfully parsed). Throw appropriate exceptions if there are problems.
The unfinished part

$this->engine    = $credentials["engine"];
$this->host      = $credentials["host"];
$this->db_name   = $credentials["name"];
$this->db_user   = $credentials["user"];
$this->db_passwd = $credentials["password"];

What I'm not doing here is check whether the indices exists in the $credentials array. That's your homework ;)
The exception

Your original code was:

} catch(PDOException $e) {
    die($e->getMessage());
}

That doesn't make sense, you catch the exception and then die with the same message? Not catching the exception will have exactly the same effect, the script will stop, so why catch it? You should catch exceptions only when you can do something productive with them, if not, just let the script fail. Usually "something productive" means gracefully recover from the error, but that's not always possible. In my code I'm doing something that might seem minor but it's very important:

}   catch(PDOException $e) {
    throw new Exception("Boo, wrong credentials!");
}

Yes, the only thing I'm doing is changing the message of the exception, the script will still fail. However PDO's constructor exception reveals your database credentials (except your password), so catching that and rethrowing it with a more generic message minimizes the risk of your database username (for example) falling in the wrong hands. Can't gracefully recover from the failure, but at least we plugged a potential security hole.

How about some setters and getters?

If you are still reading this, I'm sorry to inform you that your code doesn't work. This:

public function __construct() {
    $pdoObj = new DB();
    $this->link = $pdoObj->connect();
    return $this->link;
}

does set your database object in the $link property but it's your database object, not your PDO object. Your database object does not have a prepare() method, so this:

$query = $this->link->prepare("INSERT INTO users(username, email, password, ip_address, activation_code)
                                VALUES(:username, :email, :passwd, :ip_addr, :code)");

will simply fail. What you need to do is pass your PDO object to your User class:

class DB {

    ...

    public function getPDO() {
        return $this->pdo;
    }
}

So this simple function will always return the database's internal PDO object. Your User constructor would now be:

public function __construct() {
    $database = new DB("database.ini");

    $this->link = $database->getPDO();
}

Notice how I removed return $this->link;? Well, don't do that, a constructor returns the object it instantiates by default, there's absolutely no sense in returning anything else, it wouldn't be a constructor if it didn't give us what it constructed, would it?

Wait! There's a dependency!

This line:

$database = new DB("database.ini");

means that we don't have the option of renaming our INI file. Ever! That's not good, that's an unwanted dependency and we really don't like those. After all we went through a lot to get our database class to not be dependent on its credentials, we can't introduce another dependency now. What we are going to do is use a method called Constructor Dependency Injection, that's the simplest way to achieve Dependency Injection. It's extremely simple, all we need to do is:

public function __construct(DB $database) {
    $this->link = $database->getPDO();
}

That's right, pass the database as a parameter in the constructor, that's all there is to it. Now, to instantiate our user class all we need to do is:

$database = new DB("database.ini");
$user = new User($database);

...and our classes are dependency free! To get an idea why this is good, assume you have two databases, and you need to use them both. All you need to do is:

$database1 = new DB("database1.ini");
$user1 = new User($database1);

$database2 = new DB("database2.ini");
$user2 = new User($database2);

Does your database only have users?

If your database has any other table, and you want to model them as classes, it might be worth looking at inheritance. All your table classes have a common dependency, the database, and you don't really have to write the same constructor every time:

class Table {
    protected $database;

    public function __construct(DB $database) {
        $this->database = $database->getPDO();
    }
}

class User extends Table {

    // only user specific functions here

}

class Article extends Table {

    // only article specific functions here

}

class Comment extends Table {

    // only comment specific functions here

}

The User, Article, and Comment classes have inherited the same constructor from Table, no need to write the method more than once. As you probably already noticed, I've declared $database protected, it makes sense now as we need it to be available to the child classes. By the way that's your old $link property, since it holds a database object, makes sense to call it $database.

The register function

Sorry for being blunt, but this is a horrible mess:

Indendation is messed up.

Learn to format your code properly. Better yet, equip yourself with a decent tool to do it for you.
Your function should either return something or echo something, not both.

Preferably return something. Echoing HTML from your functions is a very poor practice, read up on separation of concerns. Simply put your function is where you put logic, and you shouldn't mix presentation with logic.
You catch an exception and die with the same message again.

Don't do that.

Since we've now have a handy getter in our database class, the way to get the reference to the PDO object in your function is:

class User {

    public function register($username, $email, $passwd, $re_passwd) {
        $link = $this->database->getPDO();

        ...

        $query = $link->prepare( ... );

        ...
    }

}

And what about static?

Well, don't worry about it. Most of the time is used as a convenience, and it's not really the better way to do things. Instantiate your objects properly, and then pass them around via one of the dependency injection techniques, you're passing around the same instance, you are not instantiating it again and again.

Further reading

Classes and Objects in the manual. Yes, you need to read everything.
Data Filtering in the manual. Again, you need to read everything.
Mutator methods.
SOLID - Don't cheat, read all the linked articles.
Inversion of Control Containers and the Dependency Injection pattern
KISS and YAGNI

Addendum

Corbin raises two important issues in a comment:

  (1) The exception shouldn't be masked. What if consuming code wants the details? A different part of code should be responsible for hiding potentially sensitive exceptions. (2) If Table (and other classes) need a PDO instance, they should accept a PDO instance, not a DB instance. There's no reason to accept the object just to call a single getter. (I also believe that this Database class shouldn't even exist though. It's pointless.)

My thoughts:

Your database class is pointless if it doesn't have any additional functionality you didn't tell us about.

As it is, it's just a wrapper for PDO that doesn't do anything more than PDO itself does, and in my answer I've assumed (hoped?) that you do have a reason for it and would further develop it to enhance PDO (perhaps adding a query cache, or a log?). But if you don't have any further plans for it, then it would be preferable to just scrap it and just use PDO and just feed your PDO object in Table::__construct().
You shouldn't be catching exceptions if you can't do anything productive, preferably gracefully recovering from the error.

Now, I do suggest that you should catch the PDOException just in case your database credentials are leaked, but that's more of a scare tactic than a solid suggestion. I've seen one too many new developers do similar mistakes, and perhaps my suggestion there was a bit drastic.

What you need to keep in mind is that this particular PDOException should not reach the end user. If there's other code in between your database instantiation and the end user (a top level script, perhaps), it would make much more sense to deal with the exception there, and if you can't recover from it do something like throwing a 404 or 503 error (whichever applies).

Take my suggestion to hide the details of the connection PDOException with a grain of salt, I'm just tired of dealing with the same crap again and again. There was a time when such an exception created a world of trouble for me, but in a typical production environment it shouldn't be easy for anyone to do anything particularly harmful just because they caught a glimpse of your database username.


Function that helps with reaching nested objects /**
 * Returns the value of a nested property of source object if it exists.
 * Even if the nested property evaluates to false it will be returned.
 * If the nested property does not exist, undefined will be returned.
 * @param {Object} source       source object
 * @param {String} path         path without first dot e.g. "response.http.statusCode"
 * @param {String=} sourceName  if provided, any catched error will be logged to console
 * @returns {*}                 value under the source.path or undefined
 */
function reach(source, path, sourceName) {
    var result;
    try {
        if (sourceName) {
            eval('var ' + sourceName + '=source; result=' + sourceName + '.' + path);
        } else {
            eval('result=source.' + path);
        }
    } catch (err) {
        if (sourceName) {
            console.log(err);
        }
    }
    return result;
}

I know that eval is dangerous, but I made the assumption that it's the caller's responsibility.

Very similar to an earlier answer I did with regards to deep value extraction. Here's the code:

function createResolver(keypath){
  return new Function('root', `
    try { return root.${keypath}; }
    catch(e){ return undefined; }
 `);
}

// Return foo.bar.baz
var obj1 = { foo: { bar: { baz: 'bam!!!' }}};
var resolver1 = createResolver('foo.bar.baz');
var value1 = resolver1(obj1);

One difference between this one and your approach is that your approach seems to evaluate on every call. This can be slow as you are having the browser evaluate the string JavaScript on every call. A constructed function on the other hand can be cached. This means you can construct the function once, store the constructed function, and call it over and over again without having to re-evaluate the string every time.

// keypath-resolver store
var resolvers = {}

function createResolver(keypath){
  return new Function('root', `
    try { return root.${keypath}; }
    catch(e){ return undefined; }
 `);
}

// Create a resolver for foo.bar.baz. We only have JS evaluate the string here.
resolvers['foo.bar.baz'] = createResolver('foo.bar.baz');

var obj1 = { foo: { bar: { baz: 'bam!!!' }}};
var obj2 = { foo: { bar: { baz: 'pow!!!' }}};

// We just call the function. No evaluation!
resolvers['foo.bar.baz'](obj1);
resolvers['foo.bar.baz'](obj2);

Although these two approaches seem to act the same, one issue with eval is that it evaluates the string in the scope it is called. This means it can leak variables or override code that's visible in the current scope, just like you did with result. A generated function on the other hand does not. It does not form a closure of the scope it was created from, and the immediate outer scope is the global scope. This makes it relatively safer than eval.

Your reliance of console.log is also something to ponder on. If it's an error, is it not better to actually throw than just log the error? Also, if the keypath isn't fully resolved, this means the value isn't present which makes it technically undefined. Consider returning undefined instead of logging an error if you want errors to be handled.


Soccer winning probabilies from ELO strength indicator What can I do to improve this function, which calculates the win expectancy of a soccer team, given their strength:

def winning_prob(first, second, first_at_home=False, second_at_home=False):
    """ Return winning probability, given ELO ratings of two soccer teams

    Args:
        first: ELO rating of first team
        second: ELO rating of second team
        first_at_home: is first team playing at home?
        second_at_home: is second team playing at home?

    Returns:
        Winning probability of first team against second team
    """
    if first_at_home:
        first = first + 100
    if second_at_home:
        second = second + 100

    difference = (first - second) / 400.0
    return 1.0 / (10**(-difference) + 1.0)

Especially the two boolean switches look clunky to me, but the explanation is simpler that way. What improvements do you see?

With the possibility of first_at_home and second_at_home both being True, there really is not too much to improve on here.

The only things I see are:

Instead of doing first = first + 100, you can do first += 100.
Instead of doing -((first - second)/ 400.0) you can do (second - first)/ 400.0.
There is also no need to specify your operands as floats (400.0 vs 400) because Python does float division with the basic / operator.

Here are my improvements:

def winning_prob(first, second, first_at_home=False, second_at_home=False):
    """ Return winning probability, given ELO ratings of two soccer teams

    Args:
        first: ELO rating of first team
        second: ELO rating of second team
        first_at_home: is first team playing at home?
        second_at_home: is second team playing at home?

    Returns:
        Winning probability of first team against second team
    """
    if first_at_home:
        first += 100
    if second_at_home:
        second += 100

    return 1 / (10**((second - first)/400) + 1)


Sentinel based find with conditional fallback to std::find I've got bored, so I decided to write a code using ancient knowledge of sentinels.

Sentinel based find

Basically what we do is put value that is needed to find at the end, and then write an endless loop. Breaking from loop happens in the body of it, so one branch (which probably won't make much, since it is a loop branch, but still) is eliminated. Requires the type to have noexcept copy assign and noexcept copy construct (in C++17, since I guess T t = value will have guaranteed copy elision).

Is there any possibilities to make this faster without compromising functionality?

#ifndef FAST_FIND_HPP
#define FAST_FIND_HPP

#include <type_traits>
#include <utility>
#include <iterator>

namespace impl
{
    template <typename BidirIt, typename T>
    BidirIt find(std::true_type, BidirIt first, BidirIt last, const T& value)
    {
        if (first == last)
        {
            return last;
        }

        auto real_last = std::prev(last);
        if (*real_last == value)
        {
            return real_last;
        }

        auto copy = *real_last;
        *real_last = value;

        auto it = first;
        for (; ; ++it)
        {
            if (*it == value)
                break;
        }

        *real_last = copy;

        if (it == real_last)
        {
            return last;
        }

        return it;

    }

    template <typename BidirIt, typename T>
    BidirIt find(std::false_type, BidirIt first, BidirIt last, const T& value)
    {
        return std::find(first, last, value);
    }
}

template <typename BidirIt, typename T>
BidirIt find(BidirIt first, BidirIt last, const T& value)
{
    return impl::find(std::integral_constant<bool, std::is_nothrow_copy_assignable<std::remove_reference_t<decltype(*first)> >::value> {},
        first, last, value);
    //I'd like to allow T to be different from *first

}

#endif

Usage:

#include "fast_find.hpp"
#include "randomsequence.h"
#include <iostream>
#include <vector>

int main()
{
    std::vector<int> v(10'000'000);
    random_int_generator<> gen(0, 10'000); //generate values in range
    gen(v.begin(), v.end());

    int n;
    std::cin >> n;
    auto it = find(v.begin(), v.end(), n);
    if (it != v.end())
    {
        std::cout << "Found it!\n" << *it << '\n';
    }
    else
    {
        std::cout << "Sadness\n";
    }
}

Here is the random_int_generator<>, do not review this, I posted it to make it easier for you to benchmark if needed:

#pragma once

#include <random>

template <typename T = int, class Engine = std::random_device>
class random_int_generator
{
    Engine engine;
    std::uniform_int_distribution<T> dist;
public:
    random_int_generator(T begin = 0, T end = std::numeric_limits<T>::max()) :
        dist(begin, end)
    {

    }

    template <typename OutputIt>
    void operator()(OutputIt first, OutputIt last)
    {
        while (first != last)
        {
            *first++ = dist(engine);
        }
    }

    T operator()()
    {
        return dist(engine);
    }
};

You can find current version in my github repo (it's a mess, I hope you won't mind that).

std::integral_constant<bool, std::is_nothrow_copy_assignable<std::remove_reference_t<decltype(*first)> >::value>

is a very complicated synonym for

std::is_nothrow_copy_assignable_t<std::remove_reference_t<decltype(*first)>>

in this context, because all the UnaryTypeTraits are defined by the standard to be "publicly and unambiguously derived" (directly or indirectly) from their BaseCharacteristic of std::true_type or std::false_type.

Your code returns std::prev(last) when *last == value, even if there's an instance of value prior to that in the sequence. This isn't a bug (presumably), but it's surprising enough that you should probably document it explicitly somewhere; otherwise I'd assume a function named find to have semantics exactly the same as std::find [alg.find] — i.e., "return an iterator to the first matching element".

Checking is_nothrow_copy_assignable<...> doesn't really buy you anything, if you aren't going to check is_nothrow_copy_constructible<...> and is_nothrow_equality_comparable<...> and is_nothrow_prefix_incrementable<...>... and so on, for all the other operations you do inside the "critical section" of your function (between the line where you blow away the old value of *real_last and the line where you restore it).

Alternatively, you could use a try/catch to make sure that you always restore the old value of *real_last. Since C++ lacks the "finally" construct that exists in languages such as Java and Python, I recommend a generic scope-guard construct such as Auto() if you find yourself doing this kind of thing very often.

Your code has a problem with empty ranges.


jquery update hidden and a tags First off, not the greatest with jQuery but know enough to get by. I created this mock up that is base on my application and wanted to see if there is a better way to update hidden values and a tags. When the form is saved I am using the sortorder to find the corresponding .ui-sortable li that has a tag and hidden fields to get updated on the page. I am trying to update the correct one and looping all of them checking with the sortorder  I just wanted to know if there is a better way to do this?

https://jsfiddle.net/tjmcdevitt/vh9n5srL/35/

actual code from a project

$("#submit-form").submit(function (event) {
    event.preventDefault();
    var sortOrder = $(this).data('sortorder');
        $('.ui-sortable li').each(function (i) {
        if (i == sortOrder) {
            console.log('Updating');

            $("#References_0_UI").html(data.referenceText);
            $("#References_0_UI").attr("href", data.GuidelineExternalReference);

            $('input[name="References[0].Text"]').val(data.referenceText);
            $('input[name="References[0].Link"]').val(data.GuidelineExternalReference);
            $('input[name="References[0].GuidelineId"]').val(data.Value);
            console.log("Completed Task");
        }
});

The main issue I see is that the looping doesn't accomplish anything useful, since you never reference the li that you iterate over - just take the sortOrder from the form and concatenate it into the selectors you try to find.

Because you reference the #References_##_UI twice, consider saving it in a variable first - or, even better, since this is jQuery, you can chain methods on the selected collection.

It sounds like the data.referenceText is text, not HTML markup - in which case you should insert it into the DOM with .text, not with .html. (.text is faster and safer)

$('#submit-form').submit(function (event) {
  event.preventDefault();
  const sortOrder = $(this).data('sortorder');
  $(`#References_${sortOrder}_UI`)
    .text(data.referenceText)
    .prop('href', data.GuidelineExternalReference);
  $(`input[name='References[${sortOrder}].Text']`).val(data.referenceText);
  $(`input[name='References[${sortOrder}].Link']`).val(data.GuidelineExternalReference);
  $(`input[name='References[${sortOrder}].GuidelineId']`).val(data.Value);
});

The above looks mostly reasonable to me, but I'd change the HTML too, if that's permitted. Numeric-indexed IDs are never a good idea; IDs should be reserved for singular, unique elements. (You can also consider not using IDs at all, because every time there is one, a global variable is created, and globals can result in confusing behavior).

A related issue is that the submit handler here is attached to:

$('#submit-form').submit(

Since IDs must be unique in a document, this will only attach a listener to a single form, but it sounds like you have multiple forms that you want to listen for submit events to.

To solve the duplicate IDs and the numeric-indexed IDs, use the already-existing class to select the forms instead, and once you have a reference to the form in the handler, use .find to select its children elements that need to be populated.

Your 3 hidden inputs are somewhat repetitive. It might look somewhat tolerable now, but if you might add more, or for the general case of linking each data property name to a particular input, consider using an object or array linking each property to the selector:

const inputNamesByDataProps = {
  referenceText: 'Text',
  GuidelineExternalReference: 'Link',
  Value: 'GuidelineId',
};
$('.form-horizontal').on('submit', (event) => {
  event.preventDefault();
  const $this = $(this);
  $this.find('a')
    .text(data.referenceText)
    .prop('href', data.GuidelineExternalReference);
  for (const [dataProp, inputName] of Object.entries(inputNamesByDataProps)) {
    $this.find(`input[name$=${inputName}]`).val(data[dataProp]);
  }
});

(The [name$=${inputName}] means: "Find an element whose name attribute ends with what's in inputName")

<form class="form-horizontal" data-sortorder="1">

Update Values

  Testing html.
  <input type='hidden' name='References[1].Index' value="1">
  <input type='hidden' name='References[1].Link' value="www.oldlink.com">
  <input type='hidden' name='References[1].Id' value="88">

    <button type="submit">Save</button>
    <button type="button" data-dismiss="modal">Close</button>

</form>

The input HTML name attributes look pretty repetitive too, but repetitive HTML usually isn't something to worry about, especially if your backend logic is easier to work with when the attributes are like References[1].Index. But if you wanted, you could change it to something like

<input type='hidden' name='sortorder' value="1">
<input type='hidden' name='Index' value="1">
<input type='hidden' name='Link' value="www.oldlink.com">
<input type='hidden' name='Id' value="88">

putting the [1] into the hidden sortorder input instead, and then parse the form values on the backend.


DFS with recursion and without stack this is my code to implement DFS with recursion but no stack. I ran few test cases which turned out to be positive but wanted to check the efficiency of the code.

graph = {'A': set(['B', 'C']),
         'B': set(['A', 'D', 'E']),
         'C': set(['A', 'E']),
         'D': set(['B', 'F']),
         'E': set(['C', 'B', 'F']),
         'F': set(['D', 'E'])}
visited_nodes = set()
is_visited = {}
#seen_nodes = set()
seen_nodes = []

def unseen_neighbors(start_node):
    unseen = []
    for i in graph[start_node]:
        if is_visited.get(i, 'NA') == 'NA':
            unseen.append(i)
    print ('start node and unseen ', start_node, unseen)
    return unseen

def DFS(graph, start_node):
    is_visited[start_node] = True
    #seen_nodes.append(start_node)
    #print ('seen ', seen_nodes)
    #add_neighbors(start_node)
    unseen = unseen_neighbors(start_node)
    if len(unseen) == 0:
        return
    for i in unseen:
        if is_visited.get(i, 'NA') == 'NA':
            seen_nodes.append(i)
            DFS(graph, i)

if __name__ == "__main__":
    seen_nodes.append('A')
    DFS(graph, 'A')
    print ("DFS Traversal ", seen_nodes, set(seen_nodes))

Well, your code is indeed doing DFS and it is doing tail recursion. I don't see any improvement from an algorithm perspective.
One low-hanging fruit is to get rid of seen_nodes as it seems to exist only for debugging purposes.


Creating a better BST I have created a BST like many others. I am curious to if there is a better way to write it. It seems pretty straight forward, but I feel like it can be improved on a bit.

//MRBST.h

#ifndef MRBST_H
#define MRBST_H
#include <iostream>

template <class T>
class MRBST {

private:
        struct treeNode
        {
                treeNode* left;
                treeNode* right;
                int data;

        };
        treeNode* root;
        int count;

public:
        void push(const T &);
        MRBST();
        bool search(const T &);
        void PrintPreorder();
        ~MRBST();
        bool empty() const;
        int numberOfNodes() const;
        void preorder(treeNode* pre);

};

template<class T>
MRBST<T>::MRBST()
{
    root = NULL;
    count = 0;
}

template<class T>
void MRBST<T>::push(const T & n)
{
    treeNode* d = new treeNode;
    treeNode* parent;
    d->data = n;
    d->left = NULL;
    d->right = NULL;
    parent = NULL;
    if(empty())
    {
        root = d;
        count++;
    }
    else
    {
        treeNode* current;
        current = root;

        while(current != NULL)
        {
            parent = current;
            if(d->data >= current->data)
            {
                current = current->right;
            }
            else
            {
                current = current->left;
            }
            if(d->data <= parent->data)
            {
                parent->left = d;
            }
            else
            {
                parent->right = d;
            }
        }
        count++;
    }
}

template<class T>
int MRBST<T>::numberOfNodes() const
{
    return count;
}

template<class T>
bool MRBST<T>::empty() const
{
    return root == NULL;
}

template<class T>
void MRBST<T>::PrintPreorder()
{
    preorder(root);
}

template<class T>
void MRBST<T>::preorder(treeNode* pre)
{
    if(pre != NULL)
    {
        std::cout << " " << pre->data << " ";
        if(pre->left)
        {
            preorder(pre->left);
        }
        if(pre->right)
        {
            preorder(pre->right);
        }
    }
}

template<class T>
MRBST<T>::~MRBST()
{
    delete root;

}
#endif

There are several improvements that can be done, starting from the class declaration:

The class name is not very descriptive. MRBST could be an initialism for many things. BinarySearchTree is much better. Don't be afraid of using long names if they convey information more clearly.
One thing doesn't make sense here: The class is a template class, but the data field of the treeNode is an int. If you are only storing ints, then the class doesn't have to be a template. You should however replace that int with T and make it generic.
The naming convention of treeNode is a little unusual for types. This kind of camelCase notation is usually applied to variables. Types themselves are more commonly declared using CamelCase, fist letter uppercase.
PrintPreorder() breaks your naming convention for methods. printPreorder() would be more consistent.
Ordering of class data and member methods: I find more adequate placing public methods and data first, followed by protected methods/data and lastly anything private. This is convenient for readers of the class declaration, since they will be interested in the public interface most of the time.
Order of the public fields: Place constructors first, followed by assignment/move operators, then the destructor and the rest after those. The fist thing you want to inspect when reading a class declaration is how the constructor(s), destructor and assignment are implemented, because that gives you hints about how the class lifetime is managed.
Unnamed function parameters in push() and search(): You should always name function parameters in the header declaration because that's a form of code documentation.
Remove excessive blank lines from the header file. There are some unexpected blank lines in there that serve no purpose. It looks untidy.

More aspect to improve in the implementation:

Please don't use NULL in C++ code. Your compiler should be C++11 ready by now, so use nullptr.
PrintPreorder() and preorder() should also be const methods since they don't mutate member state. Also the treeNode pointer that preorder() takes should be a const pointer. E.g.: const treeNode* pre.
Currently, your class leaks memory. You are only deleting the root of the tree. All leaf nodes will leak. You need to recursively delete all nodes before deleting the root. The process is basically the same as how you did to print the tree.

There are probably more details that can be improved, as well as architectural improvements. Other reviews might want to expand on those.


AutoSuggest: Better way to implement focus after inserting a snippet Introduction:

I am working on a javascript plugin AutoSuggest, My aim is to create an IDE like autocomplete experience in web applications using JavaScript.

One of the features I have implemented is focusing a particular portion of inserted snippet after insertion. This is how I am asking input for focus.

  focusText: [StartIndex, EndIndex]

  StartIndex and EndIndex of the content that should be in focus
  after inserting the text. As of now these indexes should be calculated
  as (0 - numberOfCharactersFromEnd)

  focusHtml: [StartIndex, EndIndex]

  StartIndex and EndIndex of the content that should be in focus after  inserting the HTML. As of now these indexes should be calculated as (0 -  numberOfCharactersFromEnd). This should not include the characters from  HTML tags

Example:

{
    value: 'script',
    insertText: '

I do feel that negative numbers seems a bit backwards compared to most other libraries I have used. I am not sure what all would be involved with reworking this code to utilize positive numbers but it seems it may be hefty. Though maybe you could accept positive numbers and convert them to negatives by subtracting them from the length of the string.

I see one spot that may have a possible performance improvement- in the recursive call to setSelection():

setSelection(
    lastFocus - lastNode.textContent.length,
    Array.from(lastNode.childNodes),
    method
);

here Array.from() is called to copy the array of nodes from the childNodes collection  from lastNode. The spread operator can also be used to put those child nodes into an array (from the node collection), allowing you to eliminate that extra function call.

setSelection(
    lastFocus - lastNode.textContent.length,
    [...lastNode.childNodes],
    method
);


Long arithmetic addition implemented in functional style This implementation uses ECMA6 syntax and babel as transpiler. You can use this code to add integers which are bigger than Number.MAX_SAFE_INTEGER.

var _ = require('underscore'),
    assert = require('assert');

var strAdd = function(lnum, rnum) {
  var {rem, res} = _.chain(
    _.zip(
      lnum.split('').reverse(),
      rnum.split('').reverse()
    )
  ).reduce(
    ({rem, res}, [left, right]) => {
      var sum = Number(left || 0) + Number(right || 0) + rem;
      res.push(
        sum % 10
      );
      return {
        res,
        rem: ~~(sum/ 10)
      };
    },
    {
      rem: 0,
      res: []
    }
  ).value();
  if (rem !== 0) {
    res.push(rem);
  }
  return res.reverse().join('');
};
assert(strAdd('1', '9') === '10', strAdd('1', '9'));
assert(strAdd('1', '0') === '1', strAdd('1', '0'));
assert(strAdd('5', '5') == '10');
assert(strAdd('2', '2') === '4');
assert(strAdd('20', '202') === '222');

Input validation

The function will happily return non-sense result when an argument contains not only digits.
Unless you are absolutely certain that this function is used in a safe context,
it would be good to validate the inputs.

Going even further,
I would create a validate function that returns a number or else throws an exception, and call it after splitting:

  lnum.split('').map(validate).reverse(),
  rnum.split('').map(validate).reverse()

This way the summing step will not need anymore the Number conversions,
 a bit simpler:

  var sum = (left || 0) + (right || 0) + rem;

Cleverness

I find this clever, but hacky:

rem: ~~(sum/ 10)

I suggest to just spell out the intention nicely:

rem: sum > 9 ? 1 : 0

On the other hand, instead of this:

if (rem !== 0) {

Since the value of rem is guaranteed to be either 0 or 1,
I think it's totally fine to be lazy and rely on the truthiness:

if (rem) {


Simple calculator to evaluate arithmetic expressions I've studied Java for 2 and a half months. Got bored and decided to make a basic calculator that takes any basic expression with 2 integers/doubles. E.g. 5.5x10, 5+5, 5-2.5, 6/2.

Is this a good way to approach this task?
How could it be improved?

The subtraction has a "+"  because substring takes into account the "-" as part of the second substring and the parseDouble turns that into a minus therefor two minuses would make a plus. I corrected the multiplication and division and i left the addition and subtraction how it was. I would correct all in the future.

/*
 * Author v0lk
 * Simple calculator program
 * 10/01/18
 */

import java.util.*;

public class Calc {

    public static void main(String[] args) {
        Calc c = new Calc();

        c.repeatCalc();

    }

    Scanner input = new Scanner(System.in);

    public void repeatCalc() {

        while (1>0) {
            calculator();
        }
    }

    public void calculator() {
        double result=0;
        String problm = "";
        System.out.println("Use: \n\"/\" to divide \n\"x\" to multiply \n\"+\" for addition \n\"-\" to substract \n\n>");

        problm = input.nextLine();

        for (int i=0; i<problm.length();i++) {

            if (problm.charAt(i)=='/') {

                result = Double.parseDouble(problm.substring(0, i)) / Double.parseDouble(problm.substring(i+1,problm.length()));

            }

            if  (problm.charAt(i)=='-') {
                result = Double.parseDouble(problm.substring(0, i)) + Double.parseDouble(problm.substring(i,problm.length()));

            }

            else if  (problm.charAt(i)=='x') {
                result = Double.parseDouble(problm.substring(0, i)) * Double.parseDouble(problm.substring(i+1,problm.length()));

            }

            else if  (problm.charAt(i)=='+') {

                result = Double.parseDouble(problm.substring(0, i)) + Double.parseDouble(problm.substring(i,problm.length()));

            }
        }

        System.out.println("Answer is: " + result);
    }

}

Thanks for sharing your code!

Pros

You actually created an instance of your class instead of using static methods
you have more than one method.

Cons

Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

Naming Conventions

Please read (and follow) the
Java Naming Conventions

E.g.: method names should be or start with a verb. Your method calculator() should better be named calculate().

Care for correct spelling

You named a variable problm. What is that? I suspect you misspelled problem, but it could something completely different.

Declare variables as late as possible

Your method calculator() has two variables immediately declared at the beginning of the method. you first variable is result but it is the last one referenced. It is not needed untill the start of the for loop. Therefore it should be declared there.

The reason is that "late declaration" simplifies the use of your IDEs automated refactorings. One of them is extract method.
You can use it to put the for loop into a separate method.
If you do it on your code as it is you would get this:

public void calculator() {
    double result=0;
    String problm = "";
    System.out.println("Use: \n\"/\" to divide \n\"x\" to multiply \n\"+\" for addition \n\"-\" to substract \n\n>");

    problm = input.nextLine();

    result = calculateResult(problm, result);

    System.out.println("Answer is: " + result);
}

private double calculateResult(String problm, double result){
    for (int i=0; i<problm.length();i++) {
       // calculate the actual result...
    }
    return result;
}

You new method got an useless parameter.

If you had declared the variable result before the loop you could hav included it in the extracted patr and your code would be like this:

public void calculator() {
    System.out.println("Use: \n\"/\" to divide \n\"x\" to multiply \n\"+\" for addition \n\"-\" to substract \n\n>");

    String problm = input.nextLine();

    double result = calculateResult(problm);

    System.out.println("Answer is: " + result);
}

private double calculateResult(String problm){
    double result=0;
    for (int i=0; i<problm.length();i++) {
       // calculate the actual result...
    }
    return result;
}


Dropdown menu that opens on click using plain Javascript I made a simple dropdown menu which opens when clicked, and closes when the user clicks anywhere outside the menu. The following is the codebase:

function deactivateAllDropdownTriggers() {
   document.querySelectorAll('.dropdown-trigger.active').forEach(function(elem) {
        elem.classList.remove('active');
    })
}

function handleDropdownClicks(event) {
    if (event.target.matches('.dropdown-trigger')) {
        if (event.target.classList.contains('active')) {
            event.target.classList.remove('active');
        } else {
            deactivateAllDropdownTriggers();
            event.target.classList.add('active');
        }
    } else {
        if (!event.target.matches('.dropdown-menu *')) {
            deactivateAllDropdownTriggers();
        }
    }
}

document.addEventListener('click', handleDropdownClicks, false);
.dropdown {
    display: inline-block;
    position: relative;
}
.dropdown .dropdown-menu {
    display: none;
    position: absolute;
    -moz-box-shadow: 0 2px 4px rgba(0, 0, 0, 0.15);
    -webkit-box-shadow: 0 2px 4px rgba(0, 0, 0, 0.15);
    box-shadow: 0 2px 4px rgba(0, 0, 0, 0.15);
    width: auto;
    border: 1px solid rgba(0, 0, 0, 0.15);
    border-radius: var(--border-radius);
    background-color: #ffffff;
    z-index: 1;
}
.dropdown .dropdown-menu ul {
    padding: 0;
    margin: 0;
}
.dropdown .dropdown-menu ul > li {
    list-style: none;
    margin: 0;
}
.dropdown .dropdown-menu ul > li.dropdown-menu-content {
    padding: 0.6rem 1.2rem;
    white-space: nowrap;
}
.dropdown .dropdown-menu ul > li.dropdown-menu-divider {
    height: 1px;
    background-color: rgba(0, 0, 0, 0.05);
}
.dropdown .dropdown-menu ul > li > a {
    display: block;
    text-decoration: none;
    padding: 0.6rem 1.2rem;
    color: rgba(0, 0, 0, 0.8);
    cursor: pointer;
    white-space: nowrap;
    min-width: 12rem;
}
.dropdown .dropdown-menu ul > li > a:hover {
    background-color: rgba(0, 0, 0, 0.025);
}
.dropdown .dropdown-trigger.on-click.active + .dropdown-menu {
    display: block;
}

    <button class="dropdown-trigger on-click">Pick your weapon</button>

                Weapons

Sword

Lance

Axe

Bow

    <button class="dropdown-trigger on-click">Pick your class</button>

                Classes

Fighter

Archer

Thief

Ninja

One thing that concerns me is the open event listener. I could not create a system where an event listener is added to the document when a menu is opened, and the event listener is removed when the menu is closed. Perhaps one of you could help me with that.

Anyway, would appreciate a review of this code.

Overall the code looks fine. I just have a couple suggestions about the JavaScript and CSS.

JS

querySelectorAll vs getElementsByClassName

Generally document.getElementsByClassName will be quicker than document.querySelectorAll (see this post for more information) and the former also returns a live HTMLCollection so it wouldn't need to be queried each time. Bearing in mind that deactivateAllDropdownTriggers() looks for elements with both class names dropdown-trigger and active, only the latter is really needed. If active applies to other elements, then perhaps a name like active-dropdown would help narrow down elements. In order to iterate over the items in that collection, they would need to be put into an array - that can be done with the spread operator ...

const activeElements = document.getElementsByClassName('active');
function deactivateAllDropdownTriggers() {
  [...activeElements].forEach(elem => elem.classList.remove('active'));
}

CSS

Useless class on-click

It appears that the on-click class is only utilized in the last selector (i.e. .dropdown .dropdown-trigger.on-click.active + .dropdown-menu) but that class name could be removed since it doesn't appear to be used anywhere else

default values

Some rules set values to what should be the default values - e.g. margin: 0 for the unordered list and list items, and cursor: pointer for the anchors. Those shouldn't be needed unless there are other styles matching the selectors that would add different values - e.g. for other page contents or a browser/plugin stylesheet


Downloading the oldest available version of some data files I am using the cURL tool in MATLAB 2013b with Ubuntu to download a whole bunch of files. The files have one of three possible versions: 1.0.0, 2.0.0, or 2.1.0. Using the HTTP, I first check the headers with three queries to see which version exists. Then whichever exists I download it. If none exists (because of nonexistent dates) then I just move on.

Is there any other more efficient way of querying and then downloading the files? For example, check 1.0.0. If it exists, download it. If it doesn't exist then check 2.0.0 and so on. The only way I can think of doing that was some very ugly nested if-elseif statements. But this way, I query three times no matter what so a little wasteful I think.

Also any other comments on my coding style are welcome.

    clc
    close all
    clear all

    for y = 2012:2013
    for m = 1:12
    for d = 1:31

    % To limit the dates because of the available data
    if y==2012 && m < 9
        continue
    elseif y==2013 && m > 9
        break
    end

    % The date string is created with padded zeros
    if (m < 10) && (d < 10)
        thedate = [num2str(y) '0' num2str(m) '0' num2str(d)];
    elseif m < 10
        thedate = [num2str(y) '0' num2str(m) num2str(d)];
    elseif d < 10
        thedate = [num2str(y) num2str(m) '0' num2str(d)];
    else
        thedate = [num2str(y) num2str(m) num2str(d)];
    end

    newname = ['ephemA' thedate '.h5']

    % The entire file name is created
    thefile = ['http://www.rbsp-ect.lanl.gov/data_pub/rbspa/MagEphem/def/rbspa_def_MagEphem_TS04D_' thedate];

    % Use curl with --head flag to check the header to see which version exists
    [status, result1] = system(['curl --head ' thefile '_v1.0.0.h5']);
    [status, result2] = system(['curl --head ' thefile '_v2.0.0.h5']);
    [status, result3] = system(['curl --head ' thefile '_v2.1.0.h5']);

    % 200 means the file exists and is downloaded
    if ~isempty(strfind(result1,'HTTP/1.1 200'))
        [status, result] = system(['curl -o ' newname ' ' thefile '_v1.0.0.h5']);
    elseif ~isempty(strfind(result2,'HTTP/1.1 200'))
        [status, result] = system(['curl -o ' newname ' ' thefile '_v2.0.0.h5']);
    elseif ~isempty(strfind(result3,'HTTP/1.1 200'))
        [status, result] = system(['curl -o ' newname ' ' thefile '_v2.1.0.h5']);
    end

    end
    end
    end

Why don't you read the index from http://www.rbsp-ect.lanl.gov/data_pub/rbspa/MagEphem/def/ and use that to decide which files you want to download?

Also, try to give multiple files (eg. 20) to each curl call: this will enable curl to reuse the same connection for those files.

Please also note that Matlab is not the right tool for this job. For example, this would be easier in Python which has many libraries to perform HTTP requests.


Simple Hangman game in Python I recently started learning Python, and made a simple text based Hangman game. I wanted to know if there could be anything done to make the code cleaner or more efficient.

import random
import os
import sys

def main():
    pickword()

def start(word, twoplayer):
    wordarray = ''
    userguess = []
    guessright = 0
    lettersright = 0
    if len(word) < 5:
        tries = 6
    else:
        tries = 10
    usedletters = []

    if twoplayer == False:
        print(word)
        print("Welcome to hangman")
        print("I will think of a word and you will have to guess it, are you ready!")
        input("Press enter to continue")
    else:
        print("Welcome to hangman")
        print("Player 1 has already thought of a word, let's begin")
        input("Press enter to continue")

    lengthword = len(word)

    if twoplayer == False:
        for i in range(1, lengthword):                  #creates a new variable, all underscores
            userguess.append("_ ")
    else:
         for i in range(1, lengthword):                  #creates a new variable, all underscores
            userguess.append("_ ")

    wordarray = (list(word))

    if twoplayer == False:
        for i in range(0, lengthword):                  #removes \n from end of word
            if wordarray[i] == "\n":
                wordarray.remove(wordarray[i])
                break

    usedletters = []
    getuserguess(wordarray, userguess, tries, usedletters, word)

def pickword():
    word = 0
    twoplayer = input("Do you want to play 2 player[Y/N]")
    if twoplayer.lower() == "y":
        word = input("Player 1, enter the word you want player 2 to guess, don't let them see! ")

        if len(word) < 3:
            print("That word is too short")
            pickword()

        print("\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n")
        print("\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n")
        print("\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n")

        word = word.lower()
        twoplayer = True
        start(word, twoplayer)
    else:
        try:
            word = random.choice(open('hangman.txt').readlines())
            twoplayer = False
            start(word, twoplayer)

        except:
            input("Error, no word list found, press enter to exit")
            sys.exit()

def getuserguess(wordarray, userguess, tries, usedletters, word):

    print(' '.join(userguess))
    guess1 = input("Enter your guess: ")

    if len(guess1) < 1 or len(guess1) > 1 or guess1.isdigit():
        print("Invalid input")
        getuserguess(wordarray, userguess, tries, usedletters, word)
    else:
        checkuserguess(wordarray, userguess, guess1, usedletters, tries, word)

def lose(word):
    again = input("Sorry, you lost, the word was {}Would you like to play again[Y/N]".format(word))
    if again.lower() ==  "y":
        pickword()
    else:
        sys.exit()

def win():
    again = input("Congrats, you won hangman, do you want to play again?[Y/N]")
    if again == "Y" or "y":
        pickword()
    else:
        sys.exit()

def checkuserguess(wordarray, userguess, guess1, usedletters, tries, word):

    guess1list = (list(guess1))

    if guess1list[0] in usedletters:
        print("You already used that letter")
        getuserguess(wordarray, userguess, tries, usedletters, word)

    if guess1 in wordarray:
        usedletters.append(guess1)
        for i in range(0, len(wordarray)):
            if wordarray[i] == guess1list[0]:
                userguess[i] = guess1list[0]
                usedletters.append(guess1list[0])

        if "_ " not in userguess:
            print(' '.join(userguess))
            win()
        else:
            getuserguess(wordarray, userguess, tries, usedletters, word)

    else:
        if tries > 0:
            print("Sorry, that letter is not in the word, you have ", tries, " remaining")
            tries -= 1
            getuserguess(wordarray, userguess, tries, usedletters, word)
        else:
            lose(word)

main()

For this, I am going to go through your program in the order it is written:

Don't use if twoplayer == False:.  You can simply say if not twoplayer:  You don't even need that, though the first time because you do the same thing in the if and elseblocks`.
You can use " ".join("_" * lengthword) instead of  for i in range(1, lengthword): userguess.append("_ ").  Besides making it cleaner, you weren't adding enough underscores.  The second argument to range() is exclusive.  Imagine if the length of the word is two.  In that case, range(1, 2) returns [1] because the second argument is exclusive.  That has length 1 instead of the same length as the word: 2.
Don't use wordarray.remove(wordarray[i]).  Instead, use wordarray.pop(i).  If you use .remove(), the first occurrence of the word will be removed, not necessarily the one you want to be removed.
When you ask "Do you want to play 2 player?", you account for "y" and anything else.  What if the user says "yes"?  It would still count as no.  Instead, you should use a while loop:

twoplayer = input("Do you want to play 2 player? [Y/N] ")
while twoplayer.lower() not in ("y", "n"):
    twoplayer = input("[Y/N] ")

Instead of setting twoplayer inside of the if and else blocks, you can just do twoplayer = ("n", "y").index(twoplayer.lower())
If the word is too short, you don't want to call pickword() again because pickword() asks "Do you want to play 2 player?"  If the word is too short, that doesn't mean the user wants to change his mind on that question.  Instead, use a loop:

while len(word) < 3:
    print("That word is too short")
    word = input("Player 1, enter the word you want player 2 to guess, don't let them see! ")

How do you know how many lines to print?  Instead of printing an arbitrary number of lines, print("\033c") will clear the terminal.
You call start(word, twoplayer) in both the if and the else block.  That is a sign that it shouldn't be in either one.  Put it after both blocks so that you aren't duplicating code.
While we're at that point in the program, you want only one line in your try: block: word = random.choice(...).  twoplayer should be defined as I outlined above, so we don't need that here at all and start(word, twoplayer) will be outside of the if and else blocks, so we don't need that.  Besides, the only reason you want the try block is because you are dealing with a file.  If anything else is throwing an error, you want the traceback because it shouldn't be.
Don't use a bare except:  You have certain errors that you think you might get; list them here.  In this case, it's except IOError:
sys.exit(), when given an argument, uses that argument as the exit code.  In this case it is failure, so you should use sys.exit(1).
Your check for guess1's validity is not the best.  To make sure that it is one character, use len(guess1) != 1 as the check.  Also, guess1.isdigit() is probably not the only thing that is invalid.  For example, $ shouldn't be in the word, right?  Instead, use not guess1.isalpha() to make sure it is an alphabetic character.
Instead of using recursion and typing out all of those variable names, use a loop:

print(' '.join(userguess))
guess = input("Enter your guess: ")
while len(guess) != 1 or not guess.isalpha():
    print("Invalid input")
    guess = input("Try again: ")

checkuserguess(wordarray, userguess, guess, usedletters, tries, word)

You have duplicate code in lose() and win(): Would you like to play again.  Instead, define a function that asks that question, and use it in lose() and win().  Make sure that in that function you use if again.lower() == "y": as you did in lose().  if again == "Y" or "y": does not work how you would expect it to.  See this StackOverflow post for why.
You check if "_ " is in userguess, but you should be checking for just the underscore, not the underscore with a space.  If the underscore is at the end of the word, there shouldn't be a space.  If there is, you're doing it wrong and you shouldn't depend on it.

This is my first complete review, so if anyone has suggestions, I would be happy to modify it.


Tic Tac Toe "Cats Game"/End Game I have made my first game in Python.

My program can determine when there is a winner, however I am unsure what to do if the board is full and there is NO winner (a cats game).

import cTurtle
import random
import sys

def DrawTicTacToeBoard(t):
    #Vertical Lines of board
    t.pensize(3)
    t.up()
    t.goto(.5,-.5)
    t.down()
    t.goto(.5,2.5)
    t.up()
    t.goto(1.5,-.5)
    t.down()
    t.goto(1.5,2.5)
    #Horizontal Lines of board
    t.up()
    t.goto(-.5,.5)
    t.down()
    t.goto(2.5,.5)
    t.up()
    t.goto(-.5,1.5)
    t.down()
    t.goto(2.5,1.5)

def flipPlayer(currentPlayer):
    if currentPlayer==0:
        currentPlayer=1
    else:
        currentPlayer=0
    return currentPlayer

def drawWhiteBox(t,x,y):
    t.up()
    t.goto(x-.25,y+.25)
    t.down()
    t.color("white")
    t.begin_fill()
    for i in range(4):
        t.forward(.8)
        t.right(90)
    t.end_fill()
    t.color("black")

def drawMarker(t,currentPlayer,markerList,move,boardLoL):
    by=int(move[0:1])
    bx=int(move[2:])
    y=by+(.22)
    x=bx-(.15)
    drawWhiteBox(t,x,y)
    t.up()
    t.goto(x,y)
    boardLoL[by][bx]=markerList[currentPlayer]
    t.write(markerList[currentPlayer],move=False,align="left",font=("Arial",48,"normal"))

def getValidMove(validMoveList,markerList,currentPlayer,boardLoL):
    move = ""
    while move not in validMoveList:
        move = input("Enter location to place your marker " + markerList[currentPlayer] + ", or EXIT to end game => ")
        if move=="EXIT":
            sys.exit()
        if move in validMoveList:
            by=int(move[0])
            bx=int(move[2:])
            if boardLoL[by][bx]=="":
                return move
            else:
                print("Location already occupied")
                move=""

def gameWon(b):
    for row in range(3):
        if b[row][0]==b[row][1] and b[row][1]==b[row][2] and b[row][0]!="":
            print("Way to go",b[row][0])
            return b[row][0]
    for column in range(3):
        if b[0][column]==b[1][column] and b[1][column]==b[2][column] and b[0][column]!="":
            print("Way to go",b[column][0])
            return b[0][column]
    for diag in range(2):
        if b[0][0]==b[1][1] and b[1][1]==b[2][2] and b[0][0]!="":
            print("Way to go",b[0][0])
            return b[0][0]
        if b[2][0]==b[1][1] and b[1][1]==b[0][2] and b[2][0]!="":
            print("Way to go",b[2][0])
            return[2][0]

    return ""

def boardIsFull(boardLoL):
    return False

def main():
    boardLoL=[["","",""],["","",""],["","",""]]
    markerList=["O","X"]
    validMoveList=["0,0","0,1","0,2","1,0","1,1","1,2","2,0","2,1","2,2"]
    joe=cTurtle.Turtle()
    joe.setWorldCoordinates(-2,3,4,-1)
    joe.ht()
    currentPlayer=random.randint(0,1)
    DrawTicTacToeBoard(joe)
    move = ""
    while boardIsFull(boardLoL)==False and gameWon(boardLoL)=="" and move !="EXIT":
        move = getValidMove(validMoveList,markerList,currentPlayer,boardLoL)
        drawMarker(joe,currentPlayer,markerList,move,boardLoL)
        currentPlayer=flipPlayer(currentPlayer)
    gameWon(boardLoL)
    boardIsFull(boardLoL)

main()

import cTurtle
import random
import sys

Excellent!  Many people use wildcard imports, (from <module> import *), but you didn't.  You even alphabetized your imports.  Keep up the good work.

PEP 8, the Python style guide, has this to say in the Function Names section:

  Function names should be lowercase, with words separated by underscores as necessary to improve readability.

  mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

That means that DrawTicTacToeBoard would be draw_tic_tac_toe_board, for example.

Your spacing does not comply with PEP 8.  Specifically:

  Always surround these binary operators with a single space on either side: assignment (=), augmented assignment (+=,-= etc), comparisons (==,<,>,!=,<>,<=,>=,in,not in,is,is not), Booleans (and,or,not).

#Vertical Lines of board
t.pensize(3)
t.up()
t.goto(.5,-.5)
t.down()
t.goto(.5,2.5)
t.up()
t.goto(1.5,-.5)
t.down()
t.goto(1.5,2.5)

I realize that you want to save characters, but -.5 is a little hard to read.  It almost looks like the - is one argument and 5 is another argument.  The extra 0 between the two would greatly improve readability.  Speaking of saving characters, you could use a for loop ... probably.  I haven't come up with anything that I really like, though.  You should also consider a different name for your argument.  Perhaps turtle would be more descriptive.

def flipPlayer(currentPlayer):
    if currentPlayer==0:
        currentPlayer=1
    else:
        currentPlayer=0
    return currentPlayer

This function is included in two rules mentioned above, but I mention it here because there is a much easier way to do it:

def flip_player(current):
    return (current + 1) % 2

Read more about the modulus operator and when to use it.

for i in range(4):
    t.forward(.8)
    t.right(90)

Again, you should be using a more descriptive name, but I brought up the for loop because you use i as your variable name.  There isn't anything wrong with using it, but in cases where the variable is not used, it is common practice in Python to use _ as the variable name.  It just makes it a little more obvious that it is an unused variable.

move = input("Enter location to place your marker " + markerList[currentPlayer] + ", or EXIT to end game => ")

That line is 110 characters not including indentation.  PEP 8 says:

  Limit all lines to a maximum of 79 characters.

Perhaps it would be better to define a template and format it in the loop:

template = "Enter location to place your marker {}, or EXIT to end game => "
while move not in valid_move_list:
    move = input(template.format(marker_list[current_player]))
    ...

I would also change how you check your conditions in the while loop.  Here is what I would do:

while True:
    move = input(template.format(marker_list[current_player]))
    if move == "EXIT":
        sys.exit()
    elif move in valid_move_list:
        y, x = map(int, move.split())
        if not boardLoL[y][x]:
            return move
        else:
            print("Location already occupied")
    else:
        print("Invalid move")

There is no need for while to be checking anything since you account for all cases inside.  I also added an else: so that the user would know why he was being asked again for input.

def gameWon(b):
    for row in range(3):
        if b[row][0]==b[row][1] and b[row][1]==b[row][2] and b[row][0]!="":
            print("Way to go",b[row][0])
            return b[row][0]
    for column in range(3):
        if b[0][column]==b[1][column] and b[1][column]==b[2][column] and b[0][column]!="":
            print("Way to go",b[column][0])
            return b[0][column]
    for diag in range(2):
        if b[0][0]==b[1][1] and b[1][1]==b[2][2] and b[0][0]!="":
            print("Way to go",b[0][0])
            return b[0][0]
        if b[2][0]==b[1][1] and b[1][1]==b[0][2] and b[2][0]!="":
            print("Way to go",b[2][0])
            return[2][0]

You can simplify that to this:

def game_won(board):
    for row in board:
        if len(set(row)) == 1:
            return row[0]
    for column in zip(*board):
        if len(set(column)) == 1:
            return column[0]
    for diag in range(2):
        ...

I would do all of the printing outside of the function.  In your main() function, it would be like this:

print(game_won(boardLoL))

instead of:

game_won(boardLol)

I'm not crazy about the diagonal searching, but I can't think of anything better at the moment.  Maybe it's because I've been awake for ... yawn

return[2][0]

I don't think that's what you meant.  Perhaps, return b[2][0]

def boardIsFull(boardLol):
    return False

Haven't added it yet, huh?  Maybe this:

def board_is_full(board):
    return all(all(row) for row in board)

Yeah, nifty.  You see, all(row) makes sure that each item in row is a Truthy value.  Any non-empty string is Truthy, so if any of the cells in that row is empty, all(row) will return False.  Since I am running all() on each of those all() calls, it returns False if any cell in the board is empty, but True otherwise.

while boardIsFull(boardLoL)==False and gameWon(boardLoL)=="" and move !="EXIT":

Any condition is checking a boolean.  That is, if ...: is seeing if ... is True.  Using if ... == True: is redundant because you could have just used if ...:.  Consequently, if ... == False: is redundant because you could have used if not ...:  You can simplify that line like this:

while not board_is_full(boardLoL) and not game_won(boardLoL) and move != "EXIT":

That line is one character too long for PEP 8.  I would suggest changing boardLoL to board in all places.  What is LoL doing there anyway?  Are you laughing as you type?

I hope I haven't sounded too harsh, but it's been a late night.


Read function that properly handles interrupts This function is meant to be used for reading files. It returns all bytes asked unless EOF is reached. It handles interrupts and returns -1 on errors.

//safe function to read all bytes asked, only returns less bytes if EOF
static ssize_t read_all(int fdes, void *buffer, size_t size)
{
  ssize_t ret;
  ssize_t ret2;

  ret = read(fdes, buffer, size);
  if(ret == -1){
    if(errno == EINTR)
      return read_all(fdes, buffer, size);

    return -1;
  }

  if(ret && ret != size){
    ret2 = read_all(fdes, buffer + ret, size - ret);
    if(ret2 == -1)
      return -1;

    return ret + ret2;
  }

  return ret;
}

My main problem is the recursion. I don't think its major but I personally would use a loop. Given the current layout I can't quite convince myself that it works in all situations (especially since there are two alternative recursive calls).

Declare variables close to the usage point (rather than everything at the top).

    ssize_t ret;
    ssize_t ret2;

C has been updated so that you can declare variables at any point in a function. This helps in readability as you don't need to scroll far to find the variable declaration. Also if you declare in the most restrictive scope possible it helps to prevent data leaking (out of a scope).

see: https://stackoverflow.com/a/8474123/14065

Try this:

static ssize_t read_all(int fdes, void* buffer, ssize_t size)
{
    ssize_t  totalRead = 0;

    while(totalRead != size)
    {
        ssize_t thisRead = read(fdes, buffer + totalRead, size - totalRead);

        if ((thisRead == -1) && (errno == EINTR)) {
            continue;
        }
        // Note: There are other errors that may not be erros.
        // EAGAIN or EWOULDBLOCK spring to mind but may need special handling
        // as immediately calling read may just cause a busy wait you may
        // want to suspend the thread by calling sleep..

        if (thisRead == -1) {
            return -1;
        }
        if (thisRead == 0) {
            break;
        }
        totalRead += thisRead;
    }
    return totalRead;
}


Providing a consistent interface for inRange function over modded values I want to write a function named inRange which will have below interface

inRange(long a, long b, boolean inclusiveStart, boolean inclusiveEnd)

This function calculates if this.value is in between a & b. However there are two important points to note:

Two boolean values (inclusiveStart & inclusiveEnd) specify if a (or b) should be included in the range.
All 3 values viz. a, b and this.value are modded values i.e they define a circular range.

I have tried writing this function but here is my concern,

How to provide a consistent interface so that client does not have to worry about circular range. Lets say, a == b then should the function consider complete range or should it just consider a single value for the range. Or when a = 0 & b = MOD - 1 then what should be the function behavior for different combinations of boolean values.

Below is my code:

private boolean inRangeBothInclusive(long start, long end) {
    if (start == end)
    return this.value == start;
    else if (start < end) {
    return start <= this.value &&
        this.value <= end;
    } else {
    return !(end <= this.value &&
         this.value <= start);
    }
}

public boolean inRange(long start, long end, boolean startInclusive,
               boolean endInclusive) {
    /* Ranges can have bigger start and smaller end value due to mod */

    if (!startInclusive) {
    start = (start + 1) % MOD;
    }
    if (!endInclusive) {
    end = (end - 1) % MOD;
    if (end < 0)
        end += MOD;
    }
    return inRangeBothInclusive(start, end);
}

If you really need to present the boolean parameters in your client API I would do that as follows

private boolean inRangeBothInclusive(long start, long end) {
    if (start == end) {
        return this.value == start;
    }
    else if (start < end) {
        return start <= this.value &&
               this.value <= end;
    }
    return !(end <= this.value &&
             this.value <= start);
}

public boolean inRange(long start, long end, boolean startInclusive, boolean endInclusive) {
    start = startInclusive ? start : start + 1;
    end = endInclusive ? end : end - 1;
    return inRangeBothInclusive(start, end);
}