Asynchronous HTTP JSON request For asynchronous requests, I use Alamofire.
I have created one method for requesting async data.

func requestData(method: Method, urlString: String, onSuccess: (AnyObject) -> Void , onFailure: (NSError) -> Void,  sender: UIViewController, postParams:AnyObject? = nil) {

    let AFmanager = Manager(configuration: seesionConfiguration)

    let URL = NSURL(string: urlString)!
    let urlRequest = NSMutableURLRequest(URL: URL)
    urlRequest.HTTPMethod = method.rawValue

    if let param: AnyObject = postParams {
        urlRequest.HTTPBody = NSJSONSerialization.dataWithJSONObject(param, options: nil, error: nil)
    }
    if method == .POST {
        urlRequest.addValue("application/json", forHTTPHeaderField: "Content-Type")
    }
    urlRequest.addValue("application/json", forHTTPHeaderField: "Accept")

    AFmanager.request(urlRequest)
        .responseJSON { (request, response, JSONResult, error) in
            if (error == nil) {
                onSuccess(JSONResult!)
            } else {
                onFailure(error!)
            }
    }
}

Here, I have made postParams of type AnyObject as parameter can be of any dictionary type. Is there another way of defining the method so that postParams can be checked to be of type dictionary?

Any other improvement/suggestion?

I know this is technically outside the scope of the review here, but your first parameter's type is Method.  This enum desperately needs to be renamed.  When I see Method, I cannot help but think of a programming method... I think you're passing a pointer to a class's method here.  This type needs a better, more descriptive name.

Our sender parameter doesn't need to be a UIViewController.  Does it?  In fact, are we even using the sender parameter?  Can we just eliminate it?

The postParams needs to be an AnyObject parameter.  We can turn more than just dictionaries into valid JSON.  The NSJSONSerialization class offers a isValidJSONObject method for which to check whether or not we can serialize.  So, we can leave this parameter as is, and simply make this check:

if let param: AnyObject = postParams {
    if NSJSONSerialization.isValidJSONObject(param) {
        urlRequest.HTTPBody = NSJSONSerialization.dataWithJSONObject(param, options: nil, error: nil)
    } /* else {
        // call the error handler?
    } */
}

And finally, one comment on the parameters our method takes... taking two completion blocks isn't the suggested best practice.  By taking two completion blocks, you force the user into duplicating any code they want to happen whenever this callback occurs (if they want it to happen under both scenarios).   So, we should combine these arguments into a single completion block (much like AFmanager is doing in the code you're using) and more importantly, we should make this argument the last argument.

In Swift, when the last argument is a closure, we get a slightly nicer syntax.  Instead of something like:

someObject.someMethod(param1: foo, param2: bar, closureParam: { println("hello world") })

We get a slightly better syntax:

someObject.someMethod(param1: foo, param2: bar) {
    println("hello world")
}

Again, you made use of that when you used the AFmanager, so let's make our code follow suit.  Combine your success and error completion handlers into a single closure parameter which is called no matter success or failure, and move it to the last argument to give users a better syntax option (and so we don't have any dangling parameters after the space-consuming closure).


Animating the color of a line in WPF I'm thinking about how to do a color transition for a line in WPF. I'm looking for this to be as simple and succinct as possible, and also the "correct" way in the WPF world.

This is what I have, taking the line from it's previous color to Colors.LightGreen in 0.1 seconds.

Line TargetLine = GetMyTargetLine();

var s = new Storyboard(){ Duration = new Duration(TimeSpan.FromSeconds(0.1f)) };
s.Children.Add(new ColorAnimation(Colors.LightGreen, s.Duration));
Storyboard.SetTarget(s.Children[0], TargetLine);
Storyboard.SetTargetProperty(s.Children[0], new PropertyPath("Stroke.Color"));
s.Begin();

And it is functional. Is this the proper way to do it in the WPF mindset? It just seems very clunky and verbose way to express what I want to do. Thoughts?

Edit:
With Snowbear's advice I can at least get it to 4 lines. The entire context of the storyboard is right here so I don't think it's a big deal to reference the only child by index. If it were any more complex than this I'd agree that it should be a named variable.

    Line TargetLine = GetMyTargetLine();

    var story = new Storyboard() { Children = { new ColorAnimation(color, new Duration(TimeSpan.FromSeconds(0.1))) } };
    Storyboard.SetTarget(story.Children[0], TargetLine);
    Storyboard.SetTargetProperty(story.Children[0], new PropertyPath("Stroke.Color"));
    story.Begin();

1) I believe it is not a good practice to give variables one-letter names. Storyboard story = ...
2) I believe you can specify Duration in ColorAnimation only. And do not set it for Storyboard.
3) I would introduce variable for ColorAnimation because s.Children[0] looks weird to me when I know that it is ColorAnimation.
4) Strangely you are using 0.1f where parameter is double anyway.
5) I would consider using object and collection initializers for Storyboard.Children.
6) Optionally I would think on removing storyboard variable and start it immediatly after constructing.

Result: my code looks differently, but readability changes are subjective:

Line TargetLine = line;

var duration = new Duration(TimeSpan.FromSeconds(0.1));
var colorAnimation = new ColorAnimation(Colors.LightGreen, duration);
Storyboard.SetTarget(colorAnimation, TargetLine);
Storyboard.SetTargetProperty(colorAnimation, new PropertyPath("Stroke.Color"));

new Storyboard {Children = {colorAnimation}}.Begin();


Autotools detect YAML library I have been updating my build tools to optionally use autotools (autoconfig/automake/libtool etc.).

As part of this change I have written a couple of M4 macros. This not being something I have done before, any input is appreciated on style or if things can be done better.

This macro checks to see if the YAML libraries are installed:

AC_DEFUN([AX_FUNC_THOR_USE_YAML],
[
    AC_ARG_WITH(
        [yamlroot],
        AS_HELP_STRING([--with-yamlroot=<location>], [Directory of YAML_ROOT])
    )
    AC_ARG_ENABLE(
        [yaml],
        AS_HELP_STRING([--disable-yaml], [Disable yaml serializsation])
    )
    AS_IF(
        [test "x$enable_yaml" != "xno"],

        ORIG_LDFLAGS="${LDFLAGS}"
        if test "${with_yamlroot}" != ""; then
            LDFLAGS="$LDFLAGS -L$with_yamlroot/lib"
        fi

        AC_CHECK_LIB(
            [yaml],
            [yaml_parser_initialize],
            [
                AC_DEFINE([HAVE_YAML], 1, [When on Yaml Serialization code will be compiled])
                with_yamllib=-lyaml
            ],
            [AC_MSG_ERROR([

Error: Could not find libyaml

You can solve this by installing libyaml
    see http://pyyaml.org/wiki/LibYAML

Alternately specify install location with:
    --with-yamlroot=<location of yaml installation>

If you do not want to use yaml serialization then it
can be disabled with:
    --disable-yaml

                ], [1])]
        )

        LDFLAGS="${ORIG_LDFLAGS}"
    )
])

If YAML is detected, then no problems. If the configure script cannot find them, the error message is generated with three options:

Disable the use of YAML
Provide the location where YAML is installed (not default)
Instructions on how to install

I only have some comments about the shell scripting elements.

This is a very archaic (and silly) way of testing flags:

    [test "x$enable_yaml" != "xno"],

You can write simpler as:

    [test "$enable_yaml" != no],

This can be simplified:

    if test "${with_yamlroot}" != ""; then

To just:

    if test "${with_yamlroot}"; then

I also find it odd to use sometimes $this_style and sometimes ${that_style}.
I prefer less typing (especially the shift key for }),
so the first, simpler style.
The only time I use braces is when appending some text directly after the variable would break the script, for example in situations like this:

authtype=...  # something
# broken, not using the $authtype variable
#modpath=/usr/libexec/apache2/mod_$authtype_dbd.so
# good
modpath=/usr/libexec/apache2/mod_${authtype}_dbd.so


Script scraper for outputting variables and functions to a text file I've been programming with Python 2.7 for about six months now and if possible, I'd like some critique to see what I might have done better or what I might be doing wrong.

My objective was to make a script scraper that finds variables and functions, outputting them to a text file along with their line number. It also organizes the output based on type and string length for readability. It works as intended, but I'd like to know if/how I could have wrote it better, cleaner.

import re

def change_line():
        parse_next['line_count'] += 1

def make_readable(list):
        fix_order = {}
        for item in list:
                fix_order[len(item)] = item
        return sort_this(fix_order)

def sort_this(dict):
        fixed = dict.keys()
        fixed.sort()
        newly_ordered = []
        for number in fixed:
                newly_ordered.append(dict[number])
        return newly_ordered

def parse_match(match):
        if '\n' in match:
                parse_next[match]()
        elif 'def' in match:
                parse_next[match] = parse_next['line_count']
                funcs.append(match)
        elif match:
                parse_next[match] = parse_next['line_count']
                varis.append(match)
        else:
                print 'error'

funcs = []
varis = []
txt_file = open('c:\\code\\test.txt', 'r')
output = open('c:\\code\\output.txt', 'w+')
found = re.findall(r'\n|def\s.+|.+ = .+', txt_file.read())

parse_next = {
        '\n': change_line,
        'line_count': 1,
}

for match in found:
        parse_match(match)

funcs = make_readable(funcs)
varis = make_readable(varis)
output.write('\t:Functions:\n\n')

for item in funcs:
        s_fix = item.replace('def ', '',)
        to_write = [s_fix, ' Line:', str(parse_next.get(item)), '\n']
        output.writelines(to_write)

output.write('\n'*2)
output.write('\t:Variables:\n\n')

for item in varis:
        to_write = [item.strip(),' Line:', str(parse_next.get(item)), '\n']
        output.writelines(to_write)

output.close()
txt_file.close()

To run it, you'll need to edit this filepath:

txt_file = open('c:\code\test.txt', 'r')

with code from a script of your choice.

Be harsh if necessary, I'd really like to get better.

A couple of suggestions:

make_readable has a very serious problem - what if two inputs have the same length? A dictionary can only have one value per key; I suggest making it a dictionary of lists of items (or a collections.defaultdict(list)) instead. Also, don't call the argument list - that's a built-in.

