Print Tweets/Mentions/Retweets I've wrote some simple code to load a JSON file from the Twitter Streaming API, parse the data, and create relationships between users based on who mentions or retweets who.  My ultimate goal is to have a file that I can load into a network analysis program to analyze.

I wrote the code to first look for a retweet, and if present, create a relationship between the author and the user retweeted.  Then look for any mentions, and if present, create a relationship between the author and any users mentioned.  This part can have multiple relationships for a single tweet, as you can mention multiple people in one tweet.  Then if neither a retweet or a mention is present, print the author and the tweet.

End goal of this is to simple parse a JSON file and return a CSV file that I can then analyze.  I will not be loading this into any type of database.

import json
import sys
import time

def main():

    for line in sys.stdin:
        line = line.strip()

        data = []

        try:
            data.append(json.loads(line))
        except ValueError as detail:
            continue

        for tweet in data:
            ## deletes any rate limited data
            if tweet.has_key('limit'):
                pass

            else:
                tweet_time = time.strftime('%Y-%m-%d %H:%M:%S', time.strptime(tweet['created_at'],'%a %b %d %H:%M:%S +0000 %Y'))

                ## if the key 'retweeted_status' is in the dict, print
                if tweet.has_key('retweeted_status'):
                    print "\t".join([
                    tweet_time,
                    tweet['id_str'],
                    tweet['user']['screen_name'],
                    tweet['retweeted_status']['user']['screen_name'],
                    "RETWEET",
                    tweet['text']
                    ]).encode('utf8')

                ## if there is a mention in the dict, print
                elif 'entities' in tweet and len(tweet['entities']['user_mentions']) > 0:
                    for u2 in tweet['entities']['user_mentions']:
                        print "\t".join([
                        tweet_time,
                        tweet['id_str'],
                        tweet['user']['screen_name'],
                        u2['screen_name'],
                        "MENTION",
                        tweet['text']
                        ]).encode('utf8')

                ## if there is no retweet and no mention, print
                else:
                    print "\t".join([
                    tweet_time,
                    tweet['id_str'],
                    tweet['user']['screen_name'],
                    "\t",
                    "TWEET",
                    tweet['text']
                    ]).encode('utf8')

if __name__ == '__main__':
    main()

I would suggest to create following new classes:

User, which stores information about user, like user_id, user_location
Text, which stores the tweet text, the language of the tweet, may be the keywords
Tweet, which contains objects of User ,Text and some other information about the tweet

I am assuming that you will be storing all this data into a database and the above mentioned structure (with some mild changes, based on your requirement) can be used to create a 3-NF database.

Another suggestion is "not" using stdin. Create a new file "Twitter.py" having function readTweet which will return tweet. This makes it easier to change the source of the tweet in the later part of the development. Suppose you want to directly take tweet from twitter API, then you just need to change the function

Your main function will look like following:

def main():

    for tweet in Twitter.getTweets():

        tweet_time = tweet.getTime()

        if tweet.isRetweeted:
            print "do something 0"
        elif tweet.hasEntities and len(tweet.Usermentions) > 0:
            for mention in tweet.userMentions:
                print "do something 1"
        else:
            print "do something 2"


Set id in constructor, or generate it when asked for? I'm using PHP, I have a class that generates an id based on the calling class, so the id of the class won't change after it has been initialised, now I'm wondering which of my two methods is preferrable.

Set the id in the constructor
Generate the id when asked for

/* Set  the id in the constructor */

abstract class Badge
{
    protected $activities;
    protected $id;

    abstract public function check();

    public function __construct(array $activities)
    {
        $this->activities = $activities;
        $this->id = $this->getId();
    }

    public function getId(): string
    {
        $class = explode('\\', get_class($this));
        $classWithoutNamespace = array_pop($class);
        $snakeCased = strtolower(preg_replace('/(?<!^)[A-Z]/', '_$0', $classWithoutNamespace));

        return "{$snakeCased}_{$this->target}";
    }

    public function toArray(): array
    {
        return [
            'id' => $this->id,
        ];
    }
}

/* Generate the id when asked for */

abstract class Badge
{
    protected $activities;

    abstract public function check();

    public function __construct(array $activities)
    {
        $this->activities = $activities;
    }

    public function getId(): string
    {
        $class = explode('\\', get_class($this));
        $classWithoutNamespace = array_pop($class);
        $snakeCased = strtolower(preg_replace('/(?<!^)[A-Z]/', '_$0', $classWithoutNamespace));

        return "{$snakeCased}_{$this->target}";
    }

    public function toArray(): array
    {
        return [
            'id' => $this->getId(),
        ];
    }
}

