Angle type in Haskell I wanted to create a type or class, which can be used to handle angles. I started with defining a class, to make it possible to store degrees in Integral, radians in Floating, etc... But I realized, that I will meet complex problems if I continue it so. For example handling the addition of two angles, like a Radian Double and a Degree Int.

To keep it simple, easy to use and develop, I decided to create one type only, and write the functions around it.

module Data.Angle where

import Data.Fixed -- mod'

data Angle a = Radians a deriving (Eq, Show)

-- Creating Angle from a value

-- | Create an Angle with the given degrees
angleFromDegrees :: (Integral d, Floating r) => d -> Angle r
angleFromDegrees x = Radians $ (realToFrac x) * pi/180

-- | Create an Angle with the given turns
angleFromTurns :: (Real t, Floating r) => t -> Angle r
angleFromTurns x = Radians $ (realToFrac x) * pi*2

-- | Create an Angle with the given turns
angleFromRadians :: (Floating r) => r -> Angle r
angleFromRadians x = Radians x

-- Get the value from Angle

-- | Get degrees from an Angle
angleValueDegrees :: (Floating r, RealFrac r, Integral d) => Angle r -> d
angleValueDegrees (Radians x) = round $ x / pi * 180.0

-- | Get radians from an Angle
angleValueRadians :: (Floating r) => Angle r -> r
angleValueRadians (Radians x) = x

-- | Get turns from Angle
angleValueTurns :: (Floating r) => Angle r -> r
angleValueTurns (Radians x) = x / (pi*2)

-- Basic functions

-- | Adding two angles
addAngle :: (Floating a) => Angle a -> Angle a -> Angle a
addAngle (Radians r1) (Radians r2) = Radians $ r1 + r2

-- | Normalize Angle: transforming back to (0-2pi)
normAngle :: (Floating a, Real a) => Angle a -> Angle a
normAngle (Radians r) = Radians $ mod' r (pi*2)

-- | Add two angles and normalize the result
addAngleNorm :: (Floating a, Real a) => Angle a -> Angle a -> Angle a
addAngleNorm a b = normAngle $ addAngle a b

-- | Distance between two angles
distAngle :: (Floating a, Real a) => Angle a -> Angle a -> Angle a
distAngle (Radians r1) (Radians r2) = Radians $ if (a' < b') then a' else b'
    where
        a' = mod' (r1-r2) (pi*2)
        b' = mod' (r2-r1) (pi*2)

-- | Flip Angle
flipAngle :: (Floating a) => Angle a -> Angle a
flipAngle (Radians r) = Radians (-r)

-- | Flip Angle and normalize the result
flipAngleNorm :: (Floating a, Real a) => Angle a -> Angle a
flipAngleNorm = normAngle . flipAngle

-- | Add degrees to Angle
addAngleDegrees :: (Floating r, Integral d) => Angle r -> d -> Angle r
addAngleDegrees ang deg = addAngle ang $ angleFromDegrees deg

-- | Add radians to Angle
addAngleRadians :: (Floating r) => Angle r -> r -> Angle r
addAngleRadians (Radians r1) r2 = Radians $ r1 + r2

-- | Add turns to Angle
addAngleTurns :: (Floating r, Real t) => Angle r -> t -> Angle r
addAngleTurns ang turn = addAngle ang $ angleFromTurns turn

-- Trigonometric functions

-- | Sine of the angle
sinAngle :: (Floating a) => Angle a -> a
sinAngle (Radians r) = sin r

-- | Cosine of the angle
cosAngle :: (Floating a) => Angle a -> a
cosAngle (Radians r) = cos r

-- | Tangent of the angle
tanAngle :: (Floating a) => Angle a -> a
tanAngle (Radians r) = tan r

-- | Cotangent of the angle
cotAngle :: (Floating a) => Angle a -> a
cotAngle (Radians r) = 1 / (tan r)

-- Inverse trigonometric functions

-- | Create angle from inverse sine
asinAngle :: (Floating a) => a -> Angle a
asinAngle x = Radians $ asin x

-- | Create angle from inverse cosine
acosAngle :: (Floating a) => a -> Angle a
acosAngle x = Radians $ acos x

-- | Create angle from inverse tangent
atanAngle :: (Floating a) => a -> Angle a
atanAngle x = Radians $ atan x

-- | Create angle from inverse cotangent
acotAngle :: (Floating a) => a -> Angle a
acotAngle x = Radians $ (pi/2) - (atan x)

The finished library is available here:

Haskell Angle Library

I would use newtype instead of data to define Angle, like this:

newtype Angle a = Radians { angleValueRadians :: a }

This will automatically create the angleValueRadians function to get the angle value, and will be a bit more efficient.

Here is a good answer explaining the differences between data, newtype, and type: https://stackoverflow.com/a/21081227/1525759