sort_this could be shortened (and the argument shouldn't be called dict):

def sort_this(d):
    return [d[key] for key in sorted(d)]

Iterating over a dictionary (e.g. in sorted) automatically iterates over the keys. Also, this will work without modification in Python 3.x (where fixed.sort() will fail).

The global variables (e.g. funcs) should either be explicitly global (i.e. put global funcs at the start of functions that use them) or, much better, passed as arguments to the functions:

def parse_match(match, funcs, varis, parse_next):

One bug you may want to look into - if a variable is defined over multiple lines:

my_list = [1, 2, 3,
           4, 5, 6]

Your code only gets the start of the definition.

Finally, it would be neat to not have to change the hard-coded strings to change the files to analyse and output to! Look into taking command line arguments (sys.argv) or using e.g. tkinter to put a GUI in. At the very least, have a higher-level function analyse(input_file, output_file) then call it at the end of your script:

if __name__ == "__main__":
    analyse("C:\\code\\test.txt", "C:\\code\\output.txt")

It is not best practice to put the variables you have to change somewhere in the middle of the file - top or bottom is easier to find!


Linked list vs array performance for RemoveAt() function I am trying to compare the RemoveAt() function performance in an array and linked list.

For array:

public T RemoveAt(int index)
{
    if (index >= this.count || index < 0)
    {
        throw new ArgumentOutOfRangeException(
            "Invalid index: " + index);
    }

    T item = this.arr[index];

    Array.Copy(this.arr, index + 1,

        this.arr, index, this.count - index - 1);

    this.arr[this.count - 1] = default(T);

    this.count--;

    return item;
}

For linked list:

public T RemoveAt(int index)
{
    if (index < 0 || index >= count)
    {
        throw new IndexOutOfRangeException("Invalid Index" + index);
    }
    else
    {
        int currentindex = 0;
        ListNode currentnode = this.head;
        ListNode prevnode = null;

        while (currentindex<index)
        {
            prevnode = currentnode;
            currentnode = currentnode.nextnode;
            currentindex++;
        }
        // Remove the found element from the list of nodes

        RemoveListNode(currentnode, prevnode);

        // Return the removed element

        return currentnode.element;

    }
}

private void RemoveListNode(ListNode node, ListNode prevNode)
{

                 prevNode.nextnode = node.nextnode;
}

Main program:

I have inserted 10K elements in each and I am trying to remove the 500th element.

Stopwatch s=new Stopwatch();
CustomArrayList<int> listusingArray = new CustomArrayList<int>(10000);
Console.WriteLine("Deleting 500th elements from array........\n");

s.Start();
listusingArray.RemoveAt(500);
s.Stop();

Console.WriteLine("Time taken to delete from array: " + s.Elapsed);

DynamicList<int> listusingDynamic = new DynamicList<int>();

s.Reset();
Console.WriteLine("Removing 500th elements from Link List........\n");

s.Start();
listusingDynamic.RemoveAt(500);
s.Stop();

Console.WriteLine("Time taken to remove from list: " + s.Elapsed);

Output:

Time taken to delete from array :00:00:00.0003040

Time taken to delete from list  :00:00:00.0008685

Shouldn't the linked list Remove at() function  be faster as it avoids Array.Copy in array?

If you time just removing the item from the list, it probably will be faster.

What you're timing is traversing the list to the correct point, then removing the item. Apparently, traversing the list is slow enough to (more than) make up for the time you're saving on the removal itself.

Although it was written about C++, most of the points Bjarne raises in his video on the subject apply about as well to most other languages.


Node.js error handling using merely if else without try catch I'm confused why you need to use try catch. I made a simply utils like this, and use it on my models' callback.

in my utils/error.js

module.exports.handleError = (err, errMsg, res) => {
    res.json({
        success: false,
        msg: `${errMsg}. ${err}`
    })
}

Then I have this in my router controller

router.get('/tasks', (req, res) => {

    Task.getTasks((err, resp) => {
        if(err){
            return handleError(err, 'Failed to get tasks', res)
        }

        res.json({
            success: true,
            msg: {tasks: resp}
        })
    })
})

What can be improved? And what is the drawback of using this approach?

My main problems with your approach are:

You have to remember to do this error handling. If you forget, you'll get a silent failure.
You have to make every middleware aware of your error handling.

The first one can't really be solved by Express. Due to the way that Express works, you'll always have to do some variant of:

if (error) {
  return doSomething(error)
}

I prefer Koa as a result because Koa uses async/await, which translates to Promises under the hood; errors that occur in a Promise will propagate correctly, regardless of if you remember to handle them or not.

As for the second result, this kinda invalidates the purpose of middleware. Middleware should only know what it does; you can easily leave error handling til later on in the chain by modifying your middleware to pass the error argument to the next middleware in the chain..

router.get('/tasks', (request, response, next) => {
  Tasks.getTasks((error, result) => {
    if (error) {
      return next(error)
    }

    ....
  })
})

And then adding the following as the final middleware in Express:

app.use(function errorHandler(error, request, response, next) {
  return handleError(error, request, response)
})

As long as all other middlewares pass the error to the next in the chain, this will work 'automagically' and you can swap out your implementation of error handling without any middleware having to change.

As a side note, I'd highly recommend making your application use Promises and async/await (Node 8 is now LTS which supports this so there's no reason not to). You can still use express if you want to, and it would look something like this instead:

router.get('/tasks', async (req, res, next) => {
  try {
    res.json({
      success: true,
      msg: {
        tasks: await getTasks()
    })
  } catch (error) {
    next(error)
  }
})

If you used Koa you wouldn't need the try/catch.

I'd also recommend not having the success flag: Use HTTP status codes to indicate success or failure.


Selection sort algorithm I am just trying to use Go so I will be thankful for all remarks and corrections:

package main

import "fmt"

func selectionSort(a []int) []int {

    for i := 0; i < len(a); i++ {

        min := i

        for j:= i + 1; j < len(a); j++ {
            if a[j] < a[min] {
                min = j;
            }
        }

        var tmp int = a[i]
        a[i] = a[min]
        a[min] = tmp
    }

    return a
}

func main() {

    values := []int { 5, 8, 4, 1, 7,  2, 3, 6 }
    result := selectionSort(values)
    fmt.Println(result)
}

On Stack Overflow duplicate questions are closed.

For your question, a search of Code Review Stack Exchange for "[go] selection sort" finds, amongst others,

Selection sort algorithm with increasing/decreasing sort options

which has several detailed answers that address your question. For example, the code for my answer, simplified for ascending order only, is

// selectionSort sorts the array a into ascending order.
// The selection sort algorithm is an in-place comparison sort
// with O(n*n) time complexity.
func selectionSort(a []int) {
    for i, x := range a[:len(a)-1] {
        k := i
        for j, y := range a[i+1 : len(a)] {
            if x > y {
                // out of order
                k = i + 1 + j
                x = y
            }
        }
        a[k], a[i] = a[i], x
    }
}

My code is idomatic Go and it's more than twice as fast as your code when running a Go benchmark which sorts 1024 random integers.

BenchmarkDemas           300       4894852 ns/op
BenchmarkPeterSO        1000       2339806 ns/op


Implementing nub (distinct) I've implemented the following nub' or "give distinct elements of list" function:

nub' :: (Eq a) => [a] -> [a]
nub' [] = []
nub' xs = nubHelper xs []

nubHelper :: (Eq a) => [a] -> [a] -> [a]
nubHelper []     acc = acc
nubHelper (y:ys) acc = if(contains y acc)
                         then nubHelper ys acc
                         else nubHelper ys (acc ++ [y])
                         where contains match acc =
                            foldl (\as x -> if(y == x) then True else as) False acc

Example:

*Main> nub' [1,2,1,1,1,1,3,4,3,1]
[1,2,3,4]

Surely there must be a cleaner way?

For the "official" answer, you can go straight to the GHC source for nub:

nub                     :: (Eq a) => [a] -> [a]
nub l                   = nub' l []
  where
    nub' [] _           = []
    nub' (x:xs) ls
        | x `elem` ls   = nub' xs ls
        | otherwise     = x : nub' xs (x:ls)

Some of the differences that stand out are:

Use a where clause to scope your helper function, and name it with a "prime" symbol.  In your case, I guess the helper would be nub''.  The helper's type can then be automatically inferred.
Use pattern matching instead of if-then-else.
Your contains helper is just the elem builtin.  They chose to write x `elem` ls instead of elem x ls, probably to read more smoothly.
It's more natural / efficient to prepend than to append elements to a list.  Instead of building acc as the result as a list of wanted elements, they build ls as the list of elements to exclude in future encounters.


Finding the sum of all numeric palindromes that are less than N I wrote a function that finds the sum of all numeric palindromes lesser than N.

const sumOfPalindromes = (n) => {
    let x = 0;
    for (let i = 0; i <= n; i++) {
      x += i == (''+i).split('').reverse().join('') ? i : 0;
    }
    return x;
}

console.log(sumOfPalindromes(10000));
console.log(sumOfPalindromes(24));

Are there shorter, more-concise alternatives to the above approach (preferably without using a for loop)?

This is not a proper review, but rather an extended comment.

Are there shorter, more-concise alternatives to the above approach

Yes. You should view it not as a programming problem, but as a recreational math exercise.
Consider a simplest case of N being an even power of 10, say 1000000. For a moment, allow leading zeroes. Now, each 6-digit palindromic number abccba corresponds to a 3-digit number abc, and in fact is 1000*abc + cba. Notice that if the sum of all 3-digit numbers is X, the sum of all 6-digit palindromic numbers is 1000*X + X = 1001*X. As for X, recall the formula for the sum of an arithmetic progression 0 + 1 + 2 + ... + 999.
To deal with leading zeroes you must subtract the sum of those palindromic numbers which do have leading zeroes. Yet again, they are in form abba0 = 1000*ab + 10*ba. Yet again, use the arithmetic progression formula to get the result.
No programming beyond a few arithmetic operations is required.
I don't want to spell out the complete solution. I hope this is enough to get you started with the arbitrary N.


"join" two list of dictionaries on Id I have two lists of dictionaries where both have the same "id" key but otherwise different data.
list1 = [{"id":1, "spam": 3, "eggs": 5}, {"id": 2, "spam": 5, "eggs": 7}]
list2 = [{"id":1, "knights": 5, "coconuts": 2}, {"id": 2, "knights": 3, "cocounts": 8}]

I know I can do a nested loop something like this (I know I can use dict.update as well, but just writing it out as an exampel):
for id in list1:
    for i in list2:
        if id['id'] == i['id]:
           id['knights'] = i['knights']
           id['coconuts'] = i['coconuts']

but can anyone tell me the more pythonic or quicker way to do this? matching two lists with millions of rows in each does not practically work this way.
Expected result is:
[{"id": 1, "spam": 3, "eggs": 5, "knights": 5, "coconuts": 2}, {"id": 2, "spam": 5, "eggs": 7, "knights": 3, "coconuts": 8}]

To improve time complexity you must sometimes trade-off space complexity. It's known as space-time tradeoff
You can transform list1 into a dictionary where key is id and value is the dictionary itself.
list1 = {d['id']: d for d in list1}
# print(list1)
# {1: {'id': 1, 'spam': 3, 'eggs': 5, 'knights': 5, 'coconuts': 2},
#  2: {'id': 2, 'spam': 5, 'eggs': 7, 'knights': 3, 'coconuts': 8}}

Now, iterate through list2 and update list1 based on id
for d in list2:
    list1[d['id']].update(d)

#print(list1)
# {1: {'id': 1, 'spam': 3, 'eggs': 5, 'knights': 5, 'coconuts': 2},
#  2: {'id': 2, 'spam': 5, 'eggs': 7, 'knights': 3, 'coconuts': 8}}

Take dict.values() to get desired output
print(list(list1.values()))
# [{'id': 1, 'spam': 3, 'eggs': 5, 'knights': 5, 'coconuts': 2},
#  {'id': 2, 'spam': 5, 'eggs': 7, 'knights': 3, 'coconuts': 8}]

Dealing with Edge cases
The above approach fails when list2 has an id which doesn't exist in list1. If that's the case then use this and you want to add that new id to output too. Using dict.setdefault
# for example list2 is as below
# [{'id': 1, 'knights': 5, 'coconuts': 2},
#  {'id': 2, 'knights': 3, 'coconuts': 8},
#  {'id': 3, 'knight': 4, 'coconuts': 5}] -> new `id` that's not in `list1`

# Here we leverage on `dict.setdefault`

for d in list2:
    list1.setdefault(d['id'], dict()).update(d)

print(list1)
# {1: {'id': 1, 'spam': 3, 'eggs': 5, 'knights': 5, 'coconuts': 2},
#  2: {'id': 2, 'spam': 5, 'eggs': 7, 'knights': 3, 'coconuts': 8},
#  3: {'id': 3, 'knight': 4, 'coconuts': 5}}

# To get list of dicts use `list(list1.values())

In case you dont want to update with new id, we can resolve this using if block here.
for d in list2:
    if d['id'] in list1:
        list1[d['id']].update(d)
# {1: {'id': 1, 'spam': 3, 'eggs': 5, 'knights': 5, 'coconuts': 2},
#  2: {'id': 2, 'spam': 5, 'eggs': 7, 'knights': 3, 'coconuts': 8}}
# No `id` 3 present in output

Using libraries that support vectorization?
If you are working on millions of rows you can leverage on pandas which has many vectorized operations. In your case we can use DataFrame.merge
import pandas as pd

df1 = pd.DataFrame(list1)
df2 = pd.DataFrame(list2)

# Now, you have to merge them on `id`

out = df1.merge(df2)

#    id  spam  eggs  knights  coconuts
# 0   1     3     5        5         2
# 1   2     5     7        3         8

To get output as a list of dict you df.to_dict with orient param set to records
out.to_dict(orient='records')

# [{'id': 1, 'spam': 3, 'eggs': 5, 'knights': 5, 'coconuts': 2},
#  {'id': 2, 'spam': 5, 'eggs': 7, 'knights': 3, 'coconuts': 8}]

If you want include new ids then set how param of df.merge to 'outer' by default it's 'inner'
df1.merge(df2, how='outer')

#    id  spam  eggs  knights  coconuts
# 0   1   3.0   5.0        5         2
# 1   2   5.0   7.0        3         8
# 2   3   NaN   NaN        4         5

Code Review

Naming variables
list1 = [...]
list2 = [...]

A new reader might not understand what's list1 and list2. Name them so they make sense to everyone.
current_users_details = [...]
new_details = [...]

This is just an example, name them accordingly. What I do is ask myself what is it storing in the data-structure?... family member details? then variable name is family_members_details, user details then user_details, pi value? then PI_VALUE... etc.

Violation of E231
list1 = [{"id":1, "spam": 3, "eggs": 5}, {"id": 2, "spam": 5, "eggs": 7}]
list2 = [{"id":1, "knights": 5, "coconuts": 2}, {"id": 2, "knights": 3, "cocounts": 8}]

The above lines use inconsistent use of spaces after :, PEP-8 suggest add a space after :.

Violation of E501
list2 = [{"id":1, "knights": 5, "coconuts": 2}, {"id": 2, "knights": 3, "cocounts": 8}]

Lines longer than 79 characters, PEP-8 standard suggests to keep characters in a line <= 79

You have missing ' here if id['id'] == i['id]:

Reusability
Make a function which takes both the lists and updates them.
def update_details(current_details, to_update_with):
    """
    updates user details
    """

    details = {d['id']: d for d in current_details}
    for user in to_update_with:
        details[user['id']].update(user)
    return details

Write a much better docstring :P

I suggest using black python code formatter.
ch3ster@ch3ster:~$ black --line-length 79 your_file.py

Formatted code
Formatted your code using black.
list1 = [{"id": 1, "spam": 3, "eggs": 5}, {"id": 2, "spam": 5, "eggs": 7}]
list2 = [
    {"id": 1, "knights": 5, "coconuts": 2},
    {"id": 2, "knights": 3, "cocounts": 8},
]

for d in list2:
    list1[d["id"]].update(d)

print(list(list1.values())) # Equivalent to print([*list1.values()])


Refactoring a Crawler I've recently ported an old project and made it object-oriented. However, I've noticed that rubocop points out the following status: Assignment Branch Condition size for fetch_images is too high. [22.41/15]. Can I make this method be made more efficient, perhaps?

Snippit

class WebCrawler
  attr_reader :agent, :tumblr, :images

  def initialize
    @agent = Mechanize.new
    @agent.user_agent_alias = Mechanize::AGENT_ALIASES.keys
      .reject { |agent| agent == 'Mechanize' }.sample
    @tumblr = URI('http://www.tumblr.com')
    @images = []
  end

  def fetch_images(blog, proxy)
    fail Exceptions::InvalidBlog unless valid_blog?(blog)
    fail Exceptions::InvalidProxy unless valid_ip?(proxy)

    tokens = proxy.split(':')
    proxy_addr = { ip: tokens[0], port: tokens[1].to_i }
    @agent.set_proxy(proxy_addr[:ip], proxy_addr[:port])

    @tumblr.host.gsub!(/^w{3}/, '').insert(0, "#{blog}")
    page = @agent.get(@tumblr)
    page.search('img[src$="jpg"], img[src$="png"]').each do |img|
      @images << img['src']
    end
    @images.keep_if do |img|
      img =~ %r{^https?://\d+|media} && img !~ /avatar/
    end
  end
end

Assignment Branch Condition size for fetch_images is too high. [22.41/15]

What that warning is saying is that, according to rubocop's standard, you have too many assignments, branches, and conditionals (ABCs) in that method. ABCs are associated with code that is complex and harder to reason about. It can also indicate that a method is trying to do too much, as I would say it is telling you about this method.

Looking over the method in question, we can clearly see it has several responsibilities:

Guard clauses for validity
Tokenizing a proxy string into a hash data structure
Setting a proxy with the data from that data structure
Tearing apart and rebuilding a URL
Fetching a page over HTTP
Parsing markup for image tags, and shoving them on an array
Filtering that array of images
Returning that array

That's a lot of responsibilities for one method. It's dealing with several different types of objects/behaviors at several different levels of abstraction. We can, and should, break it up to make it easier to manage.

Before I do that, I'm going to look at the rest of the class (the initialize method). In the initializer, I see a lot of state assignment, and I see some work beyond just state assignment.

@agent = Mechanize.new

Here you are getting a new instance of Mechanize, but you are doing it directly in the constructor. I would recommend injecting this as a parameter instead. You could even use a default parameter to give you flexibility:

def initialize(agent = Mechanize.new)
  @agent = agent
  ...

This gives you the ability to change this dependency at runtime, and to mock or stub this dependency in your tests.

@agent.user_agent_alias = Mechanize::AGENT_ALIASES.keys
  .reject { |agent| agent == 'Mechanize' }.sample

Here is some more of the extra work being done in the initialize method. Ideally, you should get a fully initialized object injected, and you should not need to to any extra configuration with it. In this case, you might want to create a MechanizeFactory class to encapsulate the construction and initialization of Mechanize objects so you don't have to worry about any of this in your constructor:

class MechanizeFactory
  def self.get
    mechanize = Mechanize.new
    mechanize.user_agent_alias = Mechanize::AGENT_ALIASES.keys
      .reject { |agent| agent == 'Mechanize' }.sample
    mechanize
  end
end

...

class WebCrawler
  def initialize(agent = MechanizeFactory.get)
    @agent = agent
    ...

@tumblr = URI('http://www.tumblr.com')

Here you are setting the URL to the Tumblr main site as an instance variable. The way you use it does not really make it appropriate to be an instance variable. I will discuss why and alternatives further down, but for now I am going to say that this line is unnecessary.

@images = []

You are creating an instance variable to hold the list of returned images. I don't see any reason that this object needs to hold this state; the list of images can simply be returned from the fetch_images method. This line is also unnecessary.

Now onto the fetch_images method.

fail Exceptions::InvalidBlog unless valid_blog?(blog)
fail Exceptions::InvalidProxy unless valid_ip?(proxy)

Here are your guard clauses. For now, I'm going to just leave them as they are.

tokens = proxy.split(':')
proxy_addr = { ip: tokens[0], port: tokens[1].to_i }
@agent.set_proxy(proxy_addr[:ip], proxy_addr[:port])

Here is one place where a lot is happening. We are doing string tokenization, data structure juggling, and setting more properties on @agent. I would first have to ask if the proxy really changes with each request. I think it would probably be better to simply set the proxy in the MechanizeFactory.get method by passing it in by parameter. If it does need to change, there are other strategies to deal with it, but for now I'm going to go on my assumption. This means those three lines are effectively gone, with their functionality relocated. The proxy parameter and proxy validation guard clause are also gone.

@tumblr.host.gsub!(/^w{3}/, '').insert(0, "#{blog}")

Now we are statefully changing that URL instance variable, which I said we no longer need. We can use a local variable, but this still deals with a lower level of abstraction than the rest of the method. We should extract it to a private helper and call that from our code. We can also just combine it with the following line to avoid the need for a temporary local variable.

class MechanizeFactory
  def self.get(proxy = '') # Or some logical default for proxy
    fail Exceptions::InvalidProxy unless valid_ip?(proxy)

    mechanize = Mechanize.new

    mechanize.user_agent_alias = Mechanize::AGENT_ALIASES.keys
      .reject { |agent| agent == 'Mechanize' }.sample

    proxy_ip, proxy_port = proxy.split(':')
    mechanize.set_proxy(proxy_ip, proxy_port)

    mechanize
  end
end

...

class WebCrawler
  def initialize(agent = MechanizeFactory.get)
    @agent = agent
  end

  def fetch_images(blog)
    page = @agent.get(get_blog_uri(blog))
    ...
  end

  private

  def get_blog_uri(blog)
    fail Exceptions::InvalidBlog unless valid_blog?(blog)
    URI("http://#{blog}.tumblr.com/")
  end
end

page.search('img[src$="jpg"], img[src$="png"]').each do |img|
  @images << img['src']
end
@images.keep_if do |img|
  img =~ %r{^https?://\d+|media} && img !~ /avatar/
end

Here you are doing an XPath search, shoveling the results onto an array, and then filtering the array. This is also at a different level of abstraction, and it can be optimized. We can move it into it's own private helper method. We don't have the @images instance variable, but page.search returns an Enumerable object that we can call map on, and then manipulate. The final product looks like:

class MechanizeFactory
  def self.get(proxy = '') # Or some logical default for proxy
    fail Exceptions::InvalidProxy unless valid_ip?(proxy)

    mechanize = Mechanize.new

    mechanize.user_agent_alias = Mechanize::AGENT_ALIASES.keys
      .reject { |agent| agent == 'Mechanize' }.sample

    proxy_ip, proxy_port = proxy.split(':')
    mechanize.set_proxy(proxy_ip, proxy_port)

    mechanize
  end
end

...

class WebCrawler
  def initialize(agent = MechanizeFactory.get)
    @agent = agent
  end

  def fetch_images(blog)
    page = @agent.get(get_blog_uri(blog))
    extract_images_from_page(page)
  end

  private

  def get_blog_uri(blog)
    fail Exceptions::InvalidBlog unless valid_blog?(blog)
    URI("http://#{blog}.tumblr.com/")
  end

  def extract_images_from_page(page)
    page.search('img[src$="jpg"], img[src$="png"]').map do |img|
      img['src']
    end.keep_if do |img|
      img =~ %r{^https?://\d+|media} && img !~ /avatar/
    end
  end
end

There is more that can be done, I am sure, but this at least starts to break the code down into smaller, more focused pieces.


Optimizing a Partition Method I am looking for assistance in optimizing a partition method I wrote for a class. It does its job correctly, but I only received half points on it, so I was wondering if there was a way to make it more efficient?

private int runPartition(int lowIndex, int highIndex)
  {
     //Sets up pivot index and values
     int pivotIndex = lowIndex;
     int pivotValue = localArray[pivotIndex];

     //Inner counter value
     int counterOne = highIndex + 1;

     //Loops through array from top down
     for (int counterTwo = highIndex; counterTwo >= lowIndex; counterTwo--)
        {
           //If element is bigger than pivot, move to end
           if (localArray[counterTwo] > pivotValue)
              {
                 //Decrements inner counter and swaps values
                 counterOne--;
                 swapValuesAtIndex(counterTwo, counterOne);
              }
        }

     //At end, swap pivot value with value BEFORE larger elements
     counterOne--;
     swapValuesAtIndex(counterOne, pivotIndex);

     //Sets new pivot index and returns
     pivotIndex = counterOne;
     return pivotIndex;
  }

I'm wondering if it is the efficiency that resulted in you getting half points:

runPartition is rather an unclear name in itself;
highIndex seems to be inclusive, which is uncommon (the number of elements is now highIndex - lowIndex + 1);
counterOne and counterTwo are not good names for indices - even i and j would have been better;
it is very likely that the localArray as a field name raises some questions; why is it a field in the first place? And why is it called local if it is not local to the method?
it seems to me that you would set the counterOne to the index you want to swap and perform the decrease afterwards (possibly using -- in the call to swapValues);
the braces are at uncommon and uneven positions when it comes to Java / indentation.


Add license to beginning of files I'm trying to come up with a script that will add license headers to certain files (depending on extension). My Bash is very average, so any guidance would be much appreciated.

I'm not quite sure what to ask for except for whether I'm making any gross mistakes.

#!/usr/bin/env bash

set -euo pipefail

shopt -s globstar nullglob extglob

lineone="Copyright (c) 2015 Acme Corp, LLC."
linetwo="Unauthorized copying of this file via any medium is strictly prohibited."
linethree="Written by John Doe <j@acme.com>, July 2015" # etc.

printf "// %s
// %s
// %s\n\n" "$lineone" "$linetwo" "$linethree" > "/tmp/$USER-license-go"

printf "{{/*
%s
%s
%s
*/}}\n\n" "$lineone" "$linetwo" "$linethree" > "/tmp/$USER-license-gohtml"

printf "# %s
# %s
# %s\n\n" "$lineone" "$linetwo" "$linethree" > "/tmp/$USER-license-shell"

typ=""
for f in **/*; do
    if [[ -f $f ]]; then
        if ! grep -q Copyright "$f"; then
            case "$f" in
                *.go             ) typ="go"      ;;
                *.gohtml         ) typ="gohtml"  ;;
                @(*.bash|*.conf) ) typ="shell"   ;;
                *                ) continue      ;;
            esac

            cat "/tmp/$USER-license-"$typ "$f" | sponge "$f"
        fi
    fi
done

This is quite fine. I have a few tips though.

Don't repeat yourself

The base path "/tmp/$USER-license-" appears many times.
This is error prone,
and it makes it difficult to change later.
It would be better to put this in a variable and reuse it everywhere.

here-document instead of printf

Instead of the printf statements to generate the headers,
here-documents would be simpler and better, for example:

cat <EOF >"$BASEPATH"-go
// $lineone
// $linetwo
// $linethree

EOF

Tabulation

Aligning things vertically like this may look nice at first:

case "$f" in
    *.go             ) typ="go"      ;;
    *.gohtml         ) typ="gohtml"  ;;
    @(*.bash|*.conf) ) typ="shell"   ;;
    *                ) continue      ;;
esac

The problem with this is if you want to change something later.
For example, I don't really see the purpose of the @(...) there,
so I would like to simplify:

case "$f" in
    *.go             ) typ="go"      ;;
    *.gohtml         ) typ="gohtml"  ;;
    *.bash|*.conf ) typ="shell"   ;;
    *                ) continue      ;;
esac

But that broke the formatting. What do I do now?
Add padding to line up the ) that is now out of line?
Or update the other lines?
The second option is not great, because I have to change many lines just because I changed one.
Just thinking about this is actually a waste of time.

Considering possible future changes,
I suggest to avoid this kind of writing style,
and stick to good old simple:

case "$f" in
    *.go) typ="go" ;;
    *.gohtml) typ="gohtml" ;;
    *.bash|*.conf) typ="shell" ;;
    *) continue ;;
esac

Alternative to sponge

I didn't know about sponge and moreutils,
I'm really glad your has question lead me to discover these gems.

On the other hand, unfortunately sponge is not a standard command,
and may not be readily available. Here's an alternative using GNU sed:

license="$BASEPATH"-$typ
sed -i -e "1r$license" -e '1{h;d}' -e '2{x;G}' "$f"


Booking appointments with MySQLi and PHP The following code books one appointment for one or more slots. I have never used database before, but have seen so many warnings regarding SQL injections. Since it has never affected me I have not paid attention.

<?php

    include('php/connect.php');

    if(isset($_POST['slots_booked'])) $slots_booked = mysqli_real_escape_string($link, $_POST['slots_booked']);
    if(isset($_POST['name'])) $name = mysqli_real_escape_string($link, $_POST['name']);
    if(isset($_POST['akeri'])) $akeri = mysqli_real_escape_string($link, $_POST['akeri']);
    if(isset($_POST['email'])) $email = mysqli_real_escape_string($link, $_POST['email']);
    if(isset($_POST['phone'])) $phone = mysqli_real_escape_string($link, $_POST['phone']);
    if(isset($_POST['typ'])) $typ = mysqli_real_escape_string($link, $_POST['typ']);
    if(isset($_POST['fri'])) $fri = mysqli_real_escape_string($link, $_POST['fri']);
    if(isset($_POST['booking_date'])) $booking_date = mysqli_real_escape_string($link, $_POST['booking_date']);

    $booking_array = array(
            "slots_booked" => $slots_booked,
            "booking_date" => $booking_date,
            "name" => $name,
            "email" => $email,
            "Åkeri" => $akeri,
            "phone" => $phone,
            "typ" => $typ,
            "fri" => $fri
    );

    $explode = explode('|', $slots_booked);

    foreach($explode as $slot) {

        if(strlen($slot) > 0) {

            $stmt = $link->prepare("INSERT INTO bookings (date, start, name, email, phone, typ, fri) VALUES (?, ?, ?, ?, ?, ?, ?)");
            $stmt->bind_param('sssssss', $booking_date, $slot, $name, $email, $phone, $typ, $fri);
            $stmt->execute();

        } // Close if

     } // Close foreach

    header("location: Done.html");
    exit;
?>

Code has to be readable

  if(isset($_POST['slots_booked'])) $slots_booked = mysqli_real_escape_string($link, $_POST['slots_booked']);

Going through this code I really have to focus to try to figure out what is happening, there is no 'flow'. Now add 7 of those and you got some serious reading ahead of you. You are using prepared statements so don't escape your values 2 times, prepared statements protect from sql injection.

Use meaningful english names

Database columns, variable names etc have to be in english and have to tell what they are responsible for.

  $booking_array = array(
  "slots_booked" => $slots_booked, // not entirely clear what this column stores
  "booking_date" => $booking_date,
  "name" => $name,
  "email" => $email,
  "Åkeri" => $akeri, // lets open up google translate
  "phone" => $phone,
  "typ" => $typ, // no idea what this column stores
  "fri" => $fri // no idea what this column stores
  );

As @Sumurai8 noted: $booking_array is not even used yet it takes up 30% of the code.

This one is just left there hanging. I suppose $slots_booked are an array of some slots? No idea what those slots represent. Variable name $explode will not help with understanding code it is used in.

 $explode = explode('|', $slots_booked);

Foreach does not start that bad. $explode is array of $slot's, it asks for renaming $explode to $slots.

What kind values do you expect $slot's to have? Checking empty($slot) does not cut it?

How short preparation of query is! Just 3 lines of code. It's great isn't it? Not necessarily. You sacrificed code's readability by making it so short. Each parameter should have proper placeholder name => :name, start => :start. I would recommend providing bindParam() with type you expect given param to have.

Here we are given another clue as to what $slot might be. It is start of bookings. Maybe instead of $slot something like $booking_start would be more understandable?

foreach($explode as $slot) {

    if(strlen($slot) > 0) {

        $stmt = $link->prepare("INSERT INTO bookings (date, start, name, email, phone, typ, fri) VALUES (?, ?, ?, ?, ?, ?, ?)");
        $stmt->bind_param('sssssss', $booking_date, $slot, $name, $email, $phone, $typ, $fri);
        $stmt->execute();

    } // Close if

} // Close foreach

Below code is example in which way your code can go and be more readable. DB object was taken from here.

class Booking {
    public $start;
    public $name;
    public $akeri;
    public $email;
    public $phone;
    public $typ;
    public $fri;
    public $date;

    public function save() {
        $stmt = DB::prepare("INSERT INTO bookings (date, name) VALUES (:date, :name)");
        $stmt->bind_param(':date', $this->date);
        $stmt->bind_param(':name', $this->name);
        // rest of attributes
        $stmt->execute();
    }
}

$bookingDates = explode('|', $_POST['slots_booked']);
foreach($bookingDates as $bookingDate) {
    $booking = new Booking();
    $booking->name = $_POST['name'];
    // set rest of attributes

    $booking->save();
}

EDIT

Changed $bookingDetails to $bookingDates


How can I improve my code which dynamically populates a section of a page? My site consists of a lot of asynchronous calls, and because of this a lot of content is dynamically added to the page. For this section of code I've dynamically added tabs to a page and need to create the selected tab section. This function is fired when a tab is clicked (note that $devices_page is a pre-existing divider):

function populateDevicesPage(deviceArray) {
    // Hide the current page
    $devices_page.stop().slideUp(animationSpeed, function() {
        // Check if the $devices_page has any current children
        if($devices_page.children().length > 0)
        { /* Ignore this part, it's the below else that matters */ }
        // If not, create page elements
        else
        {
            // Create the main devices page elements
            var
                $device_name = $('<h1/>').appendTo($devices_page),
                $device_inner_page = $('<div id="devices-page-inner"/>').appendTo($devices_page),
                $commands_table = $('<table/>').appendTo($device_inner_page),
                $commands_thead = $('<thead/>').appendTo($commands_table),
                $commands_tbody = $('<tbody/>').appendTo($commands_table),
                $commands_thead_row = $('<tr/>').appendTo($commands_thead)
            ;

            // Create the table headings
            $('<th/>').text('Name').appendTo($commands_thead_row);
            $('<th/>').text('Command').appendTo($commands_thead_row);

            // Loop through each of the commands to populate the table
            for(var i=0;i<deviceArray.commands.length;i++)
            {
                var $command_tbody_row = $('<tr/>').appendTo($commands_tbody);
                $('<td/>').text(deviceArray.commands[i].name).appendTo($command_tbody_row);
                $('<td/>').text(deviceArray.commands[i].com).appendTo($command_tbody_row);
            }

            $device_name.text(deviceArray.deviceName);
        }

        // Show the newly populated page
        $devices_page.stop().slideDown(animationSpeed);
    });
}

