Removing a link from a linked list I have written a method for removing a link from a linked list for a phonebook project I'm currently working on and it works perfectly. However, I want to know whether or not my code is acceptable from a programmer's perspective. I tried to systematically structure it in a way such that it's understandable, but by doing so, this results in multiple if statements being nested inside one another.

Is this considered bad coding? Also, should I have used a recursive implementation instead? What do you think? Are there any ways to improve this code? Any constructive criticism is certainly welcome.

public Link removeLink(String surname, String firstName)
{
    Link currentLink = firstLink;
    Link previousLink = firstLink;
    if(isEmpty())
    {
        return null;    // Name was not found
    }
    // customer to delete is first element in the list
    else if((currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName))
    {
        firstLink = firstLink.next;
    }
    // customer is not the first element in the list
    else
    {
        // search until either the end of list is reached or a match is found
        while(currentLink.next!=null && !((currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName)))
        {
            previousLink = currentLink;
            currentLink = currentLink.next;
        }
        // if end of list is reached
        if(currentLink.next == null)
        {
            // check if there is a match with last element in list
            if((currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName))
            {
                previousLink.next = currentLink.next;
            }
            else
            {
                return null;
            }
        }
        // match is found somewhere in the middle of list
        else
        {
            previousLink.next = currentLink.next;
        }
    }
    numEntries--; // number of entries decrements each time a customer is deleted
    return currentLink; // return the entry that was deleted
}

Code is fine I think, although according to Robert C. Martin, generally, if you need to add a comment then you have failed to write readable code.

For example, this could be made a little easier to read and understand by extracting the block

else //customer is not the first element in the list

into a separate method.  Also the long tests such as

if ((currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName))
while (currentLink.next!=null && !((currentLink.surname).equals(surname) && ((currentLink.firstName).equals(firstName)))
if (currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName))

...contain duplicated code, namely

(currentLink.surname).equals(surname) && (currentLink.firstName).equals(firstName)

which would be better extracted into, say testForMatch()

In Summary

Long methods are frowned upon
Repeated code is hard to understand and harder to maintain
Extracting into well named methods beats good comments any day (comments can be left behind during updates and cause more confusion than is necessary with just code)


Classifying files in a directory by file extension I have the following code:

resolved = []
rejected = []
for file in files_within("./_input/"):
    file = File(file)
    if file.extension.lower() == "gdb":
        resolved.append(file)
    else:
        rejected.append(file)
MAP["resolved"] = resolved
MAP["rejected"] = rejected

And i would like to improve it to avoid this part:

MAP["resolved"] = resolved
MAP["rejected"] = rejected

But without affecting the performance.

I was thinking about that:

MAP["resolved"] = []
MAP["rejected"] = []
for file in files_within("./_input/"):
    file = File(file)
    if file.extension.lower() == "gdb":
        MAP["resolved"].append(file)
    else:
        MAP["rejected"].append(file)

But i don't know if is a good practice.

Python version 2.6.

Each MAP["resolved"] has to perform a hash on "resolved", and then check up the value in the dictionary's internal structure.
This is more costly then looking up a single variable.

Just like in the first, you can assign a local variable, resolved, but you can assign to the dictionary at the same time:

resolved = MAP["resolved"] = []

If you only mutate resolved, then you mutate MAP["resolved"] too. So you can't overwrite resolved with resolved = ['new list'], for example.
Which can get you:

MAP["resolved"] = resolved = []
MAP["rejected"] = rejected = []
for file in files_within("./_input/"):
    file = File(file)
    if file.extension.lower() == "gdb":
        resolved.append(file)
    else:
        rejected.append(file)

I am also unsure on what your functions are, I don't remember them being in the standard library, and so I won't suggest any changes to them.


Project Euler #2 Efficiency Each new term in the Fibonacci sequence is generated by adding the previous two terms.

  By starting with 1 and 2, the first 10 terms will be:

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

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

I'd like to reduce the complexity of my code.

import java.math.BigInteger;
import java.util.Scanner;

public class Solution {

    public static void main(String[] args) {

        Scanner sc = new Scanner(System.in);
        int testCases = sc.nextInt();
        for (int i = 0; i < testCases; i++) {
            BigInteger input = sc.nextBigInteger();
            System.out.println(calculate(input));
        }
    }

    public static BigInteger calculate(BigInteger input) {
        BigInteger fib1 = new BigInteger("0");
        BigInteger fib2 = new BigInteger("1");
         BigInteger sum = new BigInteger("0");

        BigInteger fib = new BigInteger("0");
        while (input.compareTo(fib)>0) {

            if (fib.doubleValue() % 2 == 0)
                sum = sum.add(fib);
            fib = fib1.add(fib2);
            fib1 = fib2;
            fib2 = fib;

        }

        return sum;
    }

}

The test

if (fib.doubleValue() % 2 == 0)

does not produce the correct result for numbers with more than 17 digits, because
that exceeds the precision of a double. Actually it returns true for 8944394323791464 and all subsequent Fibonacci numbers.
That is not relevant for the concrete problem here (see below), but if you want to work with BigInteger then you should replace this with

if (fib.remainder(bigTwo).equals(BigInteger.ZERO))

where bigTwo is defined as

BigInteger bigTwo = new BigInteger("2");

Project Euler Problem #2
asks for the sum of all even-valued Fibonacci values not exceeding 4 million,
so using BigInteger is not really necessary. All numbers fit into the range
of int, and the above test simplifies to

if (fib % 2 == 0)

(I also planned to tell that using int instead of BigInteger makes the
program more efficient, but it turned out that the difference in running
time is not significant.)


Implementation of String object as an exercise I am reading "Object-Oriented JavaScript" by Stoyan Stefanov to learn JavaScript and in the end of the chapter named "Objects", there's an exercise that asks you to implement your own String object. I have implemented some methods of String (not all), I'd be happy if you could check and let me know how it could be done better:

function MyString(primitive) {
    this.length = 0;
    var i = 0;
    // Here length is calculated and the indexer is set.
    for (var c in primitive) {
        this[i++] = primitive[c];
        this.length++;
    }

    this.toString = function() { return primitive; };
    this.valueOf = this.toString;
    this.substring = function(start, end) {
        var result = '';

        // Edge cases for the parameters.
        if ((typeof end !== 'number' && typeof parseInt(end) !== 'number') || end > this.length)
            end = this.length;
        if (end <= 0 || isNaN(end)) {
            end = start;
            start = 0;
        }

        for (var i = start; i < end; i++) {
            result += this[i];
        }

        return result;
    }
    this.charAt = function(i) {
        var x = parseInt(i);
        x = isNaN(x) ? 0 : x;
        return this.substring(x, x+1);
    };

    return this;
}

var s = new MyString('hello');
console.log(s.length);
console.log(s[0]);
console.log(s.toString());
console.log(s.valueOf());
console.log(s.charAt(1));
console.log(s.charAt('2'));
console.log(s.charAt('e'));
console.log(s.substring(1, 3));

Any and all suggestions are welcome. I'm completely new to JavaScript and would love to hear my mistakes.

Some information up front:

Object.defineProperty is an Ecmascript 5 feature, and is not available in older browsers (IE < 9).
There is likely a cleaner way of doing all of this.

This is a basic skeleton:

function MyString(primitive) {
    var length = 0, a;   // private internal variable to track length, and our iterator

    // the loop below basically ensures that this string object is immutable.
    for(a in primitive){
        (function(char){
            Object.defineProperty(this, length++, {
                get:function(){return char;}
            });
        }).call(this,primitive[a]);
    }

    // we can "get" the length, but we can't "set" it.
    Object.defineProperty(this, "length", {
        get:function(){return length;}
    });

    return this;
}

// defining methods on the prototype of an object allows us to share the same instance
// of the method without recreating it for each instance of the object.
// (reduced memory footprint)
MyString.prototype.toString = function(){
    return [].join.call(this, '');
    // since our object is "array-like", we can use array methods on it.
}

MyString.prototype.charAt = function(index){
    return this[index];
}


perform fuzzy matching on a large set of dictionary keys Any suggestions on improving the speed of below code. My dictionary is rather large in fold of * 100000 and expected to grow larger every day.
I am open to completely new ideas or approaches to do the below operation.
from pyxdameraulevenshtein import damerau_levenshtein_distance, normalized_damerau_levenshtein_distance
name = 'john doe'

word_dict = {
              'john' : ['ID1','ID2', 'AB2' ,'AS1']
             ,'doe' : ['ID1','ID4', 'AB2' ,'AS6']
             ,'jahn' : ['ID3','ID2', 'AB2' ,'AS5']
             }
# perform iteration on dictionary keys . compute and filter if damerau edit distance is less than 2
all_matches = []
for nWord in set(name.split()):
    match = []
    match += [(word,list(word_dict[word]), int((1-normalized_damerau_levenshtein_distance(nWord,word))* 100))\
    for word in word_dict if damerau_levenshtein_distance(nWord,word) in [0,1,2]]
    all_matches.append(match)

Take a look at BKTrees. Here is a less technical description. There is a Python library on PyPI: pybktree.
You create the BKTree once (once a day?) from the keys in the dictionary. Then run all the queries against the BKTree.
It would look something like this untested code:
import pybktree
from pyxdameraulevenshtein import damerau_levenshtein_distance

tree = pybktree.BKTree(damerau_levenshtein_distance, word_dict.keys())

all_matches = []

for target in set(name.split()):
    all_matches.extend(tree.find(target, MAX_DISTANCE))]

all_matches would be a list of (distance, word) tuples.


Codility's Permutation Check in C# I've made a few things in Java, and now I'm learning C#.
The code passes the tests with 100% final score. I want to know what things can be improved in my code.

  Task description

  A non-empty array A consisting of N integers is
  given.

  A permutation is a sequence containing each element from 1 to N once,
  and only once.

  For example, array A such that:

A[0] = 4
A[1] = 1
A[2] = 3
A[3] = 2 is a permutation, but array A such that:

A[0] = 4
A[1] = 1
A[2] = 3 is not a permutation, because value 2 is missing.

  The goal is to check whether array A is a permutation.

  Write a function:

  class Solution { public int solution(int[] A); }

  that, given an array A, returns 1 if array A is a permutation and 0 if
  it is not.

  For example, given array A such that:

A[0] = 4
A[1] = 1
A[2] = 3
A[3] = 2 the function should return 1.

  Given array A such that:

A[0] = 4
A[1] = 1
A[2] = 3 the function should return 0.

  Write an efficient algorithm for the following assumptions:

  N is an integer within the range [1..100,000]; each element of array A
  is an integer within the range [1..1,000,000,000].

