how can I improve the check name file part /**
 * Upload has a limit of 10 mb
 * @param string $dir $_SERVER['DOCUMENT_ROOT']
 * @param string $path Path do you want to upload the file
 * @param string $filetype jpg|jpeg|gif|png|doc|docx|txt|rtf|pdf|xls|xlsx|ppt|pptx
 * @param array $_FILES An associative array of items uploaded to the current script via the HTTP POST method.
 * @return string ?$fileName:False
 */
function uploadFiles($dir, $path, $filetype) {
    $dir_base = "{$dir}{$path}";
    $dateadded = date('ynj_Gis-');
    $rEFileTypes = "/^\.($filetype){1}$/i";
    $MAXIMUM_FILESIZE = 10 * 1024 * 1024;

// UPLOAD IMAGES
    $isFile = is_uploaded_file($_FILES['file']['tmp_name']);
    if ($isFile) {
        $safe_filename = $dateadded . preg_replace(array('/\s+/', '/[^-\.\w]+/'), array('_', ''), trim($_FILES['file']['name']));
        if ($_FILES['file']['size'] <= $MAXIMUM_FILESIZE && preg_match($rEFileTypes, strrchr($safe_filename, '.'))) {
            $isMove = move_uploaded_file($_FILES['file']['tmp_name'], $dir_base . $safe_filename);
        }
    }
    if ($isMove) {
        return $safe_filename;
    } else {
        return false;
    }
}

How can I improve the name check part or another things?

I have two minor comments.

First, I prefer having my variables defined in all branches, to avoid the PHP Notice:  Undefined variable:... error in logs. So, an initialization

$isMove = false;

would be nice. Btw, maybe $isMoved seems a bit more accurate/correct name. This part might also be written like this:

if (!is_uploaded_file($_FILES['file']['tmp_name'])) return false;
$safe_filename = $dateadded . preg_replace(array('/\s+/', '/[^-\.\w]+/'), array('_', ''), trim($_FILES['file']['name']));
if ($_FILES['file']['size'] > $MAXIMUM_FILESIZE) return false;
if (!preg_match($rEFileTypes, strrchr($safe_filename, '.'))) return false;
if (!move_uploaded_file($_FILES['file']['tmp_name'], $dir_base . $safe_filename)) return false;
return $safe_filename;

Depending on your taste, this might be easier to read.

My second comment is related to this:

$rEFileTypes = "/^\.($filetype){1}$/i";

What is the purpose of {1} (which means "repeat once")? I'd write that regex like this:

$rEFileTypes = "/\.($filetype)\$/i";

and I would replace

preg_match($rEFileTypes, strrchr($safe_filename, '.'))

with

preg_match($rEFileTypes, $safe_filename)

It seems to me a bit less cluttered this way, and I'd expect such regex to be faster than doing the old one + strrchr, but I have no proof that it really is. However, I don't think this is a kind of code in which speed is very important (upload of a file and moving it around will eat up much more time).

Btw, $filetypes might be a bit better name.

A bit more important comment is regarding the possibility that, given a long enough filename, your safe one will grow too long when you prepend it with a date, and you will lose the file extension (or a part of it). You might want to address that issue by checking the length and trimming the filename part if needed.

Now, for the general approach, there is a question about what you're doing with this. Usually, it is better to keep the original filenames in a database, and to store the uploaded files under a completely generic name (using, for example, an auto_increment primary key from the filenames table). Also, if you don't need filenames at all, you can then just make them generic.

Notice that your $dateadded makes it very likely that your filenames are unique, but it doesn't really guarantee it. Depending on your use and the expected number of users, this might be a potential problem (although I wouldn't expect it).


Cutting rectangle from circle Can the following code snippet be optimized or am I doing it right?

<svg xmlns="http://www.w3.org/2000/svg" width="100" height="100">
<mask id="a">
<circle cx="50" cy="50" r="50" fill="#fff"/>
<rect x="50" width="5" height="100%"/>
</mask>
<rect width="100" height="100" fill="gray" mask="url(#a)" />
</svg>

http://jsfiddle.net/5z4npr5L/6/

I don't want to use a path because I want it readable.

While I don't entirely agree with not using path, I will try to keep my answer unbiased. This is because using a path is much more developer-friendly, and opens up more options to do what you ask.

Keep your code styled, with--that's right--tabs

Using tabs in your code to indent it makes it a lot more readable and allows you to see the structure much more easily. You can see the difference in the following two code examples:

Before:

<svg xmlns="http://www.w3.org/2000/svg" width="100" height="100">
<mask id="a">
<circle cx="50" cy="50" r="50" fill="#fff"/>
<rect x="50" width="5" height="100%"/>
</mask>
<rect width="100" height="100" fill="gray" mask="url(#a)" />
</svg>

After:

<svg xmlns="http://www.w3.org/2000/svg" width="100" height="100">
    <mask id="a">
        <circle cx="50" cy="50" r="50" fill="#fff"/>
        <rect x="50" width="5" height="100%"/>
    </mask>
    <rect width="100" height="100" fill="gray" mask="url(#a)" />
</svg>

XMLNS isn't needed

Unless you're using XHTML, XML, or any other kind of page that requires an XML parser, then XMLNS is not needed. Since inline SVG is a recent development in HTML5, this is never really needed. I'm also going to add that you added the tags HTML and CSS to this, so I'll assume you don't want XMLNS.

Your final code should look something like this:

<svg width="100" height="100">
    <mask id="a">
        <circle cx="50" cy="50" r="50" fill="#fff" />
        <rect x="50" width="5" height="100%" />
    </mask>
    <rect width="100" height="100" fill="gray" mask="url(#a)" />
</svg>

There isn't much more you could optimize without using path. Using path would allow you to use things like fill-rule="evenodd", and many other options.

EDIT:

@somethinghere asked me to expound a bit on how to use the extra features available with path that would allow you to do shape cutouts much more easily.

fill-rule="evenodd"

This can be used on a <path> element and have the <path> element have multiple shapes, if the shape number is even in the path call, it will define an outside point. If it's odd, it'll define an inside point. This allows you to define two path segments, your circle and your rectangle, and have the rect mark the inside point. This is an implementation using fill-rule="evenodd" in your scenario:

<svg width="100" height="100">
    <path d="M50 0 h 5 v 100 h -5 z M0 50 a 50 50 0 1 1 0 0.0001 z" fill-rule="evenodd" fill="gray" />
</svg>

Use a <clipPath>. This works similar to the <mask> tag you used and doesn't need a <path> element either:

<svg width="100" height="100">
    <clipPath id="myClip">
        <rect x="0" y="0" width="50" height="100%" />
        <rect x="55" y="0" width="50" height="100%" />
    </clipPath>
    <circle cx="50" cy="50" r="50" fill="gray" clip-path="url(#myClip)" />
</svg>

You could use a <path> element and define the entire shape if needed, but that seems a bit counter intuitive unless you're using a tool to make the SVG instead of drawing it by hand.


Binary Search Tree in Java Please give me some feedback on how I could make it better or simpler.

public class BinarySearchTree {

    private static TreeNode root;

    public static void main(String args[]) {
        BinarySearchTree bst = new BinarySearchTree();
        root = bst.addItem(root, 10);
        bst.addItem(root, 2);
        bst.addItem(root, 6);
        bst.addItem(root, 3);
        bst.addItem(root, 1);
        bst.addItem(root, 15);
        bst.addItem(root, 12);
        bst.addItem(root, 16);

        System.out.println("** Print BST PRE ORDER **");

        bst.printPreOrderTree(root);

        System.out.println("** Print BST IN ORDER **");

        bst.printInOrderTree(root);

        System.out.println("** Print BST POST ORDER **");

        bst.printPostOrderTree(root);
    }

    private TreeNode addItem(TreeNode root, int item) {
        if (root == null) {
            root = new TreeNode(null, item, null);
            return root;
        } else {
            if (item < root.element) {
                if (root.left == null) {
                    TreeNode node = addItem(root.left, item);
                    root.left = node;
                } else {
                    addItem(root.left, item);
                }
            } else if (item > root.element) {
                if (root.right == null) {
                    TreeNode node = addItem(root.right, item);
                    root.right = node;
                } else {
                    addItem(root.right, item);
                }
            } else {
                System.out.println("Duplicate Item");
            }
        }
        return root;
    }

    private void printPreOrderTree(TreeNode root) {
        if (root != null) {
            System.out.println(root.element);
            printPreOrderTree(root.left);
            printPreOrderTree(root.right);
        }
    }

    private void printInOrderTree(TreeNode root) {
        if (root != null) {
            printInOrderTree(root.left);
            System.out.println(root.element);
            printInOrderTree(root.right);
        }
    }

    private void printPostOrderTree(TreeNode root) {
        if (root != null) {
            printPostOrderTree(root.left);
            printPostOrderTree(root.right);
            System.out.println(root.element);
        }
    }

    private static class TreeNode {
        TreeNode left;
        int element;
        TreeNode right;

        protected TreeNode(TreeNode left, int element, TreeNode right) {
            this.left = left;
            this.element = element;
            this.right = right;
        }
    }
}

static vs instance

Your variable root should probably not be static. Instead, it should be an instance variable so you could have multiple BinarySearchTrees in a single program.

node vs root

In most places, your naming of node vs root is good.
In some places, I found it a bit misleading: In printPreOrderTree(), printInOrderTree() and printPostOrderTree() I'd rather call it node than root. I'd use root only for that type of node for which the system has no parent.

element vs item (vs data?)

I think you can get rid of one of these terms and only use the other. I'd use item and dismiss element. I find element too abstract, the term could also be a supertype of node. I think item describes the payload of a node better than element.
But then again, maybe simply data would be even better?

Simpler Code in addItem()

Since you don't need to remember the variables, you can simplify the code.

Your code:

private TreeNode addItem(TreeNode root, int item) {
    if (root == null) {
        root = new TreeNode(null, item, null);
        return root;
    } else {
        if (item < root.element) {
            if (root.left == null) {
                TreeNode node = addItem(root.left, item);
                root.left = node;
            } else {
                addItem(root.left, item);
            }
        } else if (item > root.element) {
            if (root.right == null) {
                TreeNode node = addItem(root.right, item);
                root.right = node;
            } else {
                addItem(root.right, item);
            }
        } else {
            System.out.println("Duplicate Item");
        }
    }
    return root;
}

Simplified code:

private TreeNode addItem(TreeNode root, int item) {
    if (root == null)
        return new TreeNode(null, item, null);
    else if (item < root.element)
        if (root.left == null)
            root.left = addItem(root.left, item);
        else
            addItem(root.left, item);
    else if (item > root.element)
        if (root.right == null)
            root.right = addItem(root.right, item);
        else
            addItem(root.right, item);
    else
        System.out.println("Duplicate Item");
    return root;
}

And since the recursive addItem() is called anyway and checking root for null anyway, it can be even simpler:

private TreeNode addItem(TreeNode root, int item) {
    if (root == null)
        return new TreeNode(null, item, null);
    else if (item < root.element)
        root.left = addItem(root.left, item);
    else if (item > root.element)
        root.right = addItem(root.right, item);
    else
        System.out.println("Duplicate Item");
    return root;
}

Use stderr instead of stdout for error messages

Your error message

        System.out.println("Duplicate Item");

should not go to stdout but to stderr, I guess.

        System.err.println("Duplicate Item");

Use the Visitor pattern?

In printPreOrderTree, printInOrderTree and printPostOrderTree, you always visit all nodes.