As you can see there are a lot of elements being created and added to the pre-existing $devices_page divider. This does only happen once (when a tab is selected for the first time), but I was wondering if there's a much better approach I could adopt instead.

The full code can be seen here if anyone wants a further poke around: https://github.com/JamesDonnelly/WebRemoteNotifier/blob/master/js/generic.js. Do note that this is a very rough draft of a project I've only spent about 3-4 hours working on, so I imagine there are a lot of imperfections; for now I'm only really concerned about the above function.

Thanks a lot!

If you don't need the data as JSON, you could just have the server send you the table markup. Then just inject it straight into the page using .load()

Alternatively, if you want JSON data, or can't/won't mess with the server side, you might consider having a hidden, empty table in the markup to begin with, so you don't need to create the entire thing from scratch. Then you can just populate the header cells with the relevant text, and only worry about adding the rows to tbody.

Of course, it'll introduce tighter coupling between the markup and the JS, which you might want to avoid. Since this is a new project that's probably changing all the time, it might be best to simply stick with you current approach of doing everything in JS, to keep things flexible. But my thinking is that since you're basically hardcoding things like the table headers in JavaScript, you're better off hardcoding it the markup where it belongs, rather than generating it.

And you can also make the coupling a little looser, by using some data-* attributes in the "template" table to declare their intended content.