public static int solution(int[] A)
{
    int[] orderedPermut = new int[A.Length];
    int[] unrepeated = new int[A.Length];
    int orderedPermutSum = 0, unrepeatedSum = 0;
    for ( int i = 0; i < A.Length; i++ )
    {
        orderedPermutSum += i + 1;
        orderedPermut[i] = i + 1;
        if ( A[i] >= A.Length + 1 || unrepeated[ A[i] - 1 ] != 0 )
            /*number is greater than A's length, or it's repeated,
            /*therefore,A's not a permutation.*/
            return 0;
        else
        {
            unrepeated[A[i] - 1] = A[i];
            unrepeatedSum += A[i];
        }
    }
    if ( orderedPermutSum == unrepeatedSum )
    {
        return 1;
    }
    return 0;
}

One of the unfortunate things you'll notice about coding challenges is that the requirements often force you to write code that isn't ideal. I'm going to call these out, even though in this case they're outside of your control.

Silly requirement: The method is named solution. This name tells us nothing about the behavior of the function. Also (although less importantly), it goes against the C# method naming convention of PascalCase. A much better name would be something like IsPermutation.

Silly requirement: The parameter is named A. Single-letter variable names are almost never a good idea. Also (although less importantly), it goes against the C# parameter naming convention of camelCase. It's hard to be descriptive about the context here (since it's just a coding challenge and not a "real life" business problem), but even something like values would be better.

Silly requirement: The return type of the function is int. The given array is either a permutation of 1..N or it isn't. We don't have a range of possible return values; we have a true/false condition. A better return type would be bool.

Algorithm inefficiency: There's no need to initialize two new int[]. As you discovered, one new array is sufficient to track unique values.

Algorithm inefficiency: Again, you've already discovered this. If an array has the correct elements in some order, its sum will necessarily match that of the unordered array. Meanwhile the converse is not true; there are many arrays with the same sum which are not permutations. So you needn't bother tracking the sum at all.

Syntax: If you take only one piece of my advice, let it be this one: for loops are not the answer. If you are iterating over the indexes of an array, and the only thing you use the index for is to grab the ith element, use a foreach. The situation where you actually need to know the index, and therefore need a for loop, is very rare.

Semantics: Does an array of int take less space in memory than an array of bool? I don't know, and I don't care. If all I'm tracking is true/false values, I'm going to choose a bool[]. Because that choice tells the person reading the code why I created the variable.

Formatting: Another major difference between formatting conventions in C# and Java (besides the PascalCase/camelCase conventions I already mentioned) is C#'s convention of putting opening curly braces on the next line (known as K&R style).

With all of that advice applied, and some XML documentation comments (which enable Intellisense information when hovering or typing in Visual Studio), we'll end up with something like this:

/// <summary>
/// Check whether the given array contains each integer 1..N exactly once.
/// </summary>
/// <returns>
/// True if <paramref name="values" /> is a permutation of 1..N,
/// False otherwise.
/// </returns>
public static bool IsPermutation(int[] values)
{
    var seen = new bool[values.length];

    foreach (var value in values)
    {
        if (value < 1 || value > values.length)
        {
            // Out of range: not a permutation
            return false;
        }
        else if (seen[value - 1])
        {
            // Duplicated value: not a permutation
            return false;
        }
        else
        {
            // Value is OK. Mark as seen.
            seen[value - 1] = true;
        }
    }

    // All values in range, no duplicates: a valid permutation
    return true;
}


Equality comparison for a surname value object class I asked this question yesterday: Entity and Value Object are not part of the Ubiqtious language. Should this stop me from using them?.

I originally decided to introduce a ValueObject class as described here: Generic Value Object Equality, which contained equality methods for all value objects i.e. all Value Objects inherited from ValueObject the value object class. However, this approach was criticised.

Please see the code below:

public sealed class Surname : IEquatable<Surname>
{
    private readonly string _value;
    public Surname(string surname)
    {
        if (surname.Length==0)
            throw new ArgumentException("Invalid value.", "Surname");
        this._value = surname;
    }

    public string Value
    {
        get { return _value; }
    }

    public static bool operator ==(Surname surname1, Surname surname2)
    {
        if (!ReferenceEquals(surname1, null) &&
            ReferenceEquals(surname2, null))
        {
            return false;
        }
        if (ReferenceEquals(surname1, null) &&
             !ReferenceEquals(surname2, null))
        {
            return false;
        }
        return surname1.Equals(surname2);
    }

    public static bool operator !=(Surname surname1, Surname surname2)
    {
        return !(surname1== surname2);
    }

    public bool Equals(Surname other)
    {
        if (other != null)
        {
            return _value == other._value;
        }
        return base.Equals(other);
    }

    public override bool Equals(object obj)
    {
        return Equals(obj as Surname);
    }

    public override int GetHashCode()
    {
        unchecked
        {
            int hash = 17;
            hash = hash * 23 + _value.GetHashCode();
            return hash;
        }
    }
}

It is a DDD Value Object for a Surname.

I have added some methods for equality and:

I would like some feedback on the quality of them.
This question (point one) links to a presentation by Eric Evans where he appears to advise against introducing equality for Entities.  Is it advisable to always compare entities by reference these days?

I don't see any advantage that this class provides over a simple string. I expect a Surname to be at least case-insensitive but this class' equality is exactly the same as if it was a string.

I also don't understand why you change the hashcode of the string? You could just return the one that _value returns... but agian, this would be the same as an the one of an ordinary string.

Take a look a the StringComparer.OrdinalIgnoreCase because this is what you actually need... I guess. It would make sense.

As far as a Surname is concerned I also expect it to yield true when comparing "John" with " John    ". So you might want to Trim the value.

This kind of case-insensitive trimmed string is such a common use case that I have special class for that. It saves a lots of headaches when working this kind of data.


Tree Utility in Python I wrote this piece of code to mimic the *nix tree utility, however, I am not happy with the pad_info=[] part. It's used to make padding according to whether the parents were last child of their parents or not. Is there any alternate (and a more elegant) way to do this?

def tree(path, depth=1, max_depth=100, print_hidden=False, pad_info=[]):
    '''Print contents of directories in a tree-like format
    By default, it prints upto a depth of 100 and doesn't print hidden files,
    ie, files whose name begin with a '.'

    returns number of files, number of directories it encountered
    '''

    fileCount, dirCount = 0, 0
    files = sorted(os.listdir(path), key=lambda s: s.lower())
    if not print_hidden:
        files = [os.path.join(path, x) for x in files if not x.startswith('.')]

    for i, file in enumerate(files):
        padding = ['|   ' if x == 'nl' else '    ' for x in pad_info]
        padding = ''.join(padding)
        filename = os.path.basename(file)
        is_last = i == len(files) - 1
        prefix = '`-- ' if is_last else '|-- '

        print '%s%s%s' % (padding, prefix, filename)

        if os.path.isdir(file):
            dirCount += 1
            new_pad_info = pad_info + (['l'] if is_last else ['nl'])
            fc, dc = tree(os.path.join(path, file), depth=depth+1,
                          max_depth=max_depth, print_hidden=print_hidden,
                          pad_info=new_pad_info)
            fileCount += fc
            dirCount += dc
        else:
            fileCount += 1

    return fileCount, dirCount

Some minor style and code comments:

Be consistent in using snake_case for variables names – Most names are good, but then a fileCount and dirCount sneaks in. Also try avoid using abbreviations like fc and dc
Avoid building lists twice – You first build the list of files from  os.listdir(), and then rebuild it using files in combination with print_hidden. This can be avoided using a list comprehension with an if statement like [a for a in a_list if a == something]
Avoid one-time intermediate variables – If you use a variable just once, it can be argued that it is better to just use the expression directly. In your code this applies to padding, filename and prefix
Avoid hiding predefined variables and functions – Don't use reservered words like file in your code, which hides the original file.
When supplying all variables, you don't need to prefix them by name – In your recursive call to tree you prefix them, which is not needed as the order and presences is already stated.
Instead of storing text, store a boolean in your pad_info – To avoid string comparisons and storage, it makes more sense to store a boolean value whether the director is the last in the current directory or not. This can further simplify your code by using a preset list to choose which text to be displayed directly as True = 1 and False = 0 when used in a list context.
Maybe a personal preferences, but use """ for docstrings – If you are consistently using """ for docstrings, this allows for intermediate ' in the text, as well as enabling you to comment out larger portion of code securely using the other option of '''.
Add option to remove output – As your code returns the file and directory counts, it could be viable to foresee a situation where you are not interested in actually viewing the output. This could be added as another option to your function.
Use internal function to hide inner implementation details - It could be argued that this is a good candidate for using a function within the function. This internal function could be the one actually handling the recursion, and only having the parameters path and depth.

In other words, you could hide some of the inner workings of your function using a inner function, and let the outer function be a simple call to the inner function doing the work for you. Notice also that the inner function has full access to the outer functions variables, so need to pass them around (or treat as globals).
BUG: Your code doesnt' respect the max_depth variable – You don't have anything bailing out if the depth is too high...
Feature: No marking of empty directories – If reducing max_depth or having empty directories there is no marking to indicate that the current entry is actually a directory.

My refactored code

Here is the code when applying most of this comment to it:

import os

FOLDER_PATTERN = ['|   ', '    ']
FILE_PATTERN = ['|-- ', '`-- ']

def tree(path, do_output=True, print_hidden=False, max_depth=100):
    """Print file and directory tree starting at path.

    By default, it prints upto a depth of 100 and doesn't print hidden files,
    ie. files whose name begin with a '.'. It can be modified to only return
    the count of files and directories, and not print anything.

    Returns the tuple of number of files and number of directories
    """

    def _tree(path, depth):
        file_count, directory_count = 0, 0
        files = sorted((os.path.join(path, filename)
                           for filename in os.listdir(path)
                           if print_hidden or not filename.startswith('.')),
                       key=lambda s: s.lower())

        files_count = len(files)

        for i, filepath in enumerate(files, start = 1):

            # Print current file, based on previously gathered info
            if do_output:
                print('{}{}{}'.format(
                    ''.join(FOLDER_PATTERN[folder] for folder in parent_folders),
                    FILE_PATTERN[i == files_count],
                    os.path.basename(filepath)))

            # Recurse if we find a new subdirectory
            if os.path.isdir(filepath) and depth < max_depth:

                # Append whether current directory is last in current list or not
                parent_folders.append(i == files_count)

                # Print subdirectory and get numbers
                subdir_file_count, subdir_directory_count = \
                    _tree(os.path.join(filepath), depth+1)

                # Back in current directory, remove the newly added directory
                parent_folders.pop()

                # Update counters
                file_count += subdir_file_count
                directory_count += subdir_directory_count + 1

            elif os.path.isdir(filepath):
                directory_count += 1

            else:
                file_count += 1

        return file_count, directory_count

    parent_folders = []
    return _tree(path, 1)