```

I would go with lazy loading. Generate the id when requested
private $id;

public function getId()
{
    if ($this->id === null) {
        $this->id = //your logic to generate the id here
    }
    return $this->id;
}

and don't use anywhere else $this->id even inside the class. Use $this->getId() everywhere.
using $this->getId() everywhere will ensure that you always get the generated id (the same one everytime) and you are sure you don't get the null initial value.


From scratch pub sub definition and implementation I am finished with my implementation of PUB/SUB or Observer pattern, however I just want to see any ways that I may be able to improve this pattern. I wanted to be able to use the typical PubSub pattern except add performance enhancements by using the new key word to instantiate each new instance of a PubSub object, as well as make sure that the initialize fn as well as all of the public methods are chain-able.

Here is my PubSub definition:

(function(global) {
'use strict';

var newPubSub;
global.PubSub = {
    core: function core() {},
    init: function(moduleNs) {
        if (!global[moduleNs]) global[moduleNs] = new PubSub.core();
        else throw new Error('A module has already been initialized under this namespace');
        newPubSub = global[moduleNs];
        return newPubSub;
    }
};

var instance = PubSub.core.prototype;

/**
 * Will check for the existance of prev array under this ns and if none exists it will create
 * a new prop with the ns and the function as the sole member of the array. If an array already
 * exists under this namespace then the function is simply pushed into that namespaces array of
 * methods that it will be triggered when that ns becomes published.
 **/
instance.sub = function(ns, fn) {
    if (!newPubSub[ns]) {
        newPubSub[ns] = [fn];
    }
    else if (Array.isArray(newPubSub[ns]) && newPubSub[ns].length > 0) {
        newPubSub[ns].forEach(function(savedFn) {
            if (fn === savedFn) throw new TypeError('This function already exists under this namespace, please provide a new method for this namespace or add a new namespace to hold this method/fn');
            else {
                newPubSub[ns].push(fn);
            }
        });
    }
    return this;
};

/**
 * Will remove all methods for a listener and the listener ns is not provided,
 * or will remove a single method from a listener if the ns provided has a matching method/fn
 **/
instance.unsub = function(ns, fn) {
    function revokeAllMethods(ns) {
            if (newPubSub[ns]) {
                delete newPubSub[ns];
            }
    }
    // loops through all the methods in a namespace provided and compares to see if the method provided exists, if it does it will be removed
    function revokeSpecificMethod(ns, fn) {
        newPubSub[ns].forEach(function(savedMethod, idx) {
            // Remove the method that matches the one provided to unsubscribe
            if (fn === savedMethod) newPubSub[ns].splice(idx, 1);
            // Otherwise an error will be thrown
            else throw new Error('No method match found under the namespace '+ns);
        });
    }
    if (typeof ns === 'string') {
        if (!fn) {
            revokeAllMethods(ns);
        }
        else if (typeof fn === 'function') {
            revokeSpecificMethod(ns, fn);
        }
    }
  return this;
};

/**
 * Will trigger an event with provided params on any methods under the ns triggered
 */
instance.pub = function(ns, params) {
    if (!newPubSub[ns]) throw new TypeError('No method found under the namespace "' + ns + '" inside of Object. It has either been unsubscribed or never existed.');

    newPubSub[ns].forEach(function(fn) {
        fn.apply(null, params);
    });
    return this;
};

})(window);

Implementation:

PubSub.init('test');

test.sub('alert', function(msg){
    alert(msg);
}).pub('alert',['bob']).sub('somethingelse', function(){
    alert('look ma no params');
}).pub('somethingelse').pub('alert',['bob']).unsub('alert').pub('alert',[12]);

Error checking

Seems like you don't do all the error checking you'd need. For example, in this function

instance.sub = function(ns, fn) {
    if (!newPubSub[ns]) {
        newPubSub[ns] = [fn];
    }
    else if (Array.isArray(newPubSub[ns]) && newPubSub[ns].length > 0) {
        newPubSub[ns].forEach(function(savedFn) {
            if (fn === savedFn) throw new TypeError('This function already exists under this namespace, please provide a new method for this namespace or add a new namespace to hold this method/fn');
            else {
                newPubSub[ns].push(fn);
            }
        });
    }
    return this;
};

fn falls through the cracks if newPubSub[ns] is not an array but already exiss. Wouldn't you want to throw an error?

Similarly, you might also want to check that newPubSub[ns] has the expected form in unsub and that apply can be applied to each member of

Use closures to enforce encapsulation

All my concern about error checking relates to the fact that you are creating normal properties in these objects with your pub-sub methods, meaning that in the intervening time, a user can forget he was using a certain keyword with pub-sub and might use it for something else.

For example, here I corrupt your test variable by doing something entirely different with the alert key and forgetting I was using that for my pubsub functionality:

PubSub.init('test');

test.sub('alert', function(msg){
    alert(msg);
}).pub('alert',['bob'])

test['alert'] = "hi!"; // oops your 'alert' chain is gone

test.pub('alert', ['joe'])  //doesn't work

I think you'll want to restructure this to something like the following:

function pubsub() {
     var pub_arrays = {}
     return {
        sub: function(ns, fn){
                if(!pub_arrays[ns]) pub_arrays[ns] = [fn]
                else p
                },
        pub:  .... and so on ....

    };
}

In this way all your methods will have access to the object that holds the publishing arrays (one array for each key word) but outside users won't have access to this so they won't be able to change it.

Variable scoping in JavaScript is not a strong suit of mine, but I think something like this, using closures for encapsulation, is where you want to be if you don't trust the user, or yourself, not to accidentally...or maliciously...mess with your pub/sub methods.

Additionally, it looks like the most recent version of JavaScript may provide Class methods to encapsulate data. Read more on that in these SO posts:

https://stackoverflow.com/questions/29455692/how-does-closure-reify-the-data-encapsulation
https://stackoverflow.com/questions/8729714/closures-vs-classes-for-encapsulation


Javascript Tree Class 2 This question is the second version of the code here.

I'm writing a general tree class. Specifically, each node should have oen parent, some number of children, and hold a value.

I'm looking for general advice on how to make this code more idiomatic. I'm also looking for thoughts on the traverse function, any functions that should be present in a general tree class but aren't, and also any way to simplify the existing functions.

// Code to review
class Node {
 constructor(value = 0, children = []) {
  this.value = value;
  this.children = children;
 }

 traverse({ preorder, postorder, levelorder }) {
  if (levelorder) {
   let nodes = [this];
   while (nodes.length > 0) {
    const current = nodes.shift();
    levelorder(current);
    nodes = nodes.concat(current.children);
   }
  }
  else {
   if (preorder) preorder(this);
   this.children.forEach(n => n.traverse({ preorder, postorder }));
   if (postorder) postorder(this);
  }
  return this;
 }

 clone() {
  const copy = n => Object.assign(new Node(), n);
  let that = copy(this);
  that.traverse({ preorder: n => n.children = n.children.map(copy) });
  return that;
 }

 map(callback) {
  let that = this.clone();
  that.traverse({ levelorder: n => n.value = callback(n.value) });
  return that;
 }

 reduce(callback, initial) {
  let a = initial;
  this.traverse({ levelorder: n => a = (n === this && initial === undefined)? n.value: callback(a, n.value) });
  return a;
 }

 filter(callback) {
  let that = this.clone();
  that.traverse({ levelorder: n => n.children = n.children.filter(m => callback(m.value)) });
  return that;
 }

 every(callback) {
  return Boolean(this.reduce((a, b) => a && callback(b), true));
 }

 some(callback) {
  return Boolean(this.reduce((a, b) => a || callback(b), false));
 }

 find(callback) {
  return this.reduce((a, b) => (callback(b)? a.push(b): null, a), []);
 }

 includes(value) {
  return this.some(a => a === value);
 }
}

// Testing code
let tree = new Node(0);
let a = new Node(1);
let b = new Node(2);
let c = new Node(3);
let d = new Node(4);
let e = new Node(5);
let f = new Node(6);
tree.children = [a, b, c];
a.children = [d];
d.children = [e, f];

console.log("\nmap", tree.map(a => a + "2"));
console.log("\nreduce", tree.reduce((a, b) => String(a) + String(b)));
console.log("\nfilter", tree.filter(a => a < 3));

console.log("\nevery", tree.every(a => a < 3));
console.log("\nsome", tree.some(a => a < 3))

console.log("\nfind", tree.find(a => a > 2));
console.log("\nincludes", tree.includes(3));

Review

Well written API, but I am not happy with this one:

traverse({ preorder, postorder, levelorder })

preorder and postorder can be combined, but they are always mutually exclusive with levelorder.

traverse({ preorder, postorder, levelorder }) {
  if (levelorder) {
      // levelorder ..
  }
  else {
      // preorder conditional and/or postorder conditional ..
  }
  return this;
}

Instead, use 2 separate methods, each doing their own well known type of traversal.

DFS

 traverseDepthFirst({ preorder, postorder })

BFS

 traverseBreadthFirst({ levelorder })

Some general thoughts:

should you allow cyclic graphs?
should you want to traverse up the ancestors?


Return the indices of the two numbers such that they add up to a specific target (...as I learn Kotlin) I want to get the indices of the two numbers such that they add up to a specific target from a given array of integers.

Solution:

fun sum(values: Array<Int>, result: Int): Array<Int> {
  for ((i, e1) in values.withIndex()) {
    for ((j, e2) in values.drop(i + 1).withIndex()) { // ...or: values.drop(i + 1).forEachIndexed { j, e2 ->
      //println("$i : $e1 >> $j : $e2")
      if (e1 + e2 == result) {
        return arrayOf(i, i + j + 1)
      }
    }
  }
  return emptyArray()
}

Test cases:

class SumTest {
  @Test
  fun testSum() {
    Assert.assertArrayEquals(sum(arrayOf(4, 7, 9, 10, 6, 34), 10), arrayOf(0, 4))
    Assert.assertArrayEquals(sum(arrayOf(5, 7, 9, 10, 21, 34), 43), arrayOf(2, 5))
    Assert.assertArrayEquals(sum(arrayOf(2, 7, 11, 15), 9), arrayOf(0, 1))
    Assert.assertArrayEquals(sum(arrayOf(2, 7, 8, 6, 20), 11), emptyArray())
  }
}

I know this is a pretty simple algorithm, but I would like to avoid the return arrayOf(i, i + j + 1) and modifying the values array in the second loop (values.drop(i + 1).withIndex()) if possible. A functional-towards approach would be great!

sum

The word sum sounds like your taking an iterable of some sort and summing all the values in it. sumsTo would be a better name. Even then, I would expect to return all possible pairings, which the code doesn't currently do.

result

I wouldn't call it result. I'm not that familiar with Kotlin, but sometimes APIs will write to the value you pass in and so the function doesn't return anything but it modifies the result variable. Maybe target is a better choice?

Pair datatype

I would change your datatype from Array to Pair. You are returning a pair of integers satisfying the equation so why choose an array when there is a Pair data type object? I also suggest that if no solution is found that you, yes, return null.

null is often a bad design choice, but I strongly believe this is the exception. If there are no solutions we say that the solution set is null (or the empty set). So if there are no pairs the solution set is null. Reads just like math.

Generalizing to Iterable

You could just iterate over an array of values, but more generally you probably want to iterate over an Iterable collection of integers.

The one cost of this is you have to say arrayOf(...).asIterable().

The code

fun sumsTo(values: Iterable<Int>, target: Int): Pair<Int, Int>? {
  for ((i, e1) in values.withIndex()) {
    for ((j, e2) in values.drop(i + 1).withIndex()) {
      if (e1 + e2 == target) {
        return Pair(i, i + j + 1)
      }
    }
  }
  return null
}


Can this LINQ lambda expression be written any better? For the sake of knowledge, I wanted to convert a SQL query to a LINQ query expression. Here is the original SQL:

SELECT CT.COURSE_NODE_ID AS CNID, CT.NODE_TEXT
FROM    COURSE_RELATED_VERSIONS AS CRV INNER JOIN
        COURSE_TREE AS CT ON CRV.COURSE_NODE_ID = CT.COURSE_NODE_ID
WHERE (CRV.COURSE_ID = '38890') AND (CRV.COURSE_PARENT_NODE_ID = '-1')

Basically, just grabbing two fields from a table based on certain criteria (keys match, course ID matches criteria, parent node not equal -1). Here's the LINQ query expression that I came up with, using LINQPad:

from ct in COURSE_TREEs
join crv in COURSE_RELATED_VERSIONS on ct.COURSE_NODE_ID equals crv.COURSE_NODE_ID
where crv.COURSE_ID == 38890 && crv.COURSE_PARENT_NODE_ID == -1
select new {ct.COURSE_NODE_ID, ct.NODE_TEXT}

Not too bad, still understandable to my SQL-centric brain. But, for grins, I wondered what this query expression would look like as a lambda expression. For the life of me, I could not figure out the syntax. So, I used the lambda tool in LINQPad to see what my LINQ query expression would look like. Here it is:

COURSE_TREEs
.Join (
   COURSE_RELATED_VERSIONS,
   ct => ct.COURSE_NODE_ID,
   crv => crv.COURSE_NODE_ID,
   (ct, crv) =>
      new
      {
         ct = ct,
         crv = crv
      }
)
.Where (temp0 => ((temp0.crv.COURSE_ID == 38890) && (temp0.crv.COURSE_PARENT_NODE_ID ==      -1)))
.Select (
   temp0 =>
      new
      {
         COURSE_NODE_ID = temp0.ct.COURSE_NODE_ID,
         NODE_TEXT = temp0.ct.NODE_TEXT
      }
)

Whoah! Not what I figured the lambda expression query would look like. So, I'm studying the output from LINQPad on how my query expression looks as a lambda expression, and I'm wondering if it can be written any better? I'm still learning the ropes of LINQ (and lambda expressions), but I can't help but feel that the resulting lambda expression here is too complex! Am I wrong? Is it possible to write a lambda expression that produces the same output as the original SQL and query expression, but not be needlessly complex? Perhaps 'complex' is subjective, since it may only appear complex to my SQL brain. I just feel like the lambda expression generated in LINQPad can be written better... I just don't know how.

LINQ isn't as rigid about the order of clauses as SQL is (this applies to both syntaxes). This means that if you switch your Join around, you can put your Where before your Join.
If you have Select right after Join, you can combine the two together.
When the property you're using in an anonymous object creation expression has the same name as the one you're assigning it to, you can omit the name.

This means that your code could be simplified to this:

COURSE_RELATED_VERSIONS
.Where (crv => crv.COURSE_ID == 38890 && crv.COURSE_PARENT_NODE_ID == -1)
.Join (
   COURSE_TREEs,
   crv => crv.COURSE_NODE_ID,
   ct => ct.COURSE_NODE_ID,
   (crv, ct) => new { ct.COURSE_NODE_ID, ct.NODE_TEXT }
)

But in general, if you're doing something more complicated, you need to follow the pattern in your question. Which is exactly why the query syntax exists: it can make complicated queries much simpler.

Also, I would suggest that your tweak your mapping so that the names follow usual .Net naming conventions (e.g. CourseRelatedVersion or CourseNodeId).


Filtering a deep array I'm implementing a search for a deep array. It applies the search/filter to the deepest level of children below, i.e. { name: 'foo' }, { name: 'bar' }.

If all children are filtered from the parent object, the parent is removed from the list too.

Since I want to leave the original object intact, I copy each container object and add a new children property to it.

I think this has a lot of room for improvement. It feels like it can be simplified. Also, this doesn't feel DRY.

var data = [
    {
    name: 'a1', children: [
        {
        name: 'b1', children: [
            { name: 'foo' }, { name: 'bar' }
        ]
      }
    ]
  },
  {
    name: 'a2', children: [
        {
        name: 'b2', children: [
            { name: 'baz' }, { name: 'bah' }
        ]
      }
    ]
  }
];

function filterData(data, filter){
    var result = [];
    _.forEach(data, function(firstLevelItem){
    var newFirstLevelItem = _.extend({}, firstLevelItem, { children: [] });

    _.forEach(firstLevelItem.children, function(secondLevelItem){
        var newSecondLevelItem = _.extend({}, secondLevelItem, {
        children: _.filter(secondLevelItem.children, filter)
      });

      if(newSecondLevelItem.children.length > 0) {
        newFirstLevelItem.children.push(newSecondLevelItem);
      }
    });
    if(newFirstLevelItem.children.length > 0) {
        result.push(newFirstLevelItem);
    }
  });
  return result;
}

var result = filterData(data, function(item){
    return item.name === 'foo';
});

console.log(result);

https://jsfiddle.net/b2xsna5k/

I went functional on this one (as usual).

First of all, i removed the dependency from the third party library. I haven't anything in particular against that, but if the problem can be resolved easily with the standard tools, why bloat your app?

Then, since the main function deals mostly with lists, I looked for a way that would let me transform the list, filtering and transforming freely: reduce is a natural fit.

Now i can check the immediate list against a predicate (the filtering method you pass in) and recursively do the same on the list of childrens. The result of this filtering determines the final output structure.

Here's an implementation:

var data = [
    {
    name: 'a1', children: [
        {
        name: 'b1', children: [
            { name: 'gap' }, { name: 'bar' }
        ]
      }
    ]
  },
  {
    name: 'a2', children: [
        {
        name: 'b2', children: [
            { name: 'foo' }, { name: 'gap' }
        ]
      }
    ]
  }
];

function filterData(data, predicate) {

 // if no data is sent in, return null, otherwise transform the data
 return !!!data ? null : data.reduce((list, entry) => {

  let clone = null;
  if (predicate(entry)) {
   // if the object matches the filter, clone it as it is
   clone = Object.assign({}, entry)
  } else if (entry.children != null) {
   // if the object has childrens, filter the list of children
   let children = filterData(entry.children, predicate)
   if (children.length > 0) {
    // if any of the children matches, clone the parent object, overwrite
    // the children list with the filtered list
    clone = Object.assign({}, entry, {children: children})
   }
  }

  // if there's a cloned object, push it to the output list
  clone && list.push(clone)
  return list
 }, [])

}

var result = filterData(data, function(item) {
    return item.name === 'foo';
});

console.log(JSON.stringify(result, null, 2));

edit: I added some comments to the code. Shouldn't be needed, but it clarifies the steps.


A D wrapper around a C function Any problems with this D wrapper around libcomcom_run_command() C function from C library libcomcom?

Does it work as expected? Is it idiomatic? Is it the right way to do this?

module libcomcom_wrapper;

import std.string;
import std.algorithm.iteration : map;
import std.array : array;
import std.exception : ErrnoException;
import libcomcom;

string runCommand(string file, string[] argv, string[] envp, string input, int timeout = -1) {
    const(char*) output;
    size_t output_len;
    const char*[] childArgv = map!(s => s.toStringz)(argv).array;
    const char*[] childEnvp = map!(s => s.toStringz)(envp).array;
    immutable int res = libcomcom_run_command(input.ptr, input.length,
                                              &output, &output_len,
                                              file.toStringz, childArgv.ptr,
                                              childEnvp.ptr,
                                              timeout);
    if (res != 0) {
        throw new ErrnoException("Run command"); // TODO: Localization
    }
    import core.stdc.stdlib : free;
    scope(exit) free(cast(void*) output);
    return output[0..output_len].idup;
}

The code was taken here.

Nitpick: map!(s => s.toStringz)(argv) would be more idiomatic as argv.map!toStringz. Apart from that, your code looks very much correct. It should work as expected, and is indeed the way to do this.


Optimization of Factorial Function This is a solution to the CodeHhef problem FCTRL2 where we are supposed to find the factorial of numbers up to 100. I have tried several techniques but I am always exceeding their time limit (1 second). Please help me optimize this code to make it execute faster.

    import java.math.BigInteger;
    import java.io.*;
    class Abc{
        public static void main(String[] args) throws IOException
    {
        BufferedReader br=new BufferedReader(new InputStreamReader(System.in));
        BigInteger fact=new BigInteger("1");
       // BigInteger inc=new BigInteger("1");
        String str=br.readLine();
        int n=Integer.parseInt(str);

        for(int i=0;i<n;i++)
        {    String st=br.readLine();
            BigInteger number=new BigInteger(st);

            for(BigInteger bi = BigInteger.valueOf(1);bi.compareTo(number)>=0;bi = bi.add(BigInteger.ONE))
            {
                fact=fact.multiply(bi);
            }
            System.out.println(fact);
        }
    }
   }

It is unclear what n and st are in your code and you are mixing things together which makes fixing your code harder.

I therefore suggest refactoring your code:

extract the factorial calculation in its own method, without all the string conversion and input reading stuff:

private static BigInteger factorial(int n) {
  BigInteger fact = new BigInteger("1");
  for (int i = 1; i <= n; i++) {
    fact = fact.multiply(BigInteger.valueOf(i));
  }
  return fact;
}

adapt your main method accordingly

Once you have done that, you can start optimising the factorial method, for example by caching the results as you calculate them.


Long calendar related function This works but feels like alot of code for what it accomplishes. I feel like I'm maybe missing something obvious.

// When current month is same as target month, then return the current day of the month.
// Else return the number of days in the target month.
static int DefaultDaysToSynch()
{
    var now = DateTime.Now;
    int result = now.Day;
    int month = Properties.Settings.Default.StatsMonth;
    int year = Properties.Settings.Default.StatsYear;
    if (!((year == now.Year) && (month == now.Month)))
    {
        result = DateTime.DaysInMonth(year, month);
    }
    return result;
}

It is hard to improve on Jesse's answer (there are only so many ways to write this method), but I will add my alternative, which is a matter of style. I personally tend to not use var in place of any of the built-in types listed here: http://msdn.microsoft.com/en-us/library/ms228360(v=vs.90).aspx it does not save me any more space than writing "don't" instead of "do not".

I also do not like it when the lines get too long, particularly ones that involve the ternary operator, so here is my version (assuming public modifier):

public static int DefaultDaysToSynch()
{
    int month = Properties.Settings.Default.StatsMonth;
    int year = Properties.Settings.Default.StatsYear;
    var now = DateTime.Now;
    bool isTargetMonth = (year == now.Year) && (month == now.Month);
    return isTargetMonth ? now.Day : DateTime.DaysInMonth(year, month);
}

In general I highly recommend runnign StyleCop and listening to it, except for the line bool isTargetMonth = (year == now.Year) && (month == now.Month); - keep those parenthesis.
Btw, I am pretty sure that the C# compiler can handle that bool variable efficiently. The descriptive variable name also reduces (though maybe not completely) the need for a comment.

The comment would have to pass the StyleCop's standard if I wrote it out, but I am too lazy, sorry. There is not much to write really, because it does not take any arguments.

I am not a huge fan of the input values being read directly from settings - it makes this function harder to test, it makes it less "functional" as in functional style, it also allows for bad input (such as year -2598, month # 455) to go unnoticed. I would rather pass both values into the function like so:

public static int DefaultDaysToSynch(int targetYear, int targetMonth)
{
    CheckYear(targetYear);
    CheckMonth(targetMonth);
    var now = DateTime.Now;
    bool isTargetMonth = (targetYear == now.Year) && (targetMonth == now.Month);
    return isTargetMonth ? now.Day : DateTime.DaysInMonth(year, month);
}

// You will need to catch this at the right place.
// This is lengthy, I know, but makes a decent exception message.
private static CheckMonth(int monthNumber)
{
    if (monthNumber >= 1 && monthNumber <= 12)
    {
        return;
    }

    string exceptionMessage = String.Format(
        "Month number must be between 1 and 12 but it was {0}.",
        monthNumber);

    throw new ArgumentOutOfRangeException(
        paramName: "monthNumber",
        actualValue: monthNumber,
        message: exceptionMessage);
}

// Similar implementation for CheckYear


Go Go Gadget Web Crawler In A Tour of Go, you are given the following problem:

  In this exercise you'll use Go's concurrency features to parallelize a
  web crawler.

  Modify the Crawl function to fetch URLs in parallel without fetching
  the same URL twice.

This is the code you're given

package main

import (
    "fmt"
)

type Fetcher interface {
    // Fetch returns the body of URL and
    // a slice of URLs found on that page.
    Fetch(url string) (body string, urls []string, err error)
}

// Crawl uses fetcher to recursively crawl
// pages starting with url, to a maximum of depth.
func Crawl(url string, depth int, fetcher Fetcher) {
    // TODO: Fetch URLs in parallel.
    // TODO: Don't fetch the same URL twice.
    // This implementation doesn't do either:
    if depth <= 0 {
        return
    }
    body, urls, err := fetcher.Fetch(url)
    if err != nil {
        fmt.Println(err)
        return
    }
    fmt.Printf("found: %s %q\n", url, body)
    for _, u := range urls {
        Crawl(u, depth-1, fetcher)
    }
    return
}

func main() {
    Crawl("http://golang.org/", 4, fetcher)
}

// fakeFetcher is Fetcher that returns canned results.
type fakeFetcher map[string]*fakeResult

type fakeResult struct {
    body string
    urls []string
}

func (f fakeFetcher) Fetch(url string) (string, []string, error) {
    if res, ok := f[url]; ok {
        return res.body, res.urls, nil
    }
    return "", nil, fmt.Errorf("not found: %s", url)
}

// fetcher is a populated fakeFetcher.
var fetcher = fakeFetcher{
    "http://golang.org/": &fakeResult{
        "The Go Programming Language",
        []string{
            "http://golang.org/pkg/",
            "http://golang.org/cmd/",
        },
    },
    "http://golang.org/pkg/": &fakeResult{
        "Packages",
        []string{
            "http://golang.org/",
            "http://golang.org/cmd/",
            "http://golang.org/pkg/fmt/",
            "http://golang.org/pkg/os/",
        },
    },
    "http://golang.org/pkg/fmt/": &fakeResult{
        "Package fmt",
        []string{
            "http://golang.org/",
            "http://golang.org/pkg/",
        },
    },
    "http://golang.org/pkg/os/": &fakeResult{
        "Package os",
        []string{
            "http://golang.org/",
            "http://golang.org/pkg/",
        },
    },
}

And this is the solution I came up with

type Job struct {
    url   string
    depth int
    done  chan bool
}

func dedup(fetcher Fetcher, ch chan Job) {
    seen := make(map[string]bool)
    for job := range ch {
        if _, ok := seen[job.url]; ok || job.depth <= 0 {
            job.done <- true
            continue
        }
        seen[job.url] = true
        go Crawl(job, fetcher, ch)
    }
}

func Crawl(job Job, fetcher Fetcher, q chan Job) {
    body, urls, err := fetcher.Fetch(job.url)
    if err != nil {
        fmt.Println(err)
        job.done <- true
        return
    }
    fmt.Printf("found: %s %q\n", job.url, body)
    ch := make(chan bool, len(urls))
    for _, u := range urls {
        u := u
        go func() { q <- Job{u, job.depth - 1, ch} }()
    }
    for i := 0; i < len(urls); i++ {
        <-ch
    }
    job.done <- true
    return
}

func main() {
    done := make(chan bool, 1)
    q := make(chan Job)
    go dedup(fetcher, q)
    q <- Job{"http://golang.org/", 4, done}
    <-done
}

All jobs go through dedup's channel, so that a given URL is fetched only once. dedup kicks off a goroutine for each unique URL it sees, and the goroutine then adds URLs from that page back to the channel.

Go's way of thinking is new to me, so my main concerns are

Is it idiomatic?
Is it clear what's going on to someone who's familiar with Go?
Is buffering the channels in this way necessary (or even helpful)?

Your map is of booleans so you can replace if _, ok := seen[job.url]; ok || bar with if seen[job.url] || bar. The zero value (which is what gets returned for non-existing map lookups) of bool is false.
It would be better to have Crawl do something like

defer func() {
    job.done <- true
}()

immediately once only rather than sprinkle job.done <- true at all the right places and hoping you (and any future editors) don't ever miss any.
Your for loop in Crawl should either just require the channel is sufficiently buffered (as it currently is) and not bother with goroutines, or the whole for loop should be put into a single goroutine (and then you don't need any channel buffering).
for i := 0; i < len(urls); i++ should be just for i := range urls (or since you don't need i just for _ = range urls).
You seem to be using Jobs.done like sync.WaitGroup.

You could just have a single wait group that keeps track of outstanding jobs. Your main would make sure to call wg.Add(1) before sending it's first job and before it then does wg.Wait(). Mostly everywhere you do job.done <- true would then be just wg.Done(). You'd just need to make sure that your crawler did wg.Add(len(urls)) before it did wg.Done() to make sure the wait group count could never accidentally fall to zero too early.
Although having dedup check the depth is probably still a good idea, it's easy and trivially to do it in the crawl function and save some effort.

You code modified with the above points:

type Job struct {
    url   string
    depth int
}

func dedup(fetcher Fetcher, ch chan Job, wg *sync.WaitGroup) {
    seen := make(map[string]bool)
    for job := range ch {
        if seen[job.url] || job.depth <= 0 {
            wg.Done()
            continue
        }
        seen[job.url] = true
        go Crawl(job, fetcher, ch, wg)
    }
}

func Crawl(job Job, fetcher Fetcher, q chan Job, wg *sync.WaitGroup) {
    defer wg.Done()
    body, urls, err := fetcher.Fetch(job.url)
    if err != nil {
        log.Println("Crawl failure:", err) // TODO: something better with errors
        return
    }
    log.Printf("Crawl: found %s\t%q\n", job.url, body)

    if job.depth <= 1 {
        return
    }

    wg.Add(len(urls))
    for _, u := range urls {
        q <- Job{u, job.depth - 1}
    }
}

func main() {
    wg := new(sync.WaitGroup)
    wg.Add(1)

    q := make(chan Job)
    go dedup(fetcher, q, wg)
    q <- Job{"http://golang.org/", 4}
    wg.Wait()

    // We close q here so that the dedup goroutine
    // will finish. This isn't strictly needed here
    // since we're main and everything gets stopped
    // when we return; but that wouldn't be the case
    // if we were some other function in a long lived
    // program.
    close(q)
}

And runnable on the playground (by including the Go tour stuff):
http://play.golang.org/p/cuY3PZdQmI

(For comparison: your original code is also runnable on the playground.)


loadSmartConfig function I am writing a conditional that checks two values, for several different scenarios.

What I am modifying is a series of objects, some of these objects are in arrays, and some are just arrays. I need to check these values and then check a file, and then check my values (the series of objects, array objects, and arrays) and then "Update" the old data with my new data if needed. However the user has the ability to turn some sections off if they don't want their data to be overwritten in the case of changes. So in order to do this I check if a certain property exists (the ones used to turn them on or off) and then check if its on or if its off. which has led to a conditional of this size: (And this is NOT everything, I stopped here to see if there was a shorter less counter-productive way of doing this

if ((this.Config[key] === this.Config.PunishTiers || this.Config[key] === this.Config.Titles || this.Config[key] === this.Config.Ranks || this.Config[key] === this.Config.Messages && this.Config.Message.Messages) || (this.Config[key] === this.Config.Prefix && this.Config.Prefix.Prefixes))
             || (this.Config[key] === this.Config.Settings && this.Config.Settings.Settings)) {
                oldData.push(this.Config[key]);
        }

The first three checks do not have the setting, so they are fine but everything after has the check if it exists, and check for the setting to be on or off. Is there any way to reduce this? Make it neater and possibly less of a struggle when used.

This has to stick with mainly pure JavaScript if possible.

This is being used through a plugin for the oxide framework. To check a plugin's configuration file grab the data, check for local configs and apply changes where specified. or "Updates"

Here is the whole function setup:

loadSmartConfig: function(changed) {
            print("Smart Updating Data Now.")
            var oldData = [],
                newData = [];
            for (var key in this.Config) {
                if (this.Config[key] === this.Config.PunishTiers || this.Config[key] === this.Config.Titles || this.Config[key] === this.Config.Ranks) {
                    oldData.push(this.Config[key]);
                if ((this.Config[key] === this.Config.Messages && this.Config.Message.Messages)
                    || (this.Config[key] === this.Config.Prefix && this.Config.Prefix.Prefixes)
                    || (this.Config[key] === this.Config.Settings && this.Config.Settings.Settings)) {
                    oldData.push(this.Config[key]);
                }
                } else {
                    continue;

}
            }
            if (oldData.length > 0) {
                this.LoadDefaultConfig();
            }

        if (this.Config.PunishTiers !== undefined && this.Config.Titles !== undefined && this.Config.Ranks !== undefined) {
            for (var i = 0; i < oldData.length; i++) {
                if (oldData[i].length === 1) {
                    this.Config.PunishTiers = oldData[i];
                } else if (oldData[i].hasOwnProperty("authLvl")) {
                    this.Config.Titles = oldData[i];
                } else if (oldData[i].hasOwnProperty("rank")) {
                    this.Config.Ranks = oldData[i];
                } else if (oldData[i].hasOwnProperty("Messages") && oldData[i].Messages) {
                    this.Config.Messages = oldData[i];
                } else if (oldData[i].hasOwnProperty("Prefixes") && oldData[i].Prefixes) {
                    this.Config.Prefix = oldData[i];
                } else if (oldData[i].hasOwnProperty("Settings") && oldData[i].Settings){
                    this.Config.Settings = oldData[i];
                }
            }
            this.SaveConfig();
        },

this is also why I was pushing for pure JavaScript if possible.

It's hard to analyze without the whole context, but a possible workaround could be to add the whole keys in a collection, and apply a filter like _.some of underscore.js.

var aColection = [this.Config.Ranks, this.Config.Message,..]
_.some(aCollection, function (element) {
  return this.Config[key] === element
})

If you don't want add this library to your core code, you can simply copy  the definition of some.

GitHub


Multiplication of 3×3 and N×N square matrices I recently wrote a matrix module in C++.

During the development process I quoted some source code and found a problem.

For example, matrix multiplication:

This way is suitable for all N×N matrices：

void multiplyMatrix(const float32* a, const float32* b, float32* dst,
                    int32 aColumns, int32 bColumns, int32 dstColumns, int32 dstRows) {

    for (int32 i = 0; i < dstRows; i++) {

        for (int32 j = 0; j < dstColumns; j++)
            dst[i * dstColumns + j] = dotMatrix(a, b, aColumns, bColumns, j, i);

    }
}

float32 dotMatrix(const float32* a, const float32* b,
                         int32 aColumns, int32 bColumns,
                         int32 column, int32 row) {

    float32 result = 0.0f;

    int32 index = aColumns * row;
    for (int32 i = 0; i < aColumns; i++) {
        result += a[index++] * b[column];
        column += bColumns;
    }

    return result;
}

Next, I wrote a 3x3 matrix class.

class Matrix3x3
{
public:
float32 m11, m12, m13,
        m21, m22, m23,
        m31, m32, m33;

float32 element[9];

void multiply(float32 ma11, float32 ma12, float32 ma13,
              float32 ma21, float32 ma22, float32 ma23,
              float32 ma31, float32 ma32, float32 ma33) {

    float32 temp1 = m11 * ma11 + m21 * ma12 + m31 * ma13;
    float32 temp2 = m12 * ma11 + m22 * ma12 + m32 * ma13;

    m13 = m13 * ma11 + m23 * ma12 + m33 * ma13;
    m11 = temp1;
    m12 = temp2;

    temp1 = m11 * ma21 + m21 * ma22 + m31 * ma23;
    temp2 = m12 * ma21 + m22 * ma22 + m32 * ma23;
    m23   = m13 * ma21 + m23 * ma22 + m33 * ma23;
    m21   = temp1;
    m22   = temp2;

    temp1 = m11 * ma31 + m21 * ma32 + m31 * ma33;
    temp2 = m12 * ma31 + m22 * ma32 + m32 * ma33;
    m31   = m13 * ma31 + m23 * ma32 + m33 * ma33;
    m32   = temp1;
    m33   = temp2;
}
}

Obviously the first one is very convenient.

Next, I tested the time it took to calculate：

        float32 e1[9];
        e1[0] = 2.1018f;   e1[1] = -1.81754f; e1[2] = 1.2541f;
        e1[3] = 0.54194f;  e1[4] = 2.75391f;  e1[5] = -0.1167f;
        e1[6] = -5.81652f; e1[7] = -7.9381f;  e1[8] = 4.2816f;

        float32 e2[9];
        e2[0] = 2.1018f;   e2[1] = -1.81754f; e2[2] = 1.2541f;
        e2[3] = 0.54194f;  e2[4] = 2.75391f;  e2[5] = -0.1167f;
        e2[6] = -5.81652f; e2[7] = -7.9381f;  e2[8] = 4.2816f;

        Matrix3x3 a;
        a.m11 = 2.1018f;   a.m12 = -1.81754f; a.m13 = 1.2541f;
        a.m21 = 0.54194f;  a.m22 = 2.75391f;  a.m23 = -0.1167f;
        a.m31 = -5.81652f; a.m32 = -7.9381f;  a.m33 = 4.2816f;

        Matrix3x3 b = a;

        float64 timeSpent = 0;
        LARGE_INTEGER nFreq;
        LARGE_INTEGER nBeginTime;
        LARGE_INTEGER nEndTime;

        QueryPerformanceFrequency(&nFreq); // statistical frequency
        QueryPerformanceCounter(&nBeginTime);// start timer

        for (int32 i = 0; i < 100000; i++) {
            multiplyMatrix(e1, e2, dst, 3, 3, 3, 3);
        }

    QueryPerformanceCounter(&nEndTime); //end timer
            timeSpent = (float64)(nEndTime.QuadPart - nBeginTime.QuadPart) / (nFreq.QuadPart);

   printf("timeSpent1:%f\n", timeSpent);

   QueryPerformanceCounter(&nBeginTime);
   for (int32 i = 0; i < 100000; i++) {
            b.multiply(a.m11, a.m12, a.m13,
                       a.m21, a.m22, a.m23,
                       a.m31, a.m32, a.m33);
        }
   QueryPerformanceCounter(&nEndTime);
        timeSpent = (float64)(nEndTime.QuadPart - nBeginTime.QuadPart) / (nFreq.QuadPart);

printf("timeSpent2:%f\n", timeSpent);

Output：

timeSpent1:0.014277
timeSpent2:0.004649
timeSpent1:0.012684
timeSpent2:0.004522
.......
.......
timeSpent1:0.003414
timeSpent2:0.001166
timeSpent1:0.003407
timeSpent2:0.001242

Is this difference in efficiency significant or negligible?

A factor of 3 is large, but in my opinion not unexpected or abnormal. The functions that can handle a variable size matrix in their natural form (ie as they would be compiled without knowledge of the size, for example if the functions are defined in a different compilation unit than they are used in and LTO is not applied) have a lot of overhead: non-linear control flow (3 nested loops), more complicated address computation (involving multiplication by a variable).

Basically, that is the cost of generality .. but there is more to it.

From your use of QueryPerformanceCounter I assume you use MSVC (other compilers aren't much different for the following considerations). MSVC likes to unroll loops such as the one in dotMatrix by 4. It does not like to unroll such loops by 3, though it can be persuaded to do so anyway, for example by giving it a loop that makes exactly 3 iterations. So the cost of generality would work out much differently if the relevant matrix was of size 4x4 or 8x8, as in those cases only the faster unrolled codepath would be used (this still comes with overhead, but less). 3 is a bad case, only ever using the fallback codepath.

Additionally, the general matrix multiply implemented by multiplyMatrix is not scalable: it does not implement cache blocking, so for any matrix that does not fit in L1 cache it will perform badly (and even more badly when going beyond the L2 and L3 sizes). That is normal for code in general, but matrix multiplication is special in that it does not have to suffer significantly from that common effect thanks to its "O(n2) data in O(n3) time" property.

Both the general matrix multiply and the special 3x3 one could use SIMD intrinsics for extra efficiency. 3x3 is an awkward size that would cause some "wasted lanes", but it would still help. For example, it could be done like this (not tested):

#include <xmmintrin.h>

class Matrix3x3
{
public:
float32 m11, m21, m31,
        m12, m22, m32,
        m13, m23, m33, padding;

void multiply(float32 ma11, float32 ma12, float32 ma13,
              float32 ma21, float32 ma22, float32 ma23,
              float32 ma31, float32 ma32, float32 ma33) {

    __m128 col1 = _mm_loadu_ps(&m11);
    __m128 col2 = _mm_loadu_ps(&m12);
    __m128 col3 = _mm_loadu_ps(&m13);
    __m128 t1 = _mm_add_ps(_mm_add_ps(
        _mm_mul_ps(col1, _mm_set1_ps(ma11)),
        _mm_mul_ps(col2, _mm_set1_ps(ma21))),
        _mm_mul_ps(col3, _mm_set1_ps(ma31)));
    __m128 t2 = _mm_add_ps(_mm_add_ps(
        _mm_mul_ps(col1, _mm_set1_ps(ma12)),
        _mm_mul_ps(col2, _mm_set1_ps(ma22))),
        _mm_mul_ps(col3, _mm_set1_ps(ma32)));
    __m128 t3 = _mm_add_ps(_mm_add_ps(
        _mm_mul_ps(col1, _mm_set1_ps(ma13)),
        _mm_mul_ps(col2, _mm_set1_ps(ma23))),
        _mm_mul_ps(col3, _mm_set1_ps(ma33)));

    _mm_storeu_ps(&m11, t1);
    _mm_storeu_ps(&m12, t2);
    _mm_storeu_ps(&m13, t3);
}
};

The padding is a bit unfortunate (and shouldn't be private, because that makes its positioning relative to the actual matrix elements undefined), but simplifies the SIMD logic, chunks of 16 bytes are easier to deal with. It is possible to avoid the padding if required. Anyway, this results in a significant reduction in code and should be more efficient (without AVX the set1s cost more, that shouldn't be enough to undo the improvement but I didn't try it). The dllexport in the code on godbolt is not really part of the code, I just put that there to force code to be generated for an otherwise unused method.

Column-major order is used here because the columns of the result are a linear combination of the columns of the left hand matrix, which we have access to in packed memory. Similarly, the rows of the output are a linear combination of the rows of the right hand side, but we have no packed access to the rows of the right hand side, so they would be inefficient to gather. A row-oriented version of the above could be arranged for example if the right hand side was passed in as a reference to a Matrix3x3.

Passing the right hand side as matrix is probably a nicer interface anyway, with 9 separate arguments there is no choice but to write them all out separately even if the RHS is available as a matrix object, as you already experienced in your benchmark code.


Primitive solution of squeeze As some of you might know I'm working my way through The C Programming Language as recommended by the "The Definitive C Book Guide and List" on StackOverflow.

I just finished my version of squeeze(s1, s2), a function to remove all occurrences of s2 in s1 and was hoping to get some feedback on my solutions.

This exercise is part of the second chapter (ex 2.4) so we haven't really gone through pointers and de-referencing yet. So basically all experience I have with the language is with using basic and primitive data types and the previous feedback I've received here!

All sort of feedback would be greatly appreciated; syntactical changes, algorithmic shortcomings etc. Just keep in mind that I just recently started out with the language!

Here is the code:

/*
    Exercise 2.4
    squeeze (s1, s2): Remove all characters of s2 in s1.
    INPUT : s1.length >= s2 > 0.
    OUTPUT: The rest of s1 after deleting all occurances of letters in s2.
*/

#include <stdio.h>

void squeeze (char s1[], const char *s2);  /* Returns (by-ref) the resulting string s1 after removing all occurences of s2. */
int toUpper(char c);                       /* Returns the numerical representation of a hexadecimal digit. */
int contains(char toCheck, const char *toRemove); /* Function to see if toRemove contains toCheck. */

int main () {
    char s1[] = "I am a test.\0";
    const char *s2 = "AE\0";
    printf("Before manipulation: %s\n", s1);
    squeeze(s1, s2);
    printf("After manipulation:  %s", s1);
}

/*
    Returns the (by-ref) resulting string s1 after removing all occurences
    of letters in s2.
*/

void squeeze (char s1[], const char *s2) {
    int index, left_shift;

    for (index = 0, left_shift = 0; s1[index] != '\0'; index++) {
        if (contains(s1[index], s2))
            continue;
        s1[left_shift] = s1[index];
        left_shift++;
    }
    s1[left_shift] = '\0';
}
/*
    Returns the upper-case representation of char c.
*/

int toUpper (char c) {
    if (c >= 'a' && c <= 'z')
        return c - 32;
    else
        return c;
}

/*
    Returns an indicator (integer) of if
    the string toRemove contains toCheck.
*/
int contains(char toCheck, const char *toRemove) {
    int index = 0, c = 0;

    while (toRemove[index] != '\0') {
        if ((c = toUpper(toCheck)) == toRemove[index]){
            return 1;
        }
        index++;
    }
    return 0;
}

The code is relatively neat and appears to work properly, so you seem to be well on your way to mastering C.  Here are some things that may help you on that road.

Strings are automatically terminated

The code currently includes these two lines:

char s1[] = "I am a test.\0";
const char *s2 = "AE\0";

However, the explicit '\0' at the end of these strings is not necessary.  A C string is automatically terminated with an implicit '\0' character, so these lines would normally be written as:

char s1[] = "I am a test.";
const char *s2 = "AE";

Move function bodies to avoid declaration

Modern C no longer requires function declarations before every function definition.  As long as, reading from top to bottom, each function definition does not refer to anything not yet defined, the function declarations may be omitted.  So in this program, if you put the function definitions in the order toUpper, contains, squeeze and main, you can omit the declarations.  This is handy because it means you only need to maintain the function signature in one place rather than two.

Use bool instead of int where appropriate

The contains function returns 1 if the value is found or 0 otherwise.  That is, it's essentially returning a boolean value.  You can make that more clear by using #include <stdbool.h> and writing the function as:

bool contains(char toCheck, const char *toRemove) { /* ... */ }

Then the return statements can be return true; and return false;  As documented here, bool has been standard C since 1999.

Use standard functions

I know this is a learning exercise and you're just starting, but it's generally encouraged to use standard functions rather than duplicating them.  For instance, toupper has been part of the standard since 1989.

Avoid creating unneeded variables

The variable c in the contains() routine is not really needed.  Instead of this:

if ((c = toUpper(toCheck)) == toRemove[index]){

You can write this:

if (toUpper(toCheck) == toRemove[index]){


Secure/Bulletproof Image Upload Using PHP Here is my code that check and upload an image to my website.
Is this code really secure against uploading of malicious codes?
Anything I have to add to this code to  make it more secure?
I have also blocked execution of PHP in images folder using php_flag engine off in .htaccess of images folder, I can also move images folder out of the public_html (root) folder.
    <?php
if(isset($_POST['submit'])){
    // permenant variabes for upload
    $target_dir = "./images/";
    $allowed = array('jpg' => 'image/jpeg', 'png' => 'image/png', 'gif' => 'image/gif');
    $maximumsize=5000000000;
    //variables form file
    $filetempname=$_FILES["image"]["tmp_name"];
    $fileinfo = new finfo(FILEINFO_MIME_TYPE);
    $filemime = $fileinfo->file($_FILES['image']['tmp_name']);
    //check if error is set
    if(!isset($_FILES['image']['error']) ){
        echo 'Invalid upload.';
        die();
    }
    // check formultiple uploads and block
    elseif(is_array($_FILES['image']['error'])){
        echo 'Only one file allowed.';
        die();
    }
    //check for any upload error
    elseif($_FILES['image']['error']===0){
        // check image size
        if($_FILES["image"]["size"] > $maximumsize){
            echo 'File size exceed maximum size.';
            die();
        }
        $ext = array_search($filemime, $allowed, true);
        if(false !== $ext){
            //upload file by giving new name
            $newfilename = md5(uniqid('', true));
            $newfilename= $newfilename.'.'.array_search($filemime, $allowed);
            $target_file = $target_dir .$newfilename;
            if (move_uploaded_file($_FILES["image"]["tmp_name"], $target_file)) {
                echo "uploaded.";
            }
            //upload failed message
            else{
                echo "upload failed";
                die();
            }
        }
        // if file format is wrong
        else{
            echo "invalid file format";
            die();
        }
    }
    // if there is any upload error
    else{
        echo "error in upload";
        die();
    }
}
else{
    echo "not submited form properly";
}
?>

Do you have any suggestions to make this more secure or can you spot any flaws in this script that could be used by hackers to upload shell or PHP scripts?

Questions

Is this code really secure against uploading of malicious codes ? Anything i have to add to this code to make it more secure?

My initial thought is to ensure that the uploaded file is not an executable file or a file that may be run as a separate script- e.g. PHP or some other language the server may handle. The Mime type checking from finfo::file() should handle that.
You could also :

check that a GD function like imagecreatefromgif(), imagecreatefromjpeg(), etc. does not return FALSE
check the permissions on the file after it is uploaded to ensure anything that shouldn’t be executed cannot be executed - perhaps use is_executable(), fileperms and chmod
like you say: “move images folder out of public_html (root) folder.”- Ensure that any uploaded files are not stored in a web accessible directory unless that is your desire. This may lead to needing a (PHP/other) script or web server module to read files.

Suggestions
avoid excess else
Whenever a conditional block contains a die statement then subsequent code does not need to be prefixed with else e.g.

//check if error is set
if(!isset($_FILES['image']['error']) ){
    echo 'Invalid upload.';
    die();
}
// check formultiple uploads and block
elseif(is_array($_FILES['image']['error'])){
    echo 'Only one file allowed.';
    die();
}

When else is used it adds more complexity to the decision tree.
Returning early
Going along with the previous point, in some parts of the code when a precondition is not met then an error message is printed to the screen before die() is called. However this is not true in all cases - e.g. the first conditional:

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

Flipping the logic on that so the error message that is displayed when that condition is false and adding a call to die would allow the indentation levels to be decreased dramatically, which would likely make the code more readable. Further improvement would come from moving some logic to functions - e.g. a function to determine if the file can be uploaded - if not then return early.
With such changes the decision tree of the logic could go from something like this:

screenshot captured from source: Your code sucks, let's fix it - By Rafael Dohms at 15:17
To this:

screenshot captured from source: Your code sucks, let's fix it - By Rafael Dohms at 15:28
storing max size

    $maximumsize=5000000000;

Since that value shouldn’t be reassigned during the runtime it is advisable to use a constant instead, and a common convention in many programming languages is to use all caps, which is in-line with the PHP Standards recommendation PSR-1 - e.g.
const MAXIMUM_SIZE = 5000000000;

And define those near the top of the code or in a configuration file to allow quick access should you ever need to change the value - that way you won’t need to go hunting through the code to find it.
Array syntax
There isn't anything wrong with using array() but as of the time of writing, PHP has Active support for versions 8.0 and 7.4, and since PHP 5.4 arrays can be declared with short array syntax (PHP 5.4).

$allowed = array('jpg' => 'image/jpeg', 'png' => 'image/png', 'gif' => 'image/gif');

Can be shortened slightly to
$allowed = ['jpg' => 'image/jpeg', 'png' => 'image/png', 'gif' => 'image/gif'];

checking mimetype
$filemime could either be string or Boolean - check for Boolean, then if it isn’t then in_array() could be used to determine if it exists in the array instead of using array_search()
Appending to $newfilename
The extension is appended to the file name:

 $newfilename= $newfilename.'.'.array_search($filemime, $allowed);

The dot equals operator can be used to append to the variable:
 $newfilename .= '.' . array_search($filemime, $allowed);

Spaces added for readability.


fast reciprocal (1/x) I was seeing alot of machine learning applications calculate fitness via 1.0 / x. Division in computers is slow so I came up with a version that is about ~36% fast on my architecture (-march=x86-64).
#include <cmath>    //std::exp2, std::log
#include <numbers>  //std::numbers::log2e
#include <stdlib.h> //rand, srand
#include <iostream> //std::cout

float reciprical( float y )
{
    //e^-ln(y)
    //std::pow( log10e,  -std::log(y) )                   //~19s
    return std::exp2(std::numbers::log2e * -std::log(y)); //~17s
}

float reciprical2( float y )
{
    return 1/y;                                           //~23s
}

int main(){
    srand(1);
    for(int i=0; i < 10000000000; ++i) std::cout << reciprical(rand());
}

I got inspiration from https://codegolf.stackexchange.com/a/114588

Including IO in a benchmark is no good. I tried some different benchmarks on quick-bench and the results were different: the versions with Expensive Functions (log, exp, etc) were much slower. Looking at the assembly, there was no "cheating" such as vectorizing the benchmark loop with the plain division - on the other hand, maybe that wouldn't be cheating, but a realistic representation of how it may work out in real situations. So you could even say that the division is likely to be better than this result shows in many (but not all) situations.
Such results do depend on the specific micro-architecture though. As a specific example, divss on Intel Haswell and before had less than half the throughput than on Intel Broadwell and later. log and exp can be implemented without division, so the relative cost will be different on different processors. That may also explain part of the differences we observed, but that does not take away that benchmarks should not include significant sources of overhead that aren't the subject of the benchmark.
On your architecture (and many others) there is a special hardware-accelerated approximate reciprocal. Indeed on some architectures, division is not a primitive at all and can only be implemented in terms of such an approximate reciprocal (that's tricky). For x86, you can write this, for a quite accurate but not perfect reciprocal:
float reciprocal(float y)
{
    float r = _mm_cvtss_f32(_mm_rcp_ss(_mm_set_ss(y)));
    r = r * (2 - r * y);
    return r;
}

The Newton-Raphson step is optional if you can accept a reduced precision, with a relative error of up to \$1.5 \cdot 2^{-12}\$.
Note than all the benchmarks that I linked before are for throughput, if you're in a situation where latency is important, an approximate reciprocal followed by a Newton-Raphson step is not actually good. Pentium4 may have been an exception to that, since it had a much slower division, but Pentium4 is not very relevant today.


Convert all CSV files in a given directory to JSON using Python I am attempting to convert all files with the csv extension in a given directory to json with this python script.

I am wondering if there is a better and more efficient way to do this?

Here is my code:

import csv
import json
import glob
import os

for filename in glob.glob('//path/to/file/*.csv'):
    csvfile = os.path.splitext(filename)[0]
    jsonfile = csvfile + '.json'

    with open(csvfile+'.csv') as f:
        reader = csv.DictReader(f)
        rows = list(reader)

    with open(jsonfile, 'w') as f:
        json.dump(rows, f)

EDIT:

Here is the sample input:

Here is the sample output:

[{
"username": "lanky",
"user_id": "4",
"firstname": "Joan",
"middlename": "Agetha",
"lastname": "Lanke",
"age": "36",
"usertype": "admin",
"email": "lanke.joan.wind@gmail.com"
}, {
"username": "masp",
"user_id": "56",
"firstname": "Mark",
"middlename": "Patrick",
"lastname": "Aspir",
"age": "25",
"usertype": "member",
"email": "aspirman@gmail.com"
}]

Looks good to me. It's a perfectly sensible approach.

There's just one line I'm going to criticize:

    csvfile = os.path.splitext(filename)[0]

Picking off element zero is fine, no need to change it. Other idiomatic approaches would be name, ext = ..., or an explicit discard of name, _ = ....

What I really do advocate changing is the identifier csvfile, since the .csv extension has been stripped from it. Better to simply call it name. Then an expression like name + '.csv' can be clearly seen as a csvfile expression.

You could elide the jsonfile assignment, if you like, and similarly rows, replacing each reference with the expression.


Hex encoding and decoding of ASCII strings in VBA I needed to convert some Ascii text to binary in Hex format 0x00FF....

I've written an EncodeHex and a DecodeHex function to do the conversion.

I've avoided using concatenation in favour of performance.

I'm assuming that assigning the results of CByte("&h80") to a byte array, and then using StrConv to convert the array to a Unicode string, is more efficient than assigning the results of Chr$("&h80") to a string array, and then using Join to concatenate the strings.

I need to use this in Excel, so I'm using CVErr(xlErrValue) to return errors, but this could maybe benefit from being more generic.

Sample usage

?HexEncode("FooBar")
0x466F6F426172
?HexDecode("0x466F6F426172")
FooBar

VBA Code

Option Explicit

Const HEX_STRING_PREFIX As String = "0x"
Const VBA_HEX_PREFIX As String = "&h"

Public Function HexEncode(AsciiText As String, Optional HexPrefix As String = HEX_STRING_PREFIX) As String

  If AsciiText = vbNullString Then

    HexEncode = AsciiText

  Else

    Dim asciiChars() As Byte
    asciiChars = StrConv(AsciiText, vbFromUnicode)

    ReDim hexChars(LBound(asciiChars) To UBound(asciiChars)) As String

    Dim char As Long
    For char = LBound(asciiChars) To UBound(asciiChars)
      hexChars(char) = Right$("00" & Hex$(asciiChars(char)), 2)
    Next char

    HexEncode = HexPrefix & Join(hexChars, "")
  End If

End Function

Public Function HexDecode(HexString As String, Optional HexPrefix As String = HEX_STRING_PREFIX)

  'Check if there's anything to decode

  If HexString = vbNullString Then
    HexDecode = vbNullString
    Exit Function
  Else

    If Not StrComp(Left$(HexString, Len(HexPrefix)), HexPrefix, vbTextCompare) = 0 Then
      'Unexpected string format
      GoTo DecodeError
    End If

    Dim hexRaw As String
    hexRaw = Mid$(HexString, 1 + Len(HexPrefix))

    'Check if the string is valid for decoding
    If Len(hexRaw) Mod 2 = 1 Then
      GoTo DecodeError
    End If

    Dim numHexChars As Long
    numHexChars = Len(hexRaw) / 2

    ReDim hexChars(0 To numHexChars - 1) As Byte
    Dim char As Long
    For char = 0 To numHexChars - 1
      Dim hexchar As String
      hexchar = VBA_HEX_PREFIX & Mid$(hexRaw, 1 + char * 2, 2)
      'Check if the hex-pair is actually hex
      If Not IsNumeric(hexchar) Then
        GoTo DecodeError
      End If
      hexChars(char) = CByte(hexchar)
    Next char
    'Return the concatenated bytes as a string
    HexDecode = StrConv(hexChars, vbUnicode)

  End If

SafeExit:
  Exit Function

DecodeError:
  HexDecode = CVErr(xlErrValue)

End Function

I think you have an opportunity for using Application.Caller in that error-handling subroutine, and make the function behave differently when it's used as a UDF vs. when it's called from VBA code.

Instead of GoTo-jumping when there's an invalid argument to early-return an Excel error value that's only really useful when the function is used as a UDF (and confusing when it's called from VBA code), you could be raising an actual meaningful error (which would still be jumping to the DecodeError label).

First, because we're raising an error in several places, I'd make a little utility procedure:

Private Sub OnHexDecodeError(ByVal message As String)
    Err.Raise 5, "HexDecode", message
End Sub

If Not StrComp(Left$(HexString, Len(HexPrefix)), HexPrefix, vbTextCompare) = 0 Then
  OnHexDecodeError "Parameter value '" & HexString & "' is not in the expected format."
End If

If Len(hexRaw) Mod 2 = 1 Then
  OnHexDecodeError "Parameter value '" & HexString & "' is invalid."
End If

  If Not IsNumeric(hexchar) Then
    OnHexDecodeError "Hex character '" & hexchar & "' is not a valid hexadecimal digit."
  End If

And then the error-handling subroutine could do this:

DecodeError:
  If TypeName(Application.Caller) = "Range" Then
    'function is used as a UDF
    HexDecode = CVErr(xlErrValue)
  Else
    'function is called by other VBA code
    Err.Raise Err.Number 'rethrow
  End If

That way the calling code can handle a runtime error 5 (aka invalid procedure call or argument) with a useful description:

?HexCode("test")

And now you have a function that's just as friendly to use as a UDF as it is in plain VBA code.

This doesn't strike me as intuitive:

If Len(hexRaw) Mod 2 = 1 Then

"Is value a multiple of X" is usually written as:

If value Mod x <> 0 Then

That way you're not assuming what the remainder might be - of course here it's either 1 or 0, but the point is that the way the comparison is written in foo Mod bar {comparison} never has to change, regardless of what the value of bar is.


NAQ 2018 Problem D Froggie Successful attempt at the ICPC North America Qualifier 2018 Problem D "Froggie." Any advice and all topical comments on code optimization and performance enhancement is appreciated!

Problem Summary

  You are recreating the classic ‘Frogger’ video game.

  Each level consists of a road with multiple lanes, with cars traveling in both directions. Cars within each lane are evenly-spaced and move at the same speed and in the same direction. Lane directions alternate, with the top lane moving left to right.

  ‘Froggie’ starts below the bottom lane and she travels across the road from the bottom to the top. At each time step, Froggie hops one space in one of four directions: up, down, left, or right. The goal of the game is to get Froggie across the road and above the top lane without her getting hit by a car.

  Goal

  Given a description of the road, car positions and speeds, and Froggie’s starting positions and moves, determine her outcome after the simulation. There are two possible outcomes: safely exiting the top lane or getting squished.

  In order to better plan her travel, Froggie may move left or right before entering the road. Once Froggie has entered the road she may only exit through the top of the road. That is, Froggie’s path never exits the left, right, or bottom lane boundaries.

  Input

  Each input describes one simulation. The first line of input contains two integers separated by a space: \$L\$ and \$W\$. The number of lanes is given by \$L\$ (\$1≤L≤10\$), while \$W\$ (\$3≤W≤20\$) defines the width (in number of cells) of the grid.

  This is followed by \$L\$ lines each describing the car configuration for a lane (from top to bottom). Each line contains three integers: \$O\$, the starting offset of the first car; \$I\$, the interval between cars; and \$S\$, the speed of the cars. The bounds are \$0≤O<I≤W\$ and \$0≤S≤W\$. All offsets should be computed based on the direction of lane movement.

  The final line of input starts with a single integer, \$P\$ (\$0≤P≤W−1\$) followed by a space and then a string of \$M\$ characters (\$1≤M≤L⋅W\$) from the set \$\{U,D,L,R\}\$. \$P\$ defines the starting position of Froggie, starting from the left. The string of characters defines the sequence of moves (up, down, left, right) that Froggie makes during the simulation.

  Output

  If Froggie successfully crosses the road (exiting the road from the top lane), output “safe”. If Froggie is hit by a car, or does not end above the road, output “squish”.

froggie.py

l, w = map(int, input().split())
cars = []
for i in range(l):
  cars.append(list(map(int, input().split())))
  if i % 2 != 0:
    cars[-1][0] = w - cars[-1][0] - 1
pos, moves = input().split()
pos = (l, int(pos))

dirs = { 'U': (-1, 0), 'D': (1, 0), 'L': (0, -1), 'R': (0, 1) }
for i, c in enumerate(moves):
  pos = list(map(sum, zip(pos, dirs[c])))
  row = pos[0]
  if row == -1:
    print('safe')
    exit(0)
  elif row >= l:
    continue
  car = cars[row]
  diff = car[0] + car[2] * (i + 1) if row % 2 == 0 else car[0] - car[2] * (i + 1)
  if (diff - pos[1]) % car[1] == 0:
    print('squish')
    exit(0)
print('squish')

The code in the post does not solve the problem. Consider the following input:

1 6
0 3 3
1 UU

The code in the post prints "safe", but in fact the frog is squished, since the cars have speed 3 and separation 3, meaning that a car crosses every square in the lane on every time step, and so the lane is impassable.


Function to send email, refactored based on SOLID principles I have the below method that is responsible for generating and sending a MailMessage using a SmtpClient object:

public static void SendEmail(
    string subject,
    string body,
    bool isBodyHtml,
    List<string> emails,
    List<string> ccs = null)
{
    if (emails == null)
    {
        throw new ArgumentNullException("emails");
    }
    MailMessage message = new MailMessage
    {
        Subject = subject,
        Body = body,
        IsBodyHtml = isBodyHtml
    };
    foreach (string email in emails)
    {
        message.To.Add(email);
    }
    if (ccs != null)
    {
        foreach (string cc in ccs)
        {
            message.CC.Add(cc);
        }
    }
    SmtpClient client = new SmtpClient
    {
        EnableSsl = true,
        DeliveryMethod = SmtpDeliveryMethod.Network
    };
    client.Send(message);
}

But recently I've been reading more on S.O.L.I.D and principles of OOP design, so I thought to refactor this method, and compartmentalize it more.

Am I on the right track with my refactor? Are there ways to make my code more modular/testable?

Here's my refactored code:

private static MailMessage CreateMailMessage(
    string subject,
    string body,
    bool isBodyHtml,
    List<string> emails,
    List<string> ccs = null)
{
    if (emails == null)
    {
        throw new ArgumentNullException("emails");
    }
    MailMessage message = new MailMessage
    {
        Subject = subject,
        Body = body,
        IsBodyHtml = isBodyHtml
    };
    foreach (string email in emails)
    {
        message.To.Add(email);
    }
    if (ccs != null)
    {
        foreach (string cc in ccs)
        {
            message.CC.Add(cc);
        }
    }
    return message;
}

private static SmtpClient CreateSmtpClient(
    bool enableSsl,
    SmtpDeliveryMethod deliveryMethod)
{
    SmtpClient client = new SmtpClient
    {
        EnableSsl = enableSsl,
        DeliveryMethod = deliveryMethod
    };
    return client;
}

And here's the method responsible for calling the above two and sending the email:

public static void SendEmail(string subject, string body, List<string> emails, List<string> ccs = null)
{
    using (MailMessage message = CreateMailMessage(subject, body, true, emails, ccs))
    {
        using (SmtpClient client = CreateSmtpClient(true, SmtpDeliveryMethod.Network))
        {
            client.Send(message);
        }
    }
}

I'd still say that you should break apart the mail message and SMTP client into separate classes for the "Single responsibility principle". In specific:

  A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class.

This is usually where you have a MailFactory and a SmtpClientFactory that are injected into the class that actually does the sending (MailSender). The injection (by abstract, so interfaces) would be set into the MailSender to fulfill the "Dependency inversion principle"

  One should "depend upon abstractions, [not] concretions."

Side Note

This would also let you reduce MailFactory.CreateMailMessage to MailFactory.Create (and SmtpClientFactory.CreateSmtpClient to SmtpClientFactory.Create) but that isn't directly related to SOLID. You can get why in various API naming conventions, but the short answer is that it would be redundant.

Expansion

A reason for this is when you do unit testing. We have an abstract mail client that just sticks messages into an in-memory list in the order they are sent. That way, we can inject it into the sender as ISmtpClientFactory and the rest of the code still works, not knowing that we have taken out the entire implementation of the sending for testing.

Edit 2

You also want to avoid static in general. It is implied by "injecting" but I figured I'd clarify that. Create factory instances and pass them in. That way you can swap them out.


Parking Charges exercise from Deitel's C book I'm learning to code with Deitel's C: How to program book, 6th edition. Here's Exercise 5.9 (Parking Charges):

  A parking garage charges a $2.00 minimum fee to park for up to three
  hours and an additional $0.50 per hour for each hour or part thereof
  over three hours. The maximum charge for any given 24-hour period is
  $10.00. Assume that no car parks for longer than 24 hours at a time.
  Write a program that will calculate and print the parking charges for
  each of three customers who parked their cars in this garage
  yesterday. You should enter the hours parked for each customer. Your program should print the results in a neat tabular
  format, and should calculate and print the total of yesterday's
  receipts. The program should use the function calculateCharges to
  determine the charge for each customer.

My solution is below. These exercises have no answers so I don't know how can I improve this code, in which I want to know if it is possible to dispense with variables "one", "two" and "three", and also automate the printing process (after the for loop in main) inside a loop.

#include <stdio.h>

float calculateCharges( float hours );

float charge;

int main()
{
    int customer;
    float one;
    float two;
    float three = 0;
    float hours;

    for( customer = 1; customer <= 3; customer++ ) {
        printf( "Enter customer #%d parking hours: ", customer );
        scanf( "%f", &hours );

        if( customer == 1 )
            one = hours;
        else if( customer == 2 )
            two = hours;
        else
            three = hours;
    }
    printf( "%s%10s%12s", "Car", "Hours", "Cost" );
    printf( "\n%d%12.1f%12.1f", 1, one, calculateCharges( one ) );
    printf( "\n%d%12.1f%12.1f", 2, two, calculateCharges( two ) );
    printf( "\n%d%12.1f%12.1f", 3, three, calculateCharges( three ) );
    printf( "\n%s%8.1f%12.1f", "TOTAL", one + two + three, calculateCharges( one ) + calculateCharges( two ) + calculateCharges( three ) );
}

float calculateCharges( float hours )
{
    int h = hours;
    charge = 2.0;

    if( hours > 0 ) {
        if( hours <= 3 )
            return charge;

        else if( hours <= 24 ) {
            while( h > 3 ) {
                charge += .5;
                h--;
                if( charge >= 10 )
                    charge = 10;
            }
            return charge;
        }
    }
    else
        printf( "\nThe amount of time entered is not supported." );
}

I see a number of things that may help you improve your code.

Eliminate global variables where practical

The code declares and uses a global variable charge.  Global variables obfuscate the actual dependencies within code and make maintainance and understanding of the code that much more difficult.  It also makes the code harder to reuse.  For all of these reasons, it's generally far preferable to eliminate global variables by either making them local and/or passing them into and out of functions. That way the linkage is explicit and may be altered more easily if needed.  For example, to eliminate charge global, simply make it local to calculateCharges.

Consider improving names

I think hours is not a bad variable name, but one is, since hours gives the reader some idea of why the variable exists, but one could be anything.

Check return values for errors

The call to scanf can fail.  You can check the return values to make sure they haven't or your program may crash (or worse) when given malformed input or due to low system resources.  Rigorous error handling is the difference between mostly working versus bug-free software.  You should strive for the latter.

Understand variable passing

When you write this:

float calculateCharges( float hours )

It means that the function takes a float named hours as the argument.  Because it doesn't say float &hours, which would take a reference to the passed variable (a pointer, in essence), it's passed by by value.  This means that it's really not necessary to create a local copy named h; just use hours directly.

Decompose the program into smaller parts

Right now, much of the code is in main which isn't necessarily wrong, but it means that it's not only hard to reuse but also hard to troubleshoot.  Better is to separate the code into small chunks.  It makes it both easier to understand and easier to fix or improve.  For example, one might have a function for getting input and another for printing output in addition to the existing function that converts hours to a charge.

Make sure all paths return a value

The calculateCharges routine returns the dollar amount if the passed number of hours is greater than zero, but what if it's not?  Right now the code doesn't return any value which is a problem.  Rather than printing a message, better would be to return some special marker value, such as -1 that the calling code can check for and then have it decide the appropriate action.

Use an array

Instead of naming things one, two, three, create an array: float hours[3]; and loop through that.


Redundant `let`s in list monadic code There is a function with bunch of a redundant lets and ←s inside a list monad.

example ∷ (Int, Int, Int, Int, Int)
example = head $ do
  let list1 = [0,1]
  e1 ← list1
  let list2 = φ list1 e1
  e2 ← list2
  let list3 = φ list2 e2
  e3 ← list3
  let list4 = φ list3 e3
  e4 ← list4
  let list5 = φ list4 e4
  e5 ← list5
  return (e1,e2,e3,e4,e5)
    where φ ∷ [Int] → Int → [Int]

So, all list# values are unusable. How can example could be rewritten with >>= or maybe with something else to get (e1,e2,e3,e4,e5) without any lets?

Hm, this could probably be done using foldM or similar, but that would require you to convert to and from lists. My first impulse would be to try to write the top-level of example using Applicative style:

import Control.Applicative

example = head $ (,,,,) <$> m <*> m <*> m <*> m <*> m

But that doesn't work directly here, as you want each m to know the list# of the last action.

One way to realize this without breaking up the above structure would be to have a StateT monad transformer take care of passing around the current list#:

import Control.Monad.State

example = head $ flip evalStateT [0,1] $
          (,,,,) <$> m <*> m <*> m <*> m <*> m

This allows us to write m as a list monad with state:

  where m :: StateT [Int] [] Int
        m = do list <- get
               e <- lift list
               put (phi list e)
               return e

Now admittedly, this is a bit advanced. It might be easier to try to restructure phi or use a foldM type solution.


Scala heap implementation I'm a Scala beginner, so it'd be great if anyone would be so kind to give some feedback.

package heap

import scala.collection.mutable.ListBuffer

class Heap[T](compareFunction: (T, T) => Int) {

private val HIGHER = 1
private val LOWER = -1

private val heapRepresentation = new ListBuffer[T]()

private def lastIndex = heapRepresentation.size - 1

private def valueOf(index: Int): T = heapRepresentation(index)

def add(item: T): Heap[T] = {
    heapRepresentation.append(item)
    bubbleUp(lastIndex)
    this
}

def swap(index1: Int, index2: Int) {
    val temp = valueOf(index1)
    heapRepresentation.update(index1, valueOf(index2))
    heapRepresentation.update(index2, temp)
}

def bubbleUp(currentIndex: Int) {
    def getParent(i: Int) = (i - 1) / 2

    if (currentIndex > 0) {
        val parentIndex = getParent(currentIndex)

        compareFunction(valueOf(currentIndex), valueOf(parentIndex)) match {
            case LOWER =>
                swap(currentIndex, parentIndex)
                bubbleUp(parentIndex)
            case _ =>
        }
    }
}

def bubbleDown(currentIndex: Int) {
    getLowerChild(currentIndex) match {
        case Some((lowerChildIndex, lowerChildValue)) =>
            if (compareFunction(valueOf(currentIndex), lowerChildValue) == HIGHER) {
                swap(currentIndex, lowerChildIndex)
                bubbleDown(lowerChildIndex)
            }
        case None =>
    }
}

def getLowerChild(index: Int): Option[(Int, T)] = {
    def getChildrenIndices(parentIndex: Int): (Int, Int) = (2 * parentIndex + 1, 2 * parentIndex + 2)

    val (leftChildIndex, rightChildIndex) = getChildrenIndices(index)

    val areChildrenInBoundsOfHeap = (rightChildIndex <= lastIndex) && (leftChildIndex <= lastIndex)
    if (!areChildrenInBoundsOfHeap) return None

    val (leftChildValue, rightChildValue) = (heapRepresentation(leftChildIndex), heapRepresentation(rightChildIndex))

    compareFunction(leftChildValue, rightChildValue) match {
        case LOWER => Some((leftChildIndex, leftChildValue))
        case _ => Some((rightChildIndex, rightChildValue))
    }
}

def pop: Option[T] = heapRepresentation.size match {
    case 0 => None
    case _ =>

        val firstValue = heapRepresentation(0)
        if (heapRepresentation.size == 1) {
            heapRepresentation.remove(0)
            return Some(firstValue)
        }
        swap(0, lastIndex)
        heapRepresentation.remove(lastIndex)
        bubbleDown(0)
        Some(firstValue)
}

override def toString = {
    s"[$heapRepresentation](${heapRepresentation.size})"
}

}

Usage of heap class

def compareInts(first: Int, second: Int): Int =
    if (first > second) 1
    else if (first == second) 0
    else -1

val heap = new Heap[Int](compareInts)

for (i <- 0 to 3) {
    heap.add((Math.random() * 100).toInt)
}
heap
heap.pop
heap.pop
heap.pop
heap.pop

def compareStrings(first: String, second: String): Int =
    if (first > second) 1
    else if (first == second) 0
    else -1
var heap2 = new Heap[String](compareStrings)
heap2.add("a")
heap2.add("cc")
heap2.add("")

heap2.pop
heap2.pop
heap2.pop
heap2.pop

Right now, I would like to get a Scala mindset, and get to know the tricks and tips. I have a background in PHP and JavaScript, and Scala is quite harder to learn, but way more elegant, so I want to master it!

Writing utility classes is hard enough without reinventing everything from scratch each time. Whenever possible, class authors should take advantage of available tools. In this case, Scala has a trait for ordering objects in a collection: scala.math.Ordering and it should be used for the compareFunction which will undoubtedly require some adjustment of the code.

For the most part, the code is well laid out and easy to follow. Good job! Note that standard indentation for Scala code is two spaces. It's hard to tell if the four space indentation is what is used in the code, or if it was an artifact of transferring the code into this question.

While Scala pattern matching is a great tool, it is possible to over user it. I think that using it in the pop method is not just overkill, but also results in code that is harder to read. I would write it like this:

def pop: Option[T] =  match {
  if (heapRepresentation.isEmpty) None
  else {
    val firstValue = heapRepresentation(0)
    if (heapRepresentation.size == 1) {
       heapRepresentation.remove(0)
    }
    else {
      swap(0, lastIndex)
      heapRepresentation.remove(lastIndex)
      bubbleDown(0)
    }
    Some(firstValue)
  }
}


DOM Traverser in JavaScript I'm writing a function in JavaScript which, when given an element, can look for the previous or next node, assuming a depth-first order traversal. If the next element is out of bounds, it should return null.

I could of course use document.querySelectorAll('*'), find the index of the element, and then move forwards/backwards by one. But this approach seems like overkill, since I would think it would be inefficient to get the entire DOM tree as a depth-first searched structure, when I could theoretically traverse by one element using local information.

I can't shake the feeling that I'm repeating much of the logic. Can anyone tell if there's a better refactored version I'm not seeing?

//returns previous element in a depth-first search traversal
//returns null if at the start of the tree
TreeWalker.prototype.getPreviousElement = function(element) {
  var previousElement;
  if(element.previousElementSibling === null) {
    var parent = element.parentElement;
    if($(parent).is(this.rootSelector)) {
      previousElement = null;
    } else {
      previousElement = parent;
    }
  } else {
    var beforeBranch = element.previousElementSibling;
    previousElement = this.cycleToPreviousBranch(beforeBranch);
  }
  return previousElement;
};

TreeWalker.prototype.cycleToPreviousBranch = function(element) {
  var previousElement;
  if(element.children.length === 0) {
    previousElement = element;
  } else {
    var lastChild = element.lastElementChild;
    previousElement = this.cycleToPreviousBranch(lastChild);
  }
  return previousElement;
};

//returns next element in a depth-first search traversal
//returns null if at the end of the tree
TreeWalker.prototype.getNextElement = function(element) {
  var nextElement;
  if(element.children.length > 0) {
    nextElement = element.children[0];
  } else {
    nextElement = this.cycleToNextBranch(element);
  }
  return nextElement;
};

TreeWalker.prototype.cycleToNextBranch = function(element) {
  var nextElement;
  if(element.nextElementSibling !== null) {
    nextElement = element.nextElementSibling;
  } else {
    var parent = element.parentElement;
    if($(parent).is(this.rootSelector)) {
      nextElement = null;
    } else {
      nextElement = this.cycleToNextBranch(parent);
    }
  }
  return nextElement;
};

As the comments say, it's not clear what the if($(parent).is(this.rootSelector)) checks are for. You only use one this property - this.root - would it be better to have a root variable and then two simple functions rather than prototype functions?

Your own improvements are better, but they can be improved more by returning earlier. You started doing that with the return null;, but you can do it with all the values, avoiding all local variables and reducing your code indentations. I have also made your code consistently use brackets after all control structures.

//returns previous element in a depth-first search traversal
//returns null if at the start of the tree
TreeWalker.prototype.getPreviousElement = function(element) {
  if (element.previousElementSibling === null) {
    if (element === this.root) {
      return null;
    }
    return element.parentNode;
  }
  element = element.previousElementSibling;
  while (element.children.length > 0) {
    element = element.lastElementChild;
  }
  return element;
};

//returns next element in a depth-first search traversal
//returns null if at the end of the tree
TreeWalker.prototype.getNextElement = function(element) {
  if (element.children.length > 0) {
    return element.children[0];
  }
  while (element.nextElementSibling === null) {
    element = element.parentNode;
    if (element === this.root) {
      return null;
    }
  }
  return element.nextElementSibling;
};


Tic Tac Toe with Python and Kivy Accomplishment: Tic Tac Toe game with Python & Kivy

I have created a simple tic tac toe game with a random bot logic (really random after a user selects a mark). Next step would be to insert the minimax algorithm for example, but I would like to know if the path I've taken to construct my code is correct and could it be improved.

I have two files: main.py and ai.py

main.py

from kivy.app import App
from kivy.uix.button import Button
from kivy.uix.popup import Popup
from kivy.uix.label import Label
from kivy.uix.stacklayout import StackLayout
from kivy.config import Config
from ai import Ai
from random import randint

class TicTacToe(App):

    title = 'Tic Tac Toe'
    board = []
    choices = ["X","O"]

    # On application build handler
    def build(self):
        Config.set('graphics', 'width', '600')
        Config.set('graphics', 'height', '600')
        self.layout = StackLayout()
        for x in range(9): # range() explanation: http://pythoncentral.io/pythons-range-function-explained/
            bt = Button(text='', font_size=200, width=200, height=200, size_hint=(None, None), id=str(x))
            bt.bind(on_release=self.btn_pressed)
            self.board.append(bt)
            self.layout.add_widget(bt)
        return self.layout

    # On application start handler
    def on_start(self):
        self.init_players();
        greeting = "Hello Player! You are playing with \"" + self.player + "\""
        self.popup_message(greeting)

    # On button pressed handler
    def btn_pressed(self, button):
        if len(button.text.strip()) < 1: # Continue only if the button has no mark on it...
            button.text = self.player
            self.bot.make_move(self.board)
            self.check_winner()

    # Initializes players
    def init_players(self):
        rand_choice = randint(0,1);
        self.bot = Ai(self.choices[rand_choice]);
        self.player = self.choices[0] if rand_choice == 1 else self.choices[1]

    # Checks winner after every move...
    def check_winner(self):
        pass # Just void..

    def popup_message(self, msg):
        popup = Popup(title="Welcome!", content=Label(text=msg), size=(300, 100), size_hint=(None, None))
        popup.open()

if __name__ == '__main__':
    TicTacToe().run()

ai.py

from random import randint

class Ai:
    def __init__(self, choice):
        self.choice = choice

    # Main function for bot making a move
    def make_move(self, board):
        self.make_random_move(board) # Do a random move

    # Makes a random move by generating a random integer and checking if the place is free
    def make_random_move(self, board):
        free_buttons = []
        for button in board: # Simple for loop to extract the free buttons
            if len(button.text.strip()) < 1: # If the button has no mark, stripping spaces...
                free_buttons.append(button)

        if len(free_buttons) > 1: # If any free buttons left... let's make the move
            rand = randint(0, len(free_buttons) - 1) # Generate a random integer from 0 to the length of the array
            free_buttons[rand].text = self.choice

On comments

# On button pressed handler
def btn_pressed(self, button):

I think you could rename the function to on_btn_pressed and just remove the comment, as it doesn't add any information that isn't already apparent from the function name.

There are several other comments that doesn't add anything useful, like

# Initializes players
def init_players(self):

and

# Checks winner after every move...
def check_winner(self):

and

self.make_random_move(board) # Do a random move

The comments describing the build and on_start functions could do with some more explanation, as it's not apparent to me how these are used (I'm assuming they are used by kivy.app.App?).

if len(button.text.strip()) < 1: # If the button has no mark, stripping spaces...

I would extract the above check into a function, like this:

if is_button_unmarked(button):
    free_buttons.append(button)
...

def is_button_unmarked(self, button):
    return len(button.text.strip()) < 1

thus eliminating the need for a comment.

You should strive to make the code self-documenting, and use comments when you fail to do that. I recommend reading rolfl's excellent answer to a codereview question about comments.

Constructor vs method injection

I would inject the game board into the Ai constructor

self.bot = Ai(self.choices[rand_choice], board)

and set it as you do with the choice

class Ai:
    def __init__(self, choice, board):
        self.choice = choice
        self.board = board

This way you can do

self.bot.make_move()

instead of

self.bot.make_move(self.board)

which is easier to understand. As the board is used by all the functions in Ai, there's currently no good reason not to inject it in the constructor.

Reuse

Your popup_message function lets you pass any message, but the title is hardcoded. You should send the title in as an argument as well, to make the popup_message function useable by more than welcome messages (maybe you'd want to use it after deciding a winner in check_winner?).