For instance, have this in the page markup

      Name
      Command

Then, on the JS-side, do something like:

var tbody    = $("tbody"),
    template = tbody.children(".template").removeClass(".template").remove(), // removed, but still in memory
    newRow, item;

for(var i = 0, l = deviceArray.commands.length ; i < l ; i++) {
  item   = deviceArray.commands[i];
  newRow = template.clone();
  newRow.children("[data-property]").each(function () {
    var cell = $(this),
        prop = cell.data("property");
    cell.text(item[prop]);
  });
  newRow.appendTo(tbody);
}

That should populate table nicely.

Of course, for such a simple two-column table, it might be overkill. Still, if you need to populate more tables like this, you can create a pretty generic function that you just pass a "scaffold" element like the empty table above plus a data array, and use that anywhere it's needed.


Python function to determine whether a given BST is valid Here's a simple function I wrote using recursion. Does this run in \$O(n)\$ time and use \$O(n)\$ space, where \$n\$ is the number of nodes in the tree?

def is_valid(root, min, max):
    if not root:
        return True
    if min is None or max is None:
        return is_valid(root, float("-inf"), float("+inf"))
    print(root.value, min, max)
    if root.value <= min:
        return False
    if root.value >= max:
        return False
    return is_valid(root.left, min, root.value) and is_valid(root.right, root.value, max)

Stick with None

One of the things you do is effectively convert Nones into -inf and +inf. But that's not necessary. Stick with None, and then you can ignore those comparisons. I would additionally explicitly default the arguments:

def is_valid(root, min=None, max=None):
    if not root:
        return True
    if min is not None and root.value <= min:
        return False
    if max is not None and root.value >= max:
        return False
    return is_valid(root.left, min, root.value) and is_valid(root.right, root.value, max)

I also removed the print - since you don't really want random output as you go through your function.

Besides that, looks good to me. The last line is a bit long, so I might wrap it in parentheses just so I can do:

    return (is_valid(root.left, min, root.value) and
            is_valid(root.right, root.value, max))

Try to keep your lines below 80 characters.

An alternate implementation, just throwing it out as an idea for thinking about the problem completely differently is this. A BST is valid if it's in-order traversal is actually ordered. An in-order traversal would be:

def traverse_in_order(root):
    if root:
        yield from traverse_in_order(root.left)
        yield root.value
        yield from traverse_in_order(root.right)

In Python2.7, replace yield from X with for x in X: yield x. Basically, we're turning our tree into an iterable. Then, we can just pairwise iterate over this with some itertools recipes:

def pairwise(iterable):
    "s -> (s0,s1), (s1,s2), (s2, s3), ..."
    a, b = tee(iterable)
    next(b, None)
    return izip(a, b)

And just check that every pair is in strictly increasing order:

def is_valid(root):
    return all(i < j for i,j in pairwise(traverse_in_order(root)))

Not saying this is better than your original approach, but it's certainly different.


HTML / JS Slot Machine Simulator This code is a slot machine simulator that uses elements of both HTML and JavaScript. Please pardon the lack of comments.

<!DOCTYPE html>

<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
<title>www.loseloselose.com</title>

Lucky Number Slots

<svg width="550" height="250" viewBox="0 0 1600 500">

<rect x="0" y="10" width="500" height="500" fill="black" stroke="white" stroke-width="10">
</rect>

<rect x="550" y="10" width="500" height="500" fill="black" stroke="white" stroke-width="10">
</rect>

<rect x="1100" y="10" width="500" height="500" fill="black" stroke="white" stroke-width="10">
</rect>

<text x="100" y="415" font-family="courier" font-size="500" fill="white">

</text>

<text x="650" y="415" font-family="courier" font-size="500" fill="white">

</text>

<text x="1200" y="415" font-family="courier" font-size="500" fill="white">

</text>
</svg>

<form>

<input type="button" value="Click Here To Spin" onClick="Refresh()">

</form>

Add Credit

0

Some improvements:

Use of refresh() as a function used to generate the number values, which generate the values AND writes into the DOM element.

Use of a function to manage all user messages, write_message()

Remove of all the old fashion HTML tags, and change it for CSS.

A function for managing spending the "credits" to make it easier to manage them.

<!DOCTYPE html>

    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">
    <title>www.loseloselose.com</title>

    Lucky Number Slots

    <svg width="550" height="250" viewBox="0 0 1600 500">

    <rect x="0" y="10" width="500" height="500" fill="black" stroke="white" stroke-width="10">
    </rect>

    <rect x="550" y="10" width="500" height="500" fill="black" stroke="white" stroke-width="10">
    </rect>

    <rect x="1100" y="10" width="500" height="500" fill="black" stroke="white" stroke-width="10">
    </rect>

    <text x="100" y="415" font-family="courier" font-size="500" fill="white" id="first">
    0
    </text>

    <text x="650" y="415" font-family="courier" font-size="500" fill="white" id ="second">
    0
    </text>

    <text x="1200" y="415" font-family="courier" font-size="500" fill="white" id="third">
    0
    </text>
    </svg>

    <input type="button" value="Click Here To Spin" onClick="refresh()">

    Add Credit

0


How to find position (array index ) of a number in a given array? I have to find the position (array index) of a number in an array, the number can lie in between any other two numbers in the array. I have written code for this but I want some alternative way to do this.

example: let the array be given as

float arr[]={1.5, 3.65, 4.9, 8.4, 10.6, 17.5, 19.45, 21.86, 25.67, 30.65};

Now I have to find the position of 12.6 in the given array, since 12.6 lies between 10.6 and 17.5 in the above array so, it should return array index as 4.

Is there any other way to do this? I know it can be done by using binary search but I don't know how to implement it (since the number is not present in array)

The array is sorted (ascending order)

#include<stdio.h>
int main()
{
int position;
int i;
float num=12.6; // position of this number has to be determined i.e. it lies between 10.6
// and 17.5 in the array given below so it should give array index as 4
float arr[]={1.5, 3.65, 4.9, 8.4, 10.6, 17.5, 19.45, 21.86, 25.67, 30.65}; // any random array

for(i=0;i<10;i++)
{
if(arr[i] > num)
{
position=i-1;
break;
}
}
printf("array index is %d",position); // this gives index as 4

return 0;
}

Indent the Code

Most computer code is indented, in some languages indentation is used to show the level of complexity of the code, in other languages the code won't work properly unless it is indented.

Declare the Variables as They are Needed

The integer variable i can be declared in the for loop and that will limit the scope to just the for loop.

for(int i = 0; i < 10; i++)
{
    if(arr[i] > num)
    {
        position = i - 1;
        break;
    }
}

Horizontal Spacing

To make the code more readable, put spaces between the operators and the operands as shown above in the for loop example. Squashing all the code together makes it very difficult to understand what the code is actually doing.

Performance

First run the -O3 optimizer during compilation and linking. If the program were more complicated than it is then use a profiling tool such as gprof.

Linear searches are known to be a slower way to do things.

I think the position reported is off by one, if I was using this code to do an insertion sort, I wouldn't subtract one from position.

Shorten the Code.

There are 2 shorter ways to subtract 1 from position.

    --position;

and

    position -= 1;


Progressive tax program Please review this code, which should calculate the tax percentage a person will pay in a progressive tax system. The goal is to have a perfect program.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication1
{
    class TaxLevel
    {
        private decimal _Money=-1;
        /// <summary>
        /// the money count that the tax applies to
        /// </summary>
        public decimal Money
        {
            get { return _Money; }
            set { if (value > 0) { _Money = value; } else { throw new ArgumentOutOfRangeException("Money must be grater than 0"); } }
        }

        private decimal _TaxDeduction=-1;
        /// <summary>
        /// the tax level where 1.0 is 100% tax
        /// </summary>
        public decimal TaxDeduction
        {
            get { return _TaxDeduction; }
            set { if (value > 0 && value < 1.0m) { _TaxDeduction = value; } else { throw new ArgumentOutOfRangeException("TaxDeduction must be between 0.0 and 1.0"); } }
        }

     }

     class TaxCalculator
    {
        List<TaxLevel> _TaxLevels;

        public bool DeterminateTaxLevels(){

            _TaxLevels = new List<TaxLevel>();
            _TaxLevels.Add(new TaxLevel {Money = 4770m, TaxDeduction =  0.10m });
            _TaxLevels.Add(new TaxLevel { Money = 3700, TaxDeduction = 0.14m });
            _TaxLevels.Add(new TaxLevel { Money = 4249m, TaxDeduction = 0.20m });
            _TaxLevels.Add(new TaxLevel { Money = 5529m, TaxDeduction = 0.28m });
            _TaxLevels.Add(new TaxLevel { Money = 21089m, TaxDeduction = 0.31m });
            _TaxLevels.Add(new TaxLevel { Money = decimal.MaxValue, TaxDeduction = 0.42m });
        return true;
        }
         /// <summary>
         /// Gets Salley In Bruto Calculates Netto Sallery
         /// </summary>
         /// <param name="SalleryInNis">Employee Bruto Sallery</param>
         /// <returns>Neto Sallery</returns>
        public  decimal CalculateNetto(decimal SalleryInNis)
        {
            if (SalleryInNis < 0) throw new ArgumentOutOfRangeException("SalleryInNis must be greater than 0");
            if (_TaxLevels == null) throw new ArgumentNullException("DeterminateTaxLevels First");
            decimal TmpSallery =SalleryInNis;
            decimal Netto = 0.0m;
            foreach (var taxlevel in _TaxLevels)
            {
                if (TmpSallery <= 0) break;
                if (TmpSallery > taxlevel.Money)
                {
                    Netto += taxlevel.Money * (1-taxlevel.TaxDeduction);
                }
                else {
                    Netto += TmpSallery * (1-taxlevel.TaxDeduction);
                }
                TmpSallery -= taxlevel.Money;
            }
            return Netto;
        }
         /// <summary>
         /// Calculates Average Tax for employee
         /// </summary>
        /// <param name="SalleryInNis">Employee Bruto Sallery</param>
        /// <returns>AverageTax</returns>
        public  decimal CalculateAverageTax(decimal SalleryInNis)
        {
            if (SalleryInNis < 0) throw new ArgumentOutOfRangeException("SalleryInNis must be greater than 0");
            return 1- (CalculateNetto(SalleryInNis) / SalleryInNis);
        }
    }
    class Program
    {
        static void Main(string[] args)
        {
            decimal Sal= 11712000;
            TaxCalculator TaxCalculator = new TaxCalculator();
            TaxCalculator.DeterminateTaxLevels();
            Console.WriteLine("Brutto " + Sal);
            Console.WriteLine("Netto " + TaxCalculator.CalculateNetto(Sal));
            Console.WriteLine("Average Tax " + TaxCalculator.CalculateAverageTax(Sal));
            Console.Read();
        }
    }
}