Concurrent byte array access in Java with different locking I have one writer thread and 10 reader threads of a byte array. It is important to synchronize writes and reads over a "row" (16 bytes).

There are a lot less locks than rows, e.g. in the current configuration 1024 (chunkRows) rows have one lock which makes the overall operation faster, but also consumes less memory.

Here is the read method for ReadWrite locking:

public void read(int pointer, byte[] tmpBytes, int length) {
    Lock lock = locks[pointer / rowSize / chunkRows].readLock();
    lock.lock();
    try {
        System.arraycopy(bytes, pointer, tmpBytes, 0, length);
    } finally {
        lock.unlock();
    }
}

and here for simple "Object"-synchronized:

public void read(int pointer, byte[] tmpBytes, int length) {
    Object lock = locks[pointer / rowSize / chunkRows];
    synchronized (lock) {
        System.arraycopy(bytes, pointer, tmpBytes, 0, length);
    }
}

Here is the full repository which you can easily run via maven. It uses JMH to avoid nasty performance pitfalls. Of course, also write is synchronized similarly.

Now why is my ReadWrite implementation a lot slower than synchronized? (RW is roughly 40% slower)

I like Object-locking more because it uses less RAM but did I made a mistake with ReadWrite-locking or does this make sense?

I tried it on my dual core laptop as well as on more beefy quad core commodity server with simmilar results (Object-locking faster than RW). I get something like 13.0 ops/s for ReadWrite locking and 21.6 ops/s for Object locking.

Update

Several things were wrong in the benchmark which I fixed (maybe there are still glitches)

Very important bug fix was to include service.awaitTermination
make sure write and read happen at the same time and both are long enough (previously write was too short and nearly no r+w concurrency happened)
I've learned about StampedLock, could be slightly faster and also optimistic locking could be even the winner here.
minor thing: catch errors in runnable otherwise they won't appear

See all in this commit

The documentation of the ReadWriteLock says:

  Whether or not a read-write lock will improve performance over the use of a mutual exclusion lock depends on the frequency that the data is read compared to being modified, the duration of the read and write operations, and the contention for the data - that is, the number of threads that will try to read or write the data at the same time. For example, a collection that is initially populated with data and thereafter infrequently modified, while being frequently searched (such as a directory of some kind) is an ideal candidate for the use of a read-write lock. However, if updates become frequent then the data spends most of its time being exclusively locked and there is little, if any increase in concurrency. Further, if the read operations are too short the overhead of the read-write lock implementation (which is inherently more complex than a mutual exclusion lock) can dominate the execution cost, particularly as many read-write lock implementations still serialize all threads through a small section of code. Ultimately, only profiling and measurement will establish whether the use of a read-write lock is suitable for your application.

In your code, the read operations are very short, they are only copying 16 bytes. Probably, the overhead of using a more complex lock is dominating the cost of execution as stated in the documentation.


Is Event.wait necessary here? When I read some code,I think the usage of Event is not necessary,is it right?

# start server
while True:
    # accept a request here
    queue.put(info)
    event.set() # notify all the threads?
    # pass queue and event here to a Thread constructor

other place there are more than 10 threads running,

# inside a Thread class
def __init__(self, queue, event):
    threading.Thread.__init__(self)
    self.queue, self.event = queue, event

def run(self):
    while True:
        self.event.wait() # <- block here, I think this can be removed
        info = self.queue.get() # <- block here again, only one thread can get job to do?
        # do something
        self.queue.task_done()
        self.event.clear() #

As long as queue is actually Queue.Queue (or another collection with its own blocking mechanic), the event object is not required in the code you posted. I'd even say that it's used incorrectly. There is a racing condition when the server calls event.set() and a thread is just done with its current task, calling event.clear(). This leaves all threads waiting on the event, even though there's an item in the queue.

Edit: Bollocks, didn't think that one through. event.set() wakes up all threads that are hanging on event.wait(), causing them to continue waiting at queue.get(). One thread will fetch the item and process it. So, no racing condition. Just redundancy.


Temporary modifying env variables with context manager One function of a program I'm working with benefits from using all available openMP threads. The rest of the program has issues with the env variable OMP_NUM_THREADS being unequal to 1.

I'm currently using the following solution to that, and was wondering whether there were any caveats or room for improvement here. I haven't worked much with context managers yet, and greatly appreciate any help/pointers. Thanks!

import os
import multiprocessing as mp

class MaxOMP:
    def __enter__(self):
        os.environ["OMP_NUM_THREADS"] = str(mp.cpu_count())

    def __exit__(self, exc_type, exc_val, exc_tb):
        os.environ["OMP_NUM_THREADS"] = "1"

def main():
    with MaxOMP():
        run_omp_intensive_job()

Instead of MaxOMP as a name, I like MaxOmp - in general still CamelCase your names even if they include an acronym.

Additionally, there isn't any reason why this needs to be restricted to any particular environment variable. As such, I would probably make this:

class EnvironmentVariableContextManager:

    _base_value = None
    _variable = None

    def __init__(self, variable_name, value):
        self.variable = variable_name
        self.target_value = value

    @property
    def variable(self):
        return self._variable

    @variable.setter
    def variable(self, name):
        self._base_value = os.environ.get(name, None)

        if self._base_value is None:
            raise Exception(f"{name} is not a valid environment variable")

        self._variable = name

    @property
    def original_value(self):
        return self._base_value

    def __enter__(self):
        os.environ[self.variable] = self.target_value

    def __exit__(self, *):
        os.environ[self.variable] = self.original_value

This has a few benefits:

We use properties to properly encapsulate access and add some extra validation (if you don't care about invalid environment variables feel free to remove that)
We restore it to the original value, not some hardcoded value. You could also provide a way to specify an "after" value if that was desirable.

As suggested in the comments, we may have a valid use-case for an environment variable that doesn't currently have a value. In that case, we would remove the exception, and instead do this:

@variable.setter
def variable(self, name):
    self._base_value = os.environ.get(name, None)
    self._variable = name

def __exit__(self, *):
    if self.original_value is None:
        os.environ.pop(self.variable)
    else:
        os.environ[self.variable] = self.original_value

Once we have that, then we just inherit.

class MaxOmpThreads(EnvironmentVariableContextManager):

    def __init__(self, max_threads=None):
        max_threads = max_threads or mp.cpu_count()
        super().__init__("OMP_NUM_THREADS", max_threads)

Another thing to note, is that os.environ only reflects the environment variables at the time of Python startup. If this is not what you want then you'll likely need a different solution.

This also may not be thread-safe; if you might have multiple processes working on something at once, and some need one setting and some need another, then you're likely to run into issues. If your program is overall serial, and the pieces that use OMP are well defined and contained, you're probably fine.


Portable method of getting time in C Mac doesn't have the Unix clock_gettime so I am trying to create something that's really portable and only dependent on C.

So far I've come across this function:

static void current_utc_time(struct timespec *ts) {
#ifdef __MACH__ // OS X does not have clock_gettime, use clock_get_time
    clock_serv_t cclock;
    mach_timespec_t mts;
    host_get_clock_service(mach_host_self(), CALENDAR_CLOCK, &cclock);
    clock_get_time(cclock, &mts);
    mach_port_deallocate(mach_task_self(), cclock);
    ts->tv_sec = mts.tv_sec;
    ts->tv_nsec = mts.tv_nsec;
#else
  clock_gettime(CLOCK_REALTIME, ts);
#endif
}

I would like to be able to get seconds and milliseconds for my purposes. This is what I am currently doing and would like some feedback on correctness:

struct timespec requestStart, requestEnd;
current_utc_time(&requestStart);
int i = 0;
while(i < 1000000000)
{
    ++i;
}
current_utc_time(&requestEnd);
printf("total: ms:  %lu\n", (requestStart.tv_nsec - requestEnd.tv_nsec) / (unsigned long)1000000);

This prints out: total: ms:  131

It seems to be correct but I just wanted to make sure before I proceeded. Am I doing my calculations correctly and how can I get the seconds as well as milliseconds?

A few notes:

clock_gettime() is not standard C, which means it's not guaranteed to be implemented on any system that supports C (including many Linux platforms, the few who implement it stuff it into time.h, but there are only a handful of standardized functions in that header). Relying on it for "portability" is not something you should do.
There is no standard C function that provides better than 1 second time resolution, but the POSIX function gettimeofday() provides microsecond resolution.

You can use this function like this:

struct timeval before = {};
struct timeval after = {};
struct timeval result = {};

gettimeofday(&before, NULL);

// Some code you want to time

gettimeofday(&after, NULL);
timersub(&after, &before, &result);

printf("Time elapsed: %ld.%06ld\n", (size_t) result.tv_sec, (size_t) result.tv_usec);

The other option is to just use the clock() function in time.h which is part of the C standard, but as stated before the time resolution is not great.
Please don't use busy waiting (side note: that would be a place for a for loop, not a while loop).  It's wasteful of CPU resources and could even be optimized out by the compiler.  It would be much more efficient and easier to test your code with the sleep() function as then you know how many seconds long the process should take.  If you want different resolutions for causing the current thread to be suspended from execution of your code, take a look at usleep() or nanosleep().


Finding nearest city from a center place (latitude and longitude) with radius in miles This is my code for finding the nearest city within a certain radius. I am not sure, is my code is OK or is it wrong? Please review and offer suggestions on my code.

<?
//Suppose,I have selected a city and fetch the selected city's lat and long and radius
//in to this page.
$file_handle = fopen("Londonstations.csv", "r");
$i=0; $j=0; $k=0;
while (!feof($file_handle) ) {
$line_of_text = fgetcsv($file_handle, 1024);
$locat[$i++] = $line_of_text[0];
$lat[$j++] = $line_of_text[3];
$long[$k++] = $line_of_text[4];
//print $line_of_text[0]; . $line_of_text[1]. $line_of_text[2] . "
";
}
$l = $_POST['l'];//This value is received from previous page selected places latitude and longitude index from array.
$r = $_POST['r'];//This value is received from previous page selected places for //finding city that are in located in this radius
$radius = 3958.761; //earth's radius in miles
 // we'll want everything within, say, distance
$distance = $r;//This value is received from previous page, radius of selected
 //places for finding city that are in located in this radius

// latitude boundaries
$maxlat = $lat[$l] + rad2deg($distance / $radius);
$minlat = $lat[$l] - rad2deg($distance / $radius);

// longitude boundaries (longitude gets smaller when latitude increases)
$maxlng = $long[$l] + rad2deg($distance / $radius / cos(deg2rad($long[$l])));
$minlng = $long[$l] - rad2deg($distance / $radius / cos(deg2rad($long[$l])));
$n=1;
echo 'Nearest Stations:';
echo '
';
for($ii=1;$ii<$i;$ii++){
if($lat[$ii]<=$maxlat && $lat[$ii]>=$minlat){
    if($long[$ii]<=$maxlng && $long[$ii]>=$minlng)
        echo '
'.$rs[$n++] = $locat[$ii].'
';
}
}
echo '
';
?>

I din't check your math, because in any case the code doesn't do what it claims to do. The returned points are not within the given distance of the center, but belong to some square-like area.

To calculate a distance as the crow flies, notice that a cosine of the angular distance theta is

$$ \cos\theta = \cos\phi_0\cos\phi_1\cos(\psi_0 - \psi_1) + \sin\phi_0\sin\phi_1 $$

where Phi and Psi are respective latitudes and longitudes. The greater cosine, the closer points are.

Given a distance d, calculate cutoff = cos(d/(2*pi*R)). For each city in the list apply the above formula. The city is "nearby" if the result is greater than cutoff.


Google Drive folders properties caching to file I have the recursive function below to cache all the folders of my company Drive into a file. It works like a charm but takes too long, specifically >30 secs to read and save the 300 folders.

What I need is to make it faster somehow. I don't know if it is possible to make the folder reading asynchronous, or if it possible to make a webhook or something to listen to changes in the Drive's folders, which would eliminate the need to run recursively over all folders.

function generateFolderTree( idParent, tree ){
  var currParent = DriveApp.getFolderById( idParent),
      folderChildren = currParent.getFolders(),
      currChild,
      currChildId;

  if( tree.length == 0 ){
    tree.push({ 'text' : currParent.getName(), 'parent' : '#', 'id' : idParent, 'state' : { 'opened' : true }, 'li_attr':{'title': ''} });
  }

  while( folderChildren.hasNext() ){
    currChild = folderChildren.next();
    currChildId = currChild.getId();

    tree.push({ 'text' : currChild.getName(), 'parent' : idParent, 'id' : currChildId, 'li_attr':{'title': currChild.getDescription()} });

    generateFolderTree( currChildId, tree );
  }
}

function geneteAndSaveTheTree(){
  var theTree = [];

  generateFolderTree(folderToIterateId, theTree);

  DriveApp.getFileById(theFileWhereAllFoldersAreStored).setContent(JSON.stringify(theTree));
  return theTree;
}

I know you can get all folders at once for a Drive, but I think it would take even longer to find out all the folders that are child of the folderToIterateId.

Other than the fact that it's hard to read because your variables are not in Englishref, your code appears solid. Two observations:

Some comments would be beneficial, even if only function, parameter and return descriptions. Use of recursion should be clearly explained in comments, for the next poor soul who needs to figure out why they ran out of memory.
You have not explicitly handled the case of an invalid folder ID parameter in gerarJqTree().

The only optimization I'd suggest would be to do what you think would take longer; get all folders at once - but do it with the advanced Drive service so you have the container information as well. While you'll need to traverse the returned items to construct the output, you will eliminate several hundred service calls.


Splitting two ObservableCollections lightning fast I'm working on wiring up a bindable Map for Xamarin Forms, and I need a fast way of dividing up the ObservableCollections (collection of map markers) as quickly as possible. Collections are a weak area for me, and I find myself doing too many foreach's when they're not necessary.

Here's what I have now.

private static void MarkerSourceCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
{
    var markers = ((IEnumerable<XfxMarker>) sender).ToList();

    var comparer = new MapMarkerComparer();
    var offScreen = new List<XfxMarker>();
    var markersToAdd = markers.Where(m => _map.MapBounds.Contains(m.CenterPoint), out offScreen)
        .Except(_oldMarkers, comparer);

    var markersToRemove = _oldMarkers.Except(markers, comparer).Union(offScreen);
    _map.HandleMarkersChanged(markers, new MapMarkersChangedEventArgs(markersToAdd, markersToRemove));
    _oldMarkers = markers;
}

With the comparer

internal class MapMarkerComparer : IEqualityComparer<XfxMarker>
{
    public bool Equals(XfxMarker x, XfxMarker y)
    {
        if (ReferenceEquals(x, y)) return true;
        if (ReferenceEquals(x, null) || ReferenceEquals(y, null)) return false;
        var result = x.CenterPoint.Latitude.ToString() == y.CenterPoint.Latitude.ToString()
                     && x.CenterPoint.Longitude.ToString() == y.CenterPoint.Longitude.ToString();
        return result;
    }

    public int GetHashCode(XfxMarker obj)
    {
        unchecked
        {
            var hash = 0;
            if (obj.CenterPoint == null) return hash;
            hash = hash + 17;
            hash = hash*23 + obj.CenterPoint.Longitude.GetHashCode();
            hash = hash*23 + obj.CenterPoint.Latitude.GetHashCode();
            return hash;
        }
    }
}

And my custom Where extension.

    public static IEnumerable<T> Where<T>(this IEnumerable<T> input, Func<T, bool> predicate, out List<T> remainder)
    {
        var leftover = new List<T>();
        var output = new List<T>();
        foreach (var item in input)
        {
            if (predicate.Invoke(item))
            {
                output.Add(item);
            }
            else
            {
                leftover.Add(item);
            }
        }
        remainder = leftover;
        return output;
    }
}

Now I'm sure there has to be a more efficient way to split these lists, but I'm just not sure what.

NotifyCollectionChangedEventArgs

You receive a NotifyCollectionChangedEventArgs, wich holds interesting data about the items that got added NewItems and removed OldItems from the collection. There is also an Action that defines how the collection got modified. Your changed handler is implemented as a NotifyCollectionChangedAction.Reset handler. It doesn't take into account the information from the event e.

var markers = ((IEnumerable<XfxMarker>) sender).ToList();

This means, you are checking all elements against the previously cached elements by the handler container class and manually verify which ones are added and which ones removed. Let's say filter is m => _map.MapBounds.Contains(m.CenterPoint):

 var markersToAdd = markers
    .Where(m => filter(m), out offScreen).Except(_oldMarkers,comparer);

and

var markersToRemove = _oldMarkers.Except(markers, comparer).Union(offScreen);

However, in most cases I presume the action to be Add or Remove. If this is the case, you already have the new and/or old elements. For instance, on Add:

var markersAddedInSource = e.NewItems.OfType<XfxMarker>();
var markersToAdd = markersAddedInSource.Where(filter);
var markersToRemove = markersAddedInSource.Except(markersToAdd);

Or using your own extension method:

var markersAddedInSource = e.NewItems.OfType<XfxMarker>();
var markersToAdd = markersAddedInSource.Where(filter, out var markersToRemove);

This saves you a couple of iterations on the source collection and allows you to only iterate on the changed elements.

MapMarkerComparer

I don't know the type of Latitude and Longitude, but consider checking equality on their source type rather than their ToString outcome.

x.CenterPoint.Latitude.ToString() == y.CenterPoint.Latitude.ToString()

In case you have issues with rounding or truncation (likely with float or double) you could round the values in the check.

x.CenterPoint.Latitude == y.CenterPoint.Latitude

Or you could use rounding:

Math.Round(x.CenterPoint.Latitude, precision)
    == Math.Round(y.CenterPoint.Latitude, precision)

Or you could guard against a tolerance threshold:

Math.Abs(x.CenterPoint.Latitude - y.CenterPoint.Latitude) < tolerance

IEnumerable<T> Where<T>

Consider changing the method signature to

public static ILookup<bool, T> Where<T>(this IEnumerable<T> input, Func<T, bool> predicate);

Or returning a Dictionary:

public static IDictionary<bool, IEnumerable<T>> Where<T>(
    this IEnumerable<T> input, Func<T, bool> predicate);

So you don't need to worry about a return value and an out-parameter. This pattern isn't used that much anymore and is mostly used to Try* operations.

You can get the items that matched the predicate:

var results = markers.Where(filter);
var matched = results[true];
var unmatched = results[false];

Considering renaming Where to avoid confusion with LINQ's Where method.


Using Task.Wait() for waiting while task is finished (even if it already finished) Can I use task.Wait(); like that? Note that when I call task.Wait the task is probably already finished.

And probably you can suggest better pattern.

class A {
    private Task task1;
    private Task task2;

    ...
    public void Connect() {
        stayConnected = true;
        task1 = Task.Factory.StartNew(....,
            while (stayConnected) {
                ....
            }
            LongRunning);
        task2 = Task.Factory.StartNew(....,
            while (stayConnected) {
                ....
            }
            LongRunning);
    }

    private volatile bool stayConnected;

    // should be synchronous. when return everything should be disconected
    public void Disconnect() {
        stayConnected = false;
        task1.Wait();
        task1 = null;
        task2.Wait();
        task2 = null;
    }

}

You can use a CancellationToken and Task.WaitAll(...) to do this...

public class A
{
    private readonly CancellationTokenSource tokenSource = new CancellationTokenSource();
    private Task task1;
    private Task task2;

    public void Connect()
    {
        task1 = Task.Factory.StartNew(SomeWork, tokenSource.Token);
        task2 = Task.Factory.StartNew(SomeWork, tokenSource.Token);
    }

    public void Disconnect()
    {
        tokenSource.Cancel();
        Task.WaitAll(task1,task2);
    }

    public void SomeWork(object o)
    {
        CancellationToken token = (CancellationToken)o;
        while (!token.IsCancellationRequested)
        {
            Console.WriteLine("working...");
        }
    }
}

For more information on managed threads check this and this article on MSDN. Enjoy :D


Running an application continuously Any idea on how to run application continuously? I tried follow suggestions from this link, but to no avail. It might be because of my coding but I don't know how to amend it. Someone suggested that I use Windows Service, but I don't want to use it at this moment because I want to learn basics first like in the above link.