The recursion over the nodes is always the same. What differs is the event placement and the action.

If your code gets more complex and you have more such recursions over the tree, you might want to use the Visitor design pattern to separate the recursion from the action to be performed with the node.


JavaScript library to notify watchers of changes Background

Last year, while I was still entering the beginning of Immutability and Pure functions and whatnot I created a library for the community called obj-watcher.

This library is extremely simple - it allows you to register an object, and to get notified when changed happen to this object via a given API.

Problem

Everything was fine at first.  I got the maximum score for everything and then one day ...  CodeClimate changed the way they evaluate code and they rated my project as a C.

This is not good. According to them, my project is hard to maintain because it has a high cognitive load. I find that hard to believe, but I am totally biased.

What do I want?

The full code is commented via jsdoc and I know I could probably improve some functions. But I what I would really like is some feedback on my code. I would like to know if it is really that hard for someone else to understand ( PS,  removed the comments ):

const isFunction = require("lodash.isfunction");

const errors = require("./errors.js");
const callbackNotAFunction = errors.callbackNotAFunction;
const objectAlreadyWatched = errors.objectAlreadyWatched;
const objectNotWatched = errors.objectNotWatched;

const watcherFactory = () => {

    const watchMap = new Map();

    const watch = (objId, obj) => {
        if (isObjWatched(objId))
            throw objectAlreadyWatched(objId);

        watchMap.set(objId, {
            obj: obj,

            onChange: () => {}
        });
    };

    const unwatch = objId => {
        if (!isObjWatched(objId))
            throw objectNotWatched(objId);
        watchMap.delete(objId);
    };

    const get = objId => {
        if (!isObjWatched(objId))
            throw objectNotWatched(objId);
        return Object.assign({}, watchMap.get(objId).obj);
    };

    const set = (objId, newObj) => {
        if (!isObjWatched(objId))
            throw objectNotWatched(objId);

        const entry = watchMap.get(objId);
        const oldObj = Object.assign({}, entry.obj);
        entry.obj = Object.assign({}, newObj);
        entry.onChange(oldObj, entry.obj);
    };

    const onChange = (objId, callback) => {
        if (!isObjWatched(objId))
            throw objectNotWatched(objId);

        if (!isFunction(callback))
            throw callbackNotAFunction(objId);

        const entry = watchMap.get(objId);
        entry.onChange = callback;
    };

    const reset = () => {
        watchMap.clear();
    };

    const isObjWatched = objName => watchMap.has(objName);

    return Object.freeze({
        watch,
        unwatch,
        onChange,
        get,
        set,
        reset
    });
};

module.exports = watcherFactory();

CodeClimate states this code is hard due to 2 reasons:

Function watcherFactory has 47 lines of code (exceeds 25 allowed). Consider refactoring.
Function watcherFactory has a Cognitive Complexity of 12 (exceeds 5 allowed). Consider refactoring.

Here I am using the well established object factory pattern from Douglas Crockford where I have a function that creates and returns an object with methods.

This function has everything inside and is a standard way of defining public and private variables via means of closures and encapsulation.

However, after reading CodeClimate's article on cognitive complexity ( which I also recommend ):

https://docs.codeclimate.com/docs/cognitive-complexity

I think their system has an issue with factory functions because it seems them as normal function that is far too big. In a way, their system does kinda have a point... I wonder if there is a way I can refactor this to make it more obvious and win both battles.

What do you think?

Note: this ended up catching a typo instead of fixing what I said below: it was not meant to be executed immediately.

Why do you need something that's effectively an IIFE? Can't you just use:

const watchMap = new Map();

const watch = (objId, obj) => {
    if (isObjWatched(objId))
        throw objectAlreadyWatched(objId);

    watchMap.set(objId, {
        obj: obj,

        onChange: () => {}
    });
};

// ...

module.exports = {
    watch,
    unwatch,
    onChange,
    get,
    set,
    reset
};


Sorting array of strings in c Is there a simpler way with a simpler notation to do the following part of code:

void swap(char **s1, char **s2){
    char *tmp=*s1;
    *s1=*s2;
    *s2=tmp;
}

void sort(char ***m, int dim){
    int i, j, flag=1;

    for(i=0; i<dim-1 && flag==1; i++){
        flag=0;
        for(j=0; j<dim-1; j++)
            if(strcmp((*m)[j],(*m)[j+1])>0){
                swap(&(*m)[j],&(*m)[j+1]);
                flag=1;
            }
    }
}

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

    char **m, s[30];
    int i;

    m=malloc(N*sizeof(char *));
    for(i=0; i<N; i++){
        scanf("%s", s);
        m[i]=strdup(s);
    }

    sort(&m, N);
    for(i=0; i<N; i++)
        printf("%s\n", m[i]);

    for(i=0; i<N; i++)
        free(m[i]);
    free(m);

    return 0;
}

The aim is to sort an array of strings allocated dynamically.
What I've written above works but I want to know if there is a more efficient and readable way to write the function sort.

Assuming the logic of your sort code is correct.

You can just have your sort function as:

void sort(char **m, int dim);

Your sort function can be like:

void sort(char **m, int dim) {
    int i, j, flag=1;

    for(i=0; i<dim-1 && flag==1; i++){
        flag=0;
        for(j=0; j<dim-1; j++)
            if(strcmp(m[j],m[j+1])>0){
                swap(&m[j],&m[j+1]);
                flag=1;
            }
    }
}

This improves some readability. There is no need to pass &m - like (sort(&m, N)) - to your sort. Just passing m - like sort(m, N) - is enough for changing/comparing/swapping m[i] and m[j].


JPA Query to Return a Map I wrote the following JPA method that queries a database to return a distinct list of a String field (called mtfcc) of a Location object. I then took the list of strings to query for the associated name for that MTFCC.

Could I just modify the first query to get the results, without having to use the for loop to get the associated name field?

@Override
public Map<String, String> getAvailableBorderTypes() {

    // create empty map to store results in. If we don't find results, an empty hashmap will be returned
    Map<String, String> results = new HashMap<String, String>();

    EntityManager em = entityManagerFactory.createEntityManager();

    // get the distinct mtfcc's
    String jpaQuery = "SELECT DISTINCT location.mtfcc FROM Location location";

    // get the distinct mtfcc codes
    List<String> mtfccList = (List<String>) em.createQuery(jpaQuery).getResultList();

    // cycle through the mtfcc list and get the name to associate with it
    for (String mtfcc: mtfccList) {
        String nameQuery = "SELECT DISTINCT location.name FROM Location location WHERE location.mtfcc = ?1";
        String name = (String) em.createQuery(nameQuery).setParameter(1, mtfcc).getSingleResult();
        results.put(mtfcc, name);
    }

    return results;
}

Ok, I've got a possible simplification for you. The goal I had was to get to using one query:

SELECT DISTINCT location.mftcc, location.name FROM Location location

It turns out that Query.getResultList() behaves funny: It returns a List of Object[]! Each entry in the list represents a row returned from the database, where the entries in the Object[] are each field specified in the SELECT. Also, to furthur complicate matters, each field is returned as the JPA type. So, you have to treat the whole array as an Object[] and cast the individual elements.

Here's the code to get it to work:

public Map<String, String> getAvailableBorderTypes() {
   // create empty map to store results in. If we don't find results, an empty hashmap will be returned
   Map<String, String> results = new HashMap<String, String>();

   EntityManager em = entityManagerFactory.createEntityManager();

   // Construct and run query
   String jpaQuery = "SELECT DISTINCT location.mftcc, location.name FROM Location location";
   List<Object[]> resultList = em.createQuery(jpaQuery).getResultList();

   // Place results in map
   for (Object[] borderTypes: resultList) {
      results.put((String)borderTypes[0], (String)borderTypes[1]);
   }

   return results;
}


C program to create and print a linked list from the command line This program will create a linked list, set the values, and print the list by using command line options.

This is my first implementation of a linked list so I'm not sure if I did it correctly. The available options are:

-a to add nodes.

To add several nodes:-a "23 56 1" will add three nodes with the respective values

-p will print the nodes. It has to be used after -a or else it will result in an error message.

I'm not looking for any specific input, style, comments, functions used, anything.

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

struct node{
    int value;
    struct node *next_ptr;
};

struct node *head;

void add_node(int data);
void print_nodes(void);

int main(int argc, char *argv[]){
    int c;
    long node_value;
    char *ptr;
    char *token;
    const char s[2] = " "; /*Split optarg based on this string*/

    while((c = getopt(argc, argv, "pa:")) != EOF){
        switch(c){
        case 'p':
            print_nodes();
            break;
        case 'a':
            token = strtok(optarg, s); /*Split the string*/
            while(token != NULL){
                node_value = strtol(token, &ptr, 10); /*Convert each string to integer*/
                if((*ptr) != 10 && (*ptr) != 0){ /*If it's not a newline or a null then invalid input*/
                    fprintf(stderr, "Invalid number: %c", *ptr);
                    exit(EXIT_FAILURE);
                }
                add_node(node_value);
                token = strtok(NULL, s);
            }
            break;
        default:
            fprintf(stderr, "Unknown option %c, available options are '-p' and 'a'", c);
            break;
        }
    }
    argc -= optind;
    argv += optind;

    return 0;
}

void add_node(int data){
    struct node *temp = head;

    if(temp == NULL){
        head = malloc(sizeof(struct node));
        head->value = data;
        head->next_ptr = NULL;
        return;
    }

    while(temp->next_ptr != NULL){
        temp = temp->next_ptr;
    }

    if((temp->next_ptr = malloc(sizeof(struct node))) == NULL){
        fprintf(stderr, "Out of memory");
        exit(EXIT_FAILURE);
    }

    temp = temp->next_ptr;
    temp->value = data;
    temp->next_ptr = NULL;
}

void print_nodes(void){
    struct node *temp = head;
    if(temp == NULL){
            printf("Linked list is empty\n");
    }
    for(temp = head; temp != NULL; temp = temp->next_ptr){
        printf("%i\n", temp->value);
    }
}

I want to add more options, sort, search, delete and file integration, but I'd like to make sure this is working correctly first.

Nice formatting, good variable and function names. I learned more about getopt() from the program (I used to have to write my own command line parsers)! Good use of system macros such as EXIT_FAILURE.

Using Typedef

If the code used typedef for the definition of node, the code might be slightly shorter and more readable:

typedef struct node{
    int value;
    struct node *next_ptr;
} Node;

Node *head;

void add_node(int data){
    Node* temp = head;

    /* ... */
}

void print_nodes(void){
    Node* temp = head;

    /* ... */
}

By using typedef a new type is created. This also might decrease the possibility of future errors by forgetting to put the struct in at some point. This stackoverflow question discusses why it might be good to use typedef.

Global Variables

Generally the use of global variables are frowned upon. When creating, reading and debugging code global variables can be affected by side affects and it can be very difficult to find where the problem is actually occurring. This stackoverflow question talks about when it is proper to use global variables.

It might be better if the global variable NODE *head was declared in main() and then passed by reference into each function that modified it, and passed by value into each function that only used it and didn't change it.

Passing head into each of the functions would make the following changed necessary:

        case 'p':
    /*+>*/  print_nodes(head);      /* Pass by value */
            break;

            while(token != NULL){
                node_value = strtol(token, &ptr, 10); /*Convert each string to integer*/
                if((*ptr) != 10 && (*ptr) != 0){ /*If it's not a newline or a null then invalid input*/
                    fprintf(stderr, "Invalid number: %c", *ptr);
                    exit(EXIT_FAILURE);
                }
    /* ++> */   add_node(node_value, &head);        /* Pass by reference */
                token = strtok(NULL, s);
            }