If something is mandatory then use private setters and constructor arguments, and validate in the constructor (TaxLevel) [1];
Use collection initializer notation [2];
DeterminateTaxLevels should be private and called from the constructor (I would actually make it a simple attribution in the constructor);
DeterminateTaxLevels should have a better name like LoadTaxLevels, because it does not "determine" anything;
DeterminateTaxLevels has no reason to return bool - why does it?;
Salary is misspelt;
Either call the salary "bruto" or "in Nis" - you should be consistent, and I advise "bruto" because it means more than some random TLA like NIS (whatever it is);
As @Gene S said, try to follow coding standards: use brackets in ifs, use line-breaks after ifs, use camelCase on parameter names and not PascalCase, etc...

footnotes
[1]:
private class TaxLevel
{
    /// <summary>
    /// The money count that the tax applies to.
    /// </summary>
    public decimal Money { get; private set; }
    /// <summary>
    /// The tax level where 1.0 is 100% tax.
    /// </summary>
    public decimal TaxDeduction { get; private set; }

    public TaxLevel(decimal money, decimal taxDeduction) {
        if (money < 0 ) {
            throw new ArgumentOutOfRangeException("Money must be grater than 0");
        }
        if (taxDeduction < 0 || taxDeduction > 1.0m) {
            throw new ArgumentOutOfRangeException("TaxDeduction must be between 0.0 and 1.0");
        }
        Money = money;
        TaxDeduction = taxDeduction;
    }
}

[2]:
_TaxLevels = new List<TaxLevel>() {
    new TaxLevel {Money = 4770m, TaxDeduction =  0.10m },
    new TaxLevel { Money = 3700, TaxDeduction = 0.14m },
    // ...
    new TaxLevel { Money = decimal.MaxValue, TaxDeduction = 0.42m },
};


Printing a string in NASM Win16 Assembly A small script that simply prints a given string. It's an improved snippet that combines some recommendations given in my post on string helper functions.

org 100h

mov     si, hello
call    puts
ret

puts:
    jmp     .run
.putc:
    mov     ah, 0Eh
    mov     bx, 7
    int     10h
.run:
    lodsb
    cmp     al, 0
    jne     .putc
    ret

hello db "Hello World!", 0

You have all the elements. The only thing that can be eliminated is jmp .run, as follows:

org 0x100

        mov     si, Prompt
        call    puts
        ret

  puts: mov     ah, 0xe
        mov     bx, 7
 .read: lodsb
        or       al, al
        jnz     .post
        ret
 .post: int     0x10
        jmp     .read

 Prompt: db  'Hello World', 0

As INT 10H does not trash AH or BX, there is no need to reinitialize them each time through the loop.


Faster(?) factorial calculations I saw this question, but was unable to answer because of my inexistant knowledge of Rust, so I decided to try and write an algorithm on my own and put it for review.

There is the casual way to compute factorials:

\$n! = \prod\limits_{k=1}^n k\$

public static BigInteger fact(BigInteger n)  => n == 0 ? 1 : n*fact(n - 1);

The problem with this one is that with big factorials, it get's slow (or, in this case, throw a StackOverflowException.

Now, reading this "paper", I figured I'd try to implement a faster algorithm to compute factorials.

From what I understood, the equation would look something like this :

\$\prod\limits_{i=1}^{\frac{n}{2}}( \sum\limits_{z=0}^i (n - z\times2))((\lceil{\frac{n}{2}\rceil})\$  (if n is odd)\$)\$

The idea is to reduce the number of multiplications by half.

public static BigInteger Factorial(int n)
{
    BigInteger sum = n;
    BigInteger result = n;
    for (int i = n - 2; i > 1; i -= 2)
    {
        sum = (sum + i);
        result *= sum;
    }

    if (n % 2 != 0)
        result *= (BigInteger)Math.Round((double)n / 2, MidpointRounding.AwayFromZero);

    return result;
}

I decided to keep a sum variable so I don't need to redo the sum computing at each multiplication's iteration.

I'd like to see if I missed something or if this can be optimized. (I'm doing this as a mathematical exercise, so there's no need for performance, I'm looking at ways to make it more performant just because I want to understant the maths behind it).

\$0! = 1\$, which is a case not handled by the code.

The method should throw an ArgumentOutOfRangeException if n is negative.

You could also replace this

if (n % 2 != 0)
    result *= (BigInteger)Math.Round((double)n / 2, MidpointRounding.AwayFromZero);

with this

if (n % 2 != 0)
    result *= n / 2 + 1;

if you find it clearer.


Rearrange String such that similar characters are placed at least 'K' distance apart So I was asked this in an interview, and I had to write this code on a white board. Sadly I couldn't complete the code :( .

I've realized that writing code on white board is completely different from typing it on an editor. Firstly because, I type way faster than I write and secondly I can't keep adding lines in between, so the first line has to be my best line of code.

What are some of the things I should keep in mind before I write my code, so that I make sure I make minimal changes once I write?

// The idea is to calculate frequencies of characters, and create the
// string by putting highest frequency characters one after another into it
// for 'K' times, and repeat the process.
/* Example:
* S = AAABBBCC, K = 3
* Result : ABCABCAB (all 'A's are 3 distance apart, similarly with B's and C's)
*/
class Node implements Comparable<Node>{
    char c;
    int frequency;

    public Node(char c, int f) {
        this.c = c;
        this.frequency = f;
    }

    public int compareTo(Node other) {
        if (this.frequency > other.frequency)
            return -1;
        if (this.frequency < other.frequency)
            return 1;
        if (this.c < other.c)
            return -1;
        if (this.c > other.c)
            return 1;
        return 0;
    }
}

public String rearrange(String s, int k) {
    StringBuilder sb = new StringBuilder();
    // calculate frequency
    Map<Character, Integer> map = new HashMap<Character, Integer>();
    for (Character c : s.toCharArray()) {
        if (map.containsKey(c)) {
            map.put(c, map.get(c) + 1);
        } else {
            map.put(c, 1);
        }
    }
    // create frequency based heap, if duplicate frequency, order alphabetical
    PriorityQueue<Node> heap = new PriorityQueue<Node>();

    for (Character c : map.keySet()) {
        heap.add(new Node(c, map.get(c)));
    }

    int len = 0;
    // keep removing k elements from heap
    // add them to result string
    // decrease frequency and add back to heap
    while (len < s.length()) { // (n/k) * k log(n) == n log(n)

        int tempK = 0;
        Deque<Node> tempList = new LinkedList<Node>();

        while (tempK < k && !heap.isEmpty()) { // k log(n)
            Node node = heap.remove();
            sb.append(node.c);
            node.frequency--;
            if (node.frequency != 0) {
                tempList.add(node);
            }
            tempK++;
        }
        while (!tempList.isEmpty()) { //k log(n)
            heap.add(tempList.remove());
        }
        len = len + k;
    }
    return sb.toString();
}

I think it's unlikely that interviewers are expecting you to write perfect code up on the white board.  The main goals of these tasks is to get an insight into how you go about solving problems, knowledge of the language features and attention to detail.  You can demonstrate attention to detail by writing 100% correct code, however you can also demonstrate it in other ways.  My starting point is usually to clarify the requirements with the interviewers.  You can do this in the context of unit testing to help identify some test cases that you are considering and getting them to clarify what the expected outcomes from those cases would be.  These can be added to the board so that you have a reference.  So, for example:

What happens if you are passed an empty string
What happens if there is only 1 characters
What happens if you've got "aa" as suggested by @coderodde
What happens with "AAABBBCC", which is your example.  You've said it should be "ABCABCABC", which has introduced an extra C.  Is this really right?

Clarifying the problem not only demonstrates attention to detail and helps you to understand the problem, it also buys you some time to think about how best to solve the problem.

Next you need to think about the algorithm you're going to implement.  Consider jotting it down on paper or the whiteboard so that you've got a reference design, that way you can worry about the order/steps required without having to worry about the language constructs/syntax at the same time.

Have some kind of checklist in mind for what you need to consider in your solution. For example:

Public interface
Parameter checking
Error handling
Exceptions
etc

This will help you to at least consider these things.  You can then talk about them if appropriate, even if you choose not to write them up on the board.  So "I'd check 'k' to ensure it's a positive integer at the start of this method and throw IllegalArgumentException if it wasn't, however I'm not going to write it on the board so that I can concentrate on the main algorithm".

As far as your actual code you've posted goes, the spec isn't entirely clear (as demonstrated by some of questions above).  You're missing error checking on your rearrange method.  For example you don't check if k<=0, what's going to happen if this is passed?

Try to be more expressive with variable names.  It's tempting when you have to write them down to shortcut names (heap,tempK,k,tempList etc), however if you do this in much the same way it causes problems for future maintainers, it makes it harder for you to remember what the variables represent and for the interviewers to follow your algorithm.

This line also looks a bit odd to me:

Deque<Node> tempList = new LinkedList<Node>();


x86-64 Assembly - Sum of multiples of 3 or 5 I'm trying to learn some basic x86 assembly and so I've begun solving Project Euler problems. I was hoping for some critique of my code that, hopefully, includes either the efficiency of the operations or the readability / style of the code itself. I will provide the Makefile for Linux 64 bit.
The purpose of the code is to sum all numbers from [0, 1000) that are divisible by 3 or 5.
The code can be run using make RUN=euler_1.
NB:
I am aware that most compilers replace modulos of known numbers with some combination of mov and shr to avoid the integer division. For example, see this thread.
Makefile
.PHONY: clean

all:    $(RUN).elf
    ./$^

%.elf:  %.o
    ld $^ -o $@ -lc -e main -dynamic-linker /lib64/ld-linux-x86-64.so.2

%.o:    %.asm
    nasm -f elf64 $^

clean:
    rm -f *.o *.elf

euler_1.asm
extern printf
global main

section .data
fmt: db "%d", 0x0a, 0

section .text

;; main - Calculate the sum of all numbers between [0, 1000) that are divisible
;; by 3 or 5.
;;  sum : R8
main:
    ; sum = 0
    mov r8, 0
    ; for i in [0, 1000) {
    mov rcx, 0
for0:
    ; if i % 3 == 0 or i % 5 == 0 {

    ; i % 3 == 0
    mov rax, rcx
    mov rdx, 0
    mov r9, 3
    div r9
    test rdx, rdx
    jne if01
    ; sum = sum + i
    add r8, rcx
    jmp if0

if01:
    ; i % 5 == 0
    mov rax, rcx
    mov rdx, 0
    mov r9, 5
    div r9
    test rdx, rdx
    jne if0
    ; sum = sum + i
    add r8, rcx
    jmp if0
    ; }
if0:
    inc rcx
    cmp rcx, 1000
    jl  for0
    ; }

    ; printf("%d", sum)
    lea rdi, [rel fmt]
    mov rsi, r8
    mov rax, 0
    call printf

    ; sys_exit(0)
    mov rdi, 0
    mov rax, 60
    syscall

if01 and if0 are not the greatest names.

Instead of reloading r9, use two registers. Let r9 always contain 3, and r10 always contain 5.

Increment r8 in one place.

Running the loop downwards (1000 to 0), rather than upwards, spares an instruction (cmp).

mov rdx, 0 is encoded in 7  bytes. xor rdx, rdx is way shorter.

All that said, consider
main:
    mov r8, 0
    mov r9, 3
    mov r10, 5

    ; for i in (1000, 0]
    mov rcx, 999

for0:
    mov rax, rcx
    xor rdx, rdx
    div r9
    test rdx, rdx
    jeq accumulate

    mov rax, rcx
    xor rdx, rdx
    div r10
    test rdx, rdx
    jne next

accumulate:
    add r8, rcx
next:
    dec rcx
    jne  for0

PS: I hope you know that this problem has a very straightforward arithmetical solution.