Program.cs

        static int exitFlag = 0;

        private static int m_intErrCounter = 0;
        static int Main(string[] args)
        {
            int retValue = 0;

            while (exitFlag != 0)
            {
                retValue = CounterApp();
            }
            //return retValue;
            return 0;
        }

        public static int CounterApp()
        {
            string machineName = ConfigurationManager.AppSettings["MachineName"];
            string categoryName = ConfigurationManager.AppSettings["CategoryName"];
            string counterName = ConfigurationManager.AppSettings["CounterName"];
            string instanceName = ConfigurationManager.AppSettings["InstanceName"];
            PerformanceCounterCategory pcc;
            PerformanceCounter[] counters;
            m_intErrCounter = 0;

            try
            {
                // Create the appropriate PerformanceCounterCategory object.
                if (machineName.Length > 0 && instanceName.Length > 0)
                {
                    pcc = new PerformanceCounterCategory(categoryName, machineName);
                    counters = pcc.GetCounters(instanceName);
                }
                else
                {
                    pcc = new PerformanceCounterCategory(categoryName);
                    counters = pcc.GetCounters();
                }

            }
            catch (Exception)
            {
                Console.WriteLine("Couldn't found the application");
                Console.ReadKey();
                return 0;
            }

            String strBody = String.Empty;
            if (counters.Length != 0)
            {

                for (int objX = 0; objX < counters.Length; objX++)
                {
                    if ((counters[objX].CounterName == counterName) && (counters[objX].RawValue > 0))
                    {
                        strBody = "Error occured at " + counters[objX].InstanceName.ToString();
                        strBody += " : " + counters[objX].CounterName.ToString() + " thrown " + counters[objX].RawValue.ToString() + " times.";
                        m_intErrCounter++;
                        exitFlag = 1;
                    }
                }
            }
            if (m_intErrCounter > 0)
            {
                SendMail("Notification Email", strBody);
                exitFlag = 1;
            }
            return exitFlag;
        }

        private static void SendMail(String strSubj, String strBody)
        {
            MailSettingsSectionGroup mMailSettings = new MailSettingsSectionGroup();
            string mMailFrom = mMailSettings.Smtp.From;
            string mMailHost = mMailSettings.Smtp.Network.Host;
            int mMailPort = mMailSettings.Smtp.Network.Port;
            string mMailUserName = mMailSettings.Smtp.Network.UserName;
            string mMailPassword = mMailSettings.Smtp.Network.Password;

            string to = ConfigurationManager.AppSettings["To"];

            MailMessage Message = new MailMessage();
            Message.From = new MailAddress(mMailFrom);
            Message.To.Add(new MailAddress(to));

            Message.Subject = strSubj;
            Message.Body = strBody;

            SmtpClient client = new SmtpClient(mMailHost, mMailPort);

            client.EnableSsl = false;
            client.Credentials = new System.Net.NetworkCredential(mMailUserName, mMailPassword);

            client.Send(Message);
            Console.WriteLine("Success Send Message");
        }
    }
}

Your loop doesn't work because the initial value for exitFlag variable (which is used as condition for the loop) is zero, and it is only being set to 1 under a special set of circumstances which apparently doesn't happen when you run an application.

If you really want to loop forever and only quit application when it fails your loop, have your function return 1 unless it fails. It should look something like this:

int Main(string[] args)
{
    while (CounterApp() != 0)
    {
    }
    return 0;
}

public static int CounterApp()
{
    // do some stuff

    try
    {
        // do some stuff that might fail
    }
    catch (Exception)
    {
        return 0;
    }

    // do some other stuff

    return 1;
}


Text file manipulation with Java 8 and Guava I am trying to do some translation of text files. I have a translation .csv file and a file I need to translate (tab delimited). The translation is creating a file with an extra column of the translation.

I'd like to hear any suggestions on this.

import com.google.common.base.Joiner;
import com.google.common.base.Splitter;
import com.google.common.collect.Lists;
import com.google.common.io.Files;

import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class TextFileManipulationExample {

  public static void main(String[] args) {
    new TextFileManipulationExample().translate();
  }

  private void translate() {
    List<String> translationFileContent = readFile("dictionary.csv");
    Stream<List<String>> translationLines = translationFileContent.stream().map(
      line -> Splitter.on(",").splitToList(line));
    Map<List<String>, String> translationMap = translationLines.collect(Collectors.toMap(l -> Lists.newArrayList(l.subList(0, 2)), l -> l.get(2)));
    List<String> toTranslateLines = readFile("file_to_translate.txt");
    Stream<List<String>> listWithTranslation = toTranslateLines.stream().map(line -> {
      List<String> list = Lists.newArrayList(Splitter.on("\t").splitToList(line));
      if (translationMap.containsKey(list)) {
        list.add(translationMap.get(list));
      }
      return list;
    });
    String contentToWrite = listWithTranslation.map(lineList -> Joiner.on(",").join(lineList)).collect(
      Collectors.toList()).stream().reduce((t, u) -> t + "\n" + u).get();
    writeFile(contentToWrite, "translated.csv");
  }

  private void writeFile(String contentToWrite, String fileName) {
    try {
      Files.write(contentToWrite, new File(fileName), Charset.defaultCharset());
    } catch (IOException e) {
      throw new RuntimeException(e);
    }
  }

  private List<String> readFile(String fileName) {
    try {
      return Files.readLines(new File(fileName), Charset.defaultCharset());
    } catch (IOException e) {
      throw new RuntimeException(e);
    }
  }
}

You are using Java 8's functional stream programming, but you leave out method chaining. Depending on how you chain methods, it could be more or it could be less readable. You might be imagining this:

readFile("dictionary.csv").stream().map(line -> Splitter.on(",").splitToList(line)).collect(Collectors.toMap(l -> Lists.newArrayList(l.subList(0, 2)), l -> l.get(2)));

There is no way I could read that. However, consider this:

Map<List<String>, String> translationMap = readFile("dictionary.csv").stream()
            .map(line -> Splitter.on(",").splitToList(line))
            .collect(
                Collectors.toMap(
                    l -> Lists.newArrayList(l.subList(0, 2)),
                    l -> l.get(2)
                )
            );

I find this much more readable. By simply reading vertically, I can determine that the code does this:

Reads dictionary.csv into a list of strings (via readFile).
Split each string in the dictionary on ",", into a List<String>.
Collect the List<String>s into a Map, where

The keys are the first two elements of each list, still in a List
The values are the 3rd element of each list.

On the other hand, your code as is is littered with type names; do I really need to know that that is a Stream<List<String>> when that's easily determined from context?:

List<String> translationFileContent = readFile("dictionary.csv");
Stream<List<String>> translationLines = translationFileContent.stream().map(
  line -> Splitter.on(",").splitToList(line));
Map<List<String>, String> translationMap = translationLines.collect(Collectors.toMap(l -> Lists.newArrayList(l.subList(0, 2)), l -> l.get(2)));

Alternatively, if you really like the temporary variables, put some empty lines in there to separate logical sections. Also, consider using Project Lombok, where you could just do this:

List<String> translationFileContent = readFile("dictionary.csv");
val translationLines = translationFileContent.stream().map(
    line -> Splitter.on(",").splitToList(line));
val translationMap = translationLines.collect(Collectors.toMap(l -> Lists.newArrayList(l.subList(0, 2)), l -> l.get(2)));

(Coder discretion advised on where you should use val).


React Component Design Building out a React component that is supposed to hold the following information and display it in a feed-like page. The idea is to later map over the individual entries in an array.

Author
Timestamp
Message Title
Message body

Currently, I have the following code in one file (BuildingUpdateBox.js):

import React from 'react';
import "./BuildingUpdateBox.css"

const BuildingUpdateBoxHeader = (props) => {
const author = props.author ? `Author: ${props.author}` : 'No Author'
return (

    {author}
    Timestamp: {props.timestamp}

)
}

const BuildingUpdateBoxBody = (props) => {
return (

{props.title}

{props.message}

)
}

const BuildingUpdateBox = (props) => {
return (

      <BuildingUpdateBoxHeader
        author={props.content.author}
        timestamp={props.content.timestamp}
      />
      <BuildingUpdateBoxBody
        title={props.content.title}
        message={props.content.message}
      />

)
}

export default BuildingUpdateBox;

And in App.js (I currently use create-react-app):

const message = {
  author: 'Joe Smith',
  timestamp: '2018/12/22',
  title: 'Broken Heater',
  message: 'Dear tenants, we will have a plumber onsite tomorrow, who will replace the broken boiler. We apologize for the inconvenience. If you have any questions, please feel free to reach out to 123-456-7890.'
};

class App extends Component {
  render() {
    return (

        <BuildingUpdateBox content={message} />

    );
  }
}

I'm looking to hear if I should:

Place the functional components into their own files (with reasoning)
Even use 3 separate functions, since I don't ever see the Header or Body being used without the Box and vice versa
Do anything with the way I import the CSS

Generally if you want a more in depth review you should provide more code and a potential specifics you want to improve on; your code is very thin, and so it's a little difficult to help you expand on it, however, I would say that there's a few different things you can do to this to improve or follow a different direction forward.

Your use of pure components is good for BuildingUpdateBoxHeader and BuildingUpdateBoxBody as using primitive props is better for performance. However, using a content object inside your props for BuildingUpdateBox means React cannot shallow compare your props and is unable to determine when not to rerender the component, causing potentially wasted rerenders.

I would recommend trying JSS (css in js) to embed your styles directly in your application for an alternate solution to specifying className manually. This couples your css with its implementation rather than needing to update both the css & js when you modify them.


Book database for a library system I am writing a book database, and ultimately a library system, in Django. When a book is created without a title set, the ISBN will be used to automatically fetch book data from WorldCat I have just created a form to add new books to the database, but it has no real error-handling. Also, as I am new to Django, I suspect I have put a lot of my code in the wrong place.

Improving error-handling is my main incentive, but any other improvements would be welcome.

The form, in forms.py:

class AddBookForm(ModelForm):

    """docstring for AddBookForm"""

    class Meta:
        model = Book
        fields = ['isbn']

The form view, in views.py:

# This defines the navbar
pages = [['Search', '/'], ['Add Book', '/add']]

def add_book(request):
    if request.method == 'POST':
        # create a form instance and populate it with data from the request:
        form = AddBookForm(request.POST)
        # check whether it's valid:
        if form.is_valid():
            # process the data in form.cleaned_data as required
            addedBook = form.save()
    # if a GET (or any other method) we'll create a blank form
    else:
        form = AddBookForm()

    # Highlights navbar entry
    pages[1].append(True)
    context = {'pages': pages, 'form': form,}
    if 'addedBook' in locals():
        context['added'] = addedBook
    else:
        context['added'] = None
    return render(request, 'library/add.html', context)

The form template (add.html):

{% extends "library/base_site.html" %}
{% block title %}Add book{% endblock %}
{% block content %}
    {% if added %}

            <button type="button" class="close" data-dismiss="alert">
                &times;
                Close
            </button>
            Succesfully added book "{{ added.title }}" by {{ added.author }} - ISBN {{ added.isbn }}

    {% endif %}
    <form role="form" method="post" action=".">
        {% csrf_token %}

            Isbn
            <input id="id_isbn" class="form-control" maxlength="13" name="isbn" type="text" placeholder="Enter ISBN or scan barcode">

        <button type="submit" class="btn btn-default">Submit</button>
    </form>
{% endblock %}

The entire project is available on GitHub.

This is a bit strange, and more complicated than it needs to be:

if 'addedBook' in locals():
    context['added'] = addedBook
else:
    context['added'] = None

You could initialize addedBook = None, and simplify the above, and without using locals():