void add_node(int data, Node **head){
    Node* temp;

    if(*head == NULL){
        *head = malloc(sizeof(struct node));
        (*head)->value = data;
        (*head)->next_ptr = NULL;
        return;
    }

    temp = *head;
    while(temp->next_ptr != NULL){
        temp = temp->next_ptr;
    }

    if((temp->next_ptr = malloc(sizeof(struct node))) == NULL){
        fprintf(stderr, "Out of memory");
        exit(EXIT_FAILURE);
    }

    temp = temp->next_ptr;
    temp->value = data;
    temp->next_ptr = NULL;
}

void print_nodes(Node* head){
    Node* temp = head;
    if(temp == NULL){
        printf("Linked list is empty\n");
    }
    for(temp = head; temp != NULL; temp = temp->next_ptr){
        printf("%i\n", temp->value);
    }
}

This example might make the program safer and easier to debug and read.

Implicit Type Conversion

My compiler flagged the following line as an implicit type conversion:

            add_node(node_value);

because node_value is declared as a long rather than int,

    long node_value;

void add_node(int data) { /* ... */

If node_value needs to be a long because that's what strtok() is returning, it might be better to either change the input type for add_node() or to explicitly cast node_value in the call:

            add_node((int) node_value);

The actual warning message I get is implicit conversion loses integer precision: 'long to int' (Xcode 8.2 on El Capitan).

Functions that might be helpful

To implement a full linked list program some functions that might be helpful are:

NodePointer new_node(int value);
NodePointer delete_node_by_value(int value, NodePointer head);
NodePointer delete_node_by_pointer(NodePointer delete_target, NodePointer head);
NodePointer find_node(int value, NodePointer head);
void print_node(int value, NodePointer head);       // called by print_nodes

Use of the exit() Function

The use of the exit() function can be problematic, in a large software system that one is writing only a piece of, it would be better to return an error code from the add_node() function rather than call exit. In some cases such as operating systems calling exit() can have dire
consequences (shut down).

The C programming language was originally created to implement operating
systems and in some cases is still used for that purpose. While C doesn't have the exception throwing capabilities of C++, Java, C# and other more modern languages errors can be handled, either by returning error codes or using setjmp() and longjmp().


Project Euler problems 1 and 2 in python What do you think about my code? I am a beginner and just started doing some algorithmic exercises to sharpen up my python skills.

Here is code for problem #1:

"""If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9.
The sum of these multiples is 23.

Find the sum of all the multiples of 3 or 5 below 1000."""

x = 0

for i in range(1000):
    if i % 3 == 0 or i % 5 == 0:
        x += i

print(x)

Problem 2:

"""Each new term in the Fibonacci sequence is generated by adding the previous two terms.
By starting with 1 and 2, the first 10 terms will be:

1, 2, 3, 5, 8, 13, 21, 34, 55, 89, ...

By considering the terms in the Fibonacci sequence whose values do not exceed four million,
find the sum of the even-valued terms."""

numbers = [1, 2]
total = 0

for i in range(4000000):
    if i == numbers[-1] + numbers[-2]:
        numbers.append(i)

for n in numbers:
    if n % 2 == 0:
        total += n

print(total)

I'm sure there are better ways to solve those problems but would like to see what you think about my quite simple idea.

Problem 1 is probably better and more pythonic when written as a list comprehension:

x = sum(i for i in range(1000) if i % 3 == 0 or i % 5 == 0)
print(x)

For the second problem I would recommend a generator and to use sum again:

def fibonacci(max_n):
    n, prev = 1, 1
    while n <= max_n:
        yield n
        n, prev = n + prev, n

total = sum(n for n in fibonacci(4000000) if n % 2 == 0)
print(total)

This way only two ints are ever in memory (well three if you count max_n). In comparison, your code has a list of possibly large size.


Iterating towards a goal I'm pretty proud of the following, but would welcome any comments from experts:

%% Provide with a Goal and it will iterate towards it.

guess(Goal, Res) :-
    guessIterate(Goal, [], Res).

guessIterate(Goal, Guesses, Res) :-
    makeGuess(Guess),
    check_against_guesses(Guess, Guesses),
    score(Guess, Goal, ThisScore),
    (
        %% if completely correct,...
        ThisScore == [4, 0]
    ->
        Res = Guess
    ;
        write(Guess), write(' '), write(ThisScore),nl,
        NewGuesses = [guess(Guess, ThisScore) | Guesses],
        guessIterate(Goal, NewGuesses, Res)
    ).

/*
If this Guess is correct, it will pedict the right results for previous guesses
*/
check_against_guesses(_, []).
check_against_guesses(Guess, [guess(Code, Score) | TGuesses]) :-
    score(Code, Guess, Score),
    check_against_guesses(Guess, TGuesses).

makeGuess(Guess) :-
    Colours = [red, blue, green, yellow, orange, purple, pink],
    member(G1, Colours),
    member(G2, Colours),
    member(G3, Colours),
    member(G4, Colours),
    Guess = [G1, G2, G3, G4].

/*
Score a guess, [#Black, #White], by first looking for exact matches - black - and then
look for inexaxct matches - white.
It is not possible for a guess of [...red, red...] to match twice with a
single 'red' in the Goal
*/

score(Guess, Goal, Res) :-
    pass1(Guess, Goal, [], Pass1Res),
    pass2(Guess, Pass1Res, Pass2Res),
    countUp(Pass2Res, [0,0], Res).

pass1(_, [], Acc, AccRev) :-
    reverse(Acc, AccRev).

pass1([HGuess | TGuess], [HGoal | TGoal], Acc, Res) :-
    (
        HGuess == HGoal
    ->
        AccNew = [black  | Acc]
    ;
        AccNew = [HGoal | Acc]
    ),
    pass1(TGuess, TGoal, AccNew, Res).

%% check each Guess element with Goal and change Goal element to 'white' if there is a match
pass2([], Goal, GoalRev) :-
    reverse(Goal, GoalRev).

pass2([HGuess | TGuess], Goal, Res) :-
    pass2Helper(HGuess, Goal, [], ModifiedGoal),
    pass2(TGuess, ModifiedGoal, Res).

pass2Helper(_, [], Acc, AccRev) :-
    reverse(Acc, AccRev).

pass2Helper(Guess, [HGoal | TGoal], Acc, Res) :-
    (
        Guess == HGoal
    ->
        reverse(TGoal, TGoalRev),
        %% Res = [white | TGoalRev]
        append(TGoalRev, [white | Acc], Res)
    ;
        AccNew = [HGoal | Acc],
        pass2Helper(Guess, TGoal, AccNew, Res)
    ).

countUp([], Acc, Acc).
countUp([HRes|TRes], [AccB, AccW|_], Res) :-
    (
        HRes == black
    ->
        AccBNew is AccB + 1,
        countUp(TRes, [AccBNew, AccW], Res)
    ;
        (
            HRes == white
        ->
            AccWNew is AccW + 1,
            countUp(TRes, [AccB, AccWNew], Res)
        ;
            countUp(TRes, [AccB, AccW], Res)
        )
    ).

Executive summary:

please_use_very_readable_names insteadOfUnreadableOneslikeInJava.
Use higher-order predicates.
Always consider using DCGs when describing lists. pass1 and pass2 can be simplified a lot with DCGS.
Use format/2 instead of multiple write/1 calls.

Examples:

Higher order predicate maplist/2, replacing 4 almost identical member/2 calls:

make_guess(Guess) :-
    length(Guess, 4),
    Colours = [red, blue, green, yellow, orange, purple, pink],
    maplist(list_member(Colours), Guess).

list_member(Ls, M) :- member(M, Ls).

Sample query and its result:

?- make_guess(Gs).
Gs = [red, red, red, red] ;
Gs = [red, red, red, blue] ;
Gs = [red, red, red, green].

Further example, for counting the number if white elements with the higher-order predicate include/3:

list_num_white(Ls, N) :-
    include(=(white), Ls, Ws),
    length(Ws, N).

Sample query and its result:

?- list_num_white([white,black,white], N).
N = 2

Exercise: Generalize this by parametrizing the element, so that you can use list_element_count(Ls, black, N) and list_element_count(Ls, white, N).

Example for format/2: Instead of:

write(Guess), write(' '), write(ThisScore),nl

you can write:

format("~w ~w\n", [Guess,ThisScore])


Finding all indices of largest value in an array I have the following code that gives me the indices of an array's largest value:

public static void main(String[] args) {

    float[] elementList = new float[7];
    elementList[0] = 8F;
    elementList[1] = 5F;
    elementList[2] = 4F;
    elementList[3] = 3F;
    elementList[4] = 0F;
    elementList[5] = 8F;
    elementList[6] = 6F;
    // findLargestNumberLocations(elementList);
    largestNumbers(elementList);
}

private static void largestNumbers(float[] numbers) {

    float largeNumber = numbers[0];
    for (int i = 0; i < numbers.length; i++) {
        if (numbers[i] > largeNumber) {
            largeNumber = numbers[i];
        }
    }
    for (int j = 0; j < numbers.length; j++) {
        if(largeNumber == numbers[j]){
            System.out.println("largest number index "+j);
        }
    }
}

Is there any optimized way of doing this?

In this case, your performance will not be terrible, but, for really large arrays you may run in to problems.

Also, there is a bug in your code... what if the input array is empty? Then you will get an ArrayIndexOutOfBoundsException on:

float largeNumber = numbers[0];

There is always more than one way to do things, but, I want to post an answer here because one possible solution (which may or may not be faster than your solution depending on data size) has a really close relationship to another CodeReview question posted recently. When I answered that question I could only think of artifical examples of when it was 'right' to use pre/post-increments. This is a good example of when....

See this answer here: Is it bad practice to increment more than one variable in a for loop declaration?

So, here is a good example of how and when a count and an index are related in a for loop and the way they can be incremented. I have some notes at the end:

private static int[] findLargeNumberIndices(float[] numbers) {

    // create an array of at least 8 members.
    // We may need to make this bigger during processing in case
    // there's more than 8 values with the same large value
    int[] indices = new int[Math.max(numbers.length / 16 , 8)];
    // how many large values do we have?
    int count = 0;
    // what is the largest value we have?
    float largeNumber = Float.NEGATIVE_INFINITY;
    for (int i = 0; i < numbers.length; i++) {
        if (numbers[i] > largeNumber) {
            // we have a new large number value... reset our history....
            largeNumber = numbers[i];
            // setting count to zero is enough to 'clear' our previous references.
            count = 0;
            // we know there's space for at least index 0. No need to check.
            // note how we post-increment - this is a 'pattern'.
            indices[count++] = i;
        } else if (numbers[i] == largeNumber) {
            // we have another large value.
            if (count == indices.length) {
                // need to make more space for indices... increase array by 25%
                // count >>> 2 is the same as count / 4 ....
                indices = Arrays.copyOf(indices, count + (count >>> 2));
            }
            // again, use the post-increment
            indices[count++] = i;
        }
    }
    // return the number of values that are valid only.
    return Arrays.copyOf(indices, count);
}

Notes about this code:

The presentation (System.out.println(...)) is now outside the method.
It allocates a 'small' array to store the indexes. This array will grow if needed.
I picked 'good enough` initial size for the array.... but some playing with the numbers may help performance.
There is no real reason to believe that this code is faster than the two loops - creating the array may be more expensive than the second loops.
note how the i index is incemented in the for-loop, but the `count` is post-incremented each time it is used in the array.


Singleton holding and notifying registered listeners Suppose I am listening for network changes with the following listener:

interface NetListener {

    void onNetworkAvailable(boolean isWifi);

    void onNetworkUnavailable();
}

I want to have an object, accessible from anywhere within the app, holding the registered NetListeners, that can trigger the appropriate methods when necessary. The solution I came up with is this "singleton":

class NetListenerManager {

    private static List<NetListener> _listeners = new CopyOnWriteArrayList<NetListener>();

    static void addListener(final NetListener listener) {
        if (null != listener) { _listeners.add(listener); }
    }

    static void removeListener(final NetListener listener) {
        if (null != listener) { _listeners.remove(listener); }
    }

    /**
     * Called when a network connection is available.
     */
    static void notifyOnNetworkAvailable(boolean isWifi) {
        for (NetListener listener : _listeners) { listener.onNetworkAvailable(isWifi); }
    }

    /**
     * Called when a network connection becomes unavailable.
     */
    static void notifyOnNetworkUnavailable() {
        for (NetListener listener : _listeners) { listener.onNetworkUnavailable(); }
    }

    private NetListenerManager() { /* Instantiation disabled. */ }

}

So whenever e.g. a network change broadcast is received, I call NetListenerManager.notifyOnNetworkAvailable or NetListenerManager.notifyOnNetworkUnavailable,
depending on the network state.

I’m wondering if this is a good solution or if there are better ways to implement this.
Thanks!

UPDATE no.2

This is the modified listener:

interface NetListener {

    static enum NetworkType {NO_NETWORK, WIFI, MOBILE}

    void onNetworkChange(NetworkType type);

}

And the manager:

public final class NetListenerManager {

    private static final List<NetListener> NET_LISTENERS = new CopyOnWriteArrayList<NetListener>();
    private static final BlockingQueue<NetListener.NetworkType> QUEUE = new LinkedBlockingQueue<NetListener.NetworkType>();

    private static final Thread QUEUE_PROCESSOR = new Thread(new QueueProcessor());
    static { QUEUE_PROCESSOR.start(); }

    public static void addListener(final NetListener listener) {
        if (null != listener) { NET_LISTENERS.add(listener); }
    }

    public static void removeListener(final NetListener listener) {
        if (null != listener) { NET_LISTENERS.remove(listener); }
    }

    /**
     * Called when there is a network change.
     */
    public static void notifyOnNetworkChange(final NetListener.NetworkType type) {
        QUEUE.add(type);
    }

    private NetListenerManager() { /* Instantiation disabled. */ }

    /**
     * This Runnable propagates network change events to the registered NetListener listeners in a queue.
     */
    private static final class QueueProcessor implements Runnable {
        @Override
        public void run() {
            while (true) {
                try {
                    final NetListener.NetworkType type = QUEUE.take();
                    for (NetListener listener : NET_LISTENERS) { listener.onNetworkChange(type); }
                } catch (InterruptedException ie) { /* Do nothing */ }
            }
        }
    }

}

(Disclaimer: I posted this as a comment first but feel like it should also be an answer)

You should consider to use enum as a singleton as proposed by Joshua Blochs in Effective Java. This will protect you from both serialization attacks and reflection attacks on your singleton instance.

Here is a post which discusses it a lot more in-depth.
https://stackoverflow.com/questions/70689/what-is-an-efficient-way-to-implement-a-singleton-pattern-in-java/71399#71399


AsyncTask alternative with Runnable and CountdownLatch What is wrong with this pattern which emulates AsyncTask functionality?:

// on the UI thread
final CountDownLatch latch = new CountDownLatch(1);
onPreexecuteWrapper();
new Thread(new Runnable() {
        doInBackgroundWrapper();
        latch.countDown();
    }
}).start();