bash script for constructing RNA pipeline I have written a bash script that consists of multiple commands and Python scripts. The goal is to make a pipeline for detecting long non coding RNA from a certain input. Ultimately I would like to turn this into a app and host it on some bioinformatics website. One problem I am facing is using getopt tools in bash. I couldn't find a good tutorial that I understand clearly. In addition any other comments related to improving on the code is much appreciated.

#!/bin/bash
if [ "$1" == "-h" ]
then
    echo "Usage: sh $0 cuffcompare_output reference_genome blast_file"
    exit
else
wget https://github.com/TransDecoder/TransDecoder/archive/2.0.1.tar.gz && tar xvf 2.0.1 && rm -r 2.0.1
makeblastdb -in $3 -dbtype nucl -out $3.blast.out
grep '"u"' $1 | \
gffread -w transcripts_u.fa -g $2 - && \
python2.7 get_gene_length_filter.py transcripts_u.fa transcripts_u_filter.fa && \
TransDecoder-2.0.1/TransDecoder.LongOrfs -t transcripts_u_filter.fa
sed 's/ .*//' transcripts_u_filter.fa | grep ">" | sed 's/>//' > transcripts_u_filter.fa.genes
cd transcripts_u_filter.fa.transdecoder_dir
sed 's/|.*//' longest_orfs.cds | grep ">" | sed 's/>//' | uniq > longest_orfs.cds.genes
grep -v -f longest_orfs.cds.genes ../transcripts_u_filter.fa.genes > longest_orfs.cds.genes.not.genes
sed 's/^/>/' longest_orfs.cds.genes.not.genes > temp && mv temp longest_orfs.cds.genes.not.genes
python ../extract_sequences.py longest_orfs.cds.genes.not.genes ../transcripts_u_filter.fa longest_orfs.cds.genes.not.genes.fa
blastn -query longest_orfs.cds.genes.not.genes.fa -db ../$3.blast.out -out longest_orfs.cds.genes.not.genes.fa.blast.out -outfmt 6
python ../filter_sequences.py longest_orfs.cds.genes.not.genes.fa.blast.out longest_orfs.cds.genes.not.genes.fa.blast.out.filtered
grep -v -f longest_orfs.cds.genes.not.genes.fa.blast.out.filtered longest_orfs.cds.genes.not.genes.fa > lincRNA_final.fa
fi

Here is how I run it:

sh test.sh cuffcompare_out_annot_no_annot.combined.gtf /mydata/db/Brapa_sequence_v1.2.fa TE_RNA_transcripts.fa

This script looks like a wall of text and barely readable.
Let's break it down from top to bottom.

Simplify the handling of -h

Since the if at the beginning to check if the first parameter is -h will exit if true,
it would be simpler to drop the else part entirely:

if [ "$1" == -h ]
then
    echo "Usage: sh $0 cuffcompare_output reference_genome blast_file"
    exit
fi

I also dropped the quotes around the -h, they are not necessary.

Validate the command line arguments

The script expects 3 parameters, but doesn't validate if it really received them.
I suggest to rewrite the top part like this:

help() {
    exitcode=$1
    echo "Usage: sh $0 cuffcompare_output reference_genome blast_file"
    exit $exitcode
}