addedBook = None
# validation, possibly setting: addedBook = form.save()
context['added'] = addedBook

Needless to say, this "docstring" is useless:

"""docstring for AddBookForm"""

Error handling

Your current error handling is merely checking if the form is valid or not. And the form class is very basic, so only the standard validations will be applied, according to the underlying model, for example, non-integer values for an integer field will raise a validation error.

At the minimum, it would be good to tell the user about validation errors, if any. In the current code, if the form was not valid, it simply prints the same form again without giving the user a clue about what went wrong, which is not very user-friendly.

This page in the docs touches on the basics of form validation and includes examples of showing form validation errors:

https://docs.djangoproject.com/en/1.7/topics/forms/

This page is more detailed, and specific about Model forms:

https://docs.djangoproject.com/en/1.7/topics/forms/modelforms/

I recommend you re-read them and:

Add printing of form errors to the HTML output
Add custom validation methods in the form class, if it makes sense for your use case


Angular Rest API generic service and implementation service I'm wondering if there's a better way to write the ApiService and the CategoryService so that CategoryService is basically just a call to extend ApiService with the string 'category' and the base type 'ICategory' or if this is the best that I can get from typescript and angular? There will be many more endpoint services in the final app

Any other general comments on code style etc are also very welcome (or if anyone knows how to tell Webstorm to not put the constructor closing braces on the next line!!)

ApiService

import { Injectable } from '@angular/core';
import { HttpClient, HttpHeaders } from '@angular/common/http';
import { Observable } from 'rxjs';
import { environment } from '../../environments/environment';

const baseUrl = environment.serverBaseUrl;
const apiEndpoint = environment.apiEndpoint || '/api/v1/';
const apiUrl = `${baseUrl}${apiEndpoint}`;

@Injectable({
  providedIn: 'root'
})
export class ApiService {

  constructor(private http: HttpClient) {
  }

  private createTokenOptions() {
    return {headers: new HttpHeaders().set('Authorization', `Bearer ${localStorage.getItem('access_token')}`)};
  }

  get<T>(serviceEndpoint: string, id?: number): Observable<T> {
    let url = `${apiUrl}${serviceEndpoint}`;
    if (id) {
      url += `/${id}`;
    }
    return this.http.get<T>(url, this.createTokenOptions());
  }

  create<T>(serviceEndpoint, item: T): Observable<object> {
    const options = this.createTokenOptions();
    options.headers = options.headers.append('Content-Type', 'application/json');
    return this.http.post(`${apiUrl}${serviceEndpoint}`, JSON.stringify(item), options);
  }

  delete<T>(serviceEndpoint: string, id: number): Observable<T> {
    return this.http.delete<T>(`${apiUrl}${serviceEndpoint}/${id}`, this.createTokenOptions());
  }
}

CategoryService

import { Injectable } from '@angular/core';
import { ApiService } from './api.service';
import { Observable } from 'rxjs';
import { ICategory } from '../interfaces/category.model';

@Injectable({
  providedIn: 'root'
})
export class CategoryService {

  private serviceEndpoint = 'category';

  constructor(private api: ApiService) {
  }

  getCategories() {
    return this.api.get<ICategory[]>(this.serviceEndpoint);
  }

  getCategory(id: number): Observable<ICategory> {
    return this.api.get<ICategory>(this.serviceEndpoint, id);
  }

  createCategory(category: ICategory) {
    return this.api.create<ICategory>(this.serviceEndpoint, category);
  }

  deleteCategory(id: number) {
    return this.api.delete<ICategory>(this.serviceEndpoint, id);
  }
}

I think you can use HTTP Interceptors to simplify the code.

For the authorization

@Injectable()
export class AuthorizationInterceptor implements HttpInterceptor {

  constructor() {
  }

  intercept(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {

    // skip requests for local assets
    if (request.url.indexOf("assets/") >= 0) {
      return next.handle(request);
    }

    const apiReq = request.clone({
      url: request.url,
      headers: request.headers.set('Authorization', `Bearer ${localStorage.getItem('access_token')}`)
    });
    return next.handle(apiReq);
  }
}

For the base url

Something along the line

@Injectable()
export class BaseUrlInterceptor implements HttpInterceptor {

  constructor(
    @Inject("BASE_API_URL") private baseUrl: string) {
  }

  intercept(request: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {

    // skip requests for local assets
    // maybe put extra condition to allow for multiple API usage
    if (request.url.indexOf("assets/") >= 0) {
      return next.handle(request);
    }

    const apiReq = request.clone({
      url: `${this.baseUrl}${request.url}`
    });
    return next.handle(apiReq);
  }
}

Of course, the module should define the interceptors:

  providers: [
    {
      provide: HTTP_INTERCEPTORS,
      useClass: AuthorizationInterceptor,
      multi: true
    }
  ]


Buffered asynchronous writer Imagine I have connection in which I have to write numbers, represented by this contract:

interface ISocket
{
    Task WriteAsync(Byte[], Int32 offset, Int32 count, CancellationToken cancel);
}

There is one single thread calling this WriteAsync method.

Till now each caller to ISocket was buffering independently and knows when it has to flush, however I would like to centralize that logic. Also I want to make sure that when such buffer is being transmitted, I can take advantage of async/await and release the thread pool thread.

My idea would be to create something like this:

class SocketWriter
{
    private ISocket _socket;
    private Byte[] _buffer; // assume 8K
    private Int32 _cursor;

    [ ... ]

    private async Task FlushCacheIfNotEnoughSpace(Int32 length, CancellationToken cancel)
    {
        if(!FitsInBuffer(length))
        {
            await _socket.WriteAsync(_buffer, 0, _cursor, cancel).ConfigureAwait(false);
            _cursor = 0;
        }
    }

    [ ... ]

    public async Task WriteAsync(UInt64 value, CancellationToken cancel)
    {
        await FlushCacheIfNotEnoughSpace(8, cancel).ConfigureAwait(false);
        CopyToByteArray(value);
    }

    public Task Flush(CancellationToken cancel)
    {
        return FlushCacheIfAny(cancel);
    }
}

There are some memory constraints, so I cannot create more than one of those Byte[] buffers. CopyToByteArray requires enough space in the _buffer and increments _cursor accordingly.

Note that FlushCacheIfNotEnoughSpace only awaits if the given length does not fit the current buffer.

My intention is that when the buffer is not complete and there is space for another number this method call executes synchronously, or properly awaits using IO completion ports otherwise:

await _writer.WriteAsync(1UL, cancel).ConfigureAwait(false);

I wonder if there is a better or more elegant way of doing this, because it seems like a lot of "noise":

await _writer.WriteAsync(obj.Member1, cancel).ConfigureAwait(false);
await _writer.WriteAsync(obj.Member2, cancel).ConfigureAwait(false);
await _writer.WriteAsync(obj.Member3, cancel).ConfigureAwait(false);
await _writer.WriteAsync(obj.Member4, cancel).ConfigureAwait(false);

OK, now it is SOLID :)

Let's do not mix infrastructure and application code. Unfortunatly a lot of stuff is missing in .NET, so infrastructure first.

AsyncStream:

public interface IAsyncStream
{
    Task WriteAsync(
        byte[] buffer,
        int offset,
        int count,
        CancellationToken cancellationToken);

    Task FlushAsync(CancellationToken cancellationToken);
}

BufferedAsyncStream:

public class BufferedAsyncStream : IAsyncStream
{
    public BufferedAsyncStream(IAsyncStream inner, long bufferSize = 8192)
    {
        Inner = inner;
        Buffer = new byte[bufferSize];
        BufferStream = new MemoryStream(Buffer);
    }

    public async Task WriteAsync(
         byte[] buffer,
         int offset,
         int count,
         CancellationToken cancellationToken)
    {
        if (FreeSpace < count)
            await FlushAsync(cancellationToken)
                .ConfigureAwait(false);

        BufferStream.Write(buffer, offset, count);
    }

    public async Task FlushAsync(CancellationToken cancellationToken)
    {
        await Inner.WriteAsync(Buffer, 0, Allocated, cancellationToken)
            .ConfigureAwait(false);

        await Inner.FlushAsync(cancellationToken)
            .ConfigureAwait(false);

        BufferStream.Seek(0, SeekOrigin.Begin);
    }

    int FreeSpace => (int)BufferStream.Length - (int)BufferStream.Position;
    int Allocated => (int)BufferStream.Position;
    IAsyncStream Inner { get; }
    byte[] Buffer { get; }
    MemoryStream BufferStream { get; }
}

AsyncWriter:

public class AsyncWriter
{
    public AsyncWriter(IAsyncStream stream)
    {
        Stream = stream;
    }

    public Task WriteAsync(ulong value, CancellationToken cancellationToken)
    {
        var buffer = BitConverter.GetBytes(value);
        return Stream.WriteAsync(buffer, 0, buffer.Length, cancellationToken);
    }

    public Task FlushAsync(CancellationToken cancellationToken)
    {
        return Stream.FlushAsync(cancellationToken);
    }

    IAsyncStream Stream { get; }
}

Now application code. ISocket being adapted to IAsyncStream:

class SocketStream : IAsyncStream
{
    public SocketStream(ISocket socket)
    {
        Socket = socket;
    }

    public Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
    {
        return Socket.WriteAsync(buffer, offset, count, cancellationToken);
    }

    public Task FlushAsync(CancellationToken cancellationToken)
    {
        return Task.CompletedTask;
    }

    ISocket Socket { get; }
}

So, we can construct writer in the following way:

var writer = new AsyncWriter(
    new BufferedAsyncStream(
        new SocketStream(socket)));

UPDATE
You could use this overload to remove "noise":

public class AsyncWriter
{
     ...
    public async Task WriteAsync(
        CancellationToken cancellationToken, params ulong[] values)
    {
        foreach (var value in values)
            await WriteAsync(value, cancellationToken)
                .ConfigureAwait(false);
    }

Now it is this way:

  await writer
     .WriteAsync(
         cancellationToken,
         obj.Member1, obj.Member2, obj.Member3, obj.Member4)
     .ConfigureAwait(false);


Calculating total size of player data I am writing a function to calculate the total size of a "player appearance".

There are various "item slots" the player has, and if they are empty then 1 byte will be sent, otherwise there will be 2.

const getAppearanceBlockSize = (appearance: PlayerAppearance) => {
    const headObscured = isCoveringHead(appearance.outfit.head);

    return (
        40
        + (appearance.outfit.head ? 2 : 1)
        + (appearance.outfit.cape ? 2 : 1)
        + (appearance.outfit.amulet ? 2 : 1)
        + (appearance.outfit.weapon ? 2 : 1)
        + (appearance.outfit.shield ? 2 : 1)
        + (appearance.outfit.head ? 2 : 1)
        + (headObscured ? 2 : 4)
    );
};

The code seems fairly clear to me, but Code Climate has given it a high "cognitive complexity" score - indicating that the code is complex and should be broken up.

I can't think of any ways to break the code up, however, that wouldn't result in it being more complex than it currently is.

I don't know anything about CodeClimate or how it determines complexity, but this is the likely culprit right here:

return (
    40
    + (appearance.outfit.head ? 2 : 1)
    + (appearance.outfit.cape ? 2 : 1)
    + (appearance.outfit.amulet ? 2 : 1)
    + (appearance.outfit.weapon ? 2 : 1)
    + (appearance.outfit.shield ? 2 : 1)
    + (appearance.outfit.head ? 2 : 1)
    + (headObscured ? 2 : 4)
);

This is pretty hard to read and understand at a glance, and all of the ternary operators certainly don't help. Without knowing more about your object it is hard to review, but to me it would make more sense to have a method like this on your outfit object

function getSlotSize(slot) {
    let size = 0;
    if (this.slots.includes(slot)) {
        size = slotIsEmpty(slot) ? 2 : 1;
        if (slot === "head") {
            size += 2 * isCoveringHead(this.head);
        }
    }

    return size;
}

function slotIsEmpty(slot) {
    return this.[slot] === null || this.[slot] === undefined;
}

Now you've separated out your logic for determining the size of everything instead of having to jam it into one function.

An even better approach may be to OOP this, and create objects representing a slot (or a specific kind of slot) and then each one should have a getSize method. Without knowing more about your code it is hard to say whether or not that would be overkill.


Project Euler #12 - highly divisible triangular number The following is a solution to Project Euler problem number 12: which is to find the first triangular number with more than 500 divisors.

The code takes far too long to complete (over 10 minutes). Is there any way to optimize the run-time and to make it more elegant?

#include <iostream>
#include <chrono>
#include <ctime>

inline void triangle(int &num, int &interval)
{
    num = ((interval * interval) + interval ) / 2;
}

int main()
{
    long int input;
    int num = 1, interval = 1, divisor = 1, no_divisor = 0;

    std::cout << "Enter factor limit for Triangle number: ";
    std::cin >> input;

    auto start = std::chrono::steady_clock::now();

    for (;;)
    {
        while (divisor <= num)
        {
            if (0 == num % divisor) //check factor;
            {
                std::cout << "factor: " << divisor << "\n";
                no_divisor++;
                if (no_divisor > input) //divisor count over input number, jump to exit;
                {
                    goto exit;
                }
            }
            divisor++;
        }
        std::cout << "\n";
        interval++;
        triangle(num, interval);
        divisor = 1; //reset divisor back to 1;
        no_divisor = 0; //reset divisor count back to 0;
    }
    exit: std::cout << "the first Triangle number to have over " << input << " divisors : " << num << "\n";

    auto end = std::chrono::steady_clock::now();
    auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(end - start);

    std::cout << "\nIt took me " << elapsed.count() << " milliseconds." << "\n";

    return 0;
}

Optimization I've done:

using std::cout << "\n" instead of std::cout << endl to prevent flushing output buffers. Source: this
using inline void triangle(int &num, int &interval) speed up 10% time
using n(n+1)/2 according to this

Speculation:

I figured if the question asked for over 500 factors, which for example:
Triangular number 28 has over 5 factors:

So I only need to iterate up to half of the limit. But how can I implement that into my code to only iterate up to half as many factor?

Project Euler problems typically don't benefit from micro-optimizations such as inlining functions or flushing standard output. The biggest speed gains often come from algorithmic improvements. Here there are two key mathematical facts that can make this computation tractable:

A number with prime factorisation n = Product_i p_i ^ e_i (where p_i are primes and e_i their exponents) has a number of divisors equal to d = Product_i (e_i + 1).
A triangle number is equal to n * (n + 1) / 2. These factors have no prime factors in common and only one of them has a factor of two. This means that the number of divisors of  a triangle number can be factored in the number of the divisors of n and n+1.

In order to code this algorithm, a few more guidelines to writing better code:

write functions that take an int and return an int. Don't use Pascal like procedures by modifying a reference argument. This makes your code harder to reason about.
separate computation from I/O: an algorithm should ideally have no side effects, and be silent. If you want to query the result, just print that.

Below some working code:

#include <chrono>
#include <ctime>
#include <iostream>

int triangle(int n)
{
    return ((n + 1) * n) / 2;
}

// Math fact: a prime decomposition n = Prod_i p_i^e_i yields
// Prod_i (e_i + 1) different divisors
int num_divisors(int n)
{
    auto divisors = 1;  // 1 has one divisor
    {
        auto f = 2;
        auto count = 0;
        while (n % f == 0) {
            ++count;
            n /= f;     // divide by factor of 2
        }
        divisors *= (count + 1);
    }

    // <= n guarantees that a prime has 2 divisors
    for (auto f = 3; f <= n; f += 2) {
        auto count = 0;
        while (n % f == 0) {
            ++count;
            n /= f;     // divide by odd factor
        }
        divisors *= (count + 1);
    }
    return divisors;
}

int unique_divisors(int n)
{
    if (n % 2 == 0)
        n /= 2;
    return num_divisors(n);
}

// triangle(n) = (n + 1) * n / 2
// only one of them has a factor of two, and they have no prime factors in common
// we can cache one of the divisor counts while iterating over triangle numbers
int first_triangle_with_more_than_n_divisors(int n)
{
    auto i = 1;
    for (auto f1 = unique_divisors(i), f2 = unique_divisors(i + 1); f1 * f2 <= n; ++i)   {
        f1 = f2;                        // re-use result from previous iteration
        f2 = unique_divisors(i + 2);    // only one new computation
    }
    return i;
}

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

    auto const n = 500;
    auto res = first_triangle_with_more_than_n_divisors(n);

    std::cout << "the first Triangle number to have over " << n << " divisors : " << triangle(res) << "\n";

    auto stop = std::chrono::steady_clock::now();
    auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(stop - start);

    std::cout << "\nIt took me " << elapsed.count() << " milliseconds." << "\n";
}

Live Example that runs in less than 0.2 seconds.


How do I shorten my Rust code to get integer values from regex named capture groups? I am completely new to rust. My first rust code is simple text filter application for parsing log file and accumulating some information. Here is my code:

//use std::io::{self, Read};
use std::io::{self};
use regex::Regex;

fn main() -> io::Result<()> {
    let re_str = concat!(
        r"^\s+(?P<qrw1>\d+)\|(?P<qrw2>\d+)",//qrw 0|0
        r"\s+(?P<arw1>\d+)\|(?P<arw2>\d+)",//arw 34|118
        );
    let re = Regex::new(re_str).unwrap();
    //let mut buffer = String::new();
    //io::stdin().read_to_string(&mut buffer)?;
    let buffer = " 0|1 2|3\n 4|5 6|7\n 8|9 10|11\n";

    let mut lines_skipped = 0;

    let mut m_qrw1:i32 = 0;
    let mut m_qrw2:i32 = 0;
    let mut m_arw1:i32 = 0;
    let mut m_arw2:i32 = 0;

    for line in buffer.lines() {
        match re.captures(line) {
            Some(caps) => {
                // I would like to shorten these long lines =>
                let qrw1 = caps.name("qrw1").unwrap().as_str().parse::<i32>().unwrap();
                let qrw2 = caps.name("qrw2").unwrap().as_str().parse::<i32>().unwrap();
                let arw1 = caps.name("arw1").unwrap().as_str().parse::<i32>().unwrap();
                let arw2 = caps.name("arw2").unwrap().as_str().parse::<i32>().unwrap();
                if qrw1 > m_qrw1 {m_qrw1 = qrw1}
                if qrw2 > m_qrw2 {m_qrw2 = qrw2}
                if arw1 > m_arw1 {m_arw1 = arw1}
                if arw2 > m_arw2 {m_arw2 = arw2}
            }
            None => {
                lines_skipped = lines_skipped + 1;
            }
        }
    }
    println!("max qrw1: {:.2}", m_qrw1);
    println!("max qrw2: {:.2}", m_qrw2);
    println!("max arw1: {:.2}", m_arw1);
    println!("max arw2: {:.2}", m_arw2);

    Ok(())
}

Playground

This works as expected, but I think those long chained calls which I created to get integer values of regex named capture groups are a bit ugly. How do I make them shorter/more in idiomatic rust style? I've got an advice to use ? operator instead of unwrap calls but I'm not sure how to apply it in this case.

regex::Captures provides a handy implementation of Index<&str>. This lets you pull named matches out with caps[name]. Combine that with a few std APIs and you can write the same code like this:

use regex::Regex;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let re = Regex::new(concat!(
        r"^\s+(?P<qrw1>\d+)\|(?P<qrw2>\d+)",
        r"\s+(?P<arw1>\d+)\|(?P<arw2>\d+)",
    ))
    .unwrap();
    let names = ["qrw1", "qrw2", "arw1", "arw2"];
    let buffer = " 0|1 2|3\n 4|5 6|7\n 8|9 10|11\n";

    let mut maximums = [0i32; 4];

    for caps in buffer.lines().filter_map(|line| re.captures(line)) {
        for (&name, max) in names.iter().zip(&mut maximums) {
            *max = std::cmp::max(*max, caps[name].parse()?);
        }
    }
    for (&name, max) in names.iter().zip(&maximums) {
        println!("max {}: {:.2}", name, max);
    }

