Method for getting the DataTable when having connection ID and the SQL command I am quite new to C# and to the ASP.NET programming. When maintaining the legacy code, I have found rather terrible implementation of the method that is used on many places to get the DataTable object with the data filled by the SQL command.

Here is my first approach to rewrite it. Does using work with the return as shown below? Should the da and ds be released explicitly somehow? If ds is released, is the returned DataTable reference counted so that it will not be deleted?

public DataTable getdata2(string connection_string_id, string sqlcmd)
{
    string connstring = ConfigurationManager
        .ConnectionStrings[connection_string_id].ConnectionString;

    // Open the connection and return the first table.
    // A single one should be there, only.
    using (SqlConnection con = new SqlConnection(connstring))
    {
        SqlDataAdapter da = new SqlDataAdapter(sqlcmd, con);
        DataSet ds = new DataSet();
        da.Fill(ds);
        Debug.Assert(ds.Tables.Count == 1);
        return ds.Tables[0];
    }
}

Like janos already stated it is ok to return out of an using block.
Naming
Based on the naming guidelines, method names should be named using PascalCase casing. So getData2 should become GetData2 which should be still renamed to a more meaningful name.
Also input parameters should be named using camelCase casing, so connection_string_id should become connectionStringId.
If you are sure, that there will only be 1 table, you can also use the overloaded Fill() method which takes a DataTable as input parameter.
You could also stack the usings, but this is a matter of taste.
using (SqlConnection con = new SqlConnection(connstring))
using (SqlDataAdapter adapter = new SqlDataAdapter(sqlcmd, con))
{
    con.Open();
    DataTable dataTable = new DataTable();
    adapter.Fill(dataTable);
    return dataTable ;
}

In my opinion, your code does a little bit to much.

Retrieving the ConnectionString
Query the database

So let us add a method to retrieve the ConnectionString, which is then passed to the Getdata2() method.
private String GetConnectionString(String connectionStringId)
{
    return ConfigurationManager
        .ConnectionStrings[connectionStringId].ConnectionString;
}

public DataTable GetData2(String connectionString, String sqlCommand)
{

    using (SqlConnection connection = new SqlConnection(connectionString))
    using (SqlDataAdapter adapter = new SqlDataAdapter(sqlCommand, connection))
    {
        con.Open();
        DataTable dataTable = new DataTable();
        adapter.Fill(dataTable);
        return dataTable ;
    }
}

To answer when to use using: use it if the related object implements the IDisposable interface.


Binary search that returns bitwise complement of missing index While re-writing some old JavaScript code to better handle large arrays, I ended up writing my own binary search method.

I found many examples of binary search methods written in JavaScript, but all the examples I found returned -1 whenever the target was not found in the array. What I really wanted was the bitwise complement of the index where the target belonged if it did not exist in the array, like the BinarySearch method on generic lists in .NET.

This is the code I ended up writing, but I'm curious if I've overlooked any potential performance problems or logic errors. To the best of my knowledge, the code works sufficiently as designed (see my fiddle). Please help me review this code and verify that it follows common best practices.

binarySearch Method

Inputs: target: the object being sought in the array;
comparator: (optional) comparison method

Returns: If positive or zero, the index of the item in the array. If negative, the bitwise complement of the item's proper location in the array.

Array.prototype.binarySearch = function (target, comparator) {
    var l = 0,
        h = this.length - 1,
        m, comparison;
    while (l <= h) {
        m = (l + h) >> 1;
        comparison = typeof (comparator) == "undefined" ? (this[m] < target ? -1 : (this[m] > target ? 1 : 0)) : comparator(this[m], target);
        if (comparison < 0) {
            l = m + 1;
            continue;
        } else if (comparison > 0) {
            h = m - 1;
            continue;
        } else {
            return m;
        }
    }
    return~l;
};

Using the above, I also wrote a binary insert method, which adds an item to the correct location in an array. I'm curious about this also, and whether there's anything I can do to improve its performance or handle any edge case bugs.

binaryInsert Method

Inputs: target: object being inserted; duplicate: (optional) whether to add if it already exists in the array (false by default); comparator: (optional) comparison method

Returns: Index of new item added to array

Array.prototype.binaryInsert = function (target, duplicate, comparator) {
    if (typeof (duplicate) == "undefined") duplicate = false;
    var i = this.binarySearch(target, comparator);
    if (i >= 0) {
        if (duplicate) {
            this.splice(i, 0, target);
        }
    } else {
        i = ~i;
        this.splice(i, 0, target);
    }
    return i;
};

• First comment is about the way you are adding the functions to the Array prototype : by just adding them this way, you 'pollute' the keys of the array, so anyone doing a for ... in for instance will enumerate also on the added functions.
-->> Use Object.defineProperty to have those function non enumerables.
The pattern could look like :

(function () {

    var newArrayMethods = {
        //  Returns a shallow copy of this array
        copy: function () {
            return this.slice(0);
        },

        // Returns true if this array contains 'element', returns false otherwise
        contains: function (element) {
            return this.indexOf(element) >= 0 ;
        }
    }

    // let's install those methods on the prototype
    for (var newMethodName in newArrayMethods) {
        installFunction(newMethodName, newArrayMethods[newMethodName]);
    }

    function installFunction(name, fn) {
        if (Array.prototype[name]) throw ('Array method ' + name + '() already defined.');
        Object.defineProperty(Array.prototype, name, {
            value: fn
        });
    }

})();

• Second thing is that typeof is slow : you don't want to use it within a loop.
The code below also avoids one branch per loop.

function binarySearch (target, comparator) {
    var l = 0,
        h = this.length - 1,
        m = 0,
        comparison = 0;
   comparator = comparator || compareNumbers ;

    while (l <= h) {
        m = (l + h) >> 1;
        comparison = comparator(this[m], target);
        if (comparison < 0) {
            l = m + 1;
        } else if (comparison > 0) {
            h = m - 1;
        } else {
            return m;
        }
    }
    return~l;
};

function compareNumbers(a,b) { a-=b;  return (a<0) ? -1 : ( a>0 ) ? 1 : 0 ;  } ;

• Array.splice is to be avoided if you seek performances : it is slow and creates garbage.
Do the insertion yourself.
I wrote below insertion function moving forward, to avoid cache miss.

You'll notice i changed the insert function signature so it is more alike to the search (target + comparator arguments first).

Also no need to process an argument that defaults to false.

function binaryInsert(target, comparator, duplicate) {
    var i = this.binarySearch(target, comparator);
    if (i >= 0 ) {
        if (!duplicate) return i;
    } else {
        i = ~i;
    }
    insertOneAt(this, target, i);
    return i;
};

function insertOneAt(arr, item, index) {
    var tmp1=item, tmp2=item;
    var len = arr.length;
    for (; index<=len; index++) {
       tmp2 = arr[index]
       arr[index]=tmp1;
       tmp1=tmp2;
    }
}

• Detail : change your binaryInsert comment about the return value to something like :

Returns: Index of new item added to array, or of the existing item if  duplicate==false and the item was found.

• You have also some decision/changes to make about the way you handle duplicates in your search.
As of now search returns any matching item's index (compare==0), it does not ensure, for instance, that it is the smallest index.
If it's ok just mention it, otherwise you have to change a bit your search.

Since the insert relies on search, your decision on the search will affect insert's semantic as well.


Ghost4J PDF to image conversion The primary goal of this code is to convert a PDF file to images. I also create a directory under C:/Media based on a combination of the PDF name and current date and insert the uploaded PDF to the same directory as the images for safe keeping.

I am primarily concerned that I am over-complicating the process by initializing too many variables and the process by which I move the PDF to the newly created directory. I am looking for guidance or advice on how to improve this code.

public class PdfService {

public static void main(String[] args) {

    System.out.println("Test of Convert to PNG");
    convertPDFtoImage(args[0]);
 }

public static void convertPDFtoImage(String PDFFileName) {

    try {

         // load PDF document
         PDFDocument document = new PDFDocument();
         File baseFile = new File(PDFFileName);
         document.load(baseFile);

         //Get todays date as a String so that you can append it to the directory
         String df = new SimpleDateFormat("MM-dd-yy").format(new Date());

         //Get the filename without extension so that you can create a directory based on the name
         Path path = Paths.get(PDFFileName);
         String filenameWithExtension = path.getFileName().toString();
         String filename = FilenameUtils.removeExtension(filenameWithExtension);

         //add the current date to the filename to get the directory name
         String dirname = filename + "-" + df;

         //Create the directory
         Path dir =  Paths.get("C:\\Media\\" + dirname);
         Files.createDirectory(dir);

         //Move the pdf to the newly created directory
         movePDFToDirectory(baseFile, dirname, filenameWithExtension);

         // create renderer
         SimpleRenderer renderer = new SimpleRenderer();

         // set resolution (in DPI)
         renderer.setResolution(150);

         // render
         List<Image> images = renderer.render(document);

         // write images to files to disk as JPEG
            try {

                System.out.println("Begin converting PDF...");

                for (int i = 0; i < images.size(); i++) {
                    ImageIO.write((RenderedImage) images.get(i), "jpeg", new File("C:\\Media\\" + dirname + "\\" + filename + "_" + (i + 1) + ".jpeg"));
                }

                System.out.println("PDF converted to image(s) successfully.");

            } catch (IOException e) {
                System.out.println("ERROR: " + e.getMessage());
            }

       } catch (Exception e) {
         System.out.println("ERROR: " + e.getMessage());
       }
}

public static void movePDFToDirectory(File originalFile, String directory, String filename) {
        InputStream inStream = null;
        OutputStream outStream = null;

        try{

            File newFile = new File("C:\\Media\\" + directory + "\\" + filename);

            inStream = new FileInputStream(originalFile);
            outStream = new FileOutputStream(newFile);

            byte[] buffer = new byte[1024];

            int length;
            //copy the file content in bytes
            while ((length = inStream.read(buffer)) > 0){

                outStream.write(buffer, 0, length);

            }

            inStream.close();
            outStream.close();

            System.out.println("Original PDF has been copied successfully!");

        }catch(IOException e){
            e.printStackTrace();
        }

}
}

Notes:

Eventually I plan on moving this bit of code to a web application that accepts the image as an upload. As it stands I'm just passing the file location as an argument in the main method.
I plan on making the base directory (C:/Media) a dynamic variable in the event we move directories.

I see that you're using Java 7, you could use the try-with-resource for all you're IO operation. It will close the stream you're using in your application.

 try{
            File newFile = new File("C:\\Media\\" + directory + "\\" + filename);

            inStream = new FileInputStream(originalFile);
            outStream = new FileOutputStream(newFile);

            byte[] buffer = new byte[1024];

            int length;
            //copy the file content in bytes
            while ((length = inStream.read(buffer)) > 0){

                outStream.write(buffer, 0, length);

            }

            inStream.close();
            outStream.close();

            System.out.println("Original PDF has been copied successfully!");

        }catch(IOException e){
            e.printStackTrace();
        }