[ "$1" == -h ] && help 0
[ $# -lt 3 ] && help 1

I added a help function to avoid duplication, because I wasn't too reuse the same help message when exiting the program in both cases:

When the first param is -h, print help and exit with 0, meaning success
When there are less than 3 params, print help and exit with 1, meaning failure

Instead of if statements I used && expressions to make it shorter and simpler. The result is equivalent.

Give the parameters meaningful names

The parameters $1, $2 and $3 are scattered here and there in the wall of text,
and they are not exactly easy to remember.
Give them proper names right after validation:

cuffcompare_output=$1
reference_genome=$2
blast_file=$3

And use these names throughout the script.

Taming the wall of text...

Add some line breaks.
Group together lines that are most tightly related.
For example,
for the lines that are the continuation of a pipeline, put
a line break before and after would make this easier to read.
And when a pipeline gets too long,
add some more line breaks (escaped with \) to make the important ending part more visible,
for example when redirecting output to a file.

For example, I think this is more readable:

wget https://github.com/TransDecoder/TransDecoder/archive/2.0.1.tar.gz && tar xvf 2.0.1 && rm -r 2.0.1

makeblastdb -in $3 -dbtype nucl -out $3.blast.out

grep '"u"' $1 | \
gffread -w transcripts_u.fa -g $2 - && \
python2.7 get_gene_length_filter.py transcripts_u.fa transcripts_u_filter.fa && \
TransDecoder-2.0.1/TransDecoder.LongOrfs -t transcripts_u_filter.fa

sed 's/ .*//' transcripts_u_filter.fa | grep ">" | sed 's/>//' \
> transcripts_u_filter.fa.genes

cd transcripts_u_filter.fa.transdecoder_dir

sed 's/|.*//' longest_orfs.cds | grep ">" | sed 's/>//' | uniq \
> longest_orfs.cds.genes

grep -v -f longest_orfs.cds.genes ../transcripts_u_filter.fa.genes \
> longest_orfs.cds.genes.not.genes

sed 's/^/>/' longest_orfs.cds.genes.not.genes \
> temp && mv temp longest_orfs.cds.genes.not.genes

python ../extract_sequences.py longest_orfs.cds.genes.not.genes ../transcripts_u_filter.fa longest_orfs.cds.genes.not.genes.fa

blastn -query longest_orfs.cds.genes.not.genes.fa -db ../$3.blast.out -out longest_orfs.cds.genes.not.genes.fa.blast.out -outfmt 6

python ../filter_sequences.py longest_orfs.cds.genes.not.genes.fa.blast.out longest_orfs.cds.genes.not.genes.fa.blast.out.filtered

grep -v -f longest_orfs.cds.genes.not.genes.fa.blast.out.filtered longest_orfs.cds.genes.not.genes.fa > lincRNA_final.fa

As a small tip, a shorter equivalent of grep ">" | sed 's/>//' is sed -ne 's/>//p'.


Converting fixed-width text file to CSV in C I'm trying to get reacquainted with C after years of higher-level languages.

I wrote a small fixed-width to CSV converter. It works (for ASCII at least) and it's quite fast, but I wonder if I missed anything or if it can be made even faster.

I'm reading the file line by line and process the desired columns (some can be skipped), placing them into an output 8K buffer. I'm using one function to do the substring, appending and trimming trailing space (just the space, in this case I don't want to bother testing for other whitespace characters).

#include <ctype.h>

// number of columns to process
#define COLS 3
#define LINE_SIZE 256
#define BUFFER_SIZE 8192

#define INFILE "in.txt"
#define OUTFILE "out.csv"

size_t RANGES[COLS][2] = {{0, 6}, {6, 20}, {29, 3}};

/*
 * Copy from source to destination, up to len chars, trimming trailing spaces
 * Returns number of chars actually copied
 */
int trimcpy(char *destination, char *source, size_t len) {
    // trim spaces from the end - we only care about the space char
    while (len>0 && source[len-1]==' ')
        len--;

    int i = 0;
    while (i<len && *source != '\0') {
        *destination++ = *source++;
        i++;
    }
    *destination = '\0';

    return i;
}

int main(void) {
    FILE *rfp;
    FILE *wfp;

    char line[LINE_SIZE];
    char out[BUFFER_SIZE];

    rfp = fopen(INFILE, "r");
    if (rfp == NULL)
        exit(EXIT_FAILURE);

    wfp = fopen(OUTFILE, "w");
    if (wfp == NULL)
        exit(EXIT_FAILURE);

    int p = 0;

    // fgets is 4x faster than getline!
    while (fgets(line, LINE_SIZE, rfp) != NULL) {
        // write buffer if almost full (largest column is 20 chars)
        if (p > BUFFER_SIZE - 20) {
            fputs(out, wfp);
            p = 0;
        }

        // go through the columns
        for (int i=0; i<COLS; i++) {
            p += trimcpy(out+p, line+RANGES[i][0], RANGES[i][1]);
            p += trimcpy(out+p, i<COLS-1 ? "," : "\n", 1);
        }
    }

    // write any remaining data in buffer
    fputs(out, wfp);

    fclose(rfp);
    fclose(wfp);

    exit(EXIT_SUCCESS);
}

Consider const char *

const char *source allows the function to be used with const strings, better conveys code's functionality and more readily allows for optimizations.

// int trimcpy(char *destination, char *source, size_t len) {
int trimcpy(char *destination, const char *source, size_t len) {

Undefined behavior

trimcpy() begins by examining later elements of source[] even though they are not known to have been assigned.  The string may not be as long as len.

int trimcpy_alt(char *destination, char *source, size_t len) {
   // Suggest memchr() here rather than strlen() to not look too far.
   char *null_character_pos = memchr(source, '\0', len);
   if (null_character_pos) len = null_character_pos - source;
   ...

Undefined behavior 2

In a selective case of an empty file, the first call to fgets() returns NULL and then the following fputs(out, wfp); is UB as out contents are not initialized.  Add initialization or assignment.

char out[BUFFER_SIZE];
out[0] = '\0'; // add

Not trimming end spaces before \n

trimcpy() does not trim spaces just before '\n'.  I suspect this in not in line with OP's goals.

Avoid redundant information

Redundant information takes more work to maintain.   Consider dropping the 3

// #define COLS 3
size_t RANGES[][2] = {{0, 6}, {6, 20}, {29, 3}};
#define COLS (sizeof RANGES/sizeof RANGES[0])

Minor things:

Mixing types

Little reason to mix size_t and int types here for array indexing and sizing.  Recommend to  use just one: size_t (my preference) or 2) int, but not  both.

// From
int trimcpy(char *destination, char *source, size_t len) {
  ...
  int i = 0;
  while (i<len && *source != '\0') {

// To
size_t trimcpy(char *destination, char *source, size_t len) {
  ...
  size_t i = 0;
  while (i<len && *source != '\0') {

main()

  // int p = 0;
  size_t p = 0;

Subtraction with unsigned types

If above size_t employed, consider the 2 below:  Which works well should a later version of code surprisingly  define  #define BUFFER_SIZE 19?

if (p > BUFFER_SIZE - 20) {
if (p + 20 > BUFFER_SIZE) {

   The first is the same as if (p > SIZE_MAX) { or if (0) {


VB.Net interfacing with F# Euclidean Distance Algorithm I testing F# code which calculates "nearness" of two N-dimensional points using a least square euclidean distance algorithm.  The class library is written in F# and the calling will be from  VB.NET.  The code will eventually be used for scientific binary data, however I wanted a simple real world test environment.

I choose to create an rgb color table class based upon a list of color names along with their 3 digit rgb value.  The rgb values will be used for the distance algorithm.  The code works, however I'd like to make sure that the distance algorithm in written in an efficient manner.  The code will be called heavily in the final package.

My VB code will create the class instance, adds colors to the color table and finally searches for the nearest match of an unnamed color.  The results is DarkGoldenRod.

Dim GreenYellow() As Double = {173, 255, 47}
Dim Orange() As Double = {255, 127, 0}
Dim Lavender() As Double = {230, 230, 250}
Dim Magenta() As Double = {255, 0, 255}
Dim DarkGoldenRod() As Double = {184, 134, 11}

Dim test() As Double = {122, 122, 122}

Dim objFSColorTable As New MyFSColorTable("GreenYellow", GreenYellow)
objFSColorTable.AddEntry("Lavender", Lavender)
objFSColorTable.AddEntry("Orange", Orange)
objFSColorTable.AddEntry("Magenta", {255, 0, 255})
objFSColorTable.AddEntry("DarkGoldenRod", {184, 134, 11})

Dim name = objFSColorTable.ColorSearch(test)

The F# code contains the color table library which should be self explanatory

type MyColor =
  { Name: string;
    Values: float[];
    }

type MyFSColorTable(name: string, rgb:float[]) = class

  let mutable myHash = Hashtable()
  let mutable CurName = name
  let mutable CurDist = 0.0

  let mutable CurColor =
    { Name = name;
      Values = rgb;
      }

  let mutable CurList = [CurColor]

  member x.AddColor name rgb = CurList <- List.append CurList [{Name = name; Values = rgb}]
  member x.Name   = name

  // Euclidean distance between 2 vectors
  member x.Dist (V1: float[]) V2 =
    Array.zip V1 V2
      |> Array.map(fun (v1, v2) -> (v1 - v2) * (v1 - v2))
      |> Array.sum

  // Loop thru color hash and find the nearest match
  member x.ColorSearch (R1: float[]) =
    let mutable mindist = x.Dist R1 CurList.Head.Values
    let mutable name =  CurList.Head.Name
    for i in (CurList)  do
      let mutable s = 0.0
      s <- x.Dist R1 i.Values
      if s < mindist then
        mindist <- s
        name <- i.Name
    name

  // RGB Hash Table
  member x.RGBHash = myHash
  member x.AddEntry (name:string) (rgb: float[]) = x.AddColor name rgb

end

type MyColor =
  { Name: string;
    Values: float[];
    }

The usual syntax is to put the opening { right after =.
Also, why are you using floats (or Doubles in VB) for the values? Integers should be enough. And it might be easier to use existing Color type, either from System.Windows.Media or from System.Drawing. As an added advantage, you could reuse the existing definitions for various colors.
type MyFSColorTable(name: string, rgb:float[]) = class

Collection really shouldn't take its first member in constructor. Either take all colors in the constructor (which means you wouldn't need mutable collection inside), or have a method for adding new colors.
Also, using class here is not necessary. If you remove that, don't forget to also remove the end.
let mutable myHash = Hashtable()
let mutable CurName = name
let mutable CurDist = 0.0

These members don't seem to be used in the program, so you should remove them. Also, try to avoid using mutable.
let mutable CurColor =
  { Name = name;
    Values = rgb;
    }

This is only used to support the first item from constructor, remove it.
let mutable CurList = [CurColor]

CurList is not a very good name. Don't use abbreviations like Cur, they are unclear. And in this case, it doesn't add anything. A better name would be Colors.
member x.AddColor name rgb = CurList <- List.append CurList [{Name = name; Values = rgb}]

Adding to the end of list is inefficient. You can add a value to the front of the list by using ::: {Name = name; Values = rgb}::Colors.
member x.Name   = name

This member is not used. And it's very confusing, because it returns the name of the first color that was added to the collection. Remove it.
member x.Dist (V1: float[]) V2 =

This function doesn't use any instance fields, so it should be static. And it should be probably also private (i.e. static let dist V1 V2 =)
Array.map(fun (v1, v2) -> (v1 - v2) * (v1 - v2))

You could avoid the repetition by using pown: Array.map (fun (v1, v2) -> pown (v1 - v2) 2).
// Loop thru color hash and find the nearest match
member x.ColorSearch (R1: float[]) =

The comment is inaccurate, the function doesn't use any hash.

R1 is a really bad name. And ColorSearch is not a great name either, something like FindNearestColor would be much better.

The whole method is written in a purely imperative style. If you're going to write this way, there pretty much isn't a reason why you should use F#, F# is a poor imperative language.
A better way to write this method would be to use List.minBy:

member x.FindNearestColor (rgb : float[]) =
  let nearestColor =
    Colors |> List.minBy (fun color -> dist rgb color.Values)
  nearestColor.Name

Also, it seems you think searching through the list could be inefficient (because it seems you attempted to use a hash table). But you can't use hash table for something like this. You could use something like an R-tree, but implementing that would be much more complicated. So I would do that only if performance of this method was really bad.
Dim objFSColorTable As New MyFSColorTable("GreenYellow", GreenYellow)
objFSColorTable.AddEntry("Lavender", Lavender)
objFSColorTable.AddEntry("Orange", Orange)
objFSColorTable.AddEntry("Magenta", {255, 0, 255})
objFSColorTable.AddEntry("DarkGoldenRod", {184, 134, 11})

This is quite verbose. A better way would be to use collection initializer. How exactly would that look depends on how do you decide to fix initialization of MyFSColorTable.


Find attribute on class property Since reading Robert Martin's book "Clean Code" I have been inspired to revisit and refactor some of my code to break it down into succinct methods and small specialised classes.

Given the need to retrieve the NameAttribute off the Name2 property of the TestObject class below...

    public class TestObject
    {
        public string Name1 { get; set; }
        [Name("Address")]
        public string Name2 { get; set; }
    }

I have built a class called PropertyNameAttributeFinder to acheive this.

public class PropertyNameAttributeFinder
{
    #region Fields

    private readonly PropertyInfo _property;
    private NameAttribute _attributeFound;

    #endregion

    #region Construtors

    /// <summary>
    /// Initializes a new instance of the <see cref="PropertyNameAttributeFinder"/> class.
    /// </summary>
    /// <param name="property">The property.</param>
    /// <exception cref="System.ArgumentNullException">property</exception>
    public PropertyNameAttributeFinder(PropertyInfo property)
    {
        if (property == null) throw new ArgumentNullException("property");

        _property = property;
    }

    #endregion

    #region Public Members

    /// <summary>
    /// Checks for attribute.
    /// </summary>
    /// <returns>The current instance for fluid API</returns>
    public PropertyNameAttributeFinder CheckForAttribute()
    {
        SetAttributeIfExists();
        return this;
    }

    /// <summary>
    /// Gets a value indicating whether this instance has found an attribute.
    /// </summary>
    /// <value>
    /// <c>true</c> if this instance has found an attribute; otherwise, <c>false</c>.
    /// </value>
    public bool HasFoundAttribute
    {
        get { return _attributeFound != null; }
    }

    /// <summary>
    /// Gets the AttributeFound that was found.
    /// </summary>
    /// <value>
    /// The AttributeFound.
    /// </value>
    public NameAttribute AttributeFound
    {
        get
        {
            if (!HasFoundAttribute)
            {
                throw new InvalidOperationException("No attribute type was found so cannot be returned. Hint: Use HasFoundAttribute first.");
            }
            return _attributeFound;
        }
    }

    #endregion

    #region Private Members

    private void SetAttributeIfExists()
    {
        _attributeFound = _property.GetAttribute<NameAttribute>();
    }
    #endregion
}

The class will be used as follows...

    [TestMethod]
    public void AttributeFound_WhenCalledAfterCheckAttributeAndPropertyDoesHaveAtrribute_ReturnsInstanceOfAttribute()
    {
        // ARRANGE
        Type testType = typeof(TestObject);
        PropertyInfo property = testType.GetProperty("Name2");

        // ACT
        NameAttribute actual = new PropertyNameAttributeFinder(property)
            .CheckForAttribute()
            .AttributeFound;

        // ASSERT
        Assert.IsNotNull(actual);
    }

Can this class be written better to express it's intent in a clearer manner?

EDIT:
Following request from @Heslacher, here is the code from the extension method called by _property.GetAttribute<NameAttribute>(); in SetAttributeIfExists.

    /// <summary>
    /// Get an attribute for a property
    /// </summary>
    /// <typeparam name="T"></typeparam>
    /// <param name="propertyInfo"></param>
    /// <returns></returns>
    public static T GetAttribute<T>(this PropertyInfo propertyInfo)
        where T : Attribute
    {
        var attribute = propertyInfo.GetCustomAttributes(typeof(T), false).FirstOrDefault();
        return (T)attribute;
    }

Please note: It is not the code in this extension which I want reviewed, but rather the PropertyNameAttributeFinder class and how calling it reads. I.E. doe the public members clearly reveal the intent of the class.

I think that Heslacher is right and you don't strictly need this class, but as you've written it I'd like to point out a couple of things:

Many people, myself included, don't like to use regions. They just add noise - especially on such a small class
SetAttributeIfExists is one step too far IMO. The method name isn't accurate either, you're setting the value if it exists or not.
The exception message is really odd if you specify a property without the attribute:

[TestMethod]
public void AttributeFound_WhenCalledAfterCheckAttributeAndPropertyDoesntHaveAtrribute_ReturnsNull()
{
    // ARRANGE
    Type testType = typeof(TestObject);
    PropertyInfo property = testType.GetProperty("Name1");

    // ACT
    NameAttribute actual = new PropertyNameAttributeFinder(property)
        .CheckForAttribute()
        .AttributeFound;

    // ASSERT
    Assert.IsNull(actual);
}

I would get an exception saying this:

  No attribute type was found so cannot be returned. Hint: Use HasFoundAttribute first.

That's not a good hint - I've already called HasFoundAttribute. The message could also be more specific as you didn't find a NameAttribute not any old attribute type. The point is you've extended 1 line of code to an entire class for no benefit.


Executing multiple steps to handle a purchase using currying Is doing something like this utilizing currying in a practical sense?

function processOneClickPurchase(item) {
    let context = { step: 0 }

    let addItemToCart = item => {
        let cart = getSession().cart
        cart.items.push(item)
        log('Cart items', cart.items)
        return cart
    }

    ...

    let purchase = generate([
        addItemToCart,
        makePurchase,
        processBilling,
        setupShipping,
        sendPurchaseAndShippingNotification,
    ], null, cleanup)

    return purchase(item)

    function generate(steps, before, after) {
        const takeStep = (incoming, outgoing) => {
            let message = {
                step: ++context.step,
                incoming: incoming,
                outgoing: outgoing
            };

            log('step #'+(context.step), message)

            return outgoing
        }
        const walk = value => {
            return steps.reduce((from, to) => {
                return takeStep(from, to(from))
            }, value)
        }

        (before && steps.push(before)) + (after && steps.push(after))

        return function invoke(state) {
            const hasItems = Array.isArray(state) || !(state = [state])
            const processed = state.map(walk)
            return hasItems ? processed : processed[0]
        }
    }
}

https://jsfiddle.net/f7mdak6z/25/

I realize there's algebra going on with currying and maybe this is nonsensical, so we're not talking rigid math concepts. However, it seems to me this pattern is rudimentary and it feels like an application of currying over an input to achieve an output. takeStep() almost seems like a monad, etc.

I think it would definitely start to sound like a fit if the methods passed to generate() simply modified the stream, and perhaps the takeStep() function handled side-effects.

I take it here that you mean “algebra” to mean lambda calculus and by extension, combinatory logic.

The takeStep function is essentially (disregarding side-effects): $$
\lambda ab.b
$$ in lambda calculus, or in JS in terms of identity (using Haskell Curry’s combinator naming):

I = a => a
K = a => b => a
KI = K(I)

So as the takeStep function is simply disregarding the first argument, and the walk function calling the second argument with the previous (/initial) value there and then, I’m wondering if you would really be after the B-combinator $$
   \lambda fga.f(ga)
$$ (or, as JS: B = f => g => a => f(g(a)), often named compose) with possibly the C-combinator $$
   \lambda fab.fba
$$ (C = f => a => b => f(b)(a)) for reversing the arguments (often named pipe or sequence for f => g => a => g(f(a))).

(I’m not sure if the commutativity of the functions or JS evaluation strategy plays a role here.)

Regarding currying, I wonder if you were thinking of partial application instead.