You could use function composition to define many of your functions instead of pattern matching (pattern matching isn't bad, I just prefer this style):

sinAngle :: (Floating a) => Angle a -> a
sinAngle = sin . angleValueRadians

Also, if you made Angle an instance of Applicative, you could define addAngle like this:

addAngle :: (Floating a) => Angle a -> Angle a -> Angle a
addAngle r1 r2 = (+) <$> r1 <*> r2
-- or even this, because `f <$> x <*> y` is the same as `liftA2 f x y`:
addAngle = liftA2 (+)

Here's how you could do that:

instance Functor Angle where
    fmap f (Radians x) = Radians (f x)

instance Applicative Angle where
    pure = Radians
    Radians f <*> r = fmap f r


A simple program that animates the Y position of multiple elements based on the window scroll position The following code animates an item's Y position based on how much of its parent is currently visible. This is the first time I ever animated something on JavaScript based on the window scroll position and the current performance is extremely bad. I am looking for ways to get this running as fast as possible for a smooth animation.

The code I use to get a percentage between 0 and 100 based on how much an element is currently visible is the following:

    const calcVisibilityForElem = (elem) => {
      const windowHeight = window.innerHeight;
      const docScroll = window.pageYOffset || document.documentElement.scrollTop;
      const divPosition = elem.offsetTop;
      const divHeight = elem.offsetHeight;
      const hiddenBefore = docScroll - divPosition;
      const hiddenAfter = divPosition + divHeight - (docScroll + windowHeight);

      if (
        docScroll > divPosition + divHeight ||
        divPosition > docScroll + windowHeight
      ) {
        return 0;
      } else {
        let result = 100;

        if (hiddenBefore > 0) {
          result -= hiddenBefore * 100 / divHeight;
        }

        if (hiddenAfter > 0) {
          result -= hiddenAfter * 100 / divHeight;
        }

        return result;
      }
    };

The code I use to animate:

const calcVisibilityForAllArticles = () => {
  document.querySelectorAll('article').forEach((item) => {
    const top = calcVisibilityForElem(item);
    if (top !== 0) {
      item.querySelector('span').style.transform = `translateY(${Math.floor(
        top * 1.5
      )}px)`;
    }
    if (top >= 80) {
      const navItem = document.querySelector(`[href="#${item.id}"]`);
      document.querySelectorAll('nav a').forEach((navItem) => {
        navItem.classList.remove('highlight');
      });
      navItem.classList.add('highlight');
      navItem.focus();
    }
  });
};

In my process trying to boost its performance I made 2 changes, first of all, I animate using translate in the place of the top position, from what I understand changing any value other than transform causes a window repaint. The other change I made is to only animate elements that are inside my viewport. (top !== 0) === Only animate elements that are at least 1% inside the viewport.

I don't think that the code itself is very slow, however, the calcVisibilityForAllArticles function is run by:

document.onscroll = () => {
  calcVisibilityForAllArticles();
};

With means that the function is run back to back extremely fast. (With is needed for a smooth animation.)

Any help is more than welcome.

Faster does not mean smoother.

Some very quick notes.

Queries to the DOM are very slow compared too what they do. You should only query the DOM for the elements once and then just use the references you have.

When page is ready

// query the DOM a stash the elements in an array of objects so you can add
// And more DOM related info as you find it.
const articles = [...document.querySelectorAll('article')].map(element=> ({element}));

Then in the code

const calcVisibilityForAllArticles = () => {
  // Now you can loop through the element without needing to traverse the DOM each time.
  for(const item of articles){ // item holds the element as element
    const top = calcVisibilityForElem(item.element);

    // for the first pass
    if (top !== 0) {
       // for the first pass  of visible do the query and save the element
       if(!item.span) { item.span = item.querySelector('span') }
       // now you can use it directly
      item.span.style.transform = `translateY(${Math.floor(top * 1.5)}px)`;
    }
    // do the same for all the other elements you search the DOM for each animation cycle.

Timing

As you have not supplied a working example there is no way to know if you are overloading the page or not.

The important thing to remember is that all browsers have a fixed display rate of 60FPS. Making changes to the DOM at a rate higher than this will only result in shearing and flickering. It can also have a pull down effect on the browser as you block it from doing what it needs to do.

You must particularly be careful with events that are driven by the mouse. Some mice can trigger 500+ events a second. You should never update the DOM from move events that can come from the mouse.

RequestAnimationFrame

When ever you make any regular changes to the DOM use the timer callback via the function requestAnimationFrame. It is designed to present the smoothest possible animations. It is synced to the browsers frame rate, and will not present changes until the display device is not scanning out the pixels.

To use

var scrollPos;  // this holds the current scroll position

// In the scroll event only get the scroll position and make no changes to the
// DOM
function yourScrollEvent() {  ... scrollPos = foo ...}

You make the DOM updates via the frame request

 var lastScrollPos; // used to check if there is a need to animate

 function scrollLoop(){
      if(lastScrollPos !== scrollPos){ // only render if needed
          lastScrollPos = scrollPos;

          // do your DOM stuff here

       }

      requestAnimationFrame(scrollLoop); // request next frame

  }

   // start it off via the frame request.
   requestAnimationFrame(scrollLoop);

This will produce the smoothest possible animation. If it can not keep up and drops frames then you need to reduce the complexity of the animation.


My Queue implementation is correct? I have read CLRS Introduction to Algorithms book, now I'm on Elementary data structure chapter. I read about queue concept and try to implement in c.
I have tested a lot it work fine, but I have a little doubt on its working. Please tell me this implementation is correct or not.

#include <stdio.h>
#include <stdlib.h>
#define MAX 4
struct queue {
    int array[MAX];
    int head;
    int tail;
};
void ENQUEUE(struct queue* p,int data) {
// error checking for overflow
    if((p->tail == MAX-1 && p->head == 0 ) || (p->head == p->tail+1)) {
        printf("\nQUEUE IS FULL!!   ");
        return;
    }
    p->array[p->tail] = data;
    if(p->tail == MAX-1)
        p->tail = 0;
    else
        p->tail = p->tail+1;
    printf("\n%d data is added",data);
}

int DEQUEUE(struct queue* p) {
    if(p->head == p->tail) {
        printf("\nQUEUE IS EMPTY!!");
        return -1;
    }
    int x = p->array[p->head];
    if(p->head == MAX-1)
        p->head = 0;
    else
        p->head = p->head+1;
    printf("\n%d data is removed",x);
    return x;
}
void SHOWQUEUE(struct queue* p) {
    int i = 0;
    i = p->head;
    printf("p->head = %d & p->tail = %d",p->head,p->tail);
    printf("\nQUEUE ELEMENTS:");

    while(i != p->tail) {
        printf("%d ",p->array[i]);
        if(i == (MAX-1))
            i = 0;

        else
            i = i+1;
    }
}
int main() {
    struct queue* Q;
    Q = (struct queue*) malloc(sizeof (struct queue));
    if(Q == NULL) {
        printf("Memory allocation failed");
        return -1;
    }
    Q->head = 1;
    Q->tail = 1;
/*  ENQUEUE(Q,1);
    SHOWQUEUE(Q);
    DEQUEUE(Q);
    SHOWQUEUE(Q);
    ENQUEUE(Q,1);
    ENQUEUE(Q,1);
    SHOWQUEUE(Q);

    ENQUEUE(Q,1);
    ENQUEUE(Q,1);
    ENQUEUE(Q,1);
    SHOWQUEUE(Q);
    printf("head = %d & tail = %d ",Q->head,Q->tail);
*/
    ENQUEUE(Q,98);
    ENQUEUE(Q,20);
    ENQUEUE(Q,16);
    ENQUEUE(Q,2);
//  ENQUEUE(Q,1);
//  ENQUEUE(Q,2);
    SHOWQUEUE(Q);
    DEQUEUE(Q);
    DEQUEUE(Q);
    SHOWQUEUE(Q);
    ENQUEUE(Q,5);
    SHOWQUEUE(Q);
    ENQUEUE(Q,3);
    SHOWQUEUE(Q);
    ENQUEUE(Q,10);
    SHOWQUEUE(Q);
    DEQUEUE(Q);
    SHOWQUEUE(Q);
    ENQUEUE(Q,12);
    DEQUEUE(Q);
    SHOWQUEUE(Q);
    ENQUEUE(Q,2);
    DEQUEUE(Q);

    SHOWQUEUE(Q);
    ENQUEUE(Q,1);
    SHOWQUEUE(Q);
    DEQUEUE(Q);
    SHOWQUEUE(Q);
    DEQUEUE(Q);
    SHOWQUEUE(Q);
    DEQUEUE(Q);
    SHOWQUEUE(Q);
    DEQUEUE(Q);
    SHOWQUEUE(Q);
    return 0;
}

1 It would be more natural to have a field size instead of tail:

struct queue {
    int array[MAX];
    int head;
    int tail;
};

2 If you had size field, checking whether the queue is empty or
full would be a simple one-liner.

    if((p->tail == MAX-1 && p->head == 0 ) || (p->head == p->tail+1)) {
        printf("\nQUEUE IS FULL!!   ");
        return;
    }

3 It's not cool to print from algorithms unless debugging.

int DEQUEUE(struct queue* p) {
    if(p->head == p->tail) {
        printf("\nQUEUE IS EMPTY!!");
        return -1;
    }

4 Can make the output of this more pretty.

void SHOWQUEUE(struct queue* p) {
    ...
}

5 The convention is not to prepend malloc with a type.

Q = (struct queue*) malloc(sizeof (struct queue));

Instead, do this

Q = malloc(sizeof(struct queue));

6 Don't do this:

Q->head = 1;
Q->tail = 1;

Instead, you should write the function that initializes your queue.

7 Above, it's funny that you start indexing from 1.

Summa summarum

You could consider something like the following:

queue.h:

#ifndef QUEUE_H
#define QUEUE_H

#include <stdbool.h>
#include <stdlib.h>

#define CAPACITY 10

typedef struct queue {
    int storage[CAPACITY];
    size_t head;
    size_t size;
} queue;

void queue_init(queue* q);
bool queue_enqueue(queue* q, int element);
int  queue_dequeue(queue* q);
void queue_print(queue* q);

#endif

queue.c

#include "queue.h"
#include <stdlib.h>
#include <stdbool.h>
#include <stdio.h>

void queue_init(queue* q)
{
    if (!q)
    {
        return;
    }

    q->head = 0;
    q->size = 0;
}

bool queue_enqueue(queue* q, int element)
{
    if (!q || q->size == CAPACITY)
    {
        return false;
    }

    q->storage[(q->head + q->size++) % CAPACITY] = element;
    return true;
}

int queue_dequeue(queue* q)
{
    if (!q || q->size == 0)
    {
        return 0;
    }

    int ret = q->storage[q->head];
    q->head = (q->head + 1) % CAPACITY;
    q->size--;
    return ret;
}

void queue_print(queue* q)
{
    if (!q)
    {
        printf("null");
        return;
    }

    printf("[");

    for (size_t i = 0, index = q->head; i < q->size; ++i)
    {
        printf("%d", q->storage[index]);

        if (i < q->size - 1)
        {
            printf(", ");
        }

        index = (index + 1) % CAPACITY;
    }

    printf("]");
}

main.c:

#include "queue.h"
#include <stdio.h>

int main() {
    queue q;
    queue_init(&q);

    for (int i = 0; i < 5; ++i)
    {
        queue_enqueue(&q, i);
    }

    for (int i = 0; i < 10; ++i)
    {
        queue_print(&q);
        puts("");
        queue_enqueue(&q, queue_dequeue(&q));
    }

    return 0;
}

Hope that helps.


Replacing values in a file based on values in another file I am writing Python code which reads through two text files and prints the output based on those two files. Basically, the first file, demo.txt, has a matrix of numbers and the second file, days.txt, has a value. Based on that value, many digits in the first file are replaced with the $, ?, or just a space.

Anyways, my code is working perfectly, but I would like to simplify it in a way that it is easier to understand for very new programmers. So you can break it into many parts, change the list comprehensions on line 17. Also, can we break up the with statement on line 12 as well?

def repl(ch,threshold):
    if ch.isdigit():
        if int(ch) < threshold:
            return '$'
        elif int(ch)> threshold:
            return " "
        else:
            return ch
    else:
        return '?'
lines = []
with open('data') as f, open("data.txt","r") as f2:
    threshold = int(f2.read().split('=')[-1])
    for line in f:
        line = line.strip()
        line = line.split()
        line = [repl(ch,threshold) for ch in line]
        lines.append(line)
    # reset to start of demo.txt
    f.seek(0)
    for line in f:
        print(line)

for line in lines:
    print()
    for el in line:
        print(el,end=" ")

I’d go with:

def replace(word, threshold):
    """Convert any string ('word') that does not equal 'threshold'
    into: '?' if it is not an integer, '$' if it is lower than
    'threshold', or ' ' if it is greater than 'threshold'.
    """
    try:
        value = int(word)
    except ValueError:
        return '?'
    if value < threshold:
        return '$'
    if value > threshold:
        return ' '
    return word

def get_threshold(filename):
    """Read the first line of 'filename', split it around an '=' sign
    and convert the last part into an integer.
    """
    with open(filename) as f:
        return int(f.readline().split('=')[-1])

def process_file(data_file, threshold_file):
    """Replace data from 'data_file' according to the value found in
    'threshold_file'. Print the original data and then the converted
    one.
    """
    threshold = get_threshold(threshold_file)

    lines = []
    print('Original data:')
    with open(data_file) as f:
        for line in f:
            line = line.strip()
            print(line)

            # Join back the converted bits into a single line
            replaced_line = ' '.join(
                # Split the line at words boundaries and convert each
                # bit of text according to the threshold
                replace(chunck, threshold) for chunck in line.split())
            lines.append(replaced_line)

    print('\nData replaced with threshold', threshold)
    for line in lines:
        print(line)

# Does not execute anything unless run from the command line.
if __name__ == '__main__':
    # Call the main function of the program
    process_file('data', 'data.txt')

The key is to have simple functions that one can easily test in the interpreter and extensive documentation for trickier parts.

Docstrings should be enough to mostly describe what is going on; adapt to the intended audience. Using descriptive variable names can also improve the understanding.

For the processing of the data file, there are 3 possibilities:

 with open(data_file) as f:
     lines = [' '.join(replace(chunck, threshold) for chunck in line.strip().split())
         for line in f if not print(line)]

Pretty dense and it uses a test only for its side effect in order to avoid an f.seek(0) and an other iteration over the file. Not ideal. Definitely not the right choice for the purpose.
lines = []
with open(data_file) as f:
    for line in f:
        line = line.strip()
        print(line)

        lines.append(' '.join(replace(chunck, threshold) for chunck in line.split()))

Easier to read: an explicit loop reduce the complexity to decode the list-comprehension. It also allows to clean the lines before printing them and we still avoid an unnecessary f.seek(0). However, there is still a list-comprehension that might be hard to read (even with the comments) depending on your audience.
lines = []
with open(data_file) as f:
    for line in f:
        line = line.strip()
        print(line)

        converted = []
        for chunck in line.split():
            converted.append(replace(chunck, threshold))
        lines.append(' '.join(converted))

Each line is now easily understandable on its own.

I however prefer the second version since it is understandable and teaches good practices without being too aggressive. (the use of a generator expression is recommended here since we don't need the intermediate list.)

Talking about good practices:

using join is definitely better than the double for loop you use at the end of your code;
the if __name__ == '__main__': construct is recommended but can be confusing, remove it if you think it will be too much for your audience.


OOP MySQL connection class Under sage from another CR post I'm crafting an OOP MySQL connection/query class. Nothing new, but a place to start plying OOP PHP. Here is the current design,what would aid speed and portability?

conf/config.php

<?php

//Inlcude the Data Access Layer: Db
require_once (dirname(dirname(__FILE__)) . '/inc/class/dbc.php');

//DB Constants...
define('DB_HOST','dbhost');
define('DB_USER','user');
define('DB_PASS','pass');
define('DB_NAME','dbname');

?>

inc/class/dbc.php

<?php

/**
*
*/
class Db
{
    public $mysql;

    function __construct()
    {
        $this->mysql = new mysqli (DB_HOST, DB_USER, DB_PASS, DB_NAME) OR die ('Could not connect to MySQL: ' . mysqli_connect_error() );

//adding custom error checking:

    }// End Constructor

}//End Class Definition

index.php

<div id="todo">
<?php

                require_once(dirname(__FILE__) . '/conf/config.php');
                $db = new Db();
                $query = "SELECT * FROM todo ORDER BY id asc";
                $results = $db->mysql->query($query);

                if($results->num_rows) {
                    while($row = $results->fetch_object()) {
                        $title = $row->title;
                        $description = $row->description;
                        $id = $row->id;

    ?>

</div>

This code binds your Db object to the global constants.  Its going to be difficult to connect to more than 1 database.  Your DB class isn't doing anything for you.

The solution to this is: injection.  You should pass in the settings for the Db in the constructor.  Below is the PDO class from my framework which I think is serving a similar role to what you are trying to achieve with your Db class.  The injection is achieved by accepting parameters in the constructor ($setup).

/// PDO wrapper class to ensure DB implements the Evoke Core DB interface.
class PDO extends \PDO implements \Evoke\Core\Iface\DB
{
   public function __construct(Array $setup)
   {
      $setup += array(
         'DSN'      => NULL,
         'Options'  => array(\PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION),
         'Password' => NULL,
         'Username' => NULL);

      if (!isset($setup['DSN']))
      {
         throw new \InvalidArgumentException(__METHOD__ . ' requires DSN');
      }

      if (!isset($setup['Password']))
      {
         throw new \InvalidArgumentException(__METHOD__ . ' requires Password');
      }

      if (!isset($setup['Username']))
      {
         throw new \InvalidArgumentException(__METHOD__ . ' requires Username');
      }

      parent::__construct($setup['DSN'],
                          $setup['Username'],
                          $setup['Password'],
                          $setup['Options']);
   }
}

There are a number of differences with this approach:

The Db class is a Db type object (extends PDO).
Common database type functions are ensured (implements Iface\DB).
Setup for the database is injected (with the setup parameter).  This means that many databases could be connected to each with a different set of parameters injected.
Validity of the setup is enforced.

By creating an interface we can pass any type of object that implements the interface for use as our Db object.  This means our code is decoupled from the specific type of database object (I could replace PDO with mysqli very easily if I wanted to).

function index($db)
{
   if (!$db instanceof \Iface\DB)
   {
      throw new \InvalidArgumentException(__METHOD__ . ' we need a Db object.');
   }
}

Personally I have the small PDO wrapper you saw above and then a further abstraction for SQL statements.  With everything all of the dependencies are injected (normally in the constructor).  You can see more of my Db objects here.  Notice how the SQL class is injected with a Db (any sort of DB that implements the DB interface):

class SQL implements \Evoke\Core\Iface\DB
{
   protected $inTransaction = false;

   protected $setup;

   /// Class constructor
   public function __construct($setup=array())
   {
      $this->setup = array_merge(array('DB' => NULL), $setup);

      if (!$this->setup['DB'] instanceof \Evoke\Core\Iface\DB)
      {
         throw new \InvalidArgumentException(__METHOD__ . ' needs DB');
      }
   }


Checking for palindromes using dynamic memory allocation This is a homework assignment, but it's already done and it's not for a grade; it's meant as a refresher for the rest of the course. I've never done anything with dynamic memory allocation in C before, so I'm assuming I've done at least something wrong.

The goal is to write a program that takes a string from a user and checks to see if it's a palindrome. All non-letter characters are ignored, and dynamic memory allocation is required.

It works fine and I'm pretty happy with it, but I have no way of knowing if I've done things the "right" or "wrong" way. So, could anyone look over it really quickly and point out all the stupid things I've done?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(void)
{
    //str holds raw input
    //str_san holds sanitized input
    char *str = (char *)malloc(50), *str_san;
    //i and k are the index counters
    //len is the length of the sanatized string
    int i = 0, k = 0, len;

    //get a string from the user
    printf("Enter a message: \n");
    //store it in str
    scanf("%s", str);

    //make str_san as small as possible
    str_san = (char*)malloc(strlen(str) + 1);

    //copy only letters into the sanitized string
    while (str[i] != '\0') {
        if (str[i] >= 'A' && str[i] <= 'Z') {
            str_san[k] = str[i] + 32; //changes uppercase to lowercase
            k++;
        }

        if (str[i] >= 'a' && str[i] <= 'z') {
            str_san[k] = str[i];
            k++;
        }

        i++;
    }

    //str is no longer needed
    free(str);

    //terminate sanitized string
    str_san[k] = '\0';

    //set len to the length of str_san
    len = strlen(str_san);

    //check for palindrominess
    for (i = 0; i < len; i++) {
        //if the first letter does not equal the last letter then it's not a palindrome
        if (str_san[i] != str_san[len - i - 1]) {
            printf("Not a palindrome");
            return 0;
        }
    }

    //it is now safe to free str_san
    free(str_san);

    //if we've made it this far, it's a palindrome
    printf("Palindrome");

    return 0;
}

You should consider making this more modular (more functions).  It's not good to implement an entire program in main() as it could hurt readability.  While the separate comments are helpful, it's not a substitute for utilizing functions.

Essentially, you should just do the user-input and the function-calling in main().  In general, try to have main() do as less as possible.  For instance, the string-copying and palindrome check should be done in separate functions, and main() should pass the strings to them at the calls.

void copyIntoString(char* str_san) {
    // ...
}

void checkForPalindromes(char* str_san) {
    // ...
}

The deallocation should still be done in main() since the allocation was done in there.  Also, the last output can be put into checkForPalindromes() since it's part of that functionality.

I also have some additional notes:

If you need a separate variable like len, consider giving it a more accurate name so that it's known which length is being stored.  You have two strings, so the reader could easily forget which string to which it belongs.  Adding additional strings would then make it worse.
Instead of using printf() for unformatted outputs:

printf("Enter a message: \n");

use puts():

puts("Enter a message: ");

It also gives you a newline, so you can leave out yours (unless you want two).
It's clearer to have each declaration or initialization done on a separate line, so instead of this:

int i = 0, k = 0, len;

you should have this:

int i = 0;
int k = 0;
int len;

However, as the first two are loop counter variables, they should be placed right in front of the respective for loops (or in the for loops if you have C99 or above).  This will help with keeping close scope, which is good for readability and maintenance.
This doesn't need to be a while loop:

while (str[i] != '\0') {
    if (str[i] >= 'A' && str[i] <= 'Z') {
        str_san[k] = str[i] + 32; //changes uppercase to lowercase
        k++;
    }

    if (str[i] >= 'a' && str[i] <= 'z') {
        str_san[k] = str[i];
        k++;
    }

    i++;
}

Since i is being incremented each time, it can just be a for loop:

for (int i = 0; str[i] != '\0'; i++) {
    if (str[i] >= 'A' && str[i] <= 'Z') {
        str_san[k] = str[i] + 32; //changes uppercase to lowercase
        k++;
    }

    if (str[i] >= 'a' && str[i] <= 'z') {
        str_san[k] = str[i];
        k++;
    }
}

In addition, you can use isupper() instead of your own check:

if (isupper(str[i])) {

You can also use tolower() to change a character to lowercase:

str_san[k] = tolower(str[i]);

The second if can then use islower():

if (islower(str[i])) {

Lastly, those two if can be consolidated into one.  This can be done by calling tolower() whenever any letter is encountered (the character will remain intact if it cannot be made lowercase).  Since all these operations only apply to letters, each character can first be checked with isalpha().

All of these function calls may add slight overhead, but if you don't care too much about efficiency or will mostly work with smaller strings, then it shouldn't matter.

Taking each point into consideration, you should end up with this:

for (int i = 0; str[i] != '\0'; i++) {
    if (isalpha(str[i])) {
        str_san[k] = tolower(str[i]);
        k++;
    }
}


Tidying up jquery code for linking inputs I have three input boxes that are all linked to each other by some pretty basic rules:

Editing the page title will copy the contents to menu title

  Unless menu title has been manually edited
  clearing menu title sets it back to automatic

Editing the menu title will copy the contents as a url slug to slug

  Unless the slug has been manually edited
  clearing the slug sets it back to automatic
  Editing page title counts as editing menu title if it is set to automatic

The Page Title, and the other two boxes are never visible at the same time!

Here is a demo: http://jsfiddle.net/6VPpj/1/

and here is my rather horrible code, how can I clean this up?

$(function(){
var automatic = true;
var pager = true;
var cache = {};

var slug = $('#slug');
var menu = $('#menu_title');
var page = $('#page_title');

page.on('change', function() {
    if (pager) {
        menu.val(page.val());
        menu.keyup();
    }
});

menu.on('keypress keyup keydown change', function() {

    if (automatic) {
        var n = menu.val();
        if (n in cache) {
            slug.val(cache[n]);
            return;
        }

        cache[n] = toSlug(n);
        slug.val(cache[n]);

        return;
    }
});
menu.on('change', function() {
    if (menu.val() == '') {
        pager = true;
        page.change();
    }
    else {
        pager = false;
    }
});

slug.on('change', function() {
    if (slug.val() == '') {
        automatic = true;
        menu.keyup();
    }
    else {
        automatic = false;
        var n = slug.val();
        if (n in cache) {
            slug.val(cache[n]);
            return;
        }
        cache[n] = toSlug(n);
        slug.val(cache[n]);
        return;
    }
});

function toSlug(Text) {
    return Text.toLowerCase().replace(/ /g, '-').replace(/[^\w-]+/g, '').replace(/--+/g, '-');
}
});

It's not so horrible. You managed to cache your jQuery selectors, which is more than a lot of people can say. Good job there.

automatic and pager are not super great names in this context. I suggest renaming them to autoSlug/autoMenu or manualSlug/manualMenu.

You are caching toSlug, but really, it's not complex enough to warrant caching. I did some quick and dirty testing and found that you save about 10ms per 1,000 executions. That is not worth anything here. So, to reduce code size and bug potential, I suggest removing all caching.

Variables and parameters should not be capitalized unless they represent a class. As such, the Text parameter in toSlug should be text.

Finally, and this is more of a personal preference, but I find that code is easier to read and follow when reading functions and not event handlers. By that I mean this: instead of calling menu.change(), why not move that code to a function and call updateMenu()?

Here is the final result:

$(function(){
var autoSlug = true;
var autoMenu = true;

var slug = $('#slug');
var menu = $('#menu_title');
var page = $('#page_title');

page.on('change', function() {
    if (autoMenu) {
        updateMenu()
    }
});

menu.on('change', function() {
    autoMenu = (menu.val() == '');
    updateMenu()
});

slug.on('change', function() {
    autoSlug = (slug.val() == '');
    updateSlug();
});

function updateMenu(){
    if(autoMenu){
        menu.val(page.val())
    }
    if(autoSlug){
        updateSlug()
    }
}

function updateSlug(){
    var text = autoSlug ? menu.val() : slug.val();
    slug.val(toSlug(text));
}

function toSlug(text) {
    return text.toLowerCase().replace(/ /g, '-').replace(/[^\w-]+/g, '').replace(/--+/g, '-');
}
});


Python: Parse string of bracketed list items into list of strings I have a string containing several items listed in the following notation:
myString = '[A][B][C]'

And I would like to parse that to a python list of several strings:
['A', 'B', 'C']

I know this can be solved with:
myString = myString.lstrip('[')
myString = myString.rstrip(']')
myList = myString.split('][')

I'd just like to know if there is an even more pythonic way of doing it.
Compare https://stackoverflow.com/a/1653143/10983441 where the most elegant way in the end was to use pyparsing with nestedExpr.

A Regex based solution
>>> my_string_one
'[A][B][C]'
>>> re.split(r"\[([A-Z])\]", my_string_one)[1:-1:2]
['A', 'B', 'C']
>>> my_string_two
'[A][B][C][D][E]'
>>> re.split(r"\[([A-Z])\]", my_string_two)[1:-1:2]
['A', 'B', 'C', 'D', 'E']

You can use re.split with the expression \[([A-Z])\ having a capture group for the uppercase letters. This is under the assumption that your strings always follow this pattern otherwise you may not get what you expect.


Producer consumer in C++11 I'm trying to practice some C++ and I wrote a simple queue to solve the Producer-Consumer Problem.

class Queue
{

public:
Queue() : mutex(), cv(), queue(), process(true), consumer(&Queue::dequeue, this)
{
}

~Queue()
{
    process = false;
    consumer.join();
}

void enqueue(int i)
{
    std::lock_guard<std::mutex> lock(mutex);
    queue.push(i);
    cv.notify_all();
}

void dequeue()
{
    while(process)
    {
        std::unique_lock<std::mutex> lock(mutex);
        while(queue.empty())
        {
            cv.wait(lock);
        }

        if(!process)
        {
            break;
        }

        int i = queue.front();
        queue.pop();
        std::cout << "Got " << i << '\n';
    }
}

private:
std::mutex mutex;
std::condition_variable cv;
std::queue<int> queue;
bool process;
std::thread consumer;

};

Is my implementation good? Is there anything I could've done better?

Dead Lock Bug

If the queue is empty and the consumer thread is waiting in the condition variable. If you Queue goes out of scope the main thread will enter the detructor and perform join(). At this point you are locked. As both threads are waiting for the other.

Use of condition can be simplified.

    while(queue.empty())
    {
        cv.wait(lock);
    }

Can be replaced with:

    cv.wait(lock, [&queue](){return !queue.empty();});

When poping things from a queue use move.

// this performs a copy.
int i = queue.front();

// this should move the value from the que.
int i = std::move(queue.front());

Note: It makes little difference for an integer but if there is an object in the queue then it might.

Formatting

Your code is horribly formatted. Please do a better job in the future. Formatting is a critical part of coding. This is because your code will be read fro decades aftr you have written it by other humans.


Deleting large folders on remote machines I am trying to delete a large folders (with many levels of subfolders) over a network (i.e, on a remote machine).

I have the following code and it takes 10 minutes. Is there a way to improve this drastically?

public static boolean deleteDirectory(File directory) {

        File[] files = directory.listFiles(); // Value is null if the file is not an directory or if the file doesn't exist
        if(null!=files && files.length>0){
            //Gets in here only if the file is directory
            long filesCount = files.length;
            for(int i=0; i<filesCount; i++) {
                deleteDirectory(files[i]);
            }
        }
        return(directory.delete());
    }

I decide to write my own function(using native process) to delete the files(tested on windows), no external dependencies on jars.

Implementation details for windows:

I am using robocopy because rmdir fails for complex or deeply nested file paths.
The logic to delete files on windows works like Collections.retainsAll().

import java.io.File;

import java.io.IOException;

public class RecursiveDelete {

    public static void main(String[] args) throws IOException {
        String dirPath = "C:" + File.separator + "dev_tools" + File.separator + "dir_to_delete";
        System.out.println("Delete folder path=" + dirPath);
        execute(dirPath);
    }

    public static void execute(String folderToDelete) throws IOException {
        String os = System.getProperty("os.name").toLowerCase();
        File emptyFolder = createEmptyDirectory();
        if (isWindows(os)) {
            Runtime.getRuntime().exec("cmd /c robocopy " + emptyFolder + " " + folderToDelete + " /purge & rmdir " + folderToDelete);
        } else if (isUnix(os) || isMac(os) || isSolaris(os)) {
            Runtime.getRuntime().exec("rm -r -f  " + folderToDelete);
        }
    }

    private static File createEmptyDirectory() {
        File emptyFolder = new File("EmptyFolder");
        if (!emptyFolder.exists()) {
            boolean isCreated = emptyFolder.mkdir();
            System.out.println("Is empty folder created ?" + isCreated);
            System.out.println("EmptyFolder path" + emptyFolder.getAbsolutePath());
        }

        return emptyFolder;
    }

    public static boolean isWindows(String os) {
        return (os.contains("win"));
    }

    public static boolean isUnix(String os) {
        return (os.contains("nix") || os.contains("aix") || os.contains("nux"));
    }

    public static boolean isSolaris(String os) {
        return (os.contains("sunos"));
    }

    public static boolean isMac(String os) {
        return (os.contains("mac"));
    }
}


Return the last digit of a number public static int lastDigit(int a) {
    a = a % 10;
    if (a <= 0){
    a *= -1;
    }
    return a;
}

This is my solution. The course hasn't taught if else yet, so I'm wondering if there is a way to do it without the if else.

public static int lastDigit(int a) {
a = a % 10;
return a;
}

The problem with this is that it will not print the last digit of a negative number as a positive.

Use Math.abs()

 public static int lastDigit(int x) {
      return Math.abs(x % 10);
 }


SQL Group and Filter - Refining down a search including Dates The below process is designed to pick out only AsbestosUPRNs and get the lowest OverallRiskCategory where they have the newest SurveyDate. I just wanted to ensure this is the best way, it seems neat now though as well as second code, which in reality is the same thing, based on two tables.

SELECT
AsbestosUPRN,
MIN(OverallRiskCategory) AS OverallRiskCategory,
MAX(SurveyDate) AS SurveyDate
FROM TblAsbestos
GROUP BY
AsbestosUPRN

Two Table version:

SELECT
p.UCARN,
MAX(a.OverallRiskCatNumberical) as OverallRiskCatNumberical,
MAX(a.SurveyDate) AS SurveyDate
FROM TblAsbestos AS a
INNER JOIN TblProperty AS p
ON p.UPRN = a.AsbestosUPRN
GROUP BY
p.UCARN

Overall, that looks good. You've followed basic SQL styling rules, capitalising keywords for example, and the both queries are consistent. Anyone looking at that is going to struggle to find things to complain about.

A couple little things, the first AS in your second query isn't capitalised. I would change it to AS to make the query more consistent.

I'd say that the table aliases in the second query should be more meaningful than just a and p. Other than that, good job.

I would recommend using the first query, it avoids the join and, presumably, returns the same correct results.


My first (simple) template that does more than storing a value of type T I just decided to get a bit more acustomed to using templates. Maybe this is a stupid question, but in my experience in C++ there is always some pitfall lurking aroung the corner. Thus I prefer the danger of embarresment instead of building up on bad habbits. Mainly as an exercise I wrote this:

#include <functional>
#include <string>
#include <sstream>

template <typename V,template <class> class R = std::greater,typename I=void>
struct Extremum {
    typedef V    value_type;
    typedef I    index_type;
    typedef R<V> relation_type;
    V value;
    I index;
    R<V> r;
    bool operator() (V v,I i){
        bool result = r(v,value);
        if (result){
            value = v;
            index = i;
        }
        return result;
    }
    Extremum(V v,I i) : value(v),index(i) {}
    std::string toString(){
        std::stringstream s;
        s << "value= " << value << " index= " << index;
        return s.str();
    }
};

template <typename V,template <class> class R>
struct Extremum<V,R,void> {
    typedef V    value_type;
    typedef void index_type;
    typedef R<V> realtion_type;
    V value;
    R<V> r;
    bool operator() (V v){
        bool result = r(v,value);
        if (result){ value = v; }
        return result;
    }
    Extremum(V v) : value(v) {}
    std::string toString(){
        std::stringstream s;
        s << "value= " << value;
        return s.str();
    }
};

which would be used like this:

Extremum<int,std::greater,int > e(0,0);
e(10,3);
e(123,5);
std::cout << e.toString() << std::endl;

Extremum<int> g(0);
g(123);
g(1234);
g(1);
std::cout << g.toString() << std::endl;

I used a ´toString()´ because it is for debugging and printing to the screen while I might want to overload in/output operators for real in/output.

I dont like so much the ordering of the template parameters, as both the second and the third parameter would make a good fit as the last parameter so it can be ommited on the instantiation. However, I guess there is no better solution than simply choosing one ordering.

I have no idea how to provide good initial values. My idea was to use something like std::numeric_limits<V>::max, however, it depends too much on the choosen relation whether it should be max, min or something completely different. Maybe the easiest is  let the user choose them when calling the constructor.

Aside from its functionaliy, which probably could be achieved with less code and a simple lambda or the like, is there anything inherently broken with this code? Anything obvious that could be improved?

Note that this is pre-C++11, but I would also be interested in any post-C++11 stuff.

Apart from cosmetic details, I see two things that could be improved IMHO.

The template comparator R<V>

As a template argument, you are asking for template <class> class R
which is then used so R<V> r;

This narrows the possibilities since you can't pass a non-template comparator.
For instance, if you have a custom type MyType and a custom comparator for that type MyTypeComp then you won't be able to use it with your Extremum struct because MyTypeComp doesn't accept template arguments.

The fix is easy:

template <
          typename V,
          class R,
          typename I = void
         >
struct Extremum {
    typedef V    value_type;
    typedef I    index_type;
    typedef R relation_type;
    V value;
    I index;
    R r;
    //...

And you would use it so:

Extremum<int, std::greater<int>, int> e(0,0);
Extremum<MyType, MyTypeComp, int> e(MyZero,0);

The index

You write almost the same code twice: once with index, once without.
You could avoid it by passing the index in a pair.
So you have only one class Extremum<V, R> and you set V = std::pair<Value, Index> and an appropriate comparator that compares two pairs according to their Value entry only.

template <class V, class R>
struct Extremum {
    typedef V    value_type;
    typedef R    relation_type;
    V value;
    R r;
    bool operator() (V v){
        bool result = r(v,value);
        if (result) {
            value = v; //you can use std::move here
        }
        return result;
    }
    Extremum(V v, R r) : value{v}, r{r} {} //you can use std::move here too
    std::string toString() const {
        std::stringstream s;
        s << "value= " << value;
        return s.str();
    }
};

template <class RV>
struct CompByFirstEntry {
    RV delegate;
    CompByFirstEntry(RV delegate);

    template <class V, class I>
    bool operator() (std::pair<V,I> const& lhs, std::pair<V,I> const& rhs) const {
        return delegate(lhs.first, rhs.first);
    }

    //or, even:

    template <class Pair>
    bool operator() (Pair const& lhs, Pair const& rhs) const {
        return delegate(lhs.first, rhs.first);
    }
}

EDIT : Here is a working example.

What changed: define the constructor CompByFirstEntry::CompByFirstEntry and operator<< for std::pair; choose one version of the operator().
Note: you can also create a class ExtremumIndexed<V, I, R> that extends Extremum<std::pair<V,I>, MyPairComp<R>> or simply a template using directive.

std::ostream& operator<<(std::ostream& out, std::pair<int, int> const& p)
{
    out << p.first << " " << p.second;
    return out;
}

template <class V, class R>
struct Extremum {
    typedef V    value_type;
    typedef R    relation_type;
    V value;
    R r;
    bool operator() (V v){
        bool result = r(v,value);
        if (result) {
            value = v; //you can use std::move here
        }
        return result;
    }
    Extremum(V v, R r) : value{v}, r{r} {} //you can use std::move here too
    std::string toString() const {
        std::stringstream s;
        s << "value = " << value;
        return s.str();
    }
};

template <class RV>
struct CompByFirstEntry {
    RV delegate;
    CompByFirstEntry(RV delegate) : delegate{delegate} {}

    template <class V, class I>
    bool operator() (std::pair<V,I> const& lhs, std::pair<V,I> const& rhs) const {
        return delegate(lhs.first, rhs.first);
    }
};

int main(int argc, const char * argv[]) {
    using MyPairComp = CompByFirstEntry<std::greater<int>>;
    MyPairComp comp(std::greater<int>{});
    int value = 1;
    int index = 0;

    Extremum<std::pair<int,int>, MyPairComp> e({value, index}, comp);
    std::cout << e.toString() << std::endl;

    e({value + 1, index - 1});
    std::cout << e.toString() << std::endl;

    return 0;
}


Doubly Linked List using Stack in c++ Just started learning/relearning data structures for the semester exams. Most of the examples found online are too cluttered or bloated.

For myself, I implemented it from theory I learned.

It works.

I want to know if there are any mistake that I have done, anything that can be improved .. .

Thanks for your time :)

#include "iostream"
using namespace std;

struct Node{

    struct Node* prev;
    int data;
    struct Node* next;

};

Node* top = NULL;

bool isempty(){
    return  top->prev == NULL;
}

void push(int data){

    struct Node* newnode;
    newnode = new Node();

    if(!newnode){
        cout << "Heap Overflow" << endl;
    }

    newnode->prev = top;
    newnode->data = data;
    newnode->next = NULL;

    top = newnode;
}

void pop(){

    if(isempty()){
        cout << "Stack Underflow";
        exit(1);
    }

    struct Node* tmp;
    tmp = new Node();

    tmp->next = NULL;
    tmp->data = top->prev->data;
    tmp->prev = top->prev->prev;

    free(top);
    top = tmp;

}

void display(){

    struct Node* itr;
    itr = top;

    cout << itr->data << " ";
    while(itr->prev != NULL){
        cout << itr->prev->data << " ";
        itr = itr->prev;
    }
    cout << endl;
}

int main(){

    push(10);
    push(20);
    push(30);
    push(40);
    push(50);

    pop();
    display();
    pop();
    display();
    pop();
    display();
    pop();
    display();
    pop();

    return 0;
}

Welcome to Code Review!

Bugs

For every node you insert, next is null.

Encapsulation

Your code is written almost exclusively using C features.  First things first, instead of using a single global variable, use a class to encapsulate the data structure.

class Stack {
public:
    Stack() = default;
    Stack(const Stack&) = delete;
    Stack& operator=(const Stack&) = delete;
    ~Stack();

    void push(int data);
    void pop();
    void display() const;
    bool empty() const { return top == nullptr; }
private:
    struct Node {
        int data;
        Node* prev;
        Node* next;
    };
    Node* top = nullptr;
};

The way the function works is exactly the same.  A destructor should be provided to systematically handle memory deallocation:

Stack::~Stack()
{
    while (!empty()) {
        pop();
    }
}

Now Stack can be used like a normal C++ container:

int main()
{
    Stack stack1;
    stack1.push(3);
    stack1.push(1);
    stack1.push(4);
    stack1.display();
    stack1.pop();
    stack1.display();

    Stack stack2;
    // stack2 is a stack independent from stack1
} // the existing nodes are automatically freed when the main function returns

Miscellaneous

#include "..." is for your own headers.  For standard headers, use #include <iostream>.

using namespace std; at global level is considered bad practice and should be avoided because it defeats the purpose of namespaces and introduce name clashes.  You will have trouble using identifiers as common as size or count.  See Why is using namespace std; considered bad practice?

A class can be referred to directly with its name.  You don't need to add struct.  So instead of struct Node*, just say Node*.

Instead of NULL, use nullptr for null pointer constants.

It's more common to write functions in this way:

bool isempty()
{
    // ...
}

Instead of declaring a variable uninitialized and then assigning to it immediately, use initialization.

The push and pop functions are way too convoluted.  Remember you are implementing a stack, not a complete doubly linked list.  Aggregate initialization with new can be used to simplify the code:

void Stack::push(int value)
{
    top = new Node{value, top, nullptr};
    if (auto old = top->prev) {
        old->next = top;
    }
}

void Stack::pop()
{
    if (empty()) {
        throw Stack_underflow{};
    }
    delete std::exchange(top, top->prev);
}

Note that heap overflow is not something you need to worry about because new already throws an appropriate exception on allocation failure.  Also, do not free memory allocated by new with free.  This is undefined behavior.  Use new with delete instead.

When an error happens, throw an exception instead of printing a message and then exiting or even continuing execution.  There is not always an open stdout file, and if it exists, that's now how everyone wants error to be handled.  Throwing an exception allows the user to handle it in the convenient way.

The while loop in the display function should have been simplified with a for loop:

for (Node* itr = top; itr; itr = itr->prev) {
    std::cout << itr->data << '\n';
}

Note that std::endl should be avoided when you don't need the flushing semantics.  Unnecessary flushing can cause performance degradation.  See std::endl vs \n.

return 0; can be omitted in a main function.

Refined version

Putting everything together:

#include <iostream>
#include <stdexcept>
#include <utility>

struct Stack_underflow :std::exception {};

class Stack {
public:
    Stack() = default;
    Stack(const Stack&) = delete;
    Stack& operator=(const Stack&) = delete;
    ~Stack();

    void push(int value);
    void pop();
    void display() const;
    bool empty() const { return top == nullptr; }
private:
    struct Node {
        int data;
        Node* prev;
        Node* next;
    };
    Node* top = nullptr;
};

Stack::~Stack()
{
    while (!empty()) {
        pop();
    }
}

void Stack::push(int value)
{
    top = new Node{value, top, nullptr};
    if (auto old = top->prev) {
        old->next = top;
    }
}

void Stack::pop()
{
    if (empty()) {
        throw Stack_underflow{};
    }
    delete std::exchange(top, top->prev);
}

void Stack::display() const
{
    for (auto it = top; it; it = it->prev) {
        std::cout << it->data << ' ';
    }
    std::cout << '\n';
}

int main()
{
    Stack stack;
    stack.push(1);
    stack.push(2);
    stack.push(3);
    stack.push(4);
    stack.push(5);

    stack.pop();
    stack.display();
    stack.pop();
    stack.display();
}

Output:

4 3 2 1
3 2 1

(live demo)


Guess number with lower or higher hints Problem statement

  Two players - Alice and Bob. Alice needs to guess a number \$n\$, from
  range \$[1, N]\$, \$N \le 200\$

  In \$i\$th turn, Alice guesses a number \$i\$
  Bob chooses to tell the relation between \$i\$ and \$n\$ (\$n \lt i\$, \$n = i\$ or \$n \gt i\$). Bob could tell what he wants, unless
  it conflicts with what he says before
  Each turn costs \$i\$
  Alice wants to minimize the total costs
  Bob wants to maximum the total costs

  What is the minimal cost if both players take the best strategy?

Any performance improvement ideas in terms of algorithm time complexity, code bugs or code style advice is appreciated.

More specific question: I do not know how Bob could make sure there is no conflict to what he said in the past -- if he tells number wrong at one time. For example, if \$n\$ is 20, Alice guess \$i\$ = 50, if Bob tells alice \$i\$ is smaller (Bob lies), then Alice will always guess a number > 50, how could Alice achieve the final goal? I think Bob could lie, but finally he should help Alice achieve the final goal, even if in the middle Alice may take more steps (more costs) -- because of some conclusion Bob tells in the middle is not correct.

If anyone have any ideas about how Bob could lie (which will maximum cost for Bob as one goal), while at the same time he still make sure no conflict in the future (and finally guide Alice to the right number), it will be great.

The major ideas of my code is, I try to use Dynamic programming approach to build dp structure from bottom up, dp[(i,j)] means for numbers start from i and ends with j, what are minimal cost for Alice. Both i and j are inclusive.

import sys
from collections import defaultdict
class Solution(object):
    def getMoneyAmount(self, n):
        """
        :type n: int
        :rtype: int
        """
        dp = defaultdict(lambda : sys.maxint) # key: tuple (start, end), value: min cost
        for end in range(1, n+1):
            for start in range(end, 0, -1):
                if start == end:
                    dp[(start, end)] = 0
                elif start + 1 == end:
                    dp[(start, end)] = start
                else:
                    for d in range(end-1, start, -1):
                        dp[(start, end)] = min(dp[(start, end)], d + max(dp[(d+1, end)], dp[(start, d-1)]))

        return dp[(1, n)]
if __name__ == "__main__":
    s = Solution()
    print s.getMoneyAmount(10)

If I were Alice, the most optimum strategy, well, there are two. The binary search, or in Python is the bisection method, or I would use the Fibonacci method, which is mathematically superior and faster than the binary search method.

Anyway, that would be what Alice would have to do to get to the number in the least number of guesses.

Bob, on the other hand, should make the number a floating point, say with four of five decimal places. Your original question did not state either integer or floating point.

Trying to guess a floating point number is harder. Try to figure out what the \$\sqrt{32}\$ is. It is not an even square, so in math, without a program or calculator, you really need to use the brute force method. However, unlike guessing a number between 1 to 200, here you can get a good starting point.

You know the number has to be between the \$\sqrt{25} = 5\$ and \$\sqrt{36} = 6\$. Since 32 is close to the midpoint, you can start at 5.5 (\$5.5^2 = 30.25\$), which still makes it difficult, as you will need to go to at least 4 decimal places for the right answer (\$\sqrt{32} = 5.6569\$).

Bob's strategy will be to make sure the number has some decimal places in it.

This way, Alice may use the binary or Fibonacci method to get to the integer, but will have to start over for the decimal places. She can get to it quick enough, but not as quickly as with just an integer.

Now Alice, if she knows the number can be a floating point, may want to start her guesses with a .5 or .25 at the end of every number. This will shorten the guesses for when she gets \$5.25 \gt i\$, but \$5.5 \lt i\$.

You can find the Fibonacci method and algorithm here. I could not find a corresponding Python function, but it is similar to the binary search, except you use just the Fibonacci numbers to narrow down your search.

What I was going to say about Bob picking a fractional number (floating point) instead of just an integer, he doesn't have to lie when Alice guesses 5 and Bob says GT and when she says 6 he says LT. Now it is up to Alice to realize she's dealing in decimals.

Bob maximizes his chances by using decimals. However, if Alice suspects this, she can always start off with a .5 or .25 at the end of every number. She can still minimize her guesses this way, as she needs to get to the integer first. With adding a decimal to it, she has a head start at guessing the decimal number.

Edited on 2/8/2017:
I though more about your question and about looking at how, if I were Bob, I could optimize my chances to maximize my costs without violating bullet point #2.

Besides using decimals, or even if just using integers, Bob could give more details that would not only be true, it could be misleading Alice as well.

Let's say Bob picks 30 as his number, and Alice starts her guess with 100, the midpoint in a Binary search pattern.

If Bob believes that she is using this, instead of saying her guess was greater than the number, he could say it is greater than by at least 10! This is not a lie, but if Alice decides to stick to the Binary search, it Change her next mid-point guess.

Instead of guessing 50, which she would if he had just said her guess was greater than, she knows his number is between 1 and 90. Instead, she guesses 45.

You may think that oh, no, she's actually closer, but it now all depends on what Bob now says. He has a choice, but what ever he says next will in no way be in conflict nor even contradict what he has said before. And can increase the number of guesses she makes, which is his goal.

If I were Bob, I would want Alice to make her next guess far enough away from his number that her third choice will be what optimizes his chances.

Bob can say, Guess is greater than. He's not obligated to say anything more and in no way is in conflict with what he as said so far.

Now is where Alice is on the hook. She has to guess a number between 1 and 44.

If she just got the GT or LT answers, her guesses would be 100, 50, 25.

Now she, if she sticks with the binary method, has to go with 22, and that is 3 away from her normal guess of 25.

Now, if you can see how this plays out, Bob can say her guess is less than, and see if he gets her to make more guesses if he adds to what he says.

This is a simplistic approach, but with some more thought, Bob could probably anticipate this, find the right number that would be far enough away to cause Alice to make one or more guesses than she would normally have. Becaus there will come a time where she will guess in increments of one, and Bob needs her to have extra guesses due to his word manipulation.

Without working on a formula and computer program, which I can do, I could show how this would play out and find that one or more magic numbers, probably a prime number (just a guess right now but sounds logical) that will increase Alice's cost and optimizes Bob's costs.

Let me know if this does it for you, or if anyone else has improvements on or another way Bob can perform his optimization.

Let me know if you want me to work on a program.

Edited on 02/17/2017: My original formula in the comments was just a starting point. It failed when a second or more guess was given.

After writing a few formulas into a computer program and executing them, along with manual calculaton, the following formula shows that no matter what answer Bob gives, there is always a relationship given to Alice between his chosen number and her guess.

And while Alice may not be able to use this formula to get the answer quickly, according to the rules set in the question, there is no misunderstanding of the rules, which states that Bob can give any answer as long as it does not conflict with any others given prior.

I believe that I demonstrated that in word/pseudo form that by giving Alice not only a GT/LT/EQ answer, he can add a hint that minimizes his payout, and still gives Alice a truthful answer.

In addition, if Bob qualifies his answer by saying "your guess is at least 30 greater than," if Alice was to guess a number that was 35 less than her last one, and it is still greater than Bob's number, Bob would not be in conflict if he said Alice's second guess was still greater than. The reason is he said "at least." That could mean his number is 100 less than Alice's guess. There is no conflict there, as Alice should be familiar with Number Theory and even Mathematical Logic.

Regardless of Bob's number of Alice's guess, I have come up with this formula that will always show that Bob always gives the relationship to his number based on Alice's guess.

It is expressed as (Total Numbers - Bob's Number - Bob's misdirection) is equal to (Total Numbers - Alice's Guess - Bob's misdirection)±X

In this simple equation, Alice's Guess ±X will always be the inverse of Bob's number, and when added to Alice's guess will equal Bob's number.

Formula: (T-N-B)=(T-G-B)±X

Let's run some examples: T=200, N=44, B=0. G=150.

(200-44-0)=(200-150-0)±X
156=50±X
X=106
G-X=N
150-106=44

Of course, we can simplify the formula using algebra as: N=G±X

N=47, G=50
47=50±X
X=-3

Done with the full formula:

(200-47-0)=(200-50-0)±X
47=50±X
X=-3
47=50-3
47-47

Regardless of the number picked and the guess, a relationship with each number is always established.

One last where Bob says that Alice's guess is greater than by 25. Bob guesses 12. Alice guesses 60.

(200-12-25)=(200-60-25)±X
163=115±X
163=115+48
163=163

Now, to get to Bob's answer, since he said Alice's guess is greater than, we need to SUBTRACT X from G.

G-X=N
60-48=12

Any answer Bob gives, whether or not he adds an addition, which by the rules set in this question is perfectly legal, always shows the relationship between the number selected and the guess made.


Credit card validator using Luhn's algorithm I'm writing an algorithm to read from a file a list of numbers, and for each, determine if it is valid. If it is, then display which card type it is.

public class CreditCardValidator {

    /**
     * @param args the command line arguments
     * @throws java.io.FileNotFoundException
     */
    public static void main(String[] args) throws FileNotFoundException {
        Scanner scan = new Scanner(new File(args[0])); // read from a file
        while (scan.hasNextLine()) {
            String str = scan.nextLine();
            if (validate(str)) {
                System.out.println(creditCardType(str));
            } else {
                System.out.println("Invalid");
            }
        }
    }

    private static boolean validate(String str) {
        String reverse = new StringBuilder().append(str).reverse().toString();
        int[] array = new int[str.length()];
        for (int i = 0; i < array.length; i++) {
            array[i] = Integer.parseInt("" + reverse.charAt(i));
        }
        int sum = 0;
        for (int i = 0; i < array.length; i++) {
            if (i % 2 == 1) {
                array[i] *= 2;
                if (array[i] > 9) {
                    array[i] -= 9;
                }
            }
            sum += array[i];
        }
        return sum % 10 == 0;
    }

    private static String creditCardType(String str) {
        int input1Char = Integer.parseInt(str.substring(0, 1));
        int input2Chars = Integer.parseInt(str.substring(0, 2));
        int input3Chars = Integer.parseInt(str.substring(0, 3));
        int input4Chars = Integer.parseInt(str.substring(0, 4));
        int input6Chars = Integer.parseInt(str.substring(0, 6));

        if (input2Chars == 34 || input2Chars == 37) {
            if (str.length() == 15) {
                return "American Express";
            }
        }

        if (input4Chars == 6011
                || (input6Chars >= 622126 && input6Chars <= 622925)
                || (input3Chars >= 644 && input3Chars <= 649)
                || input2Chars == 65) {
            if (str.length() == 16) {
                return "Discover";
            }
        }

        if (input2Chars >= 51 && input2Chars <= 55) {
            if (str.length() >= 16 && str.length() <= 19) {
                return "MasterCard";
            }
        }

        if (input1Char == 4) {
            if (str.length() >= 13 && str.length() <= 16) {
                return "Visa";
            }
        }
        return "Invalid Credit Card Type";
    }
}

Comments

/**
 * @param args the command line arguments
 * @throws java.io.FileNotFoundException
 */

You're stating the obvious and forgetting the important things. Yes, args is the command line arguments, but that's standard for the Java main method. I'd suggest documenting which command line arguments you actually accept - namely only one, and it should be a file path!

Additionally, some of the code in the methods below looks like it could use some commenting as well. It looks complicated. Maybe I'll understand it if I look at it in greater detail...

Throw exceptions when it matters

    for (int i = 0; i < array.length; i++) {
        array[i] = Integer.parseInt("" + reverse.charAt(i));
    }

From your validate function. This will throw NumberFormatException if it fails to convert the character to an Integer. But if you throw NumberFormatException, the whole program grinds to a halt! If you encounter an unparseable character in a creditcard number, then obviously it's not a valid credit card number. Wrap it with try-catch and return false.

Readability

if (str.length() >= 16 && str.length() <= 19) {
    return "MasterCard";
}

When dealing with ranges, I prefer to write an if statement as a range, rather than two separate conditions. Consider if (16 <= str.length() && str.length() <= 19) instead. This goes for all cases where you have ranges like this.

Magic Numbers

Your third method is filled with magic numbers! What do 622126, 622925, 6011, 644, 649 and so on even mean?! In this case, I think they're all connected in some sort of way. You should add a comment in the code stating where you got the numbers from.

Simplifying the algorithm

    int[] array = new int[str.length()];
    for (int i = 0; i < array.length; i++) {
        array[i] = Integer.parseInt("" + reverse.charAt(i));
    }
    int sum = 0;
    for (int i = 0; i < array.length; i++) {
        if (i % 2 == 1) {
            array[i] *= 2;
            if (array[i] > 9) {
                array[i] -= 9;
            }
        }
        sum += array[i];
    }
    return sum % 10 == 0;

You're using two for loops, but don't use the result in the meantime. You end up only needing the sum of the contents, with some minor permutations. Let's simplify!

First, merge the two for loops.

    int[] array = new int[str.length()];
    int sum = 0;
    for (int i = 0; i < array.length; i++) {
        array[i] = Integer.parseInt("" + reverse.charAt(i));
        if (i % 2 == 1) {
            array[i] *= 2;
            if (array[i] > 9) {
                array[i] -= 9;
            }
        }
        sum += array[i];
    }
    return sum % 10 == 0;

Now that we've merged the two for loops we see that we store data in the array... but never use it afterwards! So all we really need is a single int value.

    int sum = 0;
    for (int i = 0; i < str.length(); i++) {
        int digit = Integer.parseInt("" + reverse.charAt(i));
        if (i % 2 == 1) {
            digit *= 2;
            if (digit > 9) {
                digit -= 9;
            }
        }
        sum += digit;
    }
    return sum % 10 == 0;

And there we go - it's a bit simpler now. You'll still have to add the try-catch that I suggested above yourself, though.

Other comments

String reverse = new StringBuilder().append(str).reverse().toString();

java.lang.StringBuilder can take a String as argument. Doing so would allow you to remove your append call, shortening the code, making it more readable (and potentially speeding things up slightly as you need 1 function call less, but that's negligible).


Domain Model for Simple Yes/No Working from a C# and EntityFramework background. Jimmy Bogard makes the point of modelling your domain model correctly, such that an Email Address is not a simple string but should be of type EmailAddress.
Also known as Primitive Obsession.
I've come to this part of a piece of software where a Application form is to be filled out.
For the user a Yes/No question where they can answer Yes or No is a very easy thing, you can even create really go user experiences around yes/no.
So I have quickly thought of two ways to handle Yes/No.
A complex object:
public class YesNoAnswer
{
    public YesNoAnswer(string state)
    {
        this.SetState(state);
    }

    public bool Yes { get; private set; }

    public void SetState(string state)
    {
        var uppedState = state.ToUpper();

        if (uppedState == "YES")
        {
            this.Yes = true;
            return;
        }
        if (uppedState == "NO")
        {
            this.Yes = false;
            return;
        }

        throw new ArgumentOutOfRangeException(nameof(state), "Yes or No strings not found in argument");
    }
}

Or a simple extension method:
public static class StringExtensions
{
    public static bool GetYesNoState(this string state)
    {
        var uppedState = state.ToUpper();

        if (uppedState == "YES")
        {
            return true;
        }
        if (uppedState == "NO")
        {
            return false;
        }

        throw new ArgumentOutOfRangeException(nameof(state), "Yes or No strings not found in argument");
    }
}

Obviously the validation should be done earlier in the pipeline (or at least it is in our application).
Which is better? They both achieve the same thing, the first is much clearer, neater. The second is well, slightly obfuscated - not a great deal but takes more than a second to think about what it is doing.
Usage of both
var application = new ApplicationForm { IsOkayToWork = new YesNoAnswer(state) };

or
var application = new ApplicationForm { IsOkayToWork = state.GetYesNoState() };

The idea of creating a Value Object instead of using primitives is to have a highly descriptive domain. Using the second option (the extension method) misses the point. You definitely want to be using the first.

e.g.

public class QuestionAnswer
{
    public int QuestionId { get; set; }

    public YesNoAnswer YesNoValue { get; set; }
}

Is more descriptive and specific than:

public class QuestionAnswer
{
    public int QuestionId { get; set; }

    public bool YesNoValue { get; set; }
}

or

public class QuestionAnswer
{
    public int QuestionId { get; set; }

    public string YesNoValue { get; set; }
}

One thing I would add is that you probably want a conversion to bool:

public class YesNoAnswer
{
    // omitted

    public static implicit operator bool(YesNoAnswer answer)
    {
        return (answer?.Yes).GetValueOrDefault();
    }

}

Then you can use it naturally in code:

var someAnswer = new QuestionAnswer { QuestionId = 1, YesNoValue = new YesNoAnswer("YES") }:

if (someAnswer.YesNoValue)
{

}

You should also override Equals and GetHashCode as Value Objects should have value equality not reference equality.


Trading Card Game database schema for statistics This database schema will be used in conjunction with various systems as related to an online Trading Card Game, mostly for permanent storage and statistics. Some example cases would include:

JDBC calling the DB to get information about player decks, or returning after-game stats;
Website displaying stats relating to the game via JavaScript, PHP, etc.

Any advice on naming, table relations and such are appreciated.

START TRANSACTION;
DROP SCHEMA IF EXISTS cardshifter_stats CASCADE;
CREATE SCHEMA cardshifter_stats;
SET SEARCH_PATH TO cardshifter_stats;

CREATE TABLE player
(
    id SERIAL PRIMARY KEY,
    name TEXT NOT NULL,
    password TEXT NOT NULL, -- this will be made secure once implemented
    email TEXT,
    website TEXT NULL,
    about TEXT NULL,
    create_date TIMESTAMP WITHOUT TIME ZONE DEFAULT NOW(),
    delete_date TIMESTAMP WITHOUT TIME ZONE NULL DEFAULT NULL,
    last_seen_date TIMESTAMP NULL
);
CREATE TABLE mod
(
    id SERIAL PRIMARY KEY,
    name TEXT NOT NULL,
    description TEXT NULL,
    owner_player_id INT NOT NULL,
        FOREIGN KEY (owner_player_id) REFERENCES player(id),
    create_date TIMESTAMP WITHOUT TIME ZONE NULL DEFAULT NOW(),
    delete_date TIMESTAMP WITHOUT TIME ZONE NULL DEFAULT NULL
);
CREATE TABLE player_mod
(
    player_id INT NOT NULL,
        FOREIGN KEY (player_id) REFERENCES player(id),
    mod_id INT NOT NULL,
        FOREIGN KEY (mod_id) REFERENCES mod(id)
);
CREATE TABLE card
(
    id SERIAL PRIMARY KEY,
    version INT NOT NULL,
    name TEXT NULL,
    description TEXT NULL,
    effect_description TEXT NULL,
    type TEXT NULL,
    attack INT NULL,
    health INT NULL,
    mana_cost INT NULL,
    scrap_cost INT NULL,
    scrap_value INT NULL,
    sickness INT NULL,
    attack_available INT NULL,
    create_date TIMESTAMP WITHOUT TIME ZONE DEFAULT NOW(),
    delete_date TIMESTAMP WITHOUT TIME ZONE DEFAULT NULL
);
CREATE TABLE deck
(
    id SERIAL,
    version INT NOT NULL,
    player_id INT NOT NULL,
        FOREIGN KEY (player_id) REFERENCES player(id),
    mod_id INT NOT NULL,
        FOREIGN KEY (mod_id) REFERENCES mod(id),
    card_id INT NOT NULL,
        FOREIGN KEY (card_id) REFERENCES card(id),
    card_quantity INT NOT NULL
);
CREATE TABLE game_master
(
    game_id SERIAL PRIMARY KEY,
    player1 INT,
        FOREIGN KEY (player1) REFERENCES player(id),
    player2 INT,
        FOREIGN KEY (player2) REFERENCES player(id),
    start_time TIMESTAMP WITHOUT TIME ZONE DEFAULT NOW(),
    end_time TIMESTAMP WITHOUT TIME ZONE
);
CREATE TABLE game_actions
(
    game_id INT NOT NULL,
        FOREIGN KEY (game_id) REFERENCES game_master(game_id),
    turn INT NOT NULL,
    action_player INT NOT NULL,
        FOREIGN KEY (action_player) REFERENCES player(id),
    action_card INT NULL,
        FOREIGN KEY (action_card) REFERENCES card(id),
    action_attack INT NULL,
    target_player INT NULL,
        FOREIGN KEY (action_player) REFERENCES player(id),
    target_card INT NULL,
        FOREIGN KEY (action_player) REFERENCES player(id)
);

COMMIT;

SELECT
    tables.table_schema,
    tables.table_name,
    columns.column_name,
    columns.ordinal_position,
    columns.is_nullable,
    columns.data_type
FROM information_schema.tables AS tables
    INNER JOIN information_schema.columns AS columns
        ON tables.table_name = columns.table_name
WHERE
    tables.table_schema = 'cardshifter_stats'
ORDER BY
    tables.table_name ASC,
    columns.ordinal_position ASC;

This is what the schema looks like:

I really don't see much to comment on here. The code was easy to read, well structured, and the code style was, for SQL relatively easy on the eyes, and consistent, which is good.

One inconsistency I see is:

create_date TIMESTAMP WITHOUT TIME ZONE DEFAULT NOW(),
delete_date TIMESTAMP WITHOUT TIME ZONE NULL DEFAULT NULL,
last_seen_date TIMESTAMP NULL

yet the image of your schema clearly shows that last_seen_date is also created without the time zone. When I investigated, I found:

  Note: The SQL standard requires that writing just timestamp be equivalent to timestamp without time zone, and PostgreSQL honors that behavior.

So, there is no need to specify WITHOUT TIME ZONE at all, or, if you do, you should do it consistently.

The create_date (or start_date) on all of your tables are defaulted to NOW(), but the columns are still nullable. CreateDate will never be null, and should be created as NOT NULL.

I am suspicious of the type column on card. Columns like that are typically a small domain (only a few values, with many repetitions). That should be normalized out in to a cardtype table, and the type column replaced with a type_id. This will potentially improve the performance and maintainability of the system. Of course, if your type has many different values with few repetitions, the normalization won't make sense.

Similarly, I am sispicious of the version column on deck. This is a value related to the application that does not make clear sense based on what you have presented. I can only guess what it will be used for, but, since the rest of the schema is clear, it stands out as being ... 'obtuse'.

Fnally, I expect that you are missing some indexes.

Postgres will index the primary and foreign key columns, but I suspect indices on player.name, mod.name, and card.name would be good (and consider making things like mod and card names unique....).


Extended matrix generation I am extending a matrix with some calculated values.  This code runs very slowly.

Is there a way to vectorize this with slice and hstack?

import numpy as np

def expand(X):
    X_expanded = np.zeros((X.shape[0], 6))
    for i in range(X.shape[0]):
        X_expanded[i,0]=X[i,0]
        X_expanded[i,1]=X[i,1]
        X_expanded[i,2]=X[i,0]**2
        X_expanded[i,3]=X[i,1]**2
        X_expanded[i,4]=X[i,0]*X[i,1]
        X_expanded[i,5]=1
    return X_expanded

As you hinted in your question, when working with numpy, anytime you use a Python for loop when manipulating your matrices, there is a good chance there will be an opportunity to "vectorize" the operation, and speed it up.  Your code was actually a good start as it allowed you to get the right result with more familiar coding.
To convert it to a vectorized operation you then need to describe the loop in a vector style.  In this case it was as simple as removing the for and replacing the i loop variable with :
def expand(X):
    X_expanded = np.zeros((X.shape[0], 6))
    X_expanded[:, 0] = X[:, 0]
    X_expanded[:, 1] = X[:, 1]
    X_expanded[:, 2] = X[:, 0] ** 2
    X_expanded[:, 3] = X[:, 1] ** 2
    X_expanded[:, 4] = X[:, 0] * X[:, 1]
    X_expanded[:, 5] = 1
    return X_expanded

The above will almost certaily be much faster than the original.  And depending on your needs you might stop there.  But since you also asked about stacking, you can remove the intermediate np.zeros like:
def expand(X):
    return np.stack((
        X[:, 0],
        X[:, 1],
        X[:, 0] ** 2,
        X[:, 1] ** 2,
        X[:, 0] * X[:, 1],
        np.ones(X[:, 0].shape)
    ), axis=1)

Test Code:
def original_expand(X):
    X_expanded = np.zeros((X.shape[0], 6))
    for i in range(X.shape[0]):
        X_expanded[i, 0] = X[i, 0]
        X_expanded[i, 1] = X[i, 1]
        X_expanded[i, 2] = X[i, 0] ** 2
        X_expanded[i, 3] = X[i, 1] ** 2
        X_expanded[i, 4] = X[i, 0] * X[i, 1]
        X_expanded[i, 5] = 1
    return X_expanded

data = np.array(list(zip(range(10), range(1, 11))))
assert not (expand(data) - original_expand(data)).any()


ADT for a TVM calculator I wanted to get some criticism for the below ADT. This is just a basic ADT that will be fed into a TVM calculator. Main purpose is to have it initialized and correct for input errors.

Full disclosure: I am modeling it after Code Complete 2 guidelines. You should notice if statements stating nominal cases first followed by error-checking in else statements. I am trying to come as close as possible to the guidelines in the book. However, it is difficult to grasp 900+ pages of material. Aside from the incompleteness of the class, what am I missing, or what would Code Complete 2 catch?

public class TVMData
{
public decimal DiscountRate;
public decimal PeriodLength;
public decimal CashFlowFromAsset;
public decimal CashOutFlow;
public decimal PaymentFrequency;

   decimal DiscountedCashFlow;

    public TVMData(){}

    public TVMData(double discountRate, double periodLength, double cashFlowFromAsset, double cashOutFlow, PaymentPerPeriod paymentFrequency= PaymentPerPeriod.Annual)
      : this(Convert.ToDecimal(discountRate), Convert.ToDecimal(periodLength), Convert.ToDecimal(cashFlowFromAsset), Convert.ToDecimal(cashOutFlow), paymentFrequency){}

    public TVMData (decimal discountRate, decimal periodLength, decimal cashFlowFromAsset, decimal cashOutFlow, PaymentPerPeriod paymentFrequency= PaymentPerPeriod.Annual)
    {
        this.PaymentFrequency = Convert.ToDecimal(paymentFrequency);

        if (discountRate>=0m)
        {
           this.DiscountRate= discountRate;

            if (periodLength >= 1m)
            {
                this.PeriodLength = periodLength;

                if(cashFlowFromAsset >= 0m)
                {

                 this.CashFlowFromAsset=cashFlowFromAsset;

                   if(cashOutFlow <= 0m)
                   {

                    this.CashOutFlow = cashOutFlow;

                   }

                   else
                   {
                    throw new ArgumentOutOfRangeException("Cash Out Flow must be less than or equal to Zero", "cashOutFlow");
                   }

                }

                else
                {
                throw new ArgumentOutOfRangeException("Cash Flow From Asset must be greater than Zero","cashFlowFromAsset");
                }

            }

            else
            {
            throw new ArgumentOutOfRangeException("Period Length must be equal or greater than One", "periodLength");
            }

        }

        else
        {
         throw new ArgumentOutOfRangeException("Discount Rate must be greater than Zero", "discountRate");
        }

     }

}

That book is a little dated in some of its recommendation (although still a great book).  That particular IF-ELSE recommendation leads to the ArrowCode anti-pattern, a better way (at least it reduces congnitive load) would be to use Guarded Clauses (see here too)

public TVMData(decimal discountRate, decimal periodLength, decimal cashFlowFromAsset, decimal cashOutFlow, PaymentPerPeriod paymentFrequency = PaymentPerPeriod.Annual)
        {
            if (discountRate < 0m)
                throw new ArgumentOutOfRangeException("Discount Rate must be greater than Zero", "discountRate");
            if (periodLength < 1m)
                throw new ArgumentOutOfRangeException("Period Length must be equal or greater than One", "periodLength");
            if (cashFlowFromAsset < 0m)
                throw new ArgumentOutOfRangeException("Cash Flow From Asset must be greater than Zero", "cashFlowFromAsset");
            if (cashOutFlow > 0m)
                throw new ArgumentOutOfRangeException("Cash Out Flow must be less than or equal to Zero", "cashOutFlow");

            this.DiscountRate = discountRate;
            this.PaymentFrequency = Convert.ToDecimal(paymentFrequency);
            this.PeriodLength = periodLength;
            this.CashFlowFromAsset = cashFlowFromAsset;
            this.CashOutFlow = cashOutFlow;
        }

Your Code here converts double to decimal, you may have issues with calling functions not taking into account the smaller range decimal has when compared to double

public TVMData(double discountRate, double periodLength, double cashFlowFromAsset, double cashOutFlow, PaymentPerPeriod paymentFrequency= PaymentPerPeriod.Annual)
      : this(Convert.ToDecimal(discountRate), Convert.ToDecimal(periodLength), Convert.ToDecimal(cashFlowFromAsset), Convert.ToDecimal(cashOutFlow), paymentFrequency){}

I would also move these to private if I could.  Reducing the scope and visibility of data leads to more reliable/maintainable code by reducing coupling (see here for better argument or here).

public decimal DiscountRate;
public decimal PeriodLength;
public decimal CashFlowFromAsset;
public decimal CashOutFlow;
public decimal PaymentFrequency;


Calculator implementation With some background and experience in PHP & Python, I'm trying to learn Swift by myself (web, videos, Ray Wenderlich books). I've read that a good first project for beginners is to try and write a basic calculator, so I've tried that. I'm wondering if there's a way to make my code leaner and more elegant (I've spent quite some time just getting the operator & equals keys to behave as expected). There's still no support for handling division by zero or display overflowing, the "C" and "AC" methods should be merged, but for now it does what it's supposed to do.

All digit keys are connected to buttonDigit, all operators to operation and the rest is obvious (I hope). Pardon the indentation right after the class declaration.

import UIKit

class ViewController: UIViewController {

@IBOutlet weak var display: UILabel!
var isTyping : Bool = false
var currentResult : Double = 0
var currentOperation : String? = nil
var nextOperation : String? = nil
var operand : Double = 0

// Handle digits

@IBAction func buttonDigit(sender: UIButton) {

    let digit = sender.currentTitle!
    if isTyping {
        display.text = display.text! + digit
    } else {
        display.text = digit
        isTyping = true
    }

}

// Handle operations keys

@IBAction func operation(sender: UIButton) {

    operand = displayValue
    nextOperation = sender.currentTitle

    if isTyping {
        equals()
    }

    currentOperation = nextOperation
    currentResult = displayValue
    isTyping = false
}

// Handle "equals" key

@IBAction func equals() {

    if (isTyping) {
        operand = displayValue
    }

    if (currentOperation != nil) {
        switch currentOperation! {
        case "+": display.text = "\(currentResult + operand)"
        case "-": display.text = "\(currentResult - operand)"
        case "×": display.text = "\(currentResult * operand)"
        case "÷": display.text = "\(currentResult / operand)"
        default: break
        }
    }

    currentResult = displayValue
    isTyping = false
}

// Handle AC key

@IBAction func allClear() {
    currentResult = 0
    display.text = "0"
    nextOperation = nil
    currentOperation = nil
    isTyping = false
}

// Handle C key

@IBAction func clear() {
    display.text = "0"
    isTyping = false
}

// Computed double from display label string

var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
    set {
    }
}
}

var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
    set {
    }
}

This empty setter is very disingenuous.

If we don't want to let the user set the display's text property by passing a double, then we should make this property readonly by eliminating the set:

var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
}

But I actually don't necessarily see a reason why this can't be readwrite:

var displayValue : Double {
    get {
        return (display.text as NSString!).doubleValue
    }
    set {
        display.text = String(format: "%f", newValue)
    }
}

Of course, we can use our format string to specify number of decimal places to show and such.

The biggest thing is, we need to do something with the empty set for our computed property.  If we don't want an end-user setting the value this way, then don't let them and don't let them think they can.  By removing the set method as I've done in the first example, trying to set the property will result in a compiler error.

There's another area we want to start using the compiler to our advantage--with our operations.  You're using a simple String type to track what sort of math we should be doing.  Swift is all about explicit types, so let's make an enum to handle our operations:

enum MathematicalOperation: String {
    case Addition = "+"
    case Subtraction = "-"
    case Multiplication = "x"
    case Division = "÷"
}

Now let's just our currentOperation and nextOperation to have the MathematicalOperation type:

var currentOperation : MathematicalOperation?
var nextOperation : MathematicalOperation?

We can still make use of the button's title text if we really want:

self.nextOperation = MathematicalOperation(rawValue:sender.currentTitle)

And now, let's fix up that switch:

if (currentOperation != nil) {
    switch currentOperation! {
    case "+": display.text = "\(currentResult + operand)"
    case "-": display.text = "\(currentResult - operand)"
    case "×": display.text = "\(currentResult * operand)"
    case "÷": display.text = "\(currentResult / operand)"
    default: break
    }
}

Let's get rid of the unnecessary ! operation (which when abused leads to some bad code--here is fine, but let's not get in the habbit), and let's not rely on string comparisons for the switch:

if let op = currentOperation {
    switch op {
        case .Addition: display.text = "\(currentResult + operand)"
        case .Subtraction: display.text = "\(currentResult - operand)"
        case .Multiplication: display.text = "\(currentResult * operand)"
        case .Division: display.text = "\(currentResult / operand)"
    }
}


System for inputting and monitoring worker shifts I've been building a system for inputting and monitoring shifts for casual staff, who work across multiple sites with the ability to generate accounting information.

I've had some help from Stack Overflow in building this project, as I had no prior knowledge of PHP or MySQL, and each time I posted some of my code I had comments about the lack of security.

In my system, sensitive information like salaries, work hours and things are protected by having different levels of user accounts. The code below is a snippet of code from my Admin Area allowing me to edit an account's userLevel and password.

if(isset($_POST['submit'])) {

    $editid = htmlentities($_POST['id'], ENT_QUOTES, 'UTF-8');
    $userLevel = htmlentities($_POST['userLevel'], ENT_QUOTES, 'UTF-8');

    if(!empty($_POST['password'])) {

        $password = password_hash("$_POST['password']", PASSWORD_DEFAULT)

        $sql = "UPDATE users SET userLevel = ?, password = ?, salt = ? WHERE id = ?";
        $stmt = $connection->prepare($sql);
        $stmt->bind_param('ssss', $userLevel, $password, $salt, $editid);

    } else {

        $sql = "UPDATE users SET userLevel = ? WHERE id = ?";
        $stmt = $connection->prepare($sql);
        $stmt->bind_param('ss', $userLevel, $editid);

    }

        $stmt->execute();
}

I specify UTF-8 charset at the top of my php files, and all raw output variables, posts, gets and sessions are wrapped in htmlentities tags. My account passwords are hashed and salted.

My system is only used locally and will never be used on the web (unless I post the code somewhere, which I could do to help other new people learn), but as I'm learning I was told I should get in the habit of writing secure code. So finally we get to the question.. Is the above code of a secure enough level for the web? If not, please offer suggestions.

One thing to improve on (if your PHP version allows it), is to consider using PHP's built in password_hash() function. Coupled with password_verify(), it eliminates the need to salt and hash passwords manually. More information can be found in the documentation.


Aborting a program for invalid floating-point input My friend does a bunch of Java work (commandline, still toying around) and I notice a bunch of try/catch blocks like this:

   try {
       double a = Double.parseDouble(secondInput);
   }
   catch(NumberFormatException nFE) {
       System.out.println("The input was not an integer.");
       try {
           Thread.sleep(1997);
       } catch (InterruptedException e) {
           // TODO Auto-generated catch block
           e.printStackTrace();
       }
       System.exit(0);
   }

Which, IMO doesn't look very pretty. And I'm wondering what the "better" way to write this code would be.

There are a couple of issues.

Why is the application pausing for 2 seconds before exiting?  Why not just exit straightaway?  (And if the answer is that you are running this via Windows 'cmd.exe' then you are better off handling the pausing at that level ...)
Given that you do need to pause, then you are better of writing a helper method to do something like "print a message, pause, and exit with a non-zero exit code".  Then use that in multiple places, as required.
You should be setting a non-zero exit code when you exit due to a failure.  This allows the script or whatever that launched the java app to detect the failure and (maybe) do something about it; e.g.  pause so that the user can see the error message ...
The InterruptedException is not a program error, so printing a stacktrace is probably not appropriate.

Figuring out what to do with an InterruptedException exception is not straightforward.  The exception indicates that something (either the user or the application) has interrupted the application, either before or during the sleep.

In this use-case you are just about to exit anyway, so the best strategy is to ignore it an exit anyway.
You need to decide what is a sensible thing to do.  Some possibilities are:

Ignore the interrupt and carry on regardless.
Take steps to cause the application to stop what it was doing; e.g. throw a custom unchecked "stop now" exception.  (You could also call System.exit() directly, but that can also be a bad thing to do, depending on the nature of the code.  For instance, calling System.exit() in a library ... or a webapp ... is a big non-no.)
Punt: reinstate the "interrupted" state by calling Thread.currentThread().interrupt() in the exception handler.  For instance, you might do this if some enclosing code is checking the "interrupted" flag.


Express Rest API Async and Scaling I am currently learning how to create nodejs express rest api using async making it fully scaleable and secure.

My goal is to ensure maximum functionality and speed, currently I am using MVC structure with route, controller, service and model.

Database is mysql and using mysql2 pool.

I am looking to know if the example snippets of code below are optimised as best as possible and correctly using async or is there a more efficient way ?

Route

'use strict';

const express = require('express');
const app = express.Router();
const Stats_controller = require('../controllers/stats');

app.get('/stats', Stats_controller.getStats);

module.exports = app;

Controller (unsure here about the async how its been used)

'use strict';
const statsService = require('../services/stats.js');

const getStats = async (req, res, next) => {
  statsService.getStats(function(err, result) {
    if (err)
      res.send(err);
      res.json(result);
  });
};

module.exports.getStats = getStats;

Service

'use strict';
var coinsModel = require('../models/stats.js');

const getStats =  async function(result) {
    const res = await coinsModel.getStats();
    var responseData = {
        "status":"success",
        "data":res
    }
    result(null, responseData);
}

module.exports.getStats = getStats;

Model

'use strict';
var db = require('../config/conf.js');

const getStats = async function() {
  return new Promise((resolve) => {
        db.connection.query("SELECT * FROM stats", function (err, res) {
      if(!err){
        resolve(res)
      }else{
        resolve([])
      }
    });
  });
}

module.exports.getStats = getStats;

Some comments:

Use the built-in database promise support in mysql2 rather than promisify stuff yourself.
In Model, don't hide low level database errors.  If the database has an error, let the caller see the error.  It's up to the caller what to do with errors.
There's no reason for your Model.getStats() to be async.
Service should use promises to communicate back the results, not a callback.  That's the modern way to handle asynchronous results.  When you already have a promise, don't introduce a plain callback.
In Controller.getStats(), when you get an error and do res.send(err) and then res.json(result).  You can't call both of those.  You send one response and only one response for each request.  Then, your code doesn't send any response when there is no an error.
Log every error somewhere so you can diagnose server problems when they are occuring.
As currently shown, there's no reason for Controller.getStats() to be async.  You only need a function to be async if you need to use await or you're trying to use async to catch synchronous exceptions for you.  It appears you are throwing async in the function definition any time you have an asynchronous operation.  That's not the right reason to use async on your functions.
Performance/speed of this type of code is largely going to be determined entirely by your database configuration and performance as there's nothing in this code that really influences the performance/speed much.  You aren't doing any processing of the data here.  The extra Service layer isn't adding much value, but also probably doesn't really change much compared to the DB performance.