You're not closing your streams if there is an IOException. This could lead to some potential problem in a future application. To get ride of the problem you could use the try-with-resource syntax. Your code could look like :

File newFile = new File("C:\\Media\\");
try ( InputStream inStream = new FileInputStream(newFile);
      OutputStream outStream = new FileOutputStream(newFile);) {

    byte[] buffer = new byte[1024];

    int length;
    // copy the file content in bytes
    while ((length = inStream.read(buffer)) > 0) {

        outStream.write(buffer, 0, length);

    }

    System.out.println("Original PDF has been copied successfully!");

} catch (IOException e) {
    e.printStackTrace();
}

Argument name should not start with an uppercase letter : public static void convertPDFtoPNG(String PDFFileName). Do you need to capitalize pdf ?

I would revise some of you comments. Comments should be helpful, you don't need to repeat the code. Example :

 // load PDF document
 PDFDocument document = new PDFDocument();
 File baseFile = new File(PDFFileName);
 document.load(baseFile);

Does //load PDF document is really needed ? You're already using document.load. One liner comment are most of time just repeating what you are doing in code. Other readers don't need to know what you are doing, but rather why are you doing it. I am always wondering myself when I am adding a comment : Does my code is clear enough by itself ? Do I really to add a comment or could I change the method name or variable to better express myself. Comments are hard to maintain when the code is changing.

I know you're not in a production environment yet, but don't forget to change all the e.printStackTrace for a logging of some sort.

It is worth to know that you can use / for all of you path, instead of escaping \. Java will manage the one you need for the OS you're using.

You're saying that your converting a PDF to an image, but it's not quite true. In fact, you are converting a PDF to a jpeg, which is quite different IMHO. You are hard coding the creation of the image as an jpeg, you will probably want to change in the future to support more formats. You will probably extract the writing code of the image into his own method or interface so that you have multiple implementation for each format (if needed, I am not good in image at all).


Combining two queries in one What I want is getting all names (first, middle, last) of the users along with each of their total hours of work which will be calculated by the SQL statement: CAST(SUM(timediff(dateTimeOut,dateTimeIn) as time).

I was able to achieve this with two queries, but somehow, I feel that this is inefficient (?):

$sql = "SELECT id, firstName, middleName, lastName FROM users_info";
$result = $pdo->query($sql);

$rows = array();

while($row = $result->fetch()){
     $time = $pdo->query(
                "SELECT CAST(SUM(timediff(dateTimeOut, dateTimeIn))
                 as time) as totalHoursWorked
                 FROM users_time WHERE user_id = {$row['id']}"
            )->fetch();

     $row['totalHoursWorked'] = $time['totalHoursWorked'];
     $rows[] = $row;

}

echo json_encode($rows);

I'll use the json data for a DataTable.

users_info table
+----+-----------+------------+----------+
| id | firstName | middleName | lastName |
+----+-----------+------------+----------+
|  1 | Isabelle  | Luna       | Ibarra   |
|  2 | George    | Boston     | Everett  |
|  3 | Skyler    | Land       | Cohen    |
+----+-----------+------------+----------+

users_time table:
+---------+---------+---------------------+--------------------+
| time_id | user_id |     dateTimeIn      |    dateTimeOut     |
+---------+---------+---------------------+--------------------+
|       1 |       1 | 2019-06-14 00:00:00 | 2019-06-16 09:00:0 |
|       2 |       1 | 2019-06-15 00:00:00 | 2019-06-16 08:00:0 |
|       3 |       2 | 2019-06-14 00:00:00 | 2019-06-16 09:30:0 |
|       4 |       2 | 2019-06-15 00:00:00 | 2019-06-16 08:30:0 |
|       5 |       3 | 2019-06-16 00:00:00 | 2019-06-16 09:00:0 |
+---------+---------+---------------------+--------------------+

db-fiddle link : https://www.db-fiddle.com/f/vpozKbVoXxPoGzcsBkLL36/0

Assuming that the id's in users_info exist in users_time, INNER JOIN (or just JOIN) is the way to merge the tables.  If id's in user_info might not exist in users_time then use LEFT JOIN to allow null values in totalHoursWorked.

Assuming you want all four columns' data to be included in your echo'ed json string (your posted code isn't do that), you can just fetchAll() directly into your json_encode() and echo it -- all at once.

SQL: (Demo)

$sql = "SELECT i.id,
            MAX(i.firstName),
            MAX(i.middleName),
            MAX(i.lastName),
            CAST(SUM(TIMEDIFF(t.dateTimeOut, t.dateTimeIn)) AS TIME) AS totalHoursWorked
        FROM users_info i
        INNER JOIN users_time t ON i.id = t.user_id
        GROUP BY i.id";
$query = $pdo->query($sql);
echo json_encode($query->fetchAll(PDO::FETCH_ASSOC));

If you are truly only returning a single column (totalHoursWorked) without any of the other data, then I don't see the need to involve the user_info table at all.


Handle database validation (exists, unique constraint, etc) Let's assume you are writing a REST Api with a common structure like Controller > Service > Repository > Database.
In the database there's a table with a column with a unique constraint:
   // Examples with typeorm/typescript

   // user.entity.ts
   @Entity()
   export class User {
      @PrimaryGeneratedColumn()
      id: number;

      @Unique()
      @Column()
      email: string;

      // Other properties
   }

In the updateUser method you have to check if the user exists and if the user email is not used by another one.
Which one of the following approaches would it be better?
A) Interrogate the database to check if the user with the given id exists and do it again to make sure there's not another user with the same email:
   // users.service.ts
   @Entity()
   export class UsersService {
      async updateUser(user: UserDTO): Promise<UserDTO> {
         const userById = await this.repository.findOne(user.id);
         if (!userById) {
            throw new Error('User does not exist');
         }

         const userWithSameEmail = await this.repository.findOne({
            where: { id: Not(user.id), email: user.email }
         });
         if (userWithSameEmail) {
            throw new Error('Email already in use');
         }

         // Handle errors with some interceptor
         // Rest of the method
      }
   }

B) Database calls could lead to performance issues. The orm will anyway throw an error in case of invalid input:
   @Entity()
   export class UsersService {
      async updateUser(user: UserDTO): Promise<UserDTO> {
         try {
            const updatedUserEntity = await this.repository.update(user);

            // Rest of the method
         }
         catch (e) {
            // Handle error
         }
      }
   }

Quick bonus question
Should the service layer be aware about entities and handle operations like receive the updatedUserEntity from the repository, map it to the DTO and return it to the level above?

With approach A) you may run in a concurrency problem.
For example process A and process B want to write a new entry. Both load the data from the database and check if their new entry would be valid. Both decide "YES, valid".
Now process A writes the new entry, and then process B tries to write. But A already changed the database, therefor the validation in B was executed on obsolete data.
Be aware that node will execute things concurrently, because as soon as a "process" (task) has some waiting time, another process (task) will be executed until that one is finished or also waiting for something.
With version B) this problem gets (for most db´s) fixed. On the other side, now your business logic is scattered over multiple systems, which makes maintenance harder.
--> As always, there is no YES or NO but a "depends on your context" :-)
About the bonus question:
If your application uses the same entities as the DB, then your whole application (from top to bottom) has a dependency to your database schema.
As a result, a change in your DB then has to be followed by a lot of changes in your application.  If you do the mapping in the service layer (thats where i would expect the business logic), then all changes run at least until there.
Therefor i would "map" the data in the data access layer into the entities i use in my application. Yep, in many cases the entites will look similar, but it decouples my application from my database. And my business logic will not know anything about the format that is used when data gets stored.
By the way, some people use their entities over the whole application, some create new entities for each layer and the data get maped a lot. Both approaches have their reason, personaly i work with a entity set for the whole application.
Just a few thoughts. Pick those that make sense to you and skip the rest. :-)
warm regards


Parallel map in Rust A parallel map written in Rust. I am new to Rust and wondering if there is things that could be done better or more efficiently.
use crossbeam_channel::unbounded;
use std::{thread, time};

fn parallel_map<T, U, F>(mut input_vec: Vec<T>, num_threads: usize, f: F) -> Vec<U>
where
    F: FnOnce(T) -> U + Send + Copy + 'static,
    T: Send + 'static,
    U: Send + 'static + Default,
{
    let mut output_vec: Vec<U> = Vec::with_capacity(input_vec.len());
    let mut threads = Vec::new();

    let (in_s, in_r) = unbounded::<(T, usize)>();
    let (out_s, out_r) = unbounded::<(U, usize)>();

    for _ in 0..num_threads {
        let in_r = in_r.clone();
        let out_s = out_s.clone();
        threads.push(thread::spawn(move || {
            while let Ok((value, index)) = in_r.recv() {
                let res = f(value);
                out_s.send((res, index)).expect("Failed to send");
            }
        }));
    }

    while let Some(val) = input_vec.pop() {
        in_s.send((val, input_vec.len())).expect("Failed to send");
    }

    drop(in_s);
    drop(out_s);

    let mut collect_results: Vec<(U, usize)> = Vec::with_capacity(output_vec.capacity());
    while let Ok(res) = out_r.recv() {
        collect_results.push(res);
    }

    collect_results.sort_by(|(_, a_index), (_, b_index)| a_index.partial_cmp(b_index).unwrap());

    output_vec.extend(collect_results.into_iter().map(|(val, _)| val));

    for thread in threads {
        thread.join().expect("Failed to join thread");
    }

    output_vec
}