    Ok(())
}
```


Matrix rotation algorithm This is the Matrix Rotation problem from hackerrank.com.

  You are given a 2D matrix, \$a\$, of dimension \$M×N\$ and a positive integer
  \$R\$. You have to rotate the matrix R times and print the resultant
  matrix. Rotation should be in anti-clockwise direction.

  Rotation of a \$4×5\$ matrix is represented by the following figure. Note
  that in one rotation, you have to shift elements by one step only
  (refer sample tests for more clarity).

  It is guaranteed that the minimum of \$M\$ and \$N\$ will be even.

  Input

  First line contains three space separated integers, \$M\$, \$N\$ and \$R\$, where \$M\$ is the number of rows, \$N\$ is number of columns in matrix, and \$R\$ is the number of times the matrix has to be rotated. Then \$M\$ lines follow, where each line contains \$N\$ space separated positive integers. These \$M\$ lines represent the matrix.

  Output

  Print the rotated matrix.

  Constraints

  \$2 \le M, N \le 300\$
  \$1 \le R \le 10^9\$
  \$\min(M, N) ≡ 0 \pmod 2\$
  \$1 \le a_{ij} \le 10^8\$, where \$i \in [1\ldots M]\$ and \$j \in [1\ldots N]\$

  Sample Input #00

4 4 1
1 2 3 4
5 6 7 8
9 10 11 12
13 14 15 16

  Sample Output #00

2 3 4 8
1 7 11 12
5 6 10 16
9 13 14 15

  Sample Input #01

4 4 2
1 2 3 4
5 6 7 8
9 10 11 12
13 14 15 16

  Sample Output #01

3 4 8 12
2 11 10 16
1 7 6 15
5 9 13 14

Following is how I tried to solve the problem, but it runs just too slow:

from copy import deepcopy

aM, aN, R = map(int, input().split())

pivots = min(aM, aN)//2
refMat = []
baseMat = []

for i in range(aM):
    readInput = list(map(int, input().split()))
    baseMat.append(readInput)

refMat = deepcopy(baseMat)

for i in range(pivots):
    cLimit = (aN - 1) - i
    rLimit = (aM - 1) - i
    loopLength = (aM + aN - 2 - 4*i)*2
    nbrOfRotations = R%loopLength

    for rotnIndex in range(nbrOfRotations):

        # Corner movements
        # Pivot
        refMat[i][i] = baseMat[i][i + 1]
        # Column End
        refMat[i][cLimit] = baseMat[i + 1][cLimit]
        # Row End
        refMat[rLimit][i] = baseMat[rLimit - 1][i]
        # Pivot diagonal
        refMat[rLimit][cLimit] = baseMat[rLimit][cLimit - 1]

        # Top movement
        for j in range(i+1, cLimit):
            refMat[i][j] = baseMat[i][j + 1]

        # Bottom movement
        for j in range(i+1, cLimit):
            refMat[rLimit][j] = baseMat[rLimit][j - 1]

        # Left movement
        for j in range(i+1, rLimit):
            refMat[j][i] = baseMat[j - 1][i]

        # Right movement
        for j in range(i+1, rLimit):
            refMat[j][cLimit] = baseMat[j + 1][cLimit]

        baseMat = deepcopy(refMat)

for i in refMat:
    for e in i:
        print(e, end = " ")
    print()

Note: No advanced libraries such as NumPy, SciPy etc. are allowed. However, I would love to know if they offer a better workaround.

Much better algorithm in my view:

Consider first matrix example

1   2  3  4
5   6  7  8
9  10 11 12
13 14 15 16

R=rotations

convert in 2 circles (at least either M or N is even, thus there is no single center point):

c1 = [1,2,3,4,8,12,16,15,14,13,9,5]
c2 = [6,7,11,10]

for each position calculate actual value:

c1’[i] = c1[(i+R)%c1.length]
c2’[i] = c2[(i+R)%c2.length]
…

The number of circles is min(M, N)/2 hence this strange constraint in the task :)

This solves in linear \$O(M+N)\$ complexity rather than your cubic \$O(M\cdot N\cdot R)\$ complexity.

EDIT: of course revert the circles back to the matrix format.


Reading HTTP response from UART I'm using a Telit GE864-GPS modem, which runs a dialect of Python 1.5.2. The Python module has access to the modem using a buffered UART, which may still be receiving when reading starts. The following code makes sure the entire response is read, and return either the response body or '0' on timeout.

def receiveReponse ( ):

    # MOD.secCounter() provides the current time in seconds
    timeout = MOD.secCounter() + 10

    str = ""
    length = ""
    newlinepos = 0

    while ( MOD.secCounter() < timeout ):

        newlinepos = str.find("\n\r")

        if ( (newlinepos != -1) and not length ):

            newlinepos = newlinepos + 2
            pos = str.find("Content-Length:") + 15

            while ( str[pos] != '\n' ):
                length = "%s%s" % (length, str[pos])
                pos = pos + 1

            length = int(length) + newlinepos

        else:
            MOD.sleep(5)

            # MDM receive reads the current UART buffer
            str = str + MDM.receive(1)

        if ( length and len(str) >= length ):
            return str[newlinepos:(newlinepos+length)]

    return 0

The entire procedure seems rather overcomplicated. Am I missing some obvious simplifications?

This

        while ( str[pos] != '\n' ):
            length = "%s%s" % (length, str[pos])
            pos = pos + 1

could be just

        length = string.split(str[pos:], '\n')[0]


Text based game in Java To help with learning code in my class, I've been working on this text based game to keep myself coding (almost) every day. I have a class called BasicUnit, and in it I have methods to create a custom class. I use 2 methods for this, allowing the user to enter the information for the class. I'm just wondering if I can do this in a more simplified manner?

    public void buildCustomClass(int maxHP, int maxMP, int maxSP, int baseMeleeDmg, int baseSpellDmg, int baseAC, int baseSpeed) {

    this.maxHP = maxHP;
    this.maxMP = maxMP;
    this.maxSP = maxSP;
    this.baseMeleeDmg = baseMeleeDmg;
    this.baseSpellDmg = baseSpellDmg;
    this.baseAC = baseAC;
    this.baseSpeed = baseSpeed;
    lvl = 1;
    xp = 0;
    curHP = maxHP;
    curMP = maxMP;
    curSP = maxSP;

}

public void createCustomClass() {
    kb = new Scanner(System.in);

    System.out.println("Enter the information for your class: ");
    System.out.println("Enter HP: ");
    maxHP = kb.nextInt();
    System.out.println("Enter MP: ");
    maxMP = kb.nextInt();
    System.out.println("Enter SP: ");
    maxSP = kb.nextInt();
    System.out.println("Enter Base Melee Damage: ");
    baseMeleeDmg = kb.nextInt();
    System.out.println("Enter Base Spell Damage: ");
    baseSpellDmg = kb.nextInt();
    System.out.println("Enter AC: ");
    baseAC = kb.nextInt();
    System.out.println("Enter Speed: ");
    baseSpeed = kb.nextInt();

    buildCustomClass(maxHP, maxMP, maxSP, baseMeleeDmg, baseSpellDmg, baseAC, baseSpeed);
}

Welcome to Code Review and thanks for sharing your code!

General issues

Naming

Finding good names is the hardest part in programming. So always take your time to think carefully of your identifier names.

Naming Conventions

It looks like you already know the
Java Naming Conventions.

Avoid abbreviations

In your code you use some abbreviations such as  maxSP and baseMeleeDmg.
Although this abbreviation makes sense to you (now) anyone reading your code being not familiar with the problem (like me) has a hard time finding out what this means.

If you do this to save typing work: remember that you way more often read your code than actually typing something. Also for Java you have good IDE support with code completion so that you most likely type a long identifier only once and later on select it from the IDEs code completion proposals.

  The other identifiers make perfect sense to me. [because of context] – AJD

Context looks like your friend but in fact it is your enemy.

There are two reasons:

Context depends on knowlegde and experience. But different persons have different knowlegde and experience, so for one it might be easy to remember the context and for another it might be hard. Also your own knowledge and experiences change over time so you might find it hard to remember the context of any given code snipped you wrote when you come back to it in 3 in years or even 3 moth.

The point is that in any case you need (more or less) time to bring the context back to you brain.
But the only thing that you usually don't have when you do programming as a business ist time. So anything that makes you faster in understanding the code is a money worth benefit. And not needing to remember any context is such a time saver.
You may argue that we have a very simple promblem with an easy and common context. Thats true.
But:

Real life projects usually have higher complexity and less eays to remember contexts. The point here is:
At which point is your context so complext that you switch from "acronym naming" to "verbose nameing"?

Again this point changes with you knowledge and your experience wich may lead to code that others have a hard time to understand.

The much better way to deal with it is to always white your code in a way that the dumbest person you know may be able to understand it. And this includes not to use akronyms in your identifiers that might need context to understand.
This is a training project. When you train a physical skill like Highjumping you start with a very low bar that you can easily pass even not using the flop technique just to have a safe environment.

Same is here: The problem may be simple enough to be understood having the acronymed identifiers, but for the sake of training you should avoid acronyms.

Avoid misleading naming

Both of your method names are misleading: They claim to create and build something but in reality neither one is creating or building anything.

One method is doing user interaction and the other is configuring the object.

The names of the methods should reflect that.

Add units to identifiers for physical values

Physical values do mean nothing without a unit. This is a special case of the context problem mentioned above.  The most famous example is the fail of the two space missions Mars Climate Orbiter and Mars Polar Lander. The Flight control software was build by NASA and worked with metric measurement (i.e. meters, meters per second or newton seconds) while the engine (and their driver software) has been build by Lockheed Martin which use imperial units (i.e. feet, feet per minute or Pound-force second).

The point is: not having units of physical values in your identifiers forces you to think if there is a problem or not:

double acceleration = flightManagement.calculateAcceleration();
engine.accelerate(acceleration);

But usually you don't quesion it unless you have a reason...

Having the units in the identifier names the problem becomes obvious:

double meterPerSquareSeconds =
          flightManagement.calculateAccelerationInMeterPerSquareSecond();
engine.accelerateByFeedPerSquareMinute(
          meterPerSquareSeconds); // oops

And we have the same argument again: your particular code is so easy and so small that we don't need the overhead.
But then: How do you decide at which point the overhead is needed? And again it depends on knowledge and experience which still are different among people...

Flawed implementation

Both methods using the same member variables.
E.g. you variable   maxSP: in createCustomClass() you assign it the result of kb.nextInt(). Then you pass this value as parameter to buildCustomClass() where you again assign the parameter value  again  to the same  member variable.

Beside this being useless it may lead to confusing bugs later.

keep same level of abstraction

Methods should either do "primitive" opeerations or call other methods, not both at the same time.

At the end of your method createCustomClass() you call the other method (buildCustomClass()). The better way to do this is to extract the code before the call to buildCustomClass() in a separate (private) method:

public void createCustomClass() {
    aquireDataFromUser();
    buildCustomClass(
          maxHP,
          maxMP,
          maxSP,
          baseMeleeDmg,
          baseSpellDmg,
          baseAC,
          baseSpeed);
}

private void aquireDataFromUser() {
    kb = new Scanner(System.in);

    System.out.println("Enter the information for your class: ");
    System.out.println("Enter HP: ");
    maxHP = kb.nextInt();
    System.out.println("Enter MP: ");
    maxMP = kb.nextInt();
    System.out.println("Enter SP: ");
    maxSP = kb.nextInt();
    System.out.println("Enter Base Melee Damage: ");
    baseMeleeDmg = kb.nextInt();
    System.out.println("Enter Base Spell Damage: ");
    baseSpellDmg = kb.nextInt();
    System.out.println("Enter AC: ");
    baseAC = kb.nextInt();
    System.out.println("Enter Speed: ");
    baseSpeed = kb.nextInt();
}

Beside making createCustomClass() shorter it makes the useless reassingment of the member variables obvious.