new Thread(new Runnable() {
    @Override
    public void run() {
        try {
                latch.await();
        } catch (InterruptedException e) {
            Logger.error(TAG, "Not good.");
        }

    context.runOnUiThread(new Runnable() {
        @Override
        public void run() {
            onPostExecuteWrapper();
        }
    });
}
}).start();

Here's what I think is wrong:

Two threads -> more resources (I didn't compare actually, just guessing).
No progress update functionality.
Appears to be stinky.

First thing that is wrong is that you are reinventing a wheel.

Second why not ditch the CountDownLatch and do context.runOnUiThread directly in the first runnable instead?

// on the UI thread

onPreexecuteWrapper();
new Thread(new Runnable() {
    @Override
    public void run() {
        doInBackgroundWrapper();
        context.runOnUiThread(new Runnable() {
            @Override
            public void run() {
                onPostExecuteWrapper();
            }
        });
    }
}).start();

Last thing is that it's better to use an Executor and ThreadPool than to spawn new threads.


Fisher-Yates shuffle in Scheme There are many ways to shuffle lists; for example, Racket's built-in shuffle assigns each list element with a random number as a sort key, then sorts the list, which provides an O(n log n) runtime. However, I'd like something that's O(n), and that's where Fisher-Yates comes in.

Fisher-Yates is designed for use with random-access data structures, like vectors. It will not behave efficiently for lists, unless copied to a vector first. That's what my implementation does. However, I micro-optimised my version in a couple of ways:

Instead of doing the normal swap (val = v[j]; v[j] = v[i-1]; v[i-1] = val), I simply do the equivalent of val = v[j]; v[j] = v[i-1]; yield val. I don't care about the values of the vector after i-1 at each iteration, so there's no point in updating the value of v[i-1]. I've described this implementation strategy here.
In accumulating each shuffled value into the result list directly, I skip the vector->list call.

Of course, these optimisations are minor; the runtime is still O(n). Anyway, here's the code (requires SRFI 27 for the random-integer procedure):

(define (fisher-yates-shuffle lst)
  (define vec (list->vector lst))
  (let loop ((result '())
             (i (vector-length vec)))
    (if (zero? i)
        result
        (let* ((j (random-integer i))
               (val (vector-ref vec j)))
          (vector-set! vec j (vector-ref vec (- i 1)))
          (loop (cons val result) (- i 1))))))

I'm looking for ways to make the code faster and/or more elegant, but correctness and performance are my top priorities. If there are ways to write this in a better style without impacting performance or correctness, I'd love to hear about those too.

My code was initially written in Racket, which you're welcome to critique too:

(define (fisher-yates-shuffle lst)
  (define vec (list->vector lst))
  (for/fold ((result '()))
            ((i (in-range (vector-length vec) 0 -1)))
    (define j (random i))
    (define val (vector-ref vec j))
    (vector-set! vec j (vector-ref vec (sub1 i)))
    (cons val result)))

(Note: using for/fold instead of for/list is a micro-optimisation: I don't care if the shuffled list is reversed, so using for/fold skips the reverse step that for/list uses behind the scenes.)

Your answer makes an implicit assumption:

  Racket's built-in shuffle assigns each list element with a random number as a sort key, then sorts the list, which provides an O(n log n) runtime. However, I'd like something that's O(n), and that's where Fisher-Yates comes in.

That assumption is that O(n) will necessarily be faster than O(n log n). I don't think that's true for this particular case.

Considering you referenced Racket's shuffle by name, I figured I'd do some simple profiling using that function compared to your implementation. Just for completeness, I decided to benchmark both of your shuffle implementations. I've renamed the version that uses for/fold to fisher-yates-shuffle*.

All of these procedures operate on lists, and since they're not doing any sorting or anything like that, I just decided to test them on lists generated using Racket's range function. My test harness ended up looking like this:

(define lst (range 10000))

(define (shuffle-lst shuffle-proc)
  (for ([i (in-range 10000)])
    (shuffle-proc lst)))

Using Racket's for, the result is discarded, so the only thing being profiled is the algorithm itself. I then used the following code to actually time the three procedures:

(time (shuffle-lst shuffle))
(time (shuffle-lst fisher-yates-shuffle))
(time (shuffle-lst fisher-yates-shuffle*))

I adjusted the length of the list being shuffled as well as the number of times it was shuffled in a few different ways to figure out how the algorithms interacted with different sizes of lists.

I did some initial, relatively simple benchmarking, and it seemed to indicate that your algorithm was basically never faster than simply shuffling a list. This seemed unlikely to me for a variety of reasons, so I decided to optimize everything to give your algorithm as wide a benefit as I could possibly give it.

I compiled the file using Racket's bytecode compiler and ran it with all optimizations enabled. Here were my results:

==============================================================================
|           list length |       100 |   1,000 | 10,000 | 100,000 | 1,000,000 |
|-----------------------|-----------|---------|--------|---------|-----------|
|       loop iterations | 1,000,000 | 100,000 | 10,000 |   1,000 |       100 |
|=======================|===========|=========|========|=========|===========|
|               shuffle |    11,585 |  11,929 | 12,020 |  12,394 |    16,119 |
|-----------------------|-----------|---------|--------|---------|-----------|
|  fisher-yates-shuffle |     9,676 |   9,816 |  9,888 |  10,730 |    13,285 |
|-----------------------|-----------|---------|--------|---------|-----------|
| fisher-yates-shuffle* |    10,738 |  11,112 | 11,162 |  11,895 |    14,978 |
==============================================================================

There we go. Now fisher-yates-shuffle seems to be coming out on top. That seems sort of odd, though... why did it all suddenly reverse, and why is the manual loop version so much faster than the one using for loops?

Actually, I realized there was another factor in play—the random number generator. This is a rather odd factor to even matter at all. Why would Racket's random be different from its own implementation of random-integer from SRFI 27? The answer is: I don't know.

Either way, normalizing all the implementations to use random instead of random-integer, and I got some slightly different times.

==============================================================================
|           list length |       100 |   1,000 | 10,000 | 100,000 | 1,000,000 |
|-----------------------|-----------|---------|--------|---------|-----------|
|       loop iterations | 1,000,000 | 100,000 | 10,000 |   1,000 |       100 |
|=======================|===========|=========|========|=========|===========|
|               shuffle |    12,191 |  12,178 | 11,497 |  12,733 |    16,763 |
|-----------------------|-----------|---------|--------|---------|-----------|
|  fisher-yates-shuffle |    11,313 |  11,601 | 10,831 |  12,559 |    16,002 |
|-----------------------|-----------|---------|--------|---------|-----------|
| fisher-yates-shuffle* |    11,441 |  11,269 | 10,814 |  12,716 |    15,766 |
==============================================================================

Obviously, none of this is conclusive evidence one way or the other. That said, I think some valid conclusions can be drawn.

The difference between your implementation of the Fisher-Yates shuffle and the default shuffle built-in to Racket is pretty trivial. I find it unlikely that any use-case would have shuffling as a bottleneck, of all things.
The performance differences likely vary wildly across implementations. Scheme is really just a standard, and micro-optimizing for "Scheme" doesn't really make sense unless you have a specific implementation you can micro-optimize for.
Any benchmarks are just as likely to be representative of your sorting algorithm as they are of your random number generator. Pseudorandom number generation is not free, and the higher quality generator you use, the more time it will take.

My advice? Don't worry about it. The built-in shuffle is fine, and it's standard. Use it.

Oh, your code? It's fine, I guess.


Generate a unique numeric identifier for each instance of a class I want to generate a unique ID for every instance of a given class.

The ID is of type unsigned long
When an instance is deleted, the ID is freed (can be reused)

Now, I've come up with the following implementation:

Class Foo
{
public:

  static unsigned std::set<unsigned long> s_usedID;

  static unsigned long generateID() // Generate a valid ID
  {
    static unsigned long id = 0;

     while (Foo::isIDUsed(id)) // If all ID are taken, create an infinite loop!
        ++id;

     return id;
  }

  static void addID(unsigned long id) // Add a given ID to the set of used ID
  {
    s_usedID.insert(id);
  }

  static void removeID(unsigned long id) // Remove a given ID from the set of used ID
  {
    s_usedID.erase(id);
  }

  static bool isIDUsed(unsigned long id)
  {
    return s_usedID.count(id) == 1 ? true:false;
  }

  explicit Foo() : m_id(Foo::generateID())
  {
    Foo::addID(m_id); // ID is now taken
  }

  virtual ~Foo()
  {
    Foo::removeID(m_id); // Free the ID
  }

private:

  unsigned long m_id;

};

Now for me, this logically works.

Is there a better way to implement this?
Are there any optimizations that I could make?

A few remarks:

The biggest obvious issue is the violation of encapsulation. All methods which allow you to modify the id set are public which means that outside code can change it and break the inherent assumption that each instance will have a unique id.

The only code which should be able to modify the id set are the constructor and destructor.

If you now say "ah, that won't matter it's just my code and I know not to do this" - programming is a skill and like any skill you get better at it through practice. Which typically means writing code. Therefore you should take any opportunity to practicing writing good code since that helps to form good habits. There is no excuse for being sloppy.
This:

return s_usedID.count(id) == 1 ? true:false

can be written more succinctly as:

return s_usedID.count(id) == 1;

The implementation is obviously not thread-safe. Just something to be aware of.
One way to improve it, is to just use a variable to store the current maximum id and return the next one higher if you need a new one. If an id gets freed up then you could just not worry about it.
If you really want to re-use returned id's then store them in a returnedIds set. The basic logic then becomes:

If returnedIds has any elements pop first element from set and return it
Else new id becomes current_max_id + 1


Log file name adjustment I have the following code that takes user input "path" and adds the current time into the filename after the name part and before any extension (if it exists).
It seems a little hacky to me just looking at it. What do you think?
#include <string.h>
#include <stdio.h>
#include <libgen.h>

int main() {
    char buf1[256] = {0};
    char path[] = "/var/log/mine.log";
    char *tmp = NULL;
    char time[] = "20201110120305";
    char *dir = NULL;
    char *base = NULL;
    char *file = NULL;
    char *ext = NULL;
    //split path to base and dir
    tmp = strdup(path);
    dir = dirname(tmp);
    tmp = strdup(path);
    base = basename(tmp);
    //split base to file and ext
    tmp = strchr(base,'.');
    if (tmp) {
        ext = strdup(tmp);
        file = strndup(base,(strlen(base)-strlen(ext)));
    }
    else
        file = strdup(base);
    // concat dir / file / time / ext
    if (dir)
        strcat(buf1,dir);
    strcat(buf1,"/");
    if (file)
        strcat(buf1,file);
    strcat(buf1,"-");
    if (time)
        strcat(buf1,time);
    if (ext)
        strcat(buf1,ext);
    printf("%s\n",buf1);
}

This will output /var/log/mine-20201110120305.log

Okay, you're doing a lot of [unnecessary] strdup. If we put all the code into a reusable function of its own, you don't free any of the strings, so you are leaking memory.
One way to alleviate that is to use strdupa instead of strdup. This does an alloca to get a stack frame allocation, so these strings don't have to be freed [they disappear with the stack frame when the function returns].
In fact, with better use of pointers, there's really no need to duplicate these intermediate strings at all.
And, there's still a lot of extra scanning/rescanning and strcpy/strcat.
Stylistically, tmp isn't too descriptive. And, it gets reused for two different purposes.

I've refactored your program to put the code into functions. I've added a test framework. And, I've created three additional versions that show what I would do to simplify and speed things up:
// logtod.c -- add timestamp to log file name

#define _GNU_SOURCE
#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <libgen.h>
#include <stddef.h>
#include <time.h>

int opt_e;
int opt_n;

time_t stamptod;                        // previous time
int stamplen;                           // length of stampbuf
char stampbuf[100];                     // timestamp string

// todget -- generate timestamp
// RETURNS: stamp in stampbuf and length in stamplen
void
todget(time_t tod)
{
    struct tm *tm;

    if (tod == 0)
        tod = time(NULL);

    do {
        // use cached buffer [if possible]
        if (tod == stamptod)
            break;
        stamptod = tod;

        tm = localtime(&tod);

        stamplen = sprintf(stampbuf,"-%4.4d%2.2d%2.2d%2.2d%2.2d%2.2d",
            tm->tm_year + 1900,tm->tm_mon + 1,tm->tm_mday,
            tm->tm_hour,tm->tm_min,tm->tm_sec);
    } while (0);
}

// logof_orig -- original code [leaks memory]
char *
logof_orig(char *buf1,const char *path,time_t tod)
{
    char *tmp = NULL;
    char *dir = NULL;
    char *base = NULL;
    char *file = NULL;
    char *ext = NULL;

    // get timestamp
    todget(tod);

    // split path to base and dir
    tmp = strdup(path);
    dir = dirname(tmp);
    tmp = strdup(path);
    base = basename(tmp);

    // split base to file and ext
    tmp = strchr(base,'.');
    if (tmp) {
        ext = strdup(tmp);
        file = strndup(base,strlen(base) - strlen(ext));
    }
    else
        file = strdup(base);

    buf1[0] = 0;

    // concat dir / file / time / ext
    if (dir)
        strcat(buf1,dir);
    strcat(buf1,"/");
    if (file)
        strcat(buf1,file);
    strcat(buf1,stampbuf);
    if (ext)
        strcat(buf1,ext);

    return buf1;
}

// logof_alloca -- original code [using strdupa et. al]
char *
logof_alloca(char *buf1,const char *path,time_t tod)
{
    //char buf1[1024] = { 0 };
    char *tmp = NULL;
    char *dir = NULL;
    char *base = NULL;
    char *file = NULL;
    char *ext = NULL;

    // get timestamp
    todget(tod);

    // split path to base and dir
    tmp = strdupa(path);
    dir = dirname(tmp);
    tmp = strdupa(path);
    base = basename(tmp);

    // split base to file and ext
    tmp = strchr(base,'.');
    if (tmp) {
        ext = strdupa(tmp);
        file = strndupa(base,strlen(base) - strlen(ext));
    }
    else
        file = strdupa(base);

    buf1[0] = 0;

    // concat dir / file / time / ext
    if (dir)
        strcat(buf1,dir);
    strcat(buf1,"/");
    if (file)
        strcat(buf1,file);
    strcat(buf1,stampbuf);
    if (ext)
        strcat(buf1,ext);

    return buf1;
}

// logof_stpcpy -- original code [using stpcpy vs. strcat]
char *
logof_stpcpy(char *buf1,const char *path,time_t tod)
{
    char *tmp = NULL;
    char *dir = NULL;
    char *base = NULL;
    char *file = NULL;
    char *ext = NULL;

    // get timestamp
    todget(tod);

    // split path to base and dir
    tmp = strdupa(path);
    dir = dirname(tmp);
    tmp = strdupa(path);
    base = basename(tmp);

    // split base to file and ext
    tmp = strchr(base,'.');
    if (tmp) {
        ext = strdupa(tmp);
        file = strndupa(base,strlen(base) - strlen(ext));
    }
    else
        file = strdupa(base);

    buf1[0] = 0;

    // concat dir / file / time / ext
    char *dst = buf1;
    if (dir)
        dst = stpcpy(dst,dir);
    dst = stpcpy(dst,"/");
    if (file)
        dst = stpcpy(dst,file);
    dst = stpcpy(dst,stampbuf);
    if (ext)
        dst = stpcpy(dst,ext);

    return buf1;
}

// copyout -- fill in destination buffer
char *
copyout(char *buf1,const char *path,int pathlen,time_t tod,
        const char *base,const char *ext)
{
    char *dst;
    ptrdiff_t cpylen;

    // get timestamp
    todget(tod);

    // get enough buffer space
    if (buf1 == NULL)
        buf1 = malloc(pathlen + stamplen + 1);

    dst = buf1;

    // ext must be to the right of base
    do {
        if (ext == NULL)
            break;
        if (base == NULL)
            break;
        if (ext >= base)
            break;
        ext = NULL;
    } while (0);

    // copy over all but extension
    if (ext != NULL)
        cpylen = ext - path;
    else
        cpylen = pathlen;
    dst = mempcpy(dst,path,cpylen);

    // copy over the date stamp
    dst = mempcpy(dst,stampbuf,stamplen);

    // copy over the extension
    if (ext != NULL) {
        cpylen = &path[pathlen] - ext;
        dst = mempcpy(dst,ext,cpylen);
    }

    *dst = 0;

    return buf1;
}

// logof_fix1 -- single malloc version
char *
logof_fix1(char *buf1,const char *path,time_t tod)
{

    // get length of path
    size_t pathlen = strlen(path);

    // find "base" in "/dir/base"
    const char *base = strrchr(path,'/');
    if (base != NULL)
        ++base;

    // find the file extension
    const char *ext;
    if (base != NULL)
        ext = strchr(base,'.');
    else
        ext = strrchr(path,'.');

    buf1 = copyout(buf1,path,pathlen,tod,base,ext);

    return buf1;
}

// logof_fix2 -- single pass scan
char *
logof_fix2(char *buf1,const char *path,time_t tod)
{
    const char *ext = NULL;
    const char *base = path;

    // find "base.ext" and ".ext" in "/dir/base.ext"
    const char *src = path;
    for (int chr = *src++;  chr != 0;  chr = *src++) {
        switch (chr) {
        case '/':
            base = src;
            ext = NULL;
            break;
        case '.':
            if (ext == NULL)
                ext = src - 1;
            break;
        }
    }

    // full length of path
    ptrdiff_t pathlen = (src - 1) - path;

    // get enough buffer space
    buf1 = copyout(buf1,path,pathlen,tod,base,ext);

    return buf1;
}

// logof_fix3 -- single pass scan [using strcspn]
char *
logof_fix3(char *buf1,const char *path,time_t tod)
{
    const char *ext = NULL;
    const char *base = path;

    // find "base.ext" and ".ext" in "/dir/base.ext"
    const char *src = path;
    while (1) {
        int len = strcspn(src,"/.");

        // point to start of separater
        src += len;

        // get the character
        int chr = *src;

        // end of string
        if (chr == 0)
            break;

        // separater is at the start of the source string
        if (len == 0) {
            src += 1;
            continue;
        }

        switch (chr) {
        case '/':
            base = src + 1;
            ext = NULL;
            break;
        case '.':
            if (ext == NULL)
                ext = src;
            break;
        }
    }

    // full length of path
    ptrdiff_t pathlen = src - path;

    buf1 = copyout(buf1,path,pathlen,tod,base,ext);

    return buf1;
}

// fixname -- compensensate for dirname changing "foo" into "./foo"
const char *
fixname(const char *out)
{

    if ((out[0] == '.') && (out[1] == '/'))
        out += 2;

    return out;
}

typedef long long tsc_t;

tsc_t
tscget(void)
{
    struct timespec ts;
    tsc_t tsc;

    clock_gettime(CLOCK_MONOTONIC,&ts);
    tsc = ts.tv_sec;
    tsc *= 1000000000;
    tsc += ts.tv_nsec;

    return tsc;
}

double
tscsec(tsc_t tsc)
{
    double sec;

    sec = tsc;
    sec /= 1e9;

    return sec;
}

// test control
typedef char *(*logof_p)(char *buf1,const char *path,time_t tod);
typedef struct {
    logof_p tst_fnc;
    const char *tst_tag;
    const char *tst_reason;
} dotest_t;

#define DOTEST(_fnc,_reason) \
    { .tst_fnc = logof_##_fnc, .tst_tag = #_fnc, .tst_reason = _reason },

// list of tests
dotest_t tstlist[] = {
    DOTEST(orig,"original code [leaks memory]")
    DOTEST(alloca,"original code [using strdupa et. al]")
    DOTEST(stpcpy,"original code [using stpcpy vs. strcat]")
    DOTEST(fix1,"single malloc version")
    DOTEST(fix2,"single pass scan")
    DOTEST(fix3,"single pass scan [using strcspn]")
    { .tst_fnc = NULL }
};

int fnclen;                         // maximum length of a function name
char *dotest_prevbuf;               // previous output buffer
tsc_t dotest_prevtsc;               // best time for previous test
tsc_t dotest_origtsc;               // best time for original test

// dotest_ratio -- show ratio of times
void
dotest_ratio(tsc_t curtsc,tsc_t prevtsc,const char *who)
{
    double ratio;
    const char *tag;

    if (curtsc > prevtsc) {
        tag = "slower";
        ratio = curtsc;
        ratio /= prevtsc;
    }
    else {
        tag = "faster";
        ratio = prevtsc;
        ratio /= curtsc;
    }

    printf(" -- %.3fx %s",ratio,tag);
}

// dotest -- perform test of single function
void
dotest(dotest_t *tst,const char *path)
{
    char *out;
    tsc_t tscbest = 1LL << 62;
    int showout = 0;
    int fail = 0;

    out = malloc(1000);

    // do test multiple times to eliminate variations in caching, speed step,
    // timeslicing, etc. and take the best/minimum time
    for (int iter = 10;  iter > 0;  --iter) {
        tsc_t tscbeg = tscget();

        out = tst->tst_fnc(out,path,stamptod);

        // get elapsed time
        tsc_t tscend = tscget();
        tscend -= tscbeg;

        // get best time
        if (tscend < tscbest)
            tscbest = tscend;
    }

    if (dotest_origtsc == 0)
        dotest_origtsc = tscbest;

    // compare against previous test data
    do {
        if (dotest_prevbuf == NULL) {
            showout = 1;
            break;
        }

        const char *curfix = fixname(out);
        const char *oldfix = fixname(dotest_prevbuf);
        if (strcmp(curfix,oldfix) != 0)
            fail = 1;
    } while (0);

    printf("%.9f %-*s",
        tscsec(tscbest),fnclen,tst->tst_tag);

    // compare against previous test timing
    do {
        if (dotest_prevbuf == NULL)
            break;

        dotest_ratio(tscbest,dotest_origtsc,"orig");
        dotest_ratio(tscbest,dotest_prevtsc,"prev");
    } while (0);

    if (opt_n)
        printf("\n");

    if (showout || fail)
        printf(" %s (%s)",out,fail ? "FAIL" : "PASS");

    //if (! opt_n)
        printf("\n");

    free(dotest_prevbuf);
    dotest_prevbuf = out;
    dotest_prevtsc = tscbest;
}

void
doall(const char *path)
{

    printf("\n");
    printf("PATH: %s\n",path);

    dotest_origtsc = 0;
    for (dotest_t *tst = tstlist;  tst->tst_fnc != NULL;  ++tst)
        dotest(tst,path);

    free(dotest_prevbuf);
    dotest_prevbuf = NULL;
}

int
main(int argc,char **argv)
{

    --argc;
    ++argv;

    for (;  argc > 0;  --argc, ++argv) {
        char *cp = *argv;
        if (*cp != '-')
            break;

        cp += 2;
        switch (cp[-1]) {
        case 'e':
            opt_e = ! opt_e;
            break;
        case 'n':
            opt_n = ! opt_n;
            break;
        }
    }

    // get maximum length of a function name
    for (dotest_t *tst = tstlist;  tst->tst_fnc != NULL;  ++tst) {
        int curlen = strlen(tst->tst_tag);
        if (curlen > fnclen)
            fnclen = curlen;
    }

    // show the function names and their descriptions
    for (dotest_t *tst = tstlist;  tst->tst_fnc != NULL;  ++tst)
        printf("%-*s -- %s\n",fnclen,tst->tst_tag,tst->tst_reason);

    // use the same timestamp for all tests
    todget(0);

    doall("/var/log/mine.log");
    doall("/var/log/mine");
    doall("/var/log.xxx/mine");
    doall("mine.log");
    doall("mine");
    doall("/var/log/mine.log.gz");
    doall("/.log");
    doall("/.");

    printf("complete\n");

    return 0;
}

Note that I did the fix2 version. But, I thought that using strcspn might be faster, so I added fix3.
That's because strcspn [under glibc, at least] is insanely optimized to very wide memory fetches. But, a side effect is that there's some extra table lookup initialization at entry to the function, so the strings should be long enough to overcome that initial deficit.
After looking at the resultant code, I'm thinking that the number of times strcspn has to be called, the extra complexity of the code, would make fix3 [probably] slower and less desirable than fix2

Here's the program output:
orig   -- original code [leaks memory]
alloca -- original code [using strdupa et. al]
stpcpy -- original code [using stpcpy vs. strcat]
fix1   -- single malloc version
fix2   -- single pass scan
fix3   -- single pass scan [using strcspn]

PATH: /var/log/mine.log
0.000000274 orig   /var/log/mine-20201110175328.log (PASS)
0.000000150 alloca -- 1.827x faster -- 1.827x faster
0.000000132 stpcpy -- 2.076x faster -- 1.136x faster
0.000000063 fix1   -- 4.349x faster -- 2.095x faster
0.000000067 fix2   -- 4.090x faster -- 1.063x slower
0.000000137 fix3   -- 2.000x faster -- 2.045x slower

PATH: /var/log/mine
0.000000197 orig   /var/log/mine-20201110175328 (PASS)
0.000000095 alloca -- 2.074x faster -- 2.074x faster
0.000000097 stpcpy -- 2.031x faster -- 1.021x slower
0.000000049 fix1   -- 4.020x faster -- 1.980x faster
0.000000052 fix2   -- 3.788x faster -- 1.061x slower
0.000000093 fix3   -- 2.118x faster -- 1.788x slower

PATH: /var/log.xxx/mine
0.000000219 orig   /var/log.xxx/mine-20201110175328 (PASS)
0.000000104 alloca -- 2.106x faster -- 2.106x faster
0.000000106 stpcpy -- 2.066x faster -- 1.019x slower
0.000000058 fix1   -- 3.776x faster -- 1.828x faster
0.000000062 fix2   -- 3.532x faster -- 1.069x slower
0.000000103 fix3   -- 2.126x faster -- 1.661x slower

PATH: mine.log
0.000000268 orig   ./mine-20201110175328.log (PASS)
0.000000126 alloca -- 2.127x faster -- 2.127x faster
0.000000106 stpcpy -- 2.528x faster -- 1.189x faster
0.000000061 fix1   -- 4.393x faster -- 1.738x faster
0.000000051 fix2   -- 5.255x faster -- 1.196x faster
0.000000071 fix3   -- 3.775x faster -- 1.392x slower

PATH: mine
0.000000208 orig   ./mine-20201110175328 (PASS)
0.000000088 alloca -- 2.364x faster -- 2.364x faster
0.000000089 stpcpy -- 2.337x faster -- 1.011x slower
0.000000056 fix1   -- 3.714x faster -- 1.589x faster
0.000000042 fix2   -- 4.952x faster -- 1.333x faster
0.000000047 fix3   -- 4.426x faster -- 1.119x slower

PATH: /var/log/mine.log.gz
0.000000278 orig   /var/log/mine-20201110175328.log.gz (PASS)
0.000000154 alloca -- 1.805x faster -- 1.805x faster
0.000000132 stpcpy -- 2.106x faster -- 1.167x faster
0.000000062 fix1   -- 4.484x faster -- 2.129x faster
0.000000071 fix2   -- 3.915x faster -- 1.145x slower
0.000000139 fix3   -- 2.000x faster -- 1.958x slower

PATH: /.log
0.000000256 orig   //-20201110175328.log (PASS)
0.000000134 alloca -- 1.910x faster -- 1.910x faster
0.000000110 stpcpy -- 2.327x faster -- 1.218x faster
0.000000058 fix1   -- 4.414x faster -- 1.897x faster /-20201110175328.log (FAIL)
0.000000047 fix2   -- 5.447x faster -- 1.234x faster
0.000000063 fix3   -- 4.063x faster -- 1.340x slower /.log-20201110175328 (FAIL)

PATH: /.
0.000000254 orig   //-20201110175328. (PASS)
0.000000134 alloca -- 1.896x faster -- 1.896x faster
0.000000109 stpcpy -- 2.330x faster -- 1.229x faster
0.000000053 fix1   -- 4.792x faster -- 2.057x faster /-20201110175328. (FAIL)
0.000000042 fix2   -- 6.048x faster -- 1.262x faster
0.000000063 fix3   -- 4.032x faster -- 1.500x slower /.-20201110175328 (FAIL)
complete

UPDATE:

I see your point about too much strdup(), but it's necessary with dirname() and basename() as they will mangle their argument.

As I mentioned, raw strdup leaks memory. And, even with dirname/basename some of the dups aren't needed.
And, I've never used either dirname and/or basename for pretty much the same reason [they modify their arguments]. So, I've written my own variants that copy the results to a buffer selected from a pool of static buffers

I like the approach you took with fix1 as it is still very obvious what is going on.

I intended to provide a progression from simple/slow to faster, so you can choose.

Although fix2 is arguably faster, it's a little obfuscated

Perhaps, but it's actually [loosely] derived from production code I have for doing the same thing. You wanted [some] comments. Personally, I felt your code was difficult to follow.

and would not handle extensions such as .log.gz correctly

That wasn't part of your original test data. But, my production code handled it, just not what I recreated for this post. It's a three line fix:
diff --git a/logtod/logtod.c b/logtod/logtod.c
index 28f5702..c212ba9 100644
--- a/logtod/logtod.c
+++ b/logtod/logtod.c
@@ -264,9 +264,11 @@ logof_fix2(char *buf1,const char *path,time_t tod)
        switch (chr) {
        case '/':
            base = src;
+           ext = NULL;
            break;
        case '.':
-           ext = src - 1;
+           if (ext == NULL)
+               ext = src - 1;
            break;
        }
    }

I've updated the original code block and the output

UPDATE #2:
I've updated the code and program output [again ;-)].
I moved the setting of the timestamp buffer to outside the benchmarked/timed area to [better] compare just the times for the scanning/output code (e.g. because sprintf is somewhat heavyweight and took up the bulk of the time spent).
And, I replaced a strlen call in copyout with just a pointer/offset calculation.
And, I added calculation of the speed ratio of the various methods against one another.


Regular expressions - match only specified string length I'm learning regex. Here I'm checking a string of type aA1 but want also to check it is only 3 characters long.  Is there a better way of doing it please?

str = raw_input("input in form 'aA1' please >")
check = re.compile("[a-z][A-Z][0-9]")
if len(str) == 3:
    validate = check.match(str)
    if validate:
        print "Correct string"
    else:
        print "Incorrect string"
else:
    print "Wrong length"

Ref. http://www.regular-expressions.info/anchors.html

  Anchors are a different breed. They do not match any character at all. Instead, they match a position before, after, or between characters. They can be used to "anchor" the regex match at a certain position. The caret ^ matches the position before the first character in the string. Applying ^a to abc matches a. ^b does not match abc at all, because the b cannot be matched right after the start of the string, matched by ^. See below for the inside view of the regex engine.

  Similarly, $ matches right after the last character in the string. c$
  matches c in abc, while a$ does not match at all.

With anchors you may skip length test entirely.

So:

import re
str = raw_input("input in form 'aA1' please >")
check = re.compile("^[a-z][A-Z][0-9]$")
validate = check.match(str)
if validate:
    print "Correct string"
else:
    print "Incorrect string"


Swap first and last name in C I took an entire line of input in the form of a string, and tokenized the string into substrings and obtained the first string as the first name and the second as the last name. This program as it stands is very basic and limited however my primary focus was on the creation of generic functions to do all the heavy lifting (generic as in they are not specific to the program and could be used elsewhere I've had several problems in the past where my functions are to narrowed on a specific problem, so please tell me how I did!).

The function for parsing the string, get_substr(), most likely can, and should, be improved and I am hoping for better ideas on how to do it!

/*Input name: first, last. Output name: Last, first*/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

#define MAXSIZE 1024

bool readline(char *line, const size_t buffer_len);
bool hasDigits(const char *str);
bool hasPunct(const char *str);
void chop_newline(char *str);
char *remove_leading_whitespace(char *str);
char *get_substr(char *str, char **endptr, const char *str_dim);

bool readline(char *line, const size_t buffer_len) {
    if (fgets(line, buffer_len, stdin) != NULL) {
        chop_newline(line);
        if (line[0] != NULL) {
            return true;
        }
    }
    return false;
}

bool hasDigits(const char *str) {
    for (size_t i = 0; str[i] != '\0'; ++i) {
        if (isdigit((unsigned char)str[i])) {
            return true;
        }
    }
    return false;
}

bool hasPunct(const char *str) {
    for (size_t i = 0; str[i] != '\0'; ++i) {
        if (ispunct((unsigned char)str[i])) {
            return true;
        }
    }
    return false;
}

void chop_newline(char *str) {
    str[strcspn(str, "\n")] = '\0';
}

char *remove_leading_whitespace(char *str) {
    if (str) {
        while (isspace(*str) && str) str++; //remove whitespace
    }
    return str;
}

/*
get_substr

@param **endptr, set to next character after a diliminator in string str
@param *str_dim, list of diliminators used to split string ex: " !,.-?"

when called, absorb incoming whitespace ,read until first diliminator, return substring,
and set **endptr equal to the rest of the string. If endptr is not NULL, then string has more to parse,
returns NULL if nothing was parsed else returns c-string
*/

char *get_substr(char *str, char **endptr, const char *str_dim) {
    char *substr = NULL;
    remove_leading_whitespace(str);
    substr = strtok(str, str_dim);
    *endptr = strtok(NULL, "\0");

    return substr;
}

int main() {
    char str_name[MAXSIZE];
    char *last_name = NULL, *first_name = NULL, *endptr = NULL;

    printf("Please enter your first name followed by your last name: ");
    for (;;) {
        if (readline(str_name, sizeof str_name)) {
            if (!hasDigits(str_name) && !hasPunct(str_name)) {
                break;
            }
            printf("\nTry again, name must contain letters and cannot contain numbers or punctuation...\n");
        }
        else {
            printf("\nEmpty string please try again...\n");
        }
    }

    first_name = get_substr(str_name, &endptr, " ");
    last_name = get_substr(endptr, &endptr, " ");

    if (endptr) {
        printf("Error, too much input, expected: First name Last name\n %s was not saved...\n", endptr);
    }
    else if (!last_name) {
        printf("Error must have a first and last name...\n");
    }
    else {
        printf("Your name is: %s, %s\n", last_name, first_name);
    }

    printf("Press any key to continue...\n");
    getch();
}

The function is maybe too ambitious in doing two things.

char *get_substr(char *str, char **endptr, const char *str_dim) {
    str = remove_leading_whitespace(str);
    char *substr = strtok(str, str_dim);
    *endptr = strtok(NULL, "\0");
    return substr;
}

I would keep the API simple and leave out endptr.


Simple specific classes design I am new to c#, and rather new to design-patterns. I want to create a simple and a most elegant solution for loading and validating a xml file and later on extracting its fields.
I've started with a generic IData interface, that gets implemented by a XmlFileData class, but I feel that there is much more to be improved upon, but I lack the knowledge to do it (from design, to naming variables and methods and so on).

So please, have no mercy !!

public interface IData
{
     String GetDataString();
}

class XMLValidator
    {
        private StringBuilder verificationErrorsList = new StringBuilder();
        public string ValidationSchema { get; set; }
        public String ValidationErrors
        {
            get { return verificationErrorsList.ToString(); }
        }
        public bool HasErrors
        {
            get {return (verificationErrorsList.Length > 0);}
        }

        public void Validate(IData xmlData)
        {
            try
            {
                XmlWriterSettings ws = new XmlWriterSettings();
                ws.Indent = true;

                XmlReaderSettings rs = new XmlReaderSettings();
                rs.ValidationType = ValidationType.Schema;
                rs.ValidationEventHandler += new System.Xml.Schema.ValidationEventHandler(rs_ValidationEventHandler);
                rs.Schemas.Add(null, ValidationSchema);
                rs.CloseInput = true;

                rs.ValidationFlags = XmlSchemaValidationFlags.ReportValidationWarnings |  XmlSchemaValidationFlags.ProcessIdentityConstraints |
                               XmlSchemaValidationFlags.ProcessInlineSchema |  XmlSchemaValidationFlags.ProcessSchemaLocation;

                StringReader r = new StringReader(xmlData.GetDataString());
                using (XmlReader reader = XmlReader.Create(r, rs))
                {
                    try
                    {
                        while (reader.Read()) { }
                    }
                    catch (XmlException e)
                    {
                        verificationErrorsList.Append(e.Message);
                        verificationErrorsList.Append(Environment.NewLine);
                    }

                }
            }
            catch (XmlSchemaValidationException e)
            {
                verificationErrorsList.Append(e.Message + Environment.NewLine);
            }
        }
        void rs_ValidationEventHandler(object sender, System.Xml.Schema.ValidationEventArgs e)
        {
            if (e.Severity == XmlSeverityType.Error || e.Severity == XmlSeverityType.Warning)
            {
                verificationErrorsList.Append(e.Message + Environment.NewLine);
            }
        }
    }

Usage:

        IData data = new XmlFileData(basepathXML + fileNameXML);
        XMLValidator validator = new XMLValidator();
        validator.ValidationSchema = basepathXML + fileNameXSD;
        validator.Validate(data);
        if (validator.HasErrors)
        {
            Console.WriteLine(validator.ValidationErrors);
        }
        else
        {
            Console.WriteLine("Success");
        }

A few things:

IData is not a particularly good name for the interface as it represents a source of data rather than the data itself. Therefor IDataSource seems more appropriate.

The interface is a bit ambiguous - which method am I supposed to use GetDataStream or GetDataString? Why use one or the other? Could I call both multiple times and interchangeably?
As of now it doesn't really provide enough value to warrant it's existence. You'd be better of having XmlValidator accept a TextReader and let the calling code deal with how to wrap it around the source.

Consider passing the ValidationSchema as part of the constructor for the XmlValidator - it's an integral part of its validation logic.

In the validator when the reader throws an exception because there was a problem reading from the stream for example then you catch those as validation errors which I'd consider technically wrong. Your validator should catch validation errors - i.e. anything that has to do with invalid XML. Everything else should not be of its concern. So don't catch Exception - be more specific.


Implementation of a doubly linked list in Java I'd like to improve this code.

DoublyLinkList.java

public class DoublyLinkList<T> {

    private static class Node<T> {
        private T data;
        private Node next;
        private Node prev;

        public Node(T data) {
            this.data = data;
        }

        public void displayNode() {
            System.out.print(data + " ");
        }

        @Override
        public String toString() {
            return data.toString();
        }
    }

    public Node first = null;
    public Node last = null;

    public void addFirst(T data) {
        Node newNode = new Node(data);

        if (isEmpty()) {
            newNode.next = null;
            newNode.prev = null;
            first = newNode;
            last = newNode;

        } else {
            first.prev = newNode;
            newNode.next = first;
            newNode.prev = null;
            first = newNode;
        }
    }

    public boolean isEmpty() {
        return (first == null);
    }

    public void displayList() {
        Node current = first;
        while (current != null) {
            current.displayNode();
            current = current.next;
        }
        System.out.println();
    }

    public void removeFirst() {
        if (!isEmpty()) {
            Node temp = first;

            if (first.next == null) {
                first = null;
                last = null;
            } else {
                first = first.next;
                first.prev = null;
            }
            System.out.println(temp.toString() + " is popped from the list");
        }
    }

    public void removeLast() {
        Node temp = last;

        if (!isEmpty()) {

            if (first.next == null) {
                first = null;
                last = null;
            } else {
                last = last.prev;
                last.next = null;
            }
        }
        System.out.println(temp.toString() + " is popped from the list");
    }
}

DoublyLinkListDemo.java

public class DoublyLinkListDemo {

    public static void main(String[] args) {
        DoublyLinkList newList = new DoublyLinkList();
        newList.addFirst("arun");
        newList.addFirst("prakash");
        newList.addFirst(70);
        newList.addFirst(80);
        newList.addFirst(30);
        newList.displayList();
        newList.removeFirst();
        newList.removeFirst();
        newList.removeFirst();
        newList.removeLast();

        newList.displayList();
    }
}

removeLast() assigns last to temp, which may be null.  If the list is empty, you will call toString() on null.

Otherwise, it is a pretty well made list.

I would change the names of your functions/class.

DoublyLinkList becomes DoublyLinkedList
addFirst becomes pushFront
removeFirst becomes popFront
removeLast becomes popBack
displayList becomes display ( because we know a list is calling it )
displayNode becomes display

As general advise about naming, the words you are using to describe what is happening are good hints at what names the functions/class should be.  For example, you use Doubly Linked List in the title of this post, and you use the word "popped" as output to the remove functions.


vba Copy Paste Transpose Link Following takes a range & copies, pastes, transposes & links.
There doesn't seem a way in vba to do this in 1 go that I've been able to find.
Questions are;

Is there a more efficient or safer way to do this. Keeping in mind;
-needing to do this for large ranges ie. over 100K cells.
-source & destination are in different worksheets or workbooks. So not the same worksheet.
What issues if any may exist & how to safeguard.

Thank you

Sub Foo()

    'Example1
    Call CopyPaste(Sheet1.Range("C10:D20"), Sheet2.Range("C1"))

    'Example2
    Dim wbNew As Workbook
    Set wbNew = Workbooks.Add
    Call CopyPaste(ThisWorkbook.Sheets(1).Range("C10:D20"), wbNew.Sheets(1).Range("C1"))

End Sub

Sub CopyPaste(rngSrc As Range, rngDest As Range)

    Application.ScreenUpdating = False
    ActiveWorkbook.Sheets.Add.Name = "_wsDummy_Temp_"
    Dim wsDummy As Worksheet
    Set wsDummy = ActiveWorkbook.Sheets("_wsDummy_Temp_")

    rngSrc.Copy
    wsDummy.Activate
    wsDummy.Range("A1").Select
    ActiveSheet.Paste Link:=True

    Dim vTransposed As Variant
    Dim rngSrcSrcRng As Range
    Dim vSrcSrc As Variant

    Dim rngDummy As Range
    Set rngDummy = wsDummy.Range("A1")
    Set rngDummy = rngDummy.Resize(rngSrc.Rows.Count, rngSrc.Columns.Count)
    rngDummy.Formula = Application.ConvertFormula(rngDummy.Formula, xlA1, xlA1, 1)

    Set rngSrcSrcRng = rngDummy
    vSrcSrc = rngSrcSrcRng.Formula
    vTransposed = Application.Transpose(vSrcSrc)

    Set rngDest = rngDest.Resize(rngDummy.Columns.Count, rngDummy.Rows.Count)
    rngDest.Formula = vTransposed

    rngDummy.ClearContents
    Application.DisplayAlerts = False
    wsDummy.Delete
    Application.DisplayAlerts = True
    Application.ScreenUpdating = True

End Sub

EDIT:
With the answer provided @TinMan I decided to fill over a 1M cells in a worksheet with numbers & do some benchmarking.
Original OP function: 33 to 39 seconds.
Refactored CopyPaste function: 20 to 26 seconds.
Alternate Approach TransposeLink function: 11 to 13 seconds.
It appears the last one is the fastest in the tests I did but also removes the need to use another temporary worksheet, removes need to use select or the clipboard.

Review

Private Sub CopyPaste(rngSrc As Range, rngDest As Range)

Prefixing variables with their type is a bit dated.   Of course there are times when it is invaluable such as working with forms where their is a designer and a code module.  Using simple meaningful names will make you code easier to read.

Private Sub CopyPaste(Source As Range, Destination As Range)

There is no need to name temporary objects.

ActiveWorkbook.Sheets.Add.Name = "_wsDummy_Temp_"
Dim wsDummy As Worksheet
Set wsDummy = ActiveWorkbook.Sheets("_wsDummy_Temp_")

It better to set your variables directly whenever possible.

Set wsDummy = ActiveWorkbook.Sheets.Add

Since the worksheet is just temporary and the code is short, I would use a With block and eliminate the wsDummy variable altogether.

With ActiveWorkbook.Sheets.Add
    .Paste Link:=True
    <more code>
End With

Worksheets are activated with Range("A1") selected whenever they are added.  So eliminate these lines:

wsDummy.Activate
wsDummy.Range("A1").Select

Ay-ay-ay rngSrcSrcRng!!  This variable is just an alias for rngDummy`.   Pick a name and stick with it.   I take this concept to the extreme.  You will see the same names throughout all my code projects.
IMO, consistently using simple names like data ( array ), results ( array ), result (scalar value), r (row index) , c (column index), n (generic index), text ( simple string ), contents ( simple string usually file contents), source (source object such as a range) , destination (destination object such as a range), cell, target don't just make it easier to read and modify your code but it also makes it far quicker to write the code, in the first place.
vTransposed isn't needed either.  It would be better to reuse vSrcSrc then to keep both variables in memory.
Clearing the contents of a temporary worksheet.  I'm guessing this is a remnant of code from your earlier attempts.

rngDummy.ClearContents

After your macros complete Application.DisplayAlerts and Application.ScreenUpdating are automatically reset.  So these lines can be removed:

Application.DisplayAlerts = True
Application.ScreenUpdating = True

It is best to set Application.Calculation = xlCalculationManual when changing values or formulas on a worksheet.
Refactored Code
Private Sub CopyPaste(Source As Range, Destination As Range)
    Application.ScreenUpdating = False
    Application.DisplayAlerts = False

    Dim calculationMode As XlCalculation
    calculationMode = Application.Calculation

    Dim results As Variant

    Source.Copy
    With Worksheets.Add
        .Paste Link:=True
         With .Range("A1").CurrentRegion
            results = Application.ConvertFormula(.Formula, xlA1, xlA1, 1)
            Destination.Resize(.Columns.Count, .Rows.Count) = Application.Transpose(results)
        End With
        .Delete
    End With

    Application.Calculation = calculationMode
End Sub

Alternate Approach
A more efficient method create the formula array using Range.Address(RowAbsolute:=True, ColumnAbsolute:=True, External:=True).  This will eliminate the need for a temporary worksheet and avoid the copy and pasting.
 Private Sub TransposeLink(Source As Range, Destination As Range)
    Application.ScreenUpdating = False
    Application.DisplayAlerts = False

    Dim calculationMode As XlCalculation
    calculationMode = Application.Calculation

    Dim results As Variant
    With Source
        ReDim results(1 To .Columns.Count, 1 To .Rows.Count)

        Dim r As Long, c As Long

        For r = 1 To .Rows.Count
            For c = 1 To .Columns.Count
                    results(c, r) = "=" & .Cells(r, c).Address(RowAbsolute:=True, ColumnAbsolute:=True, External:=True)
            Next
        Next

        Destination.Resize(.Columns.Count, .Rows.Count).Formula = results
    End With

    Application.Calculation = calculationMode
End Sub


Simple imperative linked list I've decided to learn a bit of C++, and so I've started with a simple linked list. There's no input to the program and should always output hlo. This is as in main I build a list consisting of hello, and use Get and last to get the data.

I don't know C++, or C, and so this is mostly a stab in the dark. I don't know if I'm using C++ correctly, but I got a working program where I'm mostly exploring pointers, so I would like to know if my usage for them is correct. Otherwise please review any and all aspects of my code.

#include <iostream>

class Node {
public:
    Node(char data_) {
        data = data_;
    }

    Node *next = NULL;
    char data;
};

class LinkedList {
public:
    Node *root = NULL;
    Node *last = NULL;
    LinkedList() {}

    void Add(char data) {
        if (root == NULL) {
            root = new Node(data);
            last = root;
        } else {
            last->next = new Node(data);
            last = last->next;
        }
    }

    char Get(int index) {
        return _Get(index)->data;
    }
private:
    Node* _Get(int index) {
        Node *node = root;
        for (int i = 0; i < index; i++) {
            if (node == NULL) {
                throw std::out_of_range("List doesn't contain that item.");
            }
            node = node->next;
        }
        return node;
    }
};

int main() {
    LinkedList list = LinkedList();
    list.Add('h');
    list.Add('e');
    list.Add('l');
    list.Add('l');
    list.Add('o');
    std::cout << list.Get(0) << list.Get(2) << list.last->data << std::endl;
    return 0;
}

Memory Leak

Your code leaks. I see calls to new but I see no smart pointers or calls to delete to reclaim the memory. You need a destructor on your linked list.

class LinkedList {
public:
    LinkedList()  {}
    ~LinkedList() {/* Reclaim dynamically allocated memory here */}
};

Rule of three

Once you correct for memory management you also realize that you are violating the rule of three. By default the compiler will automatically add copy constructor and assignment opertors for you.

LinkedList   a;
a.add('x');
LinkedList   b(a);   // Makes a copy of a
                     // But only a shallow copy.
a.add('y');
// This changes 'a' to be {'x', 'y}
// Problem is that b is now also {'x', 'y}
// **BUT** in b last points at the wrong node.

// Now things get interesting:
b.add('z');
// Now a and b are {'x', 'z'} (if you follow from root)
// a.last still points {'y'} even though this is no longer
// in the chain pointed to from a.root

The default implementation of the copy constructor (and assignment operator) is to make a shallow copy of the members. This fine as long as your members are not owned pointers. But your pointers are owned (even though you missed the deletion). So you either need to implement the copy semantics of the class or explicitly remove these methods.

class LinkedList {
public:
    LinkedList()  {}
    ~LinkedList() {/* Reclaim dynamically allocated memory here */}
    LinkedList(LinkedList const&)            = delete;
    LinkedList& operator=(LinkedList const&) = delete;
};

Prefer nullptr

In C++11 we introduced nullptr to replace NULL. This is because NULL is the integer zero and can accidentally be converted to an integer type without any warning. nullptr on the other hand has a type of std::nullptr_t and can only be converted to pointer types (not integers).

Design.

When building linked lists I prefer to use a Sentinel object. This removes the need for checking for null in your list and thus makes the code easier to write and understand. There are a lot of C++ code reviews on linked lists where I explain the principle.

Node should be private

You declare Node publicly. There is no reason for people to know the implementation details of your linked list. Make this a private member of LinkedList so show that this is an implementation detail.

Prefer to use initializer list:

    Node(char data_) {
        data = data_;
    }

    // Prefer to write like this:

    Node(char data_)
        :data(data_)
    {}

For non pod types this is because the object are constructed before the body of the constructor is entered then the constructor code is applied. If data was a non pod object that means you would first be constructing it and then applying the assignment operator (ie initializing it twice).

For pod data it makes no difference, BUT does no harm either. So it is a good habit to get into. Because that way of the code is later modified to be a non pod type (i.e. CharUTF) then doing it the prefered way keeps the code optimal.

Not a fan

    Node *root = NULL;
    Node *last = NULL;

Personally I like to see all the members initialized in the initializer list. But I can see some advantages for objects with lots of constructors in avoiding writing the same thing repeatedly.

Style Check.

Most style guides for C++ (though this is not ubiquitous so just take as advice and not a rule). User defined types have an initial capital letter, while objects (which includes functions/methods) have an initial lowercase letter.

This is because the most important thing in C++ is the types. So it is useful to be able to quickly identify types from objects.

    void Add(char data) {
    // I would prefer
    void add(char data) {

Self Documenting code

Personally I don't like add() as the name of the function. It does not tell me exactly what the function does. I think a better name would be append().

Dont' use _ in identifers.

The rules on using underscore are non trivial. Even if you know them. Other people don't know them as well as they think they do. So best to avoid to prevent accidental mistakes.

    Node* _Get(int index) { // Note this is a reserved identifier.
                            // And being a common word is very likely
                            // to be used somewhere. including the
                            // wrong header file before this is likely
                            // to generate some confusing error messages
                            // on some systems.

    // Why Note
    Node* getNode(int index)

Bug

 LinkedList  a;
 a.Add('a');
 a.Get(1); // Undefined behavior.
           // I believe _Get() is going to return a `nullptr`
           // which is then dereferenced by Get()

Declaring variables

    LinkedList list = LinkedList();
                   // ^^^^^^^^^^^^  Creates a temporary object.

Technically this is creating a LinkedList temporary object then calling the copy constructor of list to copy the temporary object then destroying the temporary object.

Fortunately most compilers will spot this and optimize out the copy and destruction and just create your object.

But this is simpler to declare as:

    LinkedList list;

Prefer \n to std::endl;

The only difference is that std::endl performs an extra flush. Forcing a manual flush is usually never the correct solution as the libraries will do this for you when required.

Main and return

The main() function is special. If the compiler detects there is no return it will plant one for you.

    return 0;

So there is no need to add it yourself. It is become standard that when main can return no other value than 0 that the return 0 is omitted to indicate that the application can never fail. So when I see a return 0 at then end I generally look around for the other situations where it can fail. So if there are potentially failure situations and you exit early with an return 1; then add a return 0; and then end otherwise don't bother.