fn main() {
    let v = vec![6, 7, 8, 9, 10, 1, 2, 3, 4, 5, 12, 18, 11, 5, 20];
    let squares = parallel_map(v, 10, |num| {
        println!("{} squared is {}", num, num * num);
        thread::sleep(time::Duration::from_millis(500));
        num * num
    });
    println!("squares: {:?}", squares);
}
```

The crate rayon implements similar functionality:
use rayon::prelude::*;

let mut par_iter = (0..5).into_par_iter().map(|x| x * x);

let squares: Vec<_> = par_iter.collect();

In your solution, the multithreading part looks OK.
The part where you collect results could be done more efficiently. Specifically, one alloc and data copy could be avoided with use of a little unsafety.
let mut output_vec: Vec<U> = Vec::with_capacity(initial_len);
unsafe {
    output_vec.set_len(initial_len);
}
while let Ok((res, index)) = out_r.recv() {
    let old = mem::replace(&mut output_vec[index], res);
    mem::forget(old);
}

Same optimization could be done a little more succintly with ptr::write. Further, you may need to assert that output_vec has correct length, and forget the vec beforehand in case the assertion fails. My concern is that above proposed code is not protected against incorrect drops if U has a destructor and code panics.
Your bound of U as Default appears to be unnecessary.


Depth First Search of graph in Python Task: Do DFS traversal from a given given graph and print output.

Is this code the correct implementation of DFS traversal in Graph?

from collections import defaultdict

class Graph(object):
    def __init__(self):
        self.graph = defaultdict(list)

    def add_edge(self, u, v):
        self.graph[u].append(v)

    def depth_first_search(self,node):
        visited = []
        stack = [node]

        while stack:

            node = stack.pop()
            if node not in visited:
                print node
                visited.append(node)

                for i in self.graph[node]:
                    stack.append(i)

G = Graph()
G.add_edge(1,2)
G.add_edge(1,4)
G.add_edge(1,5)
G.add_edge(2,3)
G.add_edge(3,5)
G.depth_first_search(1)

Classic DFS doesn't use any pruning. That means you should not have a list of visited nodes; Hence, my question in the comments. This means that for cyclic graphs DFS does not terminate by default which is why people commonly do pruning on visited nodes; however, revisiting nodes may be wanted, e.g. "list all paths from edge 1 to edge 5". Making the choice of using visited not only makes your graph acyclic, but rather "tree-ifys" (technical term ;-) ) it.

Assuming visited is an okay thing to do you don't want to check if node not in visited: after poping an element but rather before you insert it. It saves you the overhead of appending and popping visited nodes which can be quite substantial.

Further, visited should be a set (as stated in @Alex 's comment).

It could also be a good idea to use an actual queue object, be that collections.deque (for FIFO / LIFO) or a queue.PriorityQueue. The latter will slightly reduce performance (insert from O(1) to O(log(queue_size))), but offers a lot of added flexibility and easy scalability to graph search.

Setting the priority to: 1/depth is DFS, depth is BFS, sum(depth) (or sum(transition costs)) is Dijkstra's search, expected_cost_to_goal is greedy search, sum(transition costs) + expected_cost_to_goal is A*. I think that's a really cool property.


Finding the longest word without these characters My goal is to go through the list of all English words (separated by '\n' characters) and find the longest word which doesn't have any of these characters:  "gkmqvwxz". And I want to optimize it as much as possible. Here's what I came up with:

#include <string.h>
#include <ctype.h>
#include <stdlib.h>
#include <stdio.h>
#include <stddef.h>
#include <unistd.h>

static inline int is_legal(size_t beg, size_t end, char* buffer)
{
  static const char* bad = "gkmqvwxzio"; /* unwanted chars */
  for (; beg != end; ++beg) {            /* go through current word */
    char ch = tolower(buffer[beg]);      /* The char might be upper case */
    for (size_t j = 0; bad[j]; ++j)
      if (ch == bad[j])                  /* If it is found, return false */
        return 0;
  }

  return 1;                              /* else return true */
}

int main(void)
{
  char *buffer = NULL;                   /* contents of the text file */

  size_t length = 5000000;               /* maximum size */
  FILE* fp;
  fp = fopen("words.txt", "rb");

  if (fp) {
    fseek(fp, 0, SEEK_END);
    fseek(fp, 0, SEEK_SET);
    buffer = malloc(length);
    if (buffer) {
      fread(buffer, 1, length, fp);       /* read it all */
    }
    fclose(fp);
  }

  size_t beg = 0;                         /* current word boundaries */
  size_t end = 0;

  size_t mbeg = 0;                        /* result word */
  size_t mend = 0;

  while (buffer[end]) {
    beg = end++;

    for (; buffer[end] && buffer[end] != '\n'; ++end) /* read the next word */
      ; /* for loop doesn't have a body */

    if ((end - beg) > (mend - mbeg) && is_legal(beg, end, buffer)) { /* if it is a fit, save it */
      mbeg = beg;
      mend = end;
    }
  }

  printf("%.*s\n", mend - mbeg, buffer + mbeg);  /* print the output */
  return 0;
}

I read it all at once, then go through it with two indexes denoting beginning and ending of current word. When I find a word that fits, I save the corresponding indexes. Finally I print the output, which is "supertranscendentness". The output is correct, but I'd like to know:

If there's undefined behavior in my code
If there's a better way of doing this (without sacrificing performance)
If there's a way to improve the performance

Another point is the size_t length = 5000000; part. It is an estimated size of the string based of the file size.

The code is not bad as it stands, but I think there are some things that could be improved.

Think of the user

The input file name and unwanted letters are all hardcoded at the moment.  It would be nice if the user could specify one or both of these parameters on the command line.

Add error handling

There is almost no error checking or handling.  It's not hard to add, and it makes the program much more robust.  Here's how the start of main might look:

int main(int argc, char *argv[]) {
    if (argc != 2) {
        puts("Usage: longword filename");
        return 0;
    }
    FILE* fp;
    fp = fopen(argv[1], "rb");

    if (!fp) {
        perror("couldn't open words file");
        return 3;
    }
    size_t length = 5000000;
    char *buffer = malloc(length);
    if (buffer == NULL) {
        perror("couldn't allocate memory");
        return 2;
    }
    length = fread(buffer, 1, length, fp);
    if (ferror(fp)) {
        perror("couldn't read file");
        free(buffer);
        return 1;
    }
    // rest of program here
    free(buffer);
}

Consider using standard library functions

At a very small performance penalty (as measured on my machine), one could write a very clean version using only standard functions:

char *longest = NULL;
int longestlen = 0;
char *word = strtok(buffer, "\n");
while (word) {
    const int len = strlen(word);
    if (len > longestlen) {
        if (strpbrk(word, "gkmqvwxzio") == NULL) {
            longestlen = strlen(word);
            longest = word;
        }
    }
    word = strtok(NULL, "\n");
}
printf("%s\n", longest);

That is the way I'd probably write it unless there were some compelling reason that's not fast enough.

Use functions

Your is_legal function is not bad, but I'd also write a get_word_len function to fetch the length of the next word in the buffer.

static inline int get_word_len(const char *buff, const char *end) {
    int len = 0;
    for ( ; *buff != '\n' && buff < end; ++buff, ++len)
    {}
    return len;
}

Use const where practical

The is_legal function doesn't alter the passed string, so that parameter should be const.

Think carefully about the problem

The current code might print the word followed by \n, but if the words doesn't happen to be the first in the file, it will also print the \n from the previous word.  It's not necessarily wrong, but it's not consistent.

Use bool for boolean values

The implmentation of bool is in <stdbool.h> and should be used as the return type of is_legal.

Use only the required headers

In this program neither <stddef.h> nor <unistd.h> appear to be needed; I'd recommend omitting them and only including headers that are actually needed.

Consider using pointers

There may not be a performance difference in this case, but for problems like these, the use of pointers seems more natural to me.  For example:

const char *end = buffer + length;
const char *longest = buffer;
int longestlen = 0;

for (const char *curr=buffer; curr < end;  ) {
    const int wordlen = get_word_len(curr, end);
    if (wordlen > longestlen) {
        if (is_good_word(curr, wordlen)) {
            longestlen = wordlen;
            longest = curr;
        }
    }
    curr += wordlen + 1;
}
printf("%.*s\n", longestlen, longest);

Here, is_good_word is like your is_legal function:

static inline bool is_good_word(const char *curr, int wordlen) {
  static const char* bad = "gkmqvwxzio";
  for ( ; wordlen; --wordlen) {
      char ch = tolower(*curr++);
      for (const char *badptr = bad; *badptr; ++badptr) {
          if (ch == *badptr) {
              return false;
          }
      }
  }
  return true;
}

Don't leak memory

The program allocates but does not free the buffer space.  Yes, the operating system will clean up after you, but a free costs very little and allows for better memory leak checking with tools like valgrind.


Project Euler #9 implementation in Java Question:

  Special Pythagorean triplet, Euler Problem 9

  A Pythagorean triplet is a set of three natural numbers, \$a<b<c\$, for which, $$a^2+b^2=c^2$$ For example, \$3^2+4^2=9+16=25=5^2\$.

  There exists exactly one Pythagorean triplet for which \$a+b+c=1000\$.

  Find the product \$abc\$.

I got the code to work using algebraic simplifications shown below.

  Given:

  \$num=1000\$

  \$a+b+c=1000\$

  \$a^2+b^2+c^2+2ab+2bc+2ca=num^2\$

  \$a^2+b^2=c^2\$

Thus:

$$2c^2+2ab+2bc+2ca=num^2$$
$$c(c+b)+a(b+c)=\frac{num^2}{2}=500num$$
$$(a+c)(b+c)=500num$$
$$(num-b)(num-a)=500num$$
$$num-b=\frac{500num}{num-a}$$
$$b=num-\frac{500num}{num-a}$$

So, I scan through \$a\$ and if I receive an integer value for \$b\$ we have found \$b\$, and thus \$c\$ can be found through the equation \$c=num-a-b\$.

The upper limit of \$a\$ is found by assuming that \$a\$ is the smallest among the three integers,  \$a\$, \$b\$, and \$c\$ such that \$a<b<c\$. Following this assumption, let's find the max condition by rewriting \$b\$ and \$c\$ in terms of \$a\$. So, \$b = a+n\$ and \$c = a+k\$ where \$n<k\$, so that \$a<b<c\$ remains true.

Now:

$$a+b+c=num$$
$$a+(a+n)+(a+k)=num$$
$$3a+n+k=num$$
$$3a=num-(n+k)$$
$$a=\frac{num-(n + k)}{3}$$

Now, the max acceptable value of \$a\$ according to the equation could be when \$n=1\$, and \$k=2\$ since \$a<b<c\$. In other words, the difference between the values is 1.

Therefore we loop \$a\$ up to \$\frac{num}{3} - 1\$

My code manages a runtime of ~0.166526 ms:

public class Java {

public static void main(String[] args) {

    double num = 1000.0;
    double b = 0;

    for(int a = 2 ; a <= (num/3 - 1); a++){
          b = ( num - ( 500*num / (num-a)));

          if(b == (int)b){
              System.out.println((int)(a*b*(num-a-b)));
              break;
          }

    }

}

Any suggestions for setting a lower limit for \$a\$ and probably speeding up the algorithm? The upper limit seems odd since I just used \$a+b+c=1000\$ when finding it instead of the Pythagorean Theorem.

There isn't much code, so that leaves little to be reviewed. I have not considered the mathematical side of this. This is also a code stub, so when reviewed out of context it is hard to do a good review.

Comment it – As a code stub it is hard to understand what you are trying to achieve, and as such I would problably leave a comment stating what you actually wanted to achieve. Maybe just a reference to "Special Pythagorean triplet, Euler Problem 9", or similar. And possibly some of the justification of your formulas
Do standard indentation – The class and the method is at the same level, which is confusing. Do indent, but keep it a standard width, like 2 or 4 spaces. The same applies for how many lines you use between statements, ending braces, methods. Keep it clean(er)
Keep computation in loop conditions to a minimum – In your case it will most likely be optimised by the compiler, but having computational stuff in the loop conditions, like (num/3 - 1), should be avoided. Do the calculation once in front of the loop, and make it a simple comparision in the loop condition
Take care when using parenthesis – You have some unneccessary parenthesis, and some which is kind of strange, I would have written it: b = num - (500*num) / (num-a) which emphasizes that the denominator and the dividend are calculated before the actual division
Comparing doubles and int is dangerous – In most languages a double variable is usually not checked directly for equality as the imprecision in the storage format could leave to interesting effects. See Spot the defect: rounding, part two. However this might not apply to you as you are mainly interested in a whole number and the decimal parts of it...
Variable naming – Nothing indicates that num is a constant, neither in the naming nor in the declaration (i.e. you could use final). And what does num mean? Is it a number, is it a constant?
Consider using a function – In the problem description it indicates that for other numbers you could find other pythagorean triples, which could be extended into making a method like findPythagoreanTriplet(number) which could either return the first triplet, or a list of triplets. Using a function would allow for extending the code stub into maybe doing something more useful like finding other triplets matching some criteria, or similar
Computation vs presentation – An alternative view on creating a function, is that you could then split the computation from the presentation, which usually is a good thing to do. This also according to the principle of single concern for a method
Jammed if conditions – The last few days I've seen a lot of if(someCondition){, which in my view would like nicer as if (someCondition) {. Having a little space here and there increases readability, asOpposedToNotHavingSpaces...

Do be aware that this code review is making some assumptions, and might be a little overkill for something which you might just have thrown together to solve an immediate problem you had. However, becoming a robust and good programmer, starts by doing it right from the smallest through to the largest projects.


Zip Code to City, State for registration form Took me a couple of days (in between working on other projects) to get the code working, but now that I do have it working I would like to know if there is a way that I could make it work better, or clean up my act JavaScript/jQuery surrounding my JSON utilization skills.

I am using ASP.NET if that means anything to the JavaScript.

Here is the code, ASPX pertinent to the JavaScript only:

        <asp:Label runat="server" ID="lblCity" Text="City:"></asp:Label>

        <asp:TextBox runat="server" ID="txtCity" CssClass="textbox" Enabled="False" BackColor="#CCCCCC" ></asp:TextBox>

        <asp:Label runat="server" ID="lblState" Text="State:"></asp:Label>

        <asp:TextBox runat="server" ID="txtState" CssClass="textbox" Enabled="False" BackColor="#CCCCCC"></asp:TextBox>

        <asp:Label runat="server" ID="lblZip" Text="Zip Code:"></asp:Label>

        <asp:TextBox runat="server" ID="txtZip" CssClass="textbox" TabIndex="6"></asp:TextBox>

...

I got the idea from SO and various places on the net, but I have hacked the code into this usable piece of script, but is it good code?

Global variables
In your function getAddressInfoByZip, you are declaring quite a number of global variables.
This is bad practice: all these variables should be local variables because they only serve a purpose in this function.
Now, you may not notice that you are creating a bunch of global variables. The evilness is happening in this line:
var street_number = route = street = city = state = zipcode = country = formatted_address = '';

The only local variable that is being created here is street_number; all those other variables are global.
Experiment
Run this in your console:
(function() { var foo = bar = "Hello, World!"; } )();

Then, try this:
console.log(foo);

You will get an error, because now that the anonymous function is out of scope, foo no longer exists.
Now, try this:
console.log(bar);

You will see

Hello, World!

Why? Well, since you created bar in that variable chain, it is created globally because there is no var keyword assigned to it.

This if statement is a little long:
if (types == "sublocality,political" || types == "locality,political" || types == "neighborhood,political" || types == "administrative_area_level_3,political") {

I recommend you split up each conditional by line, like this:
if (types == "sublocality,political" ||
    types == "locality,political" ||
    types == "neighborhood,political" ||
    types == "administrative_area_level_3,political") {

I'm confused by this line:
...document.getElementById('<%=txtCity.ClientID%>')...
Previously, you were using JQuery to interact with elements. Why did you suddenly stop using it here? If you are going to "import" JQuery, you might as well use it.

Inside the body of a function passed to $.getJSON, you are accessing a parameter passed in to the function called results. However, every time you are accessing it, you are accessing the 0th index of the results property of this results parameter.
Why don't you just store that in a variable? It would make your code a lot cleaner in a lot of places, and it increases readability because it doesn't make someone wonder why you are specifically accessing this value each time you want to use it.
Create a variable set to this value:
results.results[0];

I recommend creating an object of constants that are the different types. That way, you won't have to keep writing, for example,
"sublocality,political"

The object only has to be really simple: it's merely to store constants:
var Type = {
    SP: "sublocality,political",
    LP: "locality,political",
    AAL1P: "administrative_area_level_1,political",
    ...
}

Note: I chose to name the identifiers using PascalCase for the object name and SHOUTCASE for the property names because most enumerated types are named like this.
Now, you can easily access a type like this:
Types.LP

This improves the readability of your code by a lot, because it gets rid of all the strings floating around your conditional statements.


Scripts to get label name from ACQ container and modify ACQ label name I have some side projects by using a private module I have. My main goals are to match the list if they started with the pre-defined prefix, then put them into a list. From that filtered list, I extract two elements each time and modify their label name. I am trying to clean up my code to make it more concise and user-friendly, also to make sure asking some questions before actually modify the file label. Ideally, I want to have a way to list down the file's label that is going to edit before actually editing them, if that is possible.

for k, v in sessList.items():
    # get the acquisitions in one session
    acqList = module.get_session_acqs(session_id = k, sort = {'timestamp': 'asc'} )
    conversion(acqList)

import itertools
import numpy as np
from pytz import utc
from datetime import *
from dateutil import *
from datetime import timedelta
from itertools import *
from math import *

def conversion(acqList):

  prefix = ['MCS','FT', 'FS', 'GNo', 'Rs', 'WHw', 'Cy']
  newList = []

#to check if there is any of the items contains the prefix
  for e in acqList:
    for pf in prefix:
      if(e.label.startswith(pf)):
        newList.append(e)

  data = np.arange(len(newList))

  def pairwise(iterable):
    return zip(*[iter(iterable)]*2)

  for v, w in pairwise(data):

    labelA = newList[v].label
    labelB = newList[w].label

#checking when the labelA or labelB has MCS then change the name
    if 'MCS' in labelA:
      newLabel = labelB + '_mc'

      acqID = newList[v].id
#here is when the modify happens from my module
      module.modify_acq(acquisition_id = acqID, label = newLabel )

    elif 'MCS' in labelB:
      newLabel = labelA + '_mc'

      acqID = newList[w].id
#here is when the modify happens from my module
      module.modify_acq(acquisition_id = acqID, label = newLabel )

Indentation

This:

def conversion(acqList):

  prefix = ['MCS','FT', 'FS', 'GNo', 'Rs', 'WHw', 'Cy']
  newList = []

#to check if there is any of the items contains the prefix
  for e in acqList:

is (as a surprise to me) valid syntax, but that doesn't mean it's a good idea. The beginning of a comment, when that comment is the only thing on the line, should match the level of indentation of the block it's in. To do otherwise is pretty confusing.

You also seem to be using two-space indentation, when the standard is four.

If-parens

This:

  if(e.label.startswith(pf)):

should become

if e.label.startswith(pf):

Nested functions

There are sometimes good reasons for nested functions, but so far as I can see, this:

  def pairwise(iterable):
    return zip(*[iter(iterable)]*2)

doesn't have any of them. You're better off moving this function to global scope.

PEP8 spacing

By the PEP8 formatting standard, this:

  module.modify_acq(acquisition_id = acqID, label = newLabel )

would become

module.modify_acq(acquisition_id=acq_id, label=new_label)

Mutability

This:

  prefix = ['MCS','FT', 'FS', 'GNo', 'Rs', 'WHw', 'Cy']

never changes, so it should be made an immutable tuple:

  prefix = ('MCS','FT', 'FS', 'GNo', 'Rs', 'WHw', 'Cy')

Namespace pollution

These:

from datetime import *
from dateutil import *
from datetime import timedelta
from itertools import *
from math import *

have a few problems. First, your timedelta import is redundant, because you already import *.

Also, it's not a great idea to import everything from these modules. You're better off either

importing specific symbols from these modules as necessary, or
importing only the module itself (i.e. import math) and then using fully-qualified references.

Either of those will significantly clean up your namespace.


Unique type ID, no RTTI Following my answer to Unique type ID in C++, I have worked towards a "safer" version that I am posting here.

This is a lightweight type that can store a unique (also across compilation units) ID per type for use at runtime, without RTTI.

Instead of using a built-in type for ID representation that is also accessible to the end user, a specific class type_id_t provides the external representation of the ID and encapsulates the actual storage type (a function pointer) that is not accessible.

A helper function template call type_id<T>() returns a type_id_t containing the ID of type T.

Hence, the following operations are only provided:

construction only by calling type_id function.
copy/move construction, as implicitly defined
copy/move assignment, as implicitly defined
comparison operators == and != between two type_id_t's

There is no default constructor for type_id_t.

Here is the implementation:

class type_id_t
{
    using sig = type_id_t();

    sig* id;
    type_id_t(sig* id) : id{id} {}

public:
    template<typename T>
    friend type_id_t type_id();

    bool operator==(type_id_t o) const { return id == o.id; }
    bool operator!=(type_id_t o) const { return id != o.id; }
};

template<typename T>
type_id_t type_id() { return &type_id<T>; }

It is interesting that function type_id returns a (function) pointer to itself, encapsulated into a type_id_t. No nasty type casting is necessary.

This is not a mechanism that provides runtime identification of a (polymorphic) object's type, but can be used as a low-level tool to build such functionality. I actually made this for the needs of my recent any, and then improved it.

Just for demonstration, we can store the ID of a number of types e.g. in a vector:

template<typename... T>
std::vector<type_id_t>
make_ids() { return {type_id<T>()...}; }

Later, we can compare the stored IDs to the ID of a given type:

template<typename T, typename A>
void comp_ids(const A& a)
{
    for (auto i : a)
        std::cout << (type_id<T>() == i) << " ";
    std::cout << std::endl;
}

and get the following:

auto ids = make_ids<
    int, bool, double const&, int&&, char(&)[8],
    int, void(int), unsigned char**, std::vector<void(*)()>, double[4][8]
>();

comp_ids<int>(ids);  // output: 1 0 0 0 0 1 0 0 0 0

Here is a live example.

Any comments are welcome.

The first thing I notice when looking at this code is the lack of documentation.

I would provide some more operators. Specifically I would go for operator< so type_id_t is usable as key for std::map and you should think about specializing std::hash so the class can be used with hash maps.

As the code is pretty short I don't find any more to say about it.

Btw.: Well done. I like this solution very much.


Hiding and revealing toggle responds very slow I have a list of 6 items, and each one on click reveals a div that runs 100% wide under the row of list items. The code works, but the div opens very slowly and kind of choppy. Is there anything I can do to help it run more smoothly?

My HTML is set up as such:

Here is my jscript:

 $(document).ready(function(){
 $(".artist-box").hide();
$(".artist-logo").click(function(e) {
e.preventDefault();
// get the clicked element
var clicked = $(this);

// get the selected element
var taggedWithSelect = $('.selected-artist');
// get the corresponding divs
var clickPartner = $(clicked.attr('href'));
var selectPartner = $(taggedWithSelect.attr('href'));
var notClicked = $('.artist-logo').not(clicked);

        // we either want to close this one or open this one and close any others
        // if this one is open, it should be tagged with select
if( clicked.hasClass('selected-artist') ) {
    clicked.removeClass('selected-artist');
    notClicked.removeClass('blur-logo');
} else {
  clicked.addClass('selected-artist');
  taggedWithSelect.removeClass('selected-artist');
  selectPartner.slideToggle();
  notClicked.addClass('blur-logo');
  clicked.removeClass('blur-logo');
}

clickPartner.slideToggle();

});

});

All this adding/removing classes are too much for this functionality. This makes it also very slow.

You can iterate over all div elements, and say if div is !== hidden, than hide it.

After that, you get the clicked element with $(this), and just say

$(this).next().slideToggle();

This is not proper code, but since you don't have thousands of divs to iterate over, just hide every open div, and open the clicked one.

Maybe you have the problem of you want to close the already open one, so just make two if's. The first one should be like:

  if($(this).next().style.visible === "true"), hide it, if not, hide
  every visbile div and make just the next to the clicked one visible.

This should be much faster, because you don't add/remove/readd classes.


Repeating a std::string n times I recently posted an answer to this SO question as I did not think any of the current answers were satisfactory (most don't even answer the question!). I was wondering if anyone could offer any improvements on my solution to this problem? I'm most interested in performance here. Also note this algorithm does not require any additional copies of the input string (if passed an r-value), ideally I'd like to keep this property.

#include <string>
#include <cstdint>  // std::size_t
#include <cmath>    // std::floor, std::log2, std::pow
#include <iterator> // std::cbegin, std::cend, std::next

std::string repeatstring(std::string str, std::size_t n)
{
    if (n == 0) {
        str.clear();
        str.shrink_to_fit();
        return str;
    }

    if (n == 1 || str.empty()) return str;

    const auto repeat_size = str.size();

    if (repeat_size == 1) {
        str.append(n - 1, str.front());
        return str;
    }

    str.reserve(repeat_size * n);

    const auto m = static_cast<decltype(n)>(std::floor(std::log2(n)));

    for (auto i = m; i > 0; --i) str += str;

    n -= static_cast<decltype(n)>(std::pow(2, m));

    str.append(std::cbegin(str), std::next(std::cbegin(str), n * repeat_size));

    return str;
}

Here is an example:

#include <iostream>
#include <chrono>

int main()
{
    auto start = std::chrono::system_clock::now();

    auto str = repeatstring("helloworld", 190238011);

    auto end = std::chrono::system_clock::now();

    //std::cout << str << std::endl;
    std::cout << "string length: " << str.length() << std::endl;

    auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(end - start);

    std::cout << "time: " << duration.count() << "ms" << std::endl;

    return 0;
}

Input as reference-to-const

Change your signature to:

std::string repeat_string(std::string const& str, std::size_t N);

First, this avoids an unnecessary copy when users don't pass their strings in by rvalue (which seems more typical?). Secondly, this allows for copy elision on the returned object, as you can't have copy elision from a function parameter. So if I called repeat_string with an lvalue, I just saved two copies.

Also, note the name change. Either use camelCase or snake_case or function names, don't just jamlotsofwordstogether. It's much harder to read.

The new signature makes some of the other code easier.

if (n == 0) {
    return {};
}

...

if (str.size() == 1) {
    return std::string(n str[0]);
}

repeat_size

This is a weird variable at best. Once you make str a reference to const, you can just access str.size() which makes much more sense. repeat_size isn't really the size of the repeat - that's what n is...

Powers of 2

std::log2 and std::pow aren't cheap, and you need neither. For the former, you can just multiply by two until you're done:

std::string result = str;
result.reserve(str.size() * n);

std::size_t m = 2;
for (; m <= n; m *= 2)
{
    result += result;
}

Now m is the first power of 2 larger than n. So you can replace

n -= static_cast<decltype(n)>(std::pow(2, m));

with:

n -= m/2;

And:

str.append(std::cbegin(str), std::next(std::cbegin(str), n * repeat_size));

is a really complicated way of writing what now becomes:

result.append(result.c_str(), n * str.size());


CSRF Token implementation I'm building a website where all data interactions are done over ajax other than the initial page load. Now I discovered the need for CSRF tokens.

I elected to deliver the token in a meta tag like so:

<meta name="csrf" content="JI8qMif9gHuD06m/HByCzg==" />

The function that generates the token is as so:

public function csrf()
{
    return str_replace('+','',base64_encode(openssl_random_pseudo_bytes(16)));
}

All controllers for ajax extend a parent controller called JsonController. The JSON controller handles the check of the CSRF token in its constructor (I'll explain why later):

function __construct()
{
    $app = \StarterKit\App::getInstance();

    //ensure request is ajax
    if(!$app->slim->request->isXhr()){
        $app->halt(403,'Access Denied to Ajax Resource');
    }
    //validate csrf token, then update.
    if($app->slim->request->isPost()){
        $fail = false;
        if(!isset($app->post['csrf'])){
            $fail = true;
        }else{
            if($app->post['csrf'] !== $app->session['csrf']){
                $fail = true;
            }
        }
        if($fail !== false){
            $app->halt(403,'Access Denied');
        }
    }

    if($app->slim->request->isGet()){
        $fail = false;
        if(!isset($app->get['csrf'])){
            $fail = true;
        }else{
            if($app->get['csrf'] !== $app->session['csrf']){
                $fail = true;
            }
        }
        if($fail !== false){
            $app->halt(403,'Access Denied');
        }
    }
    $this->app = $app;
}

Now, for the part where I am still unsure of. How should I return the new CSRF token to the client securely? Json controller has a method called __try() this method's responsibility is to invoke the particular Ajax route (method), capture its response (or an exception) and return a JSON encoded response. It is here in this function that the CSRF token is updated. I assume it is safe to return the CSRF token to the user in the JSON response here?

public function __try($method)
{
    try{
        if(!method_exists($this,$method)){
            throw new \exception('method not found');
        }
        $msg = call_user_func([$this,$method]);
    }
    catch(\exception $e){
        if($this->app->debug === true){
            $err = $e->getMessage().'|'.$e->getFile().'|'.$e->getLine();
        }else{
            $err = $e->getMessage();
        }
        $msg = ['error'=>1,'message'=>$err];
    }
    $msg['csrf'] = $this->app->session['csrf'] = $this->app->csrf();
    echo json_encode($msg, JSON_HEX_QUOT | JSON_HEX_TAG);
}

The JavaScript component of the equation here is insignificant. It basically gets/sets the CSRF token and injects it into the request body of any Ajax request (I've built an API wrapper around $.ajax so this is an automated process).

I've tested it and it works, but I'm dying to know: is this even secure? It seems like it is to me because I tried copying the session ID into Firefox and using it to access the session with no success. I'm curious to know your thoughts.

Yes, you seem to have it correct.

openssl_random_pseudo_bytes(16) is good because it is generating a 64 bit token value for the token, which is more than strong enough.

The

    //ensure request is ajax
    if(!$app->slim->request->isXhr()){
        $app->halt(403,'Access Denied to Ajax Resource');
    }

check is great because this inherently protects against CSRF. This would be checking a header such as X-Requested-With which cannot be sent cross domain without CORS. Therefore it makes it impossible for an attacker to send such a request cross domain for their malicious site.

Remember there is no need to refresh the CSRF token, apart from once per session. It can sometimes hurt usability if you do because it can stop your application working if there are multiple tabs open.

  I assume it is safe to return the CSRF token to the user in the JSON response here?

Yes, because an attacker will not be able to read the response. Of course implementing SSL/TLS so your URLs are encrypted over HTTPS is highly recommended to prevent any Man-In-The-Middle or eavesdropping attacks.


Getting data from the server via JSON callback and parsing the data to HTML I'm pretty new to web development with JS and was wondering if anyone can tell me how I can improve my code. I just feel like when it comes to maintainability my code would be very difficult to update. Can someone advice on some best practices or maybe how they do it?

JavaScript

Here is a typical onready jQuery function. It calls a JSON and goes through the object with .each and parses the data to HTML and appends it the HTML DOM.

$(document).ready(function () {

    var fullday_start = '

Field 1	 Field 2	 Date 	Notes
';

    $.each(eventsData, function () {
        day_render_html_output = fullday_start;
        day_render_html_output += renderFullRow(start, end, names, this.uniqueID);

    });

    $('#output').html(day_render_html_output);
});

Here is an example/attempt of me trying to have some sense of maintainability by creating some JS functions that render outputs in HTML with given data from the JSON that's get called from onready above.

function renderFullRow(startDate, endDate, names, uniqueID) {
    var html_output = '
Day	<select id="type_' + uniqueID + '">' + names + '</select>	';
    html_output += '' + startDate + ' - ' + endDate + '	';
    html_output += '<button type="button" class="accordion-toggle btn btn-default btn-small" data-toggle="collapse" data-parent="#accordion' + uniqueID + '" href="#collapseOne' + uniqueID + '"></button>
';
    html_output += '

<button onclick="clearMsgBox(\'' + uniqueID + '\')" type="button" class="close" data-toggle="collapse" data-parent="#accordion' + uniqueID + '" href="#collapseOne' + uniqueID + '">x</button><textarea id="msg_box_' + uniqueID + '" style="width: 92%;" rows="2" placeholder="Message..."></textarea>

';
    return html_output;
}

function clearMsgBox(ebID){
    $('#msg_box_'+ebID).val('');
}

Could someone enlighten me on how to improve code like this?

The code isn't bad. I think your taking the wrong approach though. Rendering HTML from JavaScript is ugly and as you mentioned, hard to maintain. I'd recommend looking into a client side templating solution such as Handlebars, Mustache, Dust, etc..

This will allow you to write templates rather than appending html to a variable. For example, in handlebars you could do something like:

<table class="table text-left" width="100">
    {{#each eventsData}}
       <tr....
    {{/each}}
</table>

Handlebars will then take the template and data and give you your HTML, which you can then insert into another element.


Building object from unusually custom object Say we have this method which is my main concern for this question. It's really not optimal way to get values out of it. Each time I'm running through list twice for to just get one field value for NotificationSettingsDto

private NotificationSettingsDto notificationSettingsCustomPairToDto(List<CustomPair> notificationSettingsCustomPair) {
    return NotificationSettingsDto.builder()
            .campaignEvents(notificationSettingsCustomPair.stream()
                    .anyMatch(customPair -> customPair.getKey().equals("campaignEvents")) ?
                    (Boolean) notificationSettingsCustomPair.stream().filter(customPair -> customPair.getKey().equals("campaignEvents")).collect(Collectors.toList()).get(0).getValue() : null)
            .drawResultEvents(notificationSettingsCustomPair.stream()
                    .anyMatch(customPair -> customPair.getKey().equals("drawResultEvents")) ?
                    (Boolean) notificationSettingsCustomPair.stream().filter(customPair -> customPair.getKey().equals("drawResultEvents")).collect(Collectors.toList()).get(0).getValue() : null)
            .transactionEvents(notificationSettingsCustomPair.stream()
                    .anyMatch(customPair -> customPair.getKey().equals("transactionEvents")) ?
                    (Boolean) notificationSettingsCustomPair.stream().filter(customPair -> customPair.getKey().equals("transactionEvents")).collect(Collectors.toList()).get(0).getValue() : null)
            .userWonEvents(notificationSettingsCustomPair.stream()
                    .anyMatch(customPair -> customPair.getKey().equals("userWonEvents")) ?
                    (Boolean) notificationSettingsCustomPair.stream().filter(customPair -> customPair.getKey().equals("userWonEvents")).collect(Collectors.toList()).get(0).getValue() : null)
            .build();
}

And I'm passing following List which consists of CustomPair

List<CustomPair> notificationSettingsCustomPair = Arrays.asList(
        new CustomPair<>("campaignEvents", true),
        new CustomPair<>("drawResultEvents", true),
        new CustomPair<>("transactionEvents", true),
        new CustomPair<>("userWonEvents", true));

List can also have different shapes for example like this :

   List<CustomPair> notificationSettingsCustomPair = Arrays.asList(
        new CustomPair<>("campaignEvents", true),
        new CustomPair<>("transactionEvents", false));

Or like this, you get the idea it can be in any order, with any of these 4 different fields:

     List<CustomPair> notificationSettingsCustomPair = Arrays.asList(
        new CustomPair<>("transactionEvents", true));

Model for CustomPair :

public class CustomPair<K, V> {
private K key;
private V value;
}

Model for NotificationSettingsDto :

@Builder
public class NotificationSettingsDto {

private Boolean campaignEvents;
private Boolean drawResultEvents;
private Boolean transactionEvents;
private Boolean userWonEvents;
}

notificationSettingsCustomPair.stream()
                    .anyMatch(cp-> cp.getKey().equals(something)) ?
                    (Boolean) notificationSettingsCustomPair.stream().filter(cp  -> cp.getKey().equals(something)).collect(toList()).get(0).getValue() : null

can be replaced by

notificationSettingsCustomPair.stream().filter(cp  -> cp.getKey().equals(something)).findFirst().map(v -> v.getValue()).orElse(null)

It's now only streamed once.

Please note that you use your list like a Set which I find strange, you should consider looking at Set specificities.


C function to print arrays of arbitrary data type C is naturally clumsy at polymorphism but I wanted to avoid duplication so I wrote this function. But I'm sure there are ways left to improve it. Please not that for brevity's sake not all data types have been included.

void print_int(const void *number) {
    int result = *((int*) number);
    printf("%d ", result);
}

void print_double(const void *number) {
    double result = *((double*)number);
    printf("%f ", result);
}

/// Prints arrays of arbitrary type
/// \param array The array to print
/// \param length The length of the array
/// \param type The data type xxx of the array elements in the form of "TYPE_XXX"
/// \return 1 in case of an invalid data type
int print_array(const void *array, size_t length, char *type) {

    void (*function)(const void *number);
    void *cast_array;
    size_t size;

    if (!strcmp(type, TYPE_INT)) {
        function = print_int;
        cast_array = (int *) array;
        size = sizeof(int);
    } else if (!strcmp(type, TYPE_DOUBLE)) {
        function = print_double;
        cast_array = (double *) array;
        size = sizeof(double);
    } else {
        return 1;
    }

    for (size_t i = 0; i < length; ++i) {
        function(cast_array + i * size);
    }
    printf("\n");
    return 0;
}

First of all this is a good attempt to provide polymorphism for print functionality. Few comments that might help:

I studied the print_array function and we can certainly do few improvements to it.

1) int print_array(const void *array, size_t length, char *type)

The type parameter is defined as char *. Since this function will support only a predefined types its better to have those as enum values. for e.g. you could do a typedef as

typedef enum{
INT = 0,
DOUBLE,

MAX_TYPES,
}SUPPORTED_TYPES;

2) if (!strcmp(type, TYPE_INT))
We can avoid these comparisons by making use of a Map like feature. Your current approach requires changing print_array function for any new type that we add. For e.g. tomorrow if you add support for print_char you will have to make those changes in print_array as well. Wouldn't it be better if print_array required no change. Ok so how to do it. In C++ there is a rule to replace conditional statements with polymorphism or maps. We will use maps here.

Since we already changed the type to an enum we can define a new struct which holds what functions to call for what data-types and the size of data-type as below:

typedef struct{
    void (*function)(const void *number);
    int size_of_elements;
}FunctionMap;

Now we create a mapping for data-types to function and store it in a global variable say print_map

const FunctionMap print_map[] = {
    [INT] = {print_int,sizeof(int)},
    [DOUBLE] = {print_double,sizeof(double)},
};

Now your print_array function changes just to few lines i.e. a lookup into this print_map for the type that is passed.

int print_array(const void *array, size_t length, SUPPORTED_TYPES type)
{

    assert(type<MAX_SUPPORTED);
    assert((sizeof(print_map)/sizeof(FunctionMap)) == MAX_SUPPORTED);

    void (*function)(const void *number);
    const void *cast_array = array;
    size_t size;
    size_t i;
    function = print_map[type].function;
    size = print_map[type].size_of_elements;
    for (i = 0; i < length; ++i) {
        function(cast_array + i * size);
    }
    printf("\n");
    return 0;
}

Note that I have added few asserts to make sure that the user passes proper values for type and that the implementer has add all data-type mappings in print_map. Now whenever you define a new print type say print_char you don't need to change print_array. Just add that function and its mapping in print_map and everything should work as it is.

Hope this helps.


Given two strings, a and b, determine the minimum number of character deletions required to make a and b anagrams I'm trying to figure out how my code could be improved. I solved a HackerRank problem from the Cracking the Coding Interview section, and am thinking that it should be able to be solved a simpler than what I did.

The problem is as follows:

  Strings: Making Anagrams

  Alice is taking a cryptography class and finding anagrams to be very
  useful. We consider two strings to be anagrams of each other if the
  first string's letters can be rearranged to form the second string. In
  other words, both strings must contain the same exact letters in the
  same exact frequency For example, bacdc and dcbac are anagrams,
  but bacdc and dcbad are not.

  Alice decides on an encryption scheme involving two large strings
  where encryption is dependent on the minimum number of character
  deletions required to make the two strings anagrams. Can you help her
  find this number?

  Given two strings, a and b, that may or may not be of the same
  length, determine the minimum number of character deletions required
  to make a and b anagrams. Any characters can be deleted from
  either of the strings.

  Input Format

  The first line contains a single string, a. The second line contains
  a single string, b.

  Constraints

  \$1 \le |a|,|b| \le 10^4\$
  It is guaranteed that a and b consist of lowercase English alphabetic letters (i.e., a through z).

  Output Format

  Pring a single integer denoting the number of characters you must
  delete to make the two strings anagrams of each other.

  Sample Input

cde
abc

  Sample Output

4

  Explanation

  We delete the following characters from our two strings to turn them
  into anagrams of each other:

  Remove d and e from cde to get c.
  Remove a and b from abc to get c.

  We must delete 4 characters to make both strings anagrams, so we print
  4 on a new line.

My Implementation

I am using C++, and since strings are immutable, what I decided to do was to create two int arrays (vectors, actually) that hold the ASCII value of the chars in each string. Then I would sort the vectors. Then I would iterate through the arrays together, counting the number of elements that don't exist in the other.

#include <string>
#include <vector>
#include <iostream>
#include <algorithm>

using namespace std;

int calcDeletions(vector<int> a, vector<int> b) {

    int deletions = 0;
    int ai = 0;
    int bi = 0;

    // step through arrays until the end of one is reached
    while (ai < a.size() && bi < b.size()) {
        if (a[ai] < b[bi]) {
            deletions++;
            ai++;
        }
        else if (a[ai] > b[bi]) {
            deletions++;
            bi++;
        }
        else {
            ai++;
            bi++;
        }
    }

    // carry over left-overs
    deletions += (a.size() - ai);
    deletions += (b.size() - bi);

    return deletions;
}

int number_needed(string a, string b) {
    vector<int> aInt;
    vector<int> bInt;

    // create two int vectors to store ascii val of strings
    for (int i = 0; i < a.length(); i++) {
        aInt.push_back((int)a.at(i));
    }
    for (int i = 0; i < b.length(); i++) {
        bInt.push_back((int)b.at(i));
    }

    // sort vectors
    sort(aInt.begin(), aInt.end());
    sort(bInt.begin(), bInt.end());

    // call calcDeletions function with the longer vector passed as first arg
    if (aInt.size() > bInt.size()) {
        return calcDeletions(aInt, bInt);
    }
    else {
        return calcDeletions(bInt, aInt);
    }
}

int main() {
    string a;
    cin >> a;
    string b;
    cin >> b;

    cout << number_needed(a, b) << endl;
    return 0;
}

What is a better, more efficient way of solving this problem?

I'm trying to figure out how my code could be improved. I solved a HackerRank problem from the Cracking the Coding Interview section, and am thinking that it should be able to be solved a simpler than what I did.

Probably. But these hacker problems are usually very specific. You can probably google a perfect solution in 10 minutes.

  What is a better, more efficient way of solving this problem?

Create a count of the number of each letter from both strings. To be anagrams these counts must be the same. So look across the counts and sum the differences.

  I am using C++, and since strings are immutable,

No they are not. Strings are perfectly mutable.

  what I decided to do was to create two int arrays (vectors, actually) that hold the ASCII value of the chars in each string. Then I would sort the vectors. Then I would iterate through the arrays together, counting the number of elements that don't exist in the other.

The complexity of this is (ignoring the copy to a vector)

O(n1.log(n1))      // Sort first string
O(n2.log(n2))      // Sort second string
O(max(n1,n2))      // Loop Over counting.

That seems to work. I like getting the count of each letter better. The difference in the counts is the number of letters to be deleted.

O(n1)             // Count each character into an array.
O(n2)             // Count each character into an array.
O(1)              // count the diffs.
                  // There is a loop over 255 characters (26 if you only count lower case letters).
                  // This is constant and not dependant on the size of
                  // the input and thus is `1`

Code Review.

Using Namespace

Please stop doing this.

using namespace std;

Its a bad habit that will cause you grief one day. See every other C++ code review for a good explanation.

Pass by const reference

You are passing the parameters by value. This will cause both the vectors to be copied.

int calcDeletions(vector<int> a, vector<int> b) {

Since you don't modify the values it can be made simpler by passing a const reference of each parameter. Thus having no copy.

int calcDeletions(vector<int> const& a, vector<int> const& b) {

Prefer Pre Increment

            deletions++;
            ai++;

Yes. Yes. It makes no difference for integers. But it does no harm to use pre-increment on integers either. But again it's one of those habits you should get into. There are situations where it does make a difference so it's best to pre-increment and then it will always be correct (even if you change the types of your objects).

Prefer operator[] over at()

The difference between the two is that at() performs bounds checking.

    for (int i = 0; i < a.length(); i++) {
        aInt.push_back((int)a.at(i));
    }

In this context you are guaranteed that there is no out of bounds access as you are already checking that i is smaller than a.length(). Thus you are effectively doing the check twice.

Don't use C style casts.

C style casts are hard to spot.

 (int)a.at(i)
 ^^^^^

They are also extremely dangerous (they basically tell the compiler to shut up and do what you're told as I the programmer am god and know better than you the simple compiler). In reality this is usually wrong: the compiler always knows better than you and telling it to shut up is usually hiding an error message.

As a result, C++ has its own casts. There are actually four of them:

static_cast<>()
const_cast<>()
reinterpret_cast<>()
dynamic_cast<>()

Your above cast is actually best done by a static_cast<>()

static_cast<int>(a.at(i));

BUT There is no actual need to cast a char to an integer. This will happen automatically (as a char is an integer type the conversion is automatic) with no loss in precision.

Is there a need to put the largest first.

I don't see you taking advantage of this in the function above!

    // call calcDeletions function with the longer vector passed as first arg
    if (aInt.size() > bInt.size()) {
        return calcDeletions(aInt, bInt);
    }
    else {
        return calcDeletions(bInt, aInt);
    }

Looking at the Cool Shark implementation you provided it still has a couple of issues.


Simple ConfigManager The program below basically reads configuration data from an .ini file in ini format. The exemplary configuration file was also added to this post . Let me know how I can improve it, please.

using Sahara.Core.Logging;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;

namespace Sahara.Core.Config
{
    class ConfigManager
    {
        private readonly string configFilePath;
        private readonly Dictionary<string, string> configItems;
        private readonly LogManager logManager;

        public ConfigManager(string configFile)
        {
            configFilePath = configFile;
            configItems = new Dictionary<string, string>();

            logManager = Sahara.GetServer().GetLogManager();

            try
            {
                if (File.Exists(configFilePath))
                {
                    Stopwatch stopwatch = Stopwatch.StartNew();

                    configItems = File.ReadLines(configFilePath)
                    .Where(IsConfigurationLine)
                    .Select(line => line.Split('='))
                    .ToDictionary(line => line[0], line => line[1]);

                    stopwatch.Stop();
                    logManager.Log("Loaded Config Data [" + stopwatch.ElapsedMilliseconds + "ms]", LogType.Information);
                }
                else
                {
                    logManager.Log("Using config defaults", LogType.Information);

                    configItems.Add("database.host", "localhost");
                    configItems.Add("database.username", "root");
                    configItems.Add("database.password", "");
                    configItems.Add("database.name", "database");
                    configItems.Add("database.port", "3306");
                    configItems.Add("database.max_connections", "10000");
                }
            }
            catch (Exception exception)
            {
                if (logManager != null)
                {
                    logManager.Log("Error in loading config: " + exception.Message, LogType.Error);
                    logManager.Log(exception.StackTrace, LogType.Error);
                }
            }
        }

        private bool IsConfigurationLine(string line)
        {
            return !line.StartsWith("#") && line.Contains("=");
        }

        public string GetConfigElement(string key)
        {
            if (!IsInitialized)
                throw new NotImplementedException("Config data has not get been implemented.");

            if (!configItems.ContainsKey(key))
                throw new KeyNotFoundException("Unable to find key " + key + " in config items dictionary.");
            return this.configItems[key];
        }

        public bool IsInitialized
        {
            get { return configItems != null; }
        }
    }
}

Config File (.ini):

database.host=127.0.0.1
database.username=root
database.password=*********
database.name=******
database.port=3306
database.max_connections=10000

This caught my eye:

    if (!IsInitialized)
        throw new NotImplementedException("Config data has not get been implemented.");

A NotImplementedException is usually a kind of "meta" exception that you typically find in unimplemented method stubs, e.g.

public string GetConfigElement(string key)
{
    throw new NotImplementedException();
}

It's basically ilke a "todo" marker that [intentionally] blows up at runtime:

  You might choose to throw a NotImplementedException exception in properties or methods in your own types when the that member is still in development and will only later be implemented in production code. In other words, a NotImplementedException exception should be synonymous with "still in development."

  MSDN

Throwing that exception as part of the implementation of a method is confusing; in this case the goal is to tell the caller that the current state of the object does not allow using this method yet.

In other words it should be an InvalidOperationException:

  The exception that is thrown when a method call is invalid for the object's current state.

See the remarks section on MSDN for more guidelines on throwing that specific exception.

I find the constructor is doing too many things:

var foo = new ConfigManager(path);

With this single instruction, I've just hit the file system, read the contents of some file, and cached a number of configuration settings in an encapsulated dictionary.

The problem is, if I wanted to write a unit test to document how the GetConfigElement works (and then another to document how it fails), I couldn't, because my test would depend on file I/O.

I'd suggest moving that logic out of the constructor, and delay doing that work.

It would be nice to include an indexer, so client code could do this:

var foo = new ConfigManager(path);
var port = foo["port"];

That said you're reinventing the wheel here, if this is production code you should probably be using the framework-provided XML configuration, using the System.Configuration namespace; you could store your connection strings in a dedicated <ConnectionStrings> section.


Is this a secure way to hash a password? I'm implementing a login system on app engine (I have to, so please don't tell me to use the User service, or an other way to delegate authentication), and I'm wondering whether this setup is secure.

from pbkdf2 import PBKDF2
import os

salt = os.urandom(8)
password = PBKDF2(passphrase, salt).read(32).encode("hex")

Would this be a secure way to store passwords?

Pretty much, yes. PBKDF2 is a well-established algorithm, and os.urandom is a suitable CSPRNG that can be used in salt generation on all major platforms (patched, of course).

Your implementation is also brutally simple. The simpler a system, the more secure it can be. Needless complexity leads to insecurities.

So yes, the system is secure, but it is also subject to Moore's law, just like every other computer system or piece of software. To get around this, the number of rounds (iterations) in strong encryption algorithms is a variable taken into account.

In bcrypt, for example, the number of rounds is 2^workload (default 12), and in PBKDF2 the number of rounds is an int passed to the function. Beware that if you need to use a Cython interface, your password hashing is a strongly blocking call (this has bitten me before)


Updated template class for CMS I'm not doing too well in school, so to pass my final year, and get my degree I need to do an off-school project. The teacher chose to give me the assignment of an A grade CMS, since I've always had a thing with PHP.

Now I've made a new template class for in this particular CMS, but I'm not completely sure about it, so I came here for your help. Please review my code.

Have I used things in a correct way?
Have I used things in a efficient way?
Is there a better way to achieve the same result?

class template
{

    function __construct( $db, $functions )
    {
        $this->db = $db;
        $this->functions = $functions;
    }

    public function pageExist( $id )
    {
        $query = $this->db->connection->prepare( 'SELECT id FROM ' . $this->functions->prefix( 'pages' ) . ' where id = ?' );
        $query->bind_param( 'i', $id );
        $query->execute();
        $query->store_result();
        if ( $query->num_rows > 0 )
        {
            return true;
        }
            else
        {
            return false;
        }
        $query->close();
    }

    public function fetchPage( $id )
    {
        $query = $this->db->connection->prepare( 'SELECT * FROM ' . $this->functions->prefix( 'pages' ) . ' WHERE id = ?' );
        $query->bind_param( 'i', $id );
        $query->execute();
        $result = $query->get_result();
        while ( $row = $result->fetch_assoc() )
        {
            $this->setTemplateData( 'title', $num_rows[ 'title' ] );

        }
        $query->close();
    }

    public function getTheme()
    {
        $theme = 'theme';
        $query = $this->db->connection->prepare( 'SELECT value FROM ' . $this->functions->prefix( 'settings' ) . ' WHERE name = ?' );
        $query->bind_param( 's', $theme );
        $query->execute();
        if ( ! $query )
        {
            throw new Exception( $query->error(), 1 );
        }
        $query->bind_result( $result );
        while ( $query->fetch() )
        {
            return $result;
        }

    }

    public function loadPage()
    {
        $path = './_templates/' . $this->getTheme() . '/';
            $this->content = file_get_contents( $path . 'header.tpl');
            $this->content .= file_get_contents( $path . 'middle.tpl');
            $this->content .= file_get_contents( $path . 'footer.tpl');
    }

    public function createParameter( $key, $value )
    {
        $this->parameter[ $key ] = $value;
    }

    public function langParameters( $key, $value )
    {
        $this->langParameters[ $key ] = $value;
    }

    public function setTemplateData( $key, $value )
    {
        $this->templateData[ $key ] = $value;
    }

    public function parseParameters()
    {
            foreach ($this->parameter as $key => $value)
            {
                $this->content = str_replace( '{' . $key . '}', $value, $this->content);
            }

            foreach ($this->langParameters as $key => $value)
            {
                $this->content = str_replace( '{' . $key . '}', $value, $this->content);
            }

            foreach ($this->templateData as $key => $value)
            {
                $this->content = str_replace( '{' . $key . '}', $value, $this->content);
            }

        echo $this->content;
    }

    public function handlePageLoad( $id )
    {
        if ( $this->pageExist( $id ) )
        {
            $this->fetchPage( $id );
            $this->loadPage();
            $this->parseParameters();
            return true;
        }
        else
        {
            $this->fetchPage( 404 );
            $this->loadPage();
            $this->parseParameters();
            //header('Location: index.php?page=404');
            echo '! <br /> He\'s a true genius.';
        }
    }

}

The class name, why template? Template is much better and follows the naming convention in all sane languages (oook, i know it's PHP :()

Where are $db and $functions? You missed to declare them in the class

class template
{
   private $db;
   private $functions = [];

Your

$query = $this->db->connection->prepare( 'SELECT id FROM ' . $this->functions->prefix( 'pages' ) . ' where id = ?' );

You use prepared statement for where id = ? but now for $this->functions->prefix( 'pages' ) it makes the prepared statement useless.

The same for fetchPage, getTheme.

As user45891 said, your $query->close() will never be executed due to the return. You can improve this with:

$query->close();
return $query->numRows > 0;

This block

if ( ! $query )
{
    throw new Exception( $query->error(), 1 );
}

Great, i love Exceptions but what about a more specific exception? When i write PHP code i check this list to know which exceptions php have. What about UnexpectedValueException or LogicException. LogicException should be used when the error in your code, if $query is false it means you DB is OFF or your question is wrong so the problem should be in your code.

Anyway, your check should go before you use $query! because it will fails with

 $query->bind_param( 's', $theme );

while ( $query->fetch() )
{
   return $result;
}

You need only the first value? Well, what about

$query->fetch();
return $result;

?

Anyway, you don't close $query in every function.

Your handlePageLoad don't return everytime, only if it joins $this->pageExist( $id ) path. Maybe you want to add return false;?

You could improve the this code with this:

public function handlePageLoad( $id )
{
    if ( $this->pageExist( $id ) )
    {
        $this->fetchPage( $id );
        $this->loadPage();
        $this->parseParameters();
        return true;
    }

    $this->fetchPage( 404 );
    $this->loadPage();
    $this->parseParameters();
    //header('Location: index.php?page=404');
    echo '! <br /> He\'s a true genius.';
    return false;
}

The indentation of parseParameters is not coerent with the other code (and ugly :()

As i said above, you missed to declare fields in your class. Do it! You will need to remember what fields you use and what fields your class have (and your IDE will help you more!)

How you handle the case where someone uses the same name for lang and template data?

Your parseParameters feels to be very slow with many arguments... what about RegEx?

Why you use three different arrays parameter, langParameters and templateData (except this which is filled by another method) ? They do the same thing? What about only one array?

You need it separate?

I don't use PHP everyday (only when i need to) so i'm not sure, so don't take it serious:

In getTheme you used $theme = 'theme'; because bind_param wants references ok, but what about &"theme"? I'm not sure, but you could try

Anyway, good luck!


How do I make this simple addition code less verbose? I really don't get why it has to take so much to make such a little function. Here's what I have.

template <typename A, typename B> auto add(A a, B b) -> decltype(a + b) {
   return a + b;
}

Is there a way to shrink this code while maintaining its ability to take different arguments? For a comparison, here is it in Python; there is a big difference between the two.

def add(a, b):
    return a + b

You may want to take a look at Pythy.

What you want seems a lot like:

PYTHY(add, x, y) (
    return x + y;
)

Make sure to read this part of the article, as pointed out by Konrad:

  It appears that we are derefencing a null pointer. Remember in C++
  when dereferencing a null pointer, undefined behavior occurs when
  there is an rvalue-to-lvalue conversion. However, since a
  non-capturing lambda closure is almost always implemented as an object
  with no members, undefined behavior never occurs, since it won't
  access any of its members. Its highly unlikely that a non-capturing
  lambda closure could be implemented another way since it must be
  convertible to a function pointer. But perhaps not?


Algorithm to identify a shift and its type based on on and off times Following code is supposed to identify a shift and then it should identify whether the shift is a night or a day shift. The method will take 'in time' and 'out time' as two parameters.

These are the rules:

Day shift - From 7AM to 7PM
Night shift - From 7PM to 7AM (on the next day)
The employee should report before 8AM for a day shift, otherwise the shift will not be taken into account.
The employee should report before 8PM for a night shift, otherwise the shift will not be taken into account.
If an employee leaves before the regular off time of a shift, that shift will not be taken into account.

This code gives the intended result. Please let me know is the way I've done this acceptable? Do you have any better idea to achieve the same?

public string IdentifyShift(DateTime inTime, DateTime OutTime)
{

    var compareInTimeToOutTIme = inTime.CompareTo(OutTime);

    //Two random dates used to assign value '1 day' to timespan "aDay"
    DateTime from= Convert.ToDateTime("05/01/2014 12:00:00");
    DateTime to=Convert.ToDateTime("05/02/2014 12:00:00");

    TimeSpan aDay = (to - from).Duration();

    //If OutTime is less than the InTime that means the OutTime is on the next day
    if (compareInTimeToOutTIme > 0)
    {
        OutTime = OutTime.Add(aDay);
    }

    TimeSpan shift = (OutTime - inTime).Duration();

    var compareInTimeTo8AM =inTime.CompareTo(Convert.ToDateTime( inTime .ToShortDateString() +" 08:00:00")); //InTime of a day shift should be before 08AM
    var compareOutTimeTo7PM = OutTime.CompareTo(Convert.ToDateTime(inTime .ToShortDateString ()+" 19:00:00")); //OutTime of a day shift should be after or equal to 07PM

    var compareInTimeTo8PM =inTime.CompareTo(Convert.ToDateTime(inTime.ToShortDateString ()+" 20:00:00")); //InTime of a night shift should be before 08AM

    string DayOfnightShiftOutTime= inTime.Add(aDay).ToShortDateString();
    var compareOutTimeTo7AM =OutTime.CompareTo(Convert.ToDateTime(DayOfnightShiftOutTime+" 07:00:00")); //OutTime of a night shift should be after or equal to 07AM on the next day

    if (shift.TotalHours >= 11)
    {
        if ((compareInTimeTo8AM < 0) & (compareOutTimeTo7PM >= 0))
        {
            return "Day Shift";
        }
        else if ((compareInTimeTo8PM < 0) & (compareOutTimeTo7AM >= 0))
        {
            return "Night Shift";
        }
        else
        {
            return "Not a Shift";
        }
    }
    else
    {
        return "Not a Shift";
    }
}

First Things first:

Method Signature:

public string IdentifyShift(DateTime inTime, DateTime OutTime)

C# conventions state, that public methods should be PascalCased (check), and local variables, as well as private fields, should be camelCased (partly). Furthermore your calculations are not depending on anything, but the method itself, they don't access class fields. You can, could and should make it static.

public static string IdentifyShift(DateTime inTime, DateTime outTime)

TimeSpan, one Day:

DateTime from= Convert.ToDateTime("05/01/2014 12:00:00");
DateTime to=Convert.ToDateTime("05/02/2014 12:00:00");

TimeSpan aDay = (to - from).Duration();

Have you read up on the Constructors of TimeSpan? There is one, where you can do the following:

TimeSpan aDay = new TimeSpan(1,0,0,0);

Returning Strings:

Returning strings is.. well It can be horrendous. In your case it's not a good idea: you have 3 Predefined States: DayShift, NightShift, InvalidShift. These can be extracted to an enum structure. This would change your Method signature to the Following:

public static Shift IdentifyShift(DateTime inTime, DateTime outTime)

Given the case you introduce an enum like this:

enum Shift
{
   DayShift, NightShift, InvalidShift
}

Conditions in itself:

  Day shift - From 7AM to 7PM
  Night shift - From 7PM to 7AM (on the next day)
  The employee should report before 8AM for a day shift, otherwise the shift will not be taken into account.
  The employee should report before 8PM for a night shift, otherwise the shift will not be taken into account.
  If an employee leaves before the regular off time of a shift, that shift will not be taken into account.

This gives following conditions, assuming inTime is before outTime (and they are not more than one Day apart):

DayShift: if(7AM <= inTime(hour) < 8AM AND 7PM < outTime(hour))
NightShift: if(7PM <= inTime(hour) < 8PM AND 7AM < outTime(hour))
InvalidShift: all else

Keep in mind, that as soon as the clock shows 08:00:00 / 20:00:00 respectively, that marks the shift as invalid.

Extracting Magic Numbers

You can further clarify the intentions of your Code, if you extract the magic numbers (7AM, 7PM, and so on) to named constants:

const int DAY_SHIFT_START = 7;
const int DAY_SHIFT_DEADLINE = 8;
const int NIGHT_SHIFT_START = 19;
const int NIGHT_SHIFT_DEADLINE = 20;

You can thus simplify your code to the following:

public static Shift IdentifyShift(DateTime inTime, DateTime outTime)
{
    if (inTime >= outTime)
    {
        throw new ArgumentException("inTime is not before outTime");
    }
    if ((outTime - inTime).Duration() > new TimeSpan(1, 0, 0, 0))
    {
        throw new ArgumentException("outTime is more than 24 hours after inTime");
    }

    int inHour = inTime.Hour;
    int outHour = outTime.Hour;
    if (inHour >= DAY_SHIFT_START &&
        inHour < DAY_SHIFT_DEADLINE && outHour > NIGHT_SHIFT_START)
    {
        return Shift.DayShift;
    }
    else if (inHour >= NIGHT_SHIFT_START &&
        inHour < NIGHT_SHIFT_DEADLINE && outHour > DAY_SHIFT_START)
    {
        return Shift.NightShift;
    }
    else
    {
        return Shift.InvalidShift;
    }
}


Calculating the Hamming distance That's an easy one. I just want to calculate the Hamming distance between two strings.

function hamming(str1,str2)
  local distance = 0

  -- cannot calculate Hamming distance if strings have different sizes
  if #str1 ~= #str2 then return false end

  for i = 1, #str1 do
    if str1:sub(i,i) ~= str2:sub(i,i) then
      distance = distance + 1
    end
  end

  return distance
end

Maybe the for-loop isn't the most efficient one. I'm not sure. Any improvements?

There is nothing inefficient about your loop.  You are simply counting from 1 to #str1.  Both bounds are known in advance.  This is as good as it gets with loops in Lua.

The only improvement I could think of was using string.byte instead of string.sub, avoiding the interning of 1-character substrings.  Even then, the difference is negligible, because the overhead of calling a C function (sub or byte) from the interpreter takes most of the CPU time.

Unless you run it under LuaJIT that is.  Under LuaJIT, byte will be inlined into the body of the loop (where is consists only of a bounds check + array access) and the string interning (which requires an allocation + hash table lookup) dominates the CPU time.