Slow Huffman Code in pure Python I was working on writing a fast implementation of a simple Huffman code compression of text. The idea was to write it using only the standard library, but I can't seem to find a way to make it faster. I am also looking for advise on how to write it more "Pythonic", without sacrificing speed.
I am aware that if I want speed I shouldn't use Python, but I've taken it as an exercise to test pure Python performance.
from collections import Counter, defaultdict

def huffman_compress(input_file, output_file, encoding='utf8'):
    """This functions compresses a txt file using Huffman code compression."""

    # Store the text in memory since it is faster than reading twice
    text = open(input_file, "r", encoding=encoding).read()

    # Count the times each letter appears on the text
    letter_freq = Counter(text)
    alphabet = defaultdict(str)

    # Obtain the huffman code for each letter
    while len(letter_freq) > 1:
        (letter1, count1), (letter2, count2) = letter_freq.most_common(2)
        letter_freq[letter1+letter2] = count1 + count2
        for bit, combination in enumerate([letter1, letter2]):
            for letter in combination:
                alphabet[letter] = str(bit) + alphabet[letter]
            del letter_freq[combination]

    # Save the transformation to ascii for possible the 256 characters
    bit_to_ascii = {format(x, '08b'): chr(x) for x in range(256)}

    with open(output_file, 'w') as output:
        # Transform each letter to its huffman code
        me = ''.join(alphabet[ch] for ch in text)

        # Add 0's so that the string is multiple of 8
        extra_bits = 8 - len(me) % 8
        me +=  extra_bits * '0'

        # Write the number of letters compressed and the number of bits added
        output.write(f'{chr(len(alphabet))}{extra_bits}')

        # Write the letters compressed and their huffman code for the decompression
        output.write('|'.join(c for item in alphabet.items() for c in item))

        # Transform the huffman bits to ascii and save them on the compressed file.
        output.write(''.join(bit_to_ascii[me[j:j+8]] for j in range(0, len(me), 8)))

I started with your code, added sys.argv so I could pass file paths on the
command line, downloaded a big text file (War and Peace, of course), ran your
program, and checked files sizes:
$ curl 'https://www.gutenberg.org/files/2600/2600-0.txt' -o war-peace.txt -k

$ time python huffman.py war-peace.txt encoded

real    0m11.052s
user    0m10.462s
sys 0m0.389s

$ ls -lh
-rw-r--r-- 1 fmc staff  40M Aug 24 13:51 encoded
-rw-r--r-- 1 fmc staff 3.3M Aug 24 13:50 war-peace.txt

Looks like you have inadvertently invented an expansion algorithm: it creates a
file roughly 12x bigger! Also, 11 seconds seems slow to process a meager 40M of
text. Normally Python can crunch data of that size much more quickly.
I temporarily assigned a short string (huffman) to the text variable,
bypassing file reading, and printed out some of your intermediate variables.
Although letter_freq looked fine, alphabet was the opposite of what we
want:
f 00000     # The most frequent letter has the longest code.
h 00001
u 0001
m 001
a 01
n 1

The Huffman algorithm combines the 2 elements with the least common
frequency, but you are doing the opposite. So I tweaked your code like this:
(letter1, count1), (letter2, count2) = letter_freq.most_common()[:-3:-1]

With that change, alphabet at least looks more plausible, the output file
ends up being smaller than the input file (although not by as much as I expect,
so there are probably other problems in your code), and it finishes in about 1
second rather than 11 (most likely because it's writing a much smaller output
file).
Some suggestions:

Focus on correctness first. Worry about speed later -- and only if it
truly matters (and it might, if for no other reason that educational).

Algorithms and side effects don't mix. Reorganize your code to facilitate
testing and debugging. The huffman_compress() function itself should not
concern itself with file reading and writing. It should take a blob of text and
return a blob of bytes, period. Highly algorithmic code (as Huffman is) should
never have side effects; it should live in the realm of pure functions.

Roundtrip the data. Also write a huffman_expand() function: take bytes,
return text. Without that, you cannot have any confidence in the process. In
particular, you want to be able to do the following: assert original_text == huffman_expand(huffman_compress(original_text)). That doesn't prove that
you've correctly implemented Huffman (perhaps you will invent your own special
encoding scheme, which could be cool), but at least it will prove that you can make a lossless roundtrip.


Cautious File Writer I'm trying to follow the path of Sandi Metz and others who state that aiming for small and simple is good for writing understandable code. I've written a method that takes care of writing content to a file without overwriting any existing file of the same name. I wrote it TDD style and, once the tests passed, I refactored the rather large methods into smaller methods, hoping that it would improve readability.

I hope you can tell me whether I succeeded, or how I could further improve on my code. I've considered adding modules (such as a "Appender" module) to my class in order to improve the code according to the single responsibility principle, but can't figure out whether it would actually clarify things more, or whether the added lines would make the intended  clarification void.

class CautiousWriter
  def self.write(filename, content)
    if File.exists?(filename)
      rename_existing_file_beginning_with(filename)
    end
    create_file(filename, content)
  end

  private

  def self.rename_existing_file_beginning_with(filename)
    if File.exists? append_old_to(filename)
      rename_with_old_and_number_appended(filename)
    else
      rename_with_old_appended(filename)
    end
  end

  def self.rename_with_old_and_number_appended(filename)
    old_and_num_appended = find_available_filename(filename)
    rename(filename, old_and_num_appended)
  end

  def self.find_available_filename(filename)
    number = 0
    until File.exists?(append_old_to(filename) + number.to_s) == false
      number += 1
    end
    append_old_to(filename) + number.to_s
  end

  def self.rename_with_old_appended(filename)
    rename(filename, append_old_to(filename))
  end

  def self.rename(filename, new_filename)
    File.rename(filename, new_filename)
  end

  def self.create_file(filename, content)
    File.open(filename, 'w') { |file| file.write(content) }
  end

  def self.append_old_to(filename)
    filename + '_old'
  end
end

Before looking at the code, I have a comment on the overall functionality of it. The idea is nice, but it commits a cardinal sin: It messes with files that do not belong to it.

This is A Very Bad Idea™.

Say you want to write the file "log.txt", but it already exists. Your code will rename the existing file, but that file may be in use. Some other process might expect it to be there, and fail when it gets renamed. Or your process will fail if the existing file is owned by another user or is locked. Or the other process will just start overwriting your file, since, hey, that's the one with the correct name. Or you get weird race conditions where you rename a file, and immediately after that, another process - finding the file gone - creates a new one, after which you overwrite the other process' new file; exactly what you're trying not to do. (see user50399's answer for more)

In other words, I wouldn't use this, regardless of the code itself.

But speaking of code, let's look at that.

You've certainly been diligent in dividing everything into methods - kudos. But, as ckuhn203 notes, a method with and in it indicates that the method is in fact doing several things.

I have to wonder why you're using a two-stage method of resolving file name conflicts: One is to append "_old", and the other is to also append a number. Just appending "_old" may not be enough, whereas numbers can simply increment forever (more or less). So you have one way of renaming that may or may not work, and one which will always work. So why use both? It just complicates the code.

Besides, maybe you don't want "_old" in the filename. Maybe you want "-backup" or just nothing. Right now the string is hard coded in a method body, so it's not exactly configurable.

I don't see any handling of file extensions, or anything to indicate that it's been considered. Though not required, it could be nice to rename "file.txt" to "file_old4.txt" instead of "file.txt_old4". Especially since you're using an underscore, meaning that the renamed file's extension is suddenly ".txt_old4", which isn't a real extension.
However, dealing with file extensions is a huge hassle, so if you look at something like the logrotate *nix utility, it just appends a dot and a number, instead of trying to parse file extensions, so you get files like "log.txt.3". Note that it's not using an underscore; it's simply adding an entirely new extension that's all-numeric. While not perfect, it's less likely to confuse anyone.

There's also the question of why this is a class. All its methods are class methods, and it has no state. Yet making it a class implies you can instantiate it - but that doesn't really do anything. You basically just get an instance of Object with no discernible functionality. I'd probably prefer to write it as a module instead (here are a number of ways to do that).

On a micro-level, I noticed this:

until File.exists?(append_old_to(filename) + number.to_s) == false

Saying "until something's not true" is the same as just saying "while something's true". It's cool that Ruby has the until keyword, but in this case you want a good old-fashioned while loop.
You could also avoid calling append_old_to again and again, since its return value isn't going to change. And personally, I'd use string interpolation here

number = 0
base_filename = append_old_to(filename)
while File.exists?("#{base_filename}#{number}")
  ...

Incidentally, this single method may be the most useful, if you "flip it" to produce a new filename you can use instead. I.e. don't try to rename existing files: Make sure that you have a non-conflicting filename before you write anything. Again, there's a risk of race conditions, if you get the file name before writing, but it's less risky than renaming existing files.

You might simply monkey patch the IO class with an open_uniq (or some such) method, which you pass the usual name, mode and block, but which - instead of returning the number of bytes written - returns whatever filename it actually ended up writing. So you give it "log.txt" and some data to write, and it might return "log.4.txt" because there happened to be a few of log*.txt files already.


Reduce square root to simplest radical form The function redsqrt(n) prints out the most reduced form of the square root of n. It calls on another function perfsq(n), which produces a list of perfect squares less than or equal to n.

from math import sqrt
def perfsq(n):
    # Returns list of perfect squares less than or equal to n
    l = [1]
    i, x = 0, 3
    while l[-1]+x <= n:
        l.append(l[i]+x) # Add x to get next perfect square
        i += 1
        x = 2*i+3
    return l
def redsqrt(n):
    # Prints most reduced form of square root of n
    if n < 0:
        print('Negative input')
        return
    if sqrt(n).is_integer():
        print(int(sqrt(n))) # Square root is an integer
        return
    # Find perfect squares that are factors of n
    l = [i for i in perfsq(n/2) if n%i==0 and i>1]
    if len(l)==0:
        print('\u221A',n) # Square root is irreducible
    else:
        a = int(sqrt(max(l))) # Coefficient
        b = int(n/max(l)) # Argument of the square root
        print(a,'\u221A',b) # Reduced square root

Examples:

redsqrt(5.7)
√ 5.7

redsqrt(100)
10

redsqrt(2000)
20 √ 5

redsqrt(2040506)
13 √ 12074

Style

Give your code some love and help readers by adding blank lines to separate logical sections of code. It is recommended to use two blank lines before function definition.

Space after commas and around operators also help make the code more readable.

Some of your comments would be better suited as docstrings and some other barely state what you are doing, which we could read from the code already. Comment should focus on why instead.

Lastly using meaningful variable names play a large role in making your algorithm understandable. Avoid abreviations.

from math import sqrt

def perfect_square(limit):
    accumulation_list = [1]
    index, increment = 0, 3
    while accumulation_list[-1] + increment <= limit:
        accumulation_list.append(accumulation_list[iindex] + increment)
        index += 1
        increment = 2 * index + 3
    return accumulation_list

def reduced_sqrt(n):
    """Print most reduced form of square root of n"""

    if n < 0:
        print('Negative input')
        return

    if sqrt(n).is_integer():
        print(int(sqrt(n)))
        return

    # Find perfect squares that are factors of n
    factors = [square for square in perfect_square(n/2) if n % square == 0 and square > 1]
    if len(factors) == 0:
        print('\u221A', n) # Square root is irreducible
    else:
        a = int(sqrt(max(factors))) # Coefficient
        b = int(n / max(factors)) # Argument of the square root
        print(a, '\u221A', b) # Reduced square root

Unnecessary work

Why compute the perfect squares starting from 1 if you are going to filter this value later on?
Why compute all these values at all if you're only going to take the max of all factors?
You also compute sqrt(n) and max(factor) several time when you could store the result in a variable.
Most importantly, you compute the perfect squares rather expensively only to recompute its fairly expensive sqrt when you find one as factor.

Instead you could iterate from the nearest integer lower than sqrt(n) down to 1 and see if its square is a factor of n. Once found you have your factor without computing the remaining values. If none can be found, you know that you have an irreducible square root.

A first draft could be:

from math import sqrt

def reduced_sqrt(n):
    """Print most reduced form of square root of n"""

    if n < 0:
        print('Negative input')
        return

    root = int(sqrt(n))
    found = False

    for factor_root in range(root, 1, -1):
        factor = factor_root * factor_root
        if n % factor == 0:
            found = True
            reduced = n // factor
            if reduced == 1:
                # n was a perfect square
                print(factor_root)
            else:
                print(factor_root, '\u221A', reduced)
            break

    if not found:
        # irreducible square root
        print('\u221A', n)

Reusability

It is bad practice to encore computation and output in the same function as it makes it harder to test and reuse. You should instead return a couple "coefficient/reduced square root" so that the caller can handle the logic of pretty printing the output.

This also means you need better handling of you error case. The standard in Python being to raise an exception. Here raise ValueError('negative input') should be enough. But you don't even need that as sqrt of a negative number already raise a ValueError:

>>> sqrt(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: math domain error

Lastly, returning values let you short circuit your computation and remove the need of the found flag:

from math import sqrt

def reduced_sqrt(n):
    """Return most reduced form of square root
    of n as the couple (coefficient, reduced_form)
    """

    root = int(sqrt(n))

    for factor_root in range(root, 1, -1):
        factor = factor_root * factor_root
        if n % factor == 0:
            reduced = n // factor
            return (factor_root, reduced)

    return (1, n)

def print_reduced_sqrt(n):
    coefficient, reduced = reduced_sqrt(n)
    if coefficient == 1:
        print('\u221A', reduced)
    elif reduced == 1:
        print(coefficient)
    else:
        print(coefficient, '\u221A', reduced)


Financial website scraper I wrote the code that would scrap Finviz for tickers and href that attach to the ticker. I tested with multi-page as well as one page. I'm new to python programming and therefore believe my code is terrible. I'm hoping someone would help me review it and point out ways to improve it. My goal is to have the code more scalable than what I have now.

import bs4 as bs
import requests

class finviz_crawler():
    def requesting(self,type):
        items=1
        baseurl='https://finviz.com/screener.ashx?v=111'
        if type == 'sp500':
            firstpage=baseurl+'&f=idx_sp500&o=ticker'
        elif type == "China":
            firstpage=baseurl+'&f=cap_largeover,geo_china'
        finalurl=firstpage
        headers = {'User-Agent':'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36'}
        resp=requests.get(finalurl,headers=headers)
        soup = bs.BeautifulSoup(resp.text,"lxml")
        table=soup.findAll('td',{'class':'screener-body-table-nw'})
        maxpage_a_tag=[]
        maxpage_td=soup.findAll('td',{'class':'body-table'},bgcolor="#ffffff",valign="top",width="100%")
        tickerdict={}
        for tags in maxpage_td[0].findAll('a'):
             maxpage_a_tag.append(tags)
        if not maxpage_a_tag:
            maxpage=1
        elif (maxpage_a_tag[-1].text=="next"):
            maxpage=maxpage_a_tag[-2].text
        else:
            maxpage=maxpage_a_tag[-1].text
        for page in range(int(maxpage)):
            resp=requests.get(finalurl,headers=headers)
            soup = bs.BeautifulSoup(resp.text,"lxml")
            table=soup.findAll('td',{'class':'screener-body-table-nw'})
            for row in table:
                 ticker_rows=row.findAll('a',{'class':'screener-link-primary'},href=True)
                 for tickers in ticker_rows:
                     tickerdict[tickers.text]=tickers['href']
            items+=20
            finalurl="{}{}{}".format(firstpage,"&r=",str(items))
        print(tickerdict)

Call the class using finviz_crawler().requesting('China')

Here a few suggestions (in no particular order) for improvements on your code:

Make variables have locality

The line:

items = 1

is at the top of the method, but is only used by the loop at the bottom of the method.  It is likely better located just in front of the loop that increments it.

Same applies to:

tickerdict = {}

You can break strings across multiple lines:

This:

headers = {
    'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36'}

Can be:

headers = {
    'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) '
    'AppleWebKit/537.36 (KHTML, like Gecko) '
    'Chrome/56.0.2924.87 Safari/537.36'
}

You often don't need loops:

for tags in maxpage_td[0].findAll('a'):
    maxpage_a_tag.append(tags)

likely works just fine as:

maxpage_a_tag.extend(maxpage_td[0].findAll('a'))

And then this:

maxpage_a_tag = []
maxpage_a_tag.extend(maxpage_td[0].findAll('a'))

is really just:

maxpage_a_tag = list(maxpage_td[0].findAll('a'))

Don't use python names

The name type is used by Python.  Suggest instead changing to type_ or better yet, as suggested from comments, something more descriptive, like request_type.

Reworked code:

Here is the reworked code with the above suggestions, and maybe a few others that snuck in.

class finviz_crawler():

    def requesting(self, type_):
        baseurl = 'https://finviz.com/screener.ashx?v=111'
        if type_ == 'sp500':
            firstpage = baseurl + '&f=idx_sp500&o=ticker'
        elif type_ == "China":
            firstpage = baseurl + '&f=cap_largeover,geo_china'
        else:
            raise ValueError("Unknown type {}".format(type_))
        finalurl = firstpage
        headers = {
            'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) '
                          'AppleWebKit/537.36 (KHTML, like Gecko) '
                          'Chrome/56.0.2924.87 Safari/537.36'}
        resp = requests.get(finalurl, headers=headers)
        soup = bs.BeautifulSoup(resp.text, "lxml")
        maxpage_td = soup.findAll('td', {'class': 'body-table'},
                                  bgcolor="#ffffff", valign="top",
                                  width="100%")
        maxpage_a_tag = list(maxpage_td[0].findAll('a'))
        if not maxpage_a_tag:
            maxpage = 1
        elif (maxpage_a_tag[-1].text == "next"):
            maxpage = maxpage_a_tag[-2].text
        else:
            maxpage = maxpage_a_tag[-1].text

        items = 1
        tickerdict = {}
        for page in range(int(maxpage)):
            resp = requests.get(finalurl, headers=headers)
            soup = bs.BeautifulSoup(resp.text, "lxml")
            table = soup.findAll('td', {'class': 'screener-body-table-nw'})
            find_all_args = 'a', {'class': 'screener-link-primary'}
            for row in table:
                tickerdict.update(
                    {tickers.text: tickers['href']
                     for tickers in row.findAll(*find_all_args, href=True)})
                items += 20
            finalurl = "{}{}{}".format(firstpage, "&r=", str(items))

        for data in tickerdict.items():
            print(data)

finviz_crawler().requesting('China')


Improving PHP MVC code for personal website Github for project: https://github.com/wrpd/portfolio

I have a very general understanding of MVC but haven't ever implemented it. In an effort to improve my understanding and get back into a little bit of web development I started watching and reading some tutorials and started to implement a basic MVC to redesign my personal website on.

I'm looking for critique on my current code. How can I make it more elegant?
What am I misunderstanding about MVC basics? Can I make things functionally more concise (In the code itself. For variable names and such I'm keeping it purposely verbose to aid in understanding for myself)?

Specifically, I'm wondering if my project structure is okay and how to better handle routing and initializing? My routes are currently set with a routing class like so:

<?php

router::set_route('/', function(){

    index_controller::create_view('index');

});

router::set_route('/about', function(){

    about_controller::create_view('about');

});

router::set_route('/projects', function(){

    projects_controller::create_view('projects');

});

router::set_route('/contact', function(){

    contact_controller::create_view('contact');

});

?>

Is this an optimal way of routing to my pages?

My init is currently autoloading my classes and requiring my routes. Like so:

<?php

function autoload($class_name){

    if (file_exists('./app/core/classes/'. $class_name . '.php')){

        include './app/core/classes/'. $class_name . '.php';

    } else if (file_exists('./app/controllers/' . $class_name . '.php')){

        include './app/controllers/' . $class_name . '.php';

    } else if (file_exists('./app/models/' . $class_name . '.php')){

        include './app/models/' . $class_name . '.php';

    }
}

spl_autoload_register(autoload);

require_once './app/core/routes/routes.php';
?>

Should it be a class that I instantiate in index.php or is that sufficient? I like that index.php can just contain

require_once '/path/to/init.php'

but I don't know if that's the best practice.

Essentially, I'd like to make the foundation for my personal site as solid as I can while following best practice. I've seen a lot of code examples and samples but some are quite dated and my depth of knowledge isn't deep enough to know if I'm doing things the best way that I can.

If you want to learn best practices in MVC, I would suggest you to have Slim framework. If you don't want to use an existing framework and develop your own, still, I would recommend to install it on your local and understand how they are doing. Laravel is another best, but it would be overkill for a personal site.

Check their Routing example. You are almost close in your routing, but if you want to know the best practices, they have it: http://www.slimframework.com/docs/v4/objects/routing.html

In your example, I would include init.php and routes.php inside of index.php to keep it clean and declarative. Your init may eventually grow and it would become clumsy.


Accessing a global list I'm sort of at a loss for what to do with webapps and whether what I'm doing is actually allowed or not. Here is a sample of the type of thing I'm doing at the moment. I need a list that is accessible by essentially everything.

global_list = []

@bottle.route('/')
def index():
    global_list.append(4)

@bottle.route('/test')
def test():
    return len(global_list)

@bottle.run(port='8080')

The problem (or what I think is a problem) is that it feels wrong and I've been told it is unsafe to use a global variable. This code works perfectly fine, but is there a better or safer way to handle this? Even with multiple clients at the same time, the data in the list currently does not get corrupted. What kind of issues am I going to run into if I keep using this?

Ah, the age old pragmatism vs. purism. While it sounds like you have a healthy amount of skepticism, I think this is actually one of the few instances where it's OK to use a global variable. The core of your problem is you need to share data (state) between two routes. A global variable is the simplest way of doing this if all you're doing is appending the number 4 each time.

Here's some things to consider:

Security: You mentioned security. Will any sensitive information be stored in global_list? If so, you should consider putting safegards in place (authentication and authorization) to make sure only the right people see the appropriate information. Also, if you store user input to the variable, be very careful how you use the data (SQL injection).
Persistence: Will state need to be maintained across webserver restarts? If everything is in a Python variable, it will be lost when you shut down the process. You may want to persist the data to a filesystem or database.
Resources: Are you going to be storing lots of information in this global variable? If so, the process may run out of memory, because the global variable is never garbage collected. The solution here is to store the data somewhere and generate aggregate data to use in the app instead (or do pagination).
Shared-state side effects: Anytime you share data, you have to be conscious of side effects caused by altering this data. If two routes are reading/writing from the same variable, one route could do something that breaks the other route.

More on resources:

For instance, if you store 1KB every time the / route is loaded and get 1M visitors a month, you'd be up to 1GB of memory just for global_list in around 1 month. 1KB is a lot of data, and you probably don't need all of this information to do your calculation in /test. Instead, you could aggregate the data by using global_list as a running total for the full set of data stored elsewhere (a database or filesystem).

If you're doing more than simply presenting a count of records in /test, and you need whole (or partial) records, then you can do pagination. Let's say you're presenting a list of the records in global_list. In this case, /test could simply return the first 10 records by default, but allow the user to see more pages by providing a page parameter. So if the user wants to see records 31-40, they could request /test?page=4


Dynamically set angular directive template I have recently upgraded to AngularJS 1.5 and as the title says, I want to use different templates depending on a value in the item object passed.

After some research I found multiple ways to accomplish my task and I have picked out two of the (many) ways I found. Since I'm still quite new at Angular I was curious about the best way to do it performance wise.

1st way

angular.module('MyModule').directive('myDirective',
  ['$templateRequest', '$compile', function ($templateRequest, $compile) {
    return {
        restrict: 'EA',
        replace: true,
        scope: {
            item: '='
        },
        link: function (scope, element, attr) {
            if (scope.item.component != null) {
                scope.template = 'js/MyModule/directives/my-directive/my-component.html';
            } else if (scope.item.question != null) {
                scope.template = 'js/MyModule/directives/my-directive/my-question.html';
            }

            $templateRequest(scope.template)
                .then(function(html){
                    var template = angular.element(html);
                    element.append(template);
                    $compile(template)(scope);
                });
        }
    };
}]);

2nd way

angular.module('Testview').directive('learningPlayerBody', function () {
    return {
        restrict: 'EA',
        replace: true,
        template: '

',
        scope: {
            item: '='
        },
        link: function (scope, element, attr) {
            scope.getTemplateUrl = function() {
                if (scope.item.component != null) {
                    scope.template = 'js/MyModule/directives/my-directive/my-component.html';
                } else if (scope.item.question != null) {
                    scope.template = 'js/MyModule/directives/my-directive/my-question.html';
                }
            };
        }
    };
});

To my opinion your 2nd way which uses ng-include is better because as mentioned in angular doc it says that-

  The $templateRequest service runs security checks then downloads the provided template using $http and, upon success, stores the contents inside of $templateCache. If the HTTP request fails or the response data of the HTTP request is empty, a $compile error will be thrown.

Although $templateRequest factory that was added in v3.0. $templateRequest function checks and saves to the $templateCache.

Another way you can use

templateUrl: function(element, attributes) {
  if (element[0].tagName === 'INPUT') {
    return 'templates/input.html';
  } else {
    return 'templates/default.html';
  }
}


Is there a better way to avoid repeated 'Len == 1' when interpreting booleans from strings? My solution to string interpretation is pretty fast. However, I worry about repeated if (Len == 1). That might make the function bigger in the executable. How can I improve my function, to not be as redundant while checking if Len is equal to 1?
#include <ctype.h>
signed char BoolFromStr(const char *const StrIn, register const unsigned char Len) {
    if (!Len || Len > 5 || !StrIn) {
        return -1;
    }
    switch (tolower(*StrIn)) {
        case '0':
            if (Len == 1) {
                return 0;
            }
            break;
        case 'f':
            if (Len == 1 || (Len == 5 && !memcmp(Str+1, (const char[]){'a', 'l', 's', 'e'}, 4))) {
                return 0;
            }
            break;
        case 'n':
            if (Len == 1 || (Len == 2 && Str[1] == 'o')) {
                return 0;
            }
            break;
        case '1':
            if (Len == 1) {
                return 1;
            }
        case 'y':
            if (Len == 1 || (Len == 3 && !memcmp(Str+1, (const char[]){'e', 's'}, 2))) {
                return 1;
            }
            break;
        case 't':
            if (Len == 1 || (Len == 4 && !memcmp(Str+1, (const char[]){'r', 'u', 'e'}, 3))) {
                return 1;
            }
            break;
    }
    return -1;
}

How can I improve my function, to not be as redundant while checking if Len is equal to 1?

Make the null character part of the check. Len not needed anywhere.
// if (Len == 1 || (Len == 5 && !memcmp(Str+1, (const char[]){'a', 'l', 's', 'e'}, 4))) {

if (StrIn[1]==0 || !memcmp(Str+1, (const char[]){'a', 'l', 's', 'e', 0}, 5))) {
// or
if (StrIn[1]==0 ||
    (StrIn[1]=='a' && StrIn[2]=='l' && StrIn[3]=='s' && StrIn[4]=='e' && StrIn[5]==0)) {

Bug
BoolFromStr(const char *const StrIn, register const unsigned char Len) is a wrong when the length of the string is say 257 and called as
const char *s = "ttt....ttt"; // 257 t's
BoolFromStr(s, strlen(s)); // reports 1 even though `s` is not `"t"`.

Recommend size_t and drop the register.  Do not tie the compilers hands.
BoolFromStr(const char *StrIn, size_t Len);

To reduce size and improve speed I'd consider a hashing of the first byte of StrIn
// Pseudo code

int BoolFromStr(const char *StrIn) {
  const char *tf[] = {"0", "1", "false", "true", "no", "yes", "", "" };
  unsigned hash = hashf(StrIn[0])%8;
  const char *cmps = tf[hash];
  if (StrIn[0] == cmps[0] && (StrIn[1] == 0 || (strcmp(StrIn, cmps) == 0))) {
    return hash & 1;
  }
}

See similar What's the fastest way to interpret a bool string into a number in C?.


Rust - insert into piecetable I'm writing a piece table library in Rust with the structures:
#[derive(Default)]
struct Piece {
    additional: bool,
    offset: usize,
    length: usize,
}

and
/// PieceTable contains the additional, original buffers and a vector of pieces.
/// It also maintains a length variable.
pub struct PieceTable {
    table: VecDeque<Piece>,
    orig_buffer: String,
    add_buffer: String,
    length: usize,
}

I'm fairly confident that I didn't mess anything up here, though I'm not sure if VecDeque is the best structure to hold the pieces. My insert method is:
impl PieceTable {
    ...
    pub fn insert(&mut self, index: usize, text: &string) {
        match self.piece_at(index + 1) {
            Ok((index, offset)) => {
                // isolate piece being inserted into
                let into = &self.table[index];
                // create necessary Pieces to enter
                let previous = Piece {
                    additional: into.additional,
                    offset: into.offset,
                    length: offset - into.offset,
                };
                let insert = Piece {
                    additional: true,
                    offset: self.add_buffer.len(),
                    length: text.len(),
                };
                let next = Piece {
                    additional: into.additional,
                    offset: offset,
                    length: into.length - (offset - into.offset),
                };

                // remove index, add in previous, insert, next
                self.table.remove(index);
                self.table.insert(index, previous);
                self.table.insert(index + 1, insert);
                self.table.insert(index + 2, next);

                // update buffer, length vars
                self.add_buffer.push_str(text);
                self.length += text.len();
           }
           Err(_) => self.push_str(text),
        }
    }
}

Which refers to the piece_at method:
fn piece_at(&self, index: usize) -> Result<(usize, usize>, i32> {
    if index > self.length {
        return Err(-1);
    }

    let mut remaining = index.clone();
    for (i, piece) in self.table.iter().enumerate() {
        if remaining <= piece.length {
            return Ok((i, remaining));
        }
        remaining -= piece.length;
    }

    return Err(-1);
}

It also relies on the push_str method, which is just appends a piece with the proper parameters to the end of self.table.
This algorithm is stolen basically verbatim from the python implementation at https://github.com/saiguy3/piece_table, so hopefully there's nothing terribly wrong with it that I missed. Rust is hard and I started learning it yesterday, so I'm fully expecting I horribly misused some language feature.
Thanks!

Keep in mind that there already exist rope structure implementations in Rust, which support utf8, for example an_rope, where your PieceTable::insert is Rope::insert_str: https://docs.rs/an-rope/0.3.1/an_rope/struct.Rope.html#method.insert_str
Your code is really good for someone who just started learning, and nothing stands out as a misuse of language.
Why is index + 1 passed to piece_at, not simply index?
Using VecDeque does give a little potential for speedup in random insertion and removal. Although if I was doing a program like this, my choice of the data structure would be different - I'd use BTreeSet<Piece> instead, and order the Pieces by piece.offset.
You may write a removal and an insert at the same index as a single [] = instead.
About Copy/Clone:

if your Piece derived Copy, the & in &self.table[index] could be omitted. Did you try compiling this code? Can it compile without errors? I doubt it since  the immutable borrow with & will prevent you from removal and insertion later on.
the .clone() in piece_at can be omitted, because piece: usize is Plain Old Data that is copied around with zero overhead (in syntax and in performance as well).

There are two tiny misspellings

text: &string instead of text: &str
(usize,usize>, instead of (usize,usize)>,

I'd change the Err(-1) return to carry the unit value instead as we don't need the i32 to signal anything. We return Err(()) there.


A python Hang Man Game can anyone give me advice on how i can make my code more compact and efficient, and if you can, an implementation using OOP? Since, i just started learning Object Oriented Programming and it looks real hard.

def Hangman(): # for MENU
    print("H A N G M A N")
    while True:
        choice = input('Type "play" to play the game, "exit" to quit: ')
        if choice == "play":
            hint("option")
            play_game()
        if choice == "exit":
            break

def play_game(): # FOr game mechanics
    import random
    my_list = ['python', 'java', 'kotlin', 'javascript']
    # WORD CHOICES
    global lives, chosen_word, changed_word, changed_list, hidden_word, hidden_list
    lives = 8
    winner = False
    chosen_word = random.choice(my_list)
    changed_word = chosen_word[:]
    hidden_word = (len(chosen_word))*"-"
    hidden_list = list((len(chosen_word))*"-")
    changed_list = [char for char in changed_word]

    while lives != 0:
        guess = "-"
        print('\n', hidden_word, sep="") # gives the word every guess

        while True:
            guess = input("Input a letter: ")
            if len(guess) != 1:
                print("You should print a single letter\n")
                continue
            if guess != guess.lower() or not guess.isalpha():
                print("It is not an ASCII lowercase letter\n")
                continue
            break

        if guess not in changed_word:
            if guess in chosen_word:
                print('You already typed this letter')
                continue

            else:
                print("No such letter in the word")
                lives -= 1
                hint("check_lives")

        else:
            letter_found_count = chosen_word.count(guess)

            for _ in range(letter_found_count):
                indexfound = changed_word.find(guess)
                hidden_list[indexfound] = guess
                changed_list[indexfound] = "-"
                hidden_word = "".join(hidden_list)
                changed_word = "".join(changed_list)

        if hidden_word == chosen_word:
            print("\n%s\nYou guessed the word!\nYou survived!" % hidden_word)
            winner = True
            break

    if not winner:
        print("You are hanged!")
    if hint_count > 1:
        print(f"Though you used {str(hint_count)} hints")
    print()

def hint(setting):
    if setting == "game":
        global chosen_word, changed_word, changed_list, hidden_word, hidden_list
        for _ in range(len(changed_list)):
            guess = changed_list[_]
            if guess != "-":
                letter_found_count = chosen_word.count(guess)
                for _ in range(letter_found_count):
                    indexfound = changed_word.find(guess)
                    hidden_list[indexfound] = guess
                    changed_list[indexfound] = "-"
                    hidden_word = "".join(hidden_list)
                    changed_word = "".join(changed_list)

                return changed_word, hidden_word, changed_list, hidden_list

    if setting == "option":
        global hint_flag, hint_count
        hint_count = 0
        while True:
            choice = input("With Hints? 'Yes' or 'No': ")
            if choice.lower() == "yes":
                hint_flag = True
                break
            elif choice.lower() == "no":
                hint_flag = False
                break

    if setting == "check_lives":
        if lives != 0:
            if lives < 5: # HINT CONDITION
                if hint_flag:
                    while True:
                        choice = input("For hint, answer: 'yes' or 'no': ")
                        if choice.lower() == "yes":
                            changed_word, hidden_word, changed_list, hidden_list = hint("game")
                            hint_count += 1
                            break
                        elif choice.lower() == "no":
                            break

Hangman()
```

I can make some general suggestions. Having code completely rewritten is offtopic here though.

In play_game, you import random from within the function. PEP8 notes though that:

  Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

It's conceivable that you may want to have an import conditionally happen inside of a function if the import is expensive, and would only rarely be used. That certainly isn't the case here though.

changed_list = [char for char in changed_word]

This just creates a copy of changed_word in a verbose way. You would be better off doing what you did before:

changed_list = changed_word[:]

You use far too many global variables. For clarity, those variables making up the state of the game should be explicitly passed to each function that needs them. You could use a dataclass to hold the data, and then pass the state object around.

Having everything as global with complicate testing, and makes your program harder to understand.

You have a loop that's written in a confusing way:

while True:
    guess = input("Input a letter: ")
    if len(guess) != 1:
        print("You should print a single letter\n")
        continue
    if guess != guess.lower() or not guess.isalpha():
        print("It is not an ASCII lowercase letter\n")
        continue
    break

You continue in two branches, then break at the bottom. You might as well just use an elif and else to set it up so you only break if both checks were false:

while True:
    guess = input("Input a letter: ")
    if len(guess) != 1:
        print("You should print a single letter\n")
    elif guess != guess.lower() or not guess.isalpha():
        print("It is not an ASCII lowercase letter\n")
    else:
        break

"\n%s\nYou guessed the word!\nYou survived!" % hidden_word

In more modern Python, this can be written more cleanly using f-strings like you use later:

f"\n{hidden_word}\nYou guessed the word!\nYou survived!"

if setting == "check_lives":
    if lives != 0:
        if lives < 5: # HINT CONDITION
            if hint_flag:

You don't have an else case for any of those checks, so you might as well just and them:

if setting == "check_lives" and 0 < lives < 5 and hint_flag:

That will cut down on a lot of nesting.

list((len(chosen_word))*"-")

Would make a lot more sense as just:

len("chosen_word") * ["-"]

for _ in range(len(changed_list)):
    guess = changed_list[_]

Don't call something _ unless you don't use it. If you use it, give it a good, descriptive name. Here though, you should just iterate the word directly:

for letter in changed_list:
    guess = letter  # Which mostly gets rid of the need for guess


Extending an array to a specified length, alternating directions I am working on golang and trying to understand to write good code not translated from another language, so I have written this program just for learning.

I want to perform operation on this array [1,2,3,4] with limit of 10, so that i get this output [1,2,3,4, 4,3,2,1, 1,2].

This function working correctly but I want to see suggestions for optimizations and improvements.

package main

import "fmt"

func arrayExm(array []int, limit int) []int {
    var res []int

    length := len(array)

    for i:=0; i < limit; i++ {

        if(i/length)%2 == 0{
            res = append(res,array[i%length])

        } else {
            res = append(res, array[length -i % length - 1])
        }
    }

    return res

}
func main() {
 fmt.Println(" Array with limit", arrayExm([]int{1,2,3,4},10))
}

This function working correctly but I want to see suggestions for
  optimizations and improvements.

Your source code is undocumented. See Godoc: documenting Go code.

By design, Go has a short, readable specification: The Go Programming Language Specification. As you write code, read useful parts of it in detail, for exanple, make (to make precise allocations), append (which may require multiple reallocations), copy (which has minimal bounds checking and may do block copies), and Index Expressions (which often require a bounds check), and so on.

Browse the Go standard library for useful packages. For example, the testing package provides testing and benchmark facilities.

Read and apply the Go Code Review Comments.

Your algorithm takes an array element view of the problem. Consider other algorihms. For example, a repeating, alternating pattern of input and reverse input.

  I want to see other way.

That's an excellent idea. I usually try to come up with three different ways to do things, although I often settle for two. I use code reviews to ask other people for ideas.

Here's one other way. It's a little different and much more efficient than your way. It uses the Go standard library and code review conventions, and Go's test and benchmark facilities.

alternate.go:

package main

import "fmt"

// alternate returns a list of length n
// with alternating a and reverse a.
func alternate(a []int, n int) []int {
    b := make([]int, n)
    // copy a
    copy(b, a)
    // copy reverse a
    for i, j := len(a)-1, len(a); i >= 0 && j < len(b); i, j = i-1, j+1 {
        b[j] = a[i]
    }
    // copy repeating pattern
    for i := 2 * len(a); i < n; i *= 2 {
        copy(b[i:], b[:i])
    }
    return b
}

func main() {
    a := []int{1, 2, 3, 4}
    for i := 0; i < 4*len(a)+2; i++ {
        fmt.Println(alternate(a, i), i)
    }
}

Output:

$ go run alternate.go
[] 0
[1] 1
[1 2] 2
[1 2 3] 3
[1 2 3 4] 4
[1 2 3 4 4] 5
[1 2 3 4 4 3] 6
[1 2 3 4 4 3 2] 7
[1 2 3 4 4 3 2 1] 8
[1 2 3 4 4 3 2 1 1] 9
[1 2 3 4 4 3 2 1 1 2] 10
[1 2 3 4 4 3 2 1 1 2 3] 11
[1 2 3 4 4 3 2 1 1 2 3 4] 12
[1 2 3 4 4 3 2 1 1 2 3 4 4] 13
[1 2 3 4 4 3 2 1 1 2 3 4 4 3] 14
[1 2 3 4 4 3 2 1 1 2 3 4 4 3 2] 15
[1 2 3 4 4 3 2 1 1 2 3 4 4 3 2 1] 16
[1 2 3 4 4 3 2 1 1 2 3 4 4 3 2 1 1] 17
$

alternate_test.go:

package main

import (
    "fmt"
    "testing"
)

func TestAlternate(t *testing.T) {
    a := []int{1, 2, 3, 4}
    n := 10
    got := fmt.Sprint(alternate(a, n))
    want := fmt.Sprint([]int{1, 2, 3, 4, 4, 3, 2, 1, 1, 2})
    if want != got {
        t.Errorf("want: %v got: %v", want, got)
    }
}

func BenchmarkAlternate(b *testing.B) {
    a := make([]int, 42)
    for i := range a {
        a[i] = i
    }
    b.ReportAllocs()
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        alternate(a, 1024)
    }
}

func arrayExm(array []int, limit int) []int {
    var res []int
    length := len(array)
    for i := 0; i < limit; i++ {
        if (i/length)%2 == 0 {
            res = append(res, array[i%length])
        } else {
            res = append(res, array[length-i%length-1])
        }
    }
    return res
}

func BenchmarkArrayExm(b *testing.B) {
    a := make([]int, 42)
    for i := range a {
        a[i] = i
    }
    b.ReportAllocs()
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        arrayExm(a, 1024)
    }
}

Output:

$ go test -v -bench=. alternate.go alternate_test.go
=== RUN   TestAlternate
--- PASS: TestAlternate (0.00s)
goos: linux
goarch: amd64
BenchmarkAlternate-4    500000    2576 ns/op    8192 B/op    1 allocs/op
BenchmarkArrayExm-4      50000   25783 ns/op   16376 B/op   11 allocs/op
PASS
ok      command-line-arguments  4.388s
$


XML node removal method with 5 arguments I read somewhere that you should try to have as few method arguments as possible to make code easier to read, understand and use. I agree with this to a point but I'm not sure how I can make the following method easier. I think all 5 of the arguments are required.

Should I leave it as is? I think, if I was to add some comments, I may remember what this does if I come back to it later but there's a chance I won't know exactly what each argument is for.

public static void RemoveXMLNode(string pathToDocument, string descendant, string element, string elementValue,string newDocumentPath)
{
    var xDoc = XDocument.Load(pathToDocument);
    xDoc.Descendants(descendant)
    .Where(n => (string)n.Element(element) == elementValue)
    .Remove();
    xDoc.Save(newDocumentPath);
}

The method has definitely too many arguments and the reason for that is because it does too much. When you look at its name you could think it removes a XML node but under the cover it does three things:

it loads a document
it then does its job of removing a node
it then saves the document under new name

If you properly separated these three concerns your APIs would have much simpler signatures.


Constantly checking for activity in a page then log out if no activity function executeQuery() {
  $.ajax({
    url: '../include/activity.php',
    success: function(data) {
      // do something with the return value here if you like
      if(data.error == true){
           window.location.href = "login.php";
      }

    }
  });
  setTimeout(executeQuery, 3000); // you could choose not to continue on failure...
}

$(document).ready(function() {
  // run the first time; all subsequent calls will take care of themselves
  setTimeout(executeQuery, 3000);
});

In activity.php

if (isset($_SESSION['LAST_ACTIVITY']) && (time() - $_SESSION['LAST_ACTIVITY'] > 300)) {
    // last request was more than 5 minutes ago
    session_unset();     // unset $_SESSION variable for the run-time
    session_destroy();   // destroy session data in storage
echo json_encode(array(
    'error' => true
));
}
$_SESSION['LAST_ACTIVITY'] = time(); // update last activity time stamp

if (!isset($_SESSION['CREATED'])) {
    $_SESSION['CREATED'] = time();
    echo json_encode(array(
        'error' => false
    ));
} else if (time() - $_SESSION['CREATED'] > 300) {
    // session started more than 5 minutes ago
    session_regenerate_id(true);    // change session ID for the current session an invalidate old session ID
    $_SESSION['CREATED'] = time();  // update creation time
    echo json_encode(array(
        'error' => false
    ));
}

This is how I am checking for activity of user using ajax and php.
I have doubts if this is acceptable in real life environment.
I would like to have suggestion on how to implement this cleanly , correctly easily.

I would route all ajax calls through 1 function in JavaScript, and that function then keeps track of the last call. In essence, the browser itself should know whether there was activity or not.

If for some odd reason you need multiple windows with the same session to stay open as long as one of them has activity, then I would change the timer from 3000 to 60000. Checking once a minute should be more than fine.

Finally executeQuery is a terrible name, perhaps checkTimeOut?


Groovy Map>> data manipulation method optimization Am trying to update the Map<String,List<Map<String,String>>> invoices with the invoiceErrors as below

InvoiceError // is an entity with below attributes
{ String errorMessage,
  String invoiceNumber
}
ErrorMessage                                          invoiceNumber
-------------                                       -------------------
File Error : The file is in an unsupported format   INV-Error_Test1
Line : 1 Invoice does not foot Reported             INV-Error_Test1
Line : 2 MATH ERROR                                 INV-Error_Test1
Line : 3 MATH ERROR                                 INV-Error_Test2
Line : 3 Invoice does not foot Reported             INV-Error_Test2

Am trying to achieve below map
    If the error message doesnt have a line number it need to be appended at the top level as invLineItems.put('error',['INV-Error_Test1' :File Error : The file is in an unsupported format])
   Otherwise errormessage should be appended to the  matching INVOICE and linenumber as below

invLineItems = [INV-Error_Test1:[[LINE:1, INVOICE_DATE:20150301, INVOICE_NUMBER:INV-Error_Test1, INVOICE_TOTAL:22, error : `Line : 1 Invoice does not foot Reported`],
                                [LINE:2, INVOICE_DATE:20150301, INVOICE_NUMBER:INV-Error_Test1, INVOICE_TOTAL:24, error : `Line : 2 MATH ERROR`],
                INV-Error_Test2:[[LINE:3, INVOICE_DATE:20150301, INVOICE_NUMBER:INV-Error_Test2, INVOICE_TOTAL:26, , error : `Line : 3 MATH ERROR | Line : 3 Invoice does not foot Reported`],
                                [LINE:4, INVOICE_DATE:20150301, INVOICE_NUMBER:INV-Error_Test2, INVOICE_TOTAL:28,]],
                error : [[INV-Error_Test1:`File Error : The file is in an unsupported format`]]

I wrote the below method to achieve the above

def regex = "^Line\\s(?:(\\d+)\\s)?\\s*:\\s+(\\d+)?.+";
for (e in invLineItems ){
  def errors =  lipErrors.findAll{it.invoiceNumber==e.key} // finding the error messages with the invoice number
  errors.each{  // fetching the line numbre from error message and finding the matching record the invoice number and line number in invLineItems
     int lineNumber
     if (it.errorMessage.matches(regex)) {
            Pattern p = Pattern.compile("\\d+");
            Matcher m = p.matcher(it.errorMessage);
            if (m.find()) {
                 lineNumber = Integer.parseInt(m.group());
            }
          println "lineNumber = "+lineNumber
        }

    if(e.value['LINE_ITEM_NUMBER'].find{it==lineNumber.toString()}) {
      def data = lipErrors.findAll{it.invoiceNumber==e.key && it.errorMessage.matches("^Line\\s+"+lineNumber+"?\\:\\s+"+lineNumber+"?.+")}
      e.getValue().each{it.put("error", data.errorMessage.join("|"))}

     }

  }
}

The code doesnt look like Groovy and using traditional java code mostly, am wondering whether the code can be simplified with Groovy approach

You can use Groovy's regular expression operators to create Java Matchers.

def invoices = [
'LEDES98BI V2',
'LINE|INVOICE_DATE|INVOICE_NUMBER|INVOICE_TOTAL',
'1|20150301|INV-Error_Test1|22',
'2|20150301|INV-Error_Test1|24',
'3|20150301|INV-Error_Test2|26',
'4|20150301|INV-Error_Test2|28,',
'5|20150301|INV WITH WHITESPACE|29,']

def lines = invoices
    .findAll { it.contains('|') }
    .collect { it.tokenize('|') }

def headers = lines.first()
def invoiceMap =  lines.tail().collect{
    [headers, it].transpose().collectEntries()
}.groupBy{ it.INVOICE_NUMBER }

class InvoiceError {
    String errorMessage
    String invoiceNumber
}

[
    ['INV-Error_Test1', 'File Error : The file is in an unsupported format'],
    ['INV-Error_Test1', 'Line : 1 Invoice does not foot Reported'],
    ['INV-Error_Test1', 'Line : 2 MATH ERROR'],
    ['INV-Error_Test2', 'Line : 3 MATH ERROR'],
    ['INV-Error_Test2', 'Line : 3 Invoice does not foot Reported']
].collect {
    [['invoiceNumber', 'errorMessage'], it].transpose().collectEntries()
}.collect { it as InvoiceError }
.each {error ->
   def matcher = error.errorMessage =~ /Line : (\d+) (.*)/

   if(matcher) {
       def lineNumber = matcher.group(1)
       def errorDescription = matcher.group(2)
       def line = invoiceMap[error.invoiceNumber]
           .find() { it.LINE == lineNumber }

       if(line.errors) {
           line.errors = "$line.errors |  $errorDescription"
       } else {
           line.errors = errorDescription
       }
   } else {
       if(!invoiceMap.errors) invoiceMap.errors = []
       invoiceMap.errors << [(error.invoiceNumber): error.errorMessage]
   }
}

invoiceMap

In this example, I build the InvoiceError instances from a list that's transposed (btw, thank you for that trick). Each resulting map is coerced into an instance of InvoiceError.

I took a different approach by iterating through the errors and inserting them into the invoiceMap, rather than iterating through the invoiceMap and looking up the errors.


Print left view of the tree Left view of a Binary Tree is set of nodes visible when tree is visited from left side. Left view of following tree is 12, 10, 25.

       12

    /     \
 10       30
        /    \
      25      40

Looking for code review, optimizations and best practices. Also verifying space complexity is O(n) and not O(logn) where n is number of nodes in tree.

public class LeftView<T> {

    private TreeNode<T> root;

    /**
     * Takes in a BFS representation of a tree, and converts it into a tree.
     * here the left and right children of nodes are the (2*i + 1) and (2*i + 2)nd
     * positions respectively.
     *
     * @param items The items to be node values.
     */
    public LeftView(List<? extends T> items) {
        create(items);
    }

    private void create (List<? extends T> items) {
        root = new TreeNode<T>(null, items.get(0), null);

        final Queue<TreeNode<T>> queue = new LinkedList<TreeNode<T>>();
        queue.add(root);

        final int half = items.size() / 2;

        for (int i = 0; i < half; i++) {
            if (items.get(i) != null) {
                final TreeNode<T> current = queue.poll();
                final int left = 2 * i + 1;
                final int right = 2 * i + 2;

                if (items.get(left) != null) {
                    current.left = new TreeNode<T>(null, items.get(left), null);
                    queue.add(current.left);
                }
                if (right < items.size() && items.get(right) != null) {
                    current.right = new TreeNode<T>(null, items.get(right), null);
                    queue.add(current.right);
                }
            }
        }
    }

    private static class TreeNode<E> {
        TreeNode<E> left;
        E item;
        TreeNode<E> right;

        TreeNode(TreeNode<E> left, E item, TreeNode<E> right) {
            this.left = left;
            this.item = item;
            this.right = right;
        }
    }

    /**
     * Returns the left view of the binary tree
     *
     * @return  the left view of binary tree.
     */
    public List<T> leftView() {
        if (root == null) { throw new NoSuchElementException("The root is null"); }

        final List<T> leftView = new ArrayList<T>();
        computeLeftView(root, leftView, 1);
        return leftView;
    }

    private void computeLeftView(TreeNode<T> node, List<T> leftView, int currentDepth) {
        if (node != null) {
            if (currentDepth > leftView.size()) {
                leftView.add(node.item);
            }
            computeLeftView(node.left, leftView, currentDepth + 1);
            computeLeftView(node.right, leftView, currentDepth + 1);
        }
    }

    public static void main(String[] args) {
        final LeftView<Integer> leftView1 = new LeftView<Integer>(Arrays.asList(1, 2, 3, 4, 5, 6, 7));
        assertEquals(Arrays.asList(1, 2, 4)  , leftView1.leftView());

        final LeftView<Integer> leftView2 = new LeftView<Integer>(Arrays.asList(1, 2, 3, 4, null, 6, 7));
        assertEquals(Arrays.asList(1, 2, 4)  , leftView2.leftView());

        final LeftView<Integer> leftView3 = new LeftView<Integer>(Arrays.asList(1, 2, 3, null, 5, 6, 7));
        assertEquals(Arrays.asList(1, 2, 5)  , leftView3.leftView());

        final LeftView<Integer> leftView4 = new LeftView<Integer>(Arrays.asList(1, 2, 3, null, null, 6, 7));
        assertEquals(Arrays.asList(1, 2, 6)  , leftView4.leftView());
    }
}

Although the algorithms seems fine, there are issues with the choosen abstractions.

LeftView is a factory of lists, which are left views of an inner tree. LeftView is not a data structure itself, but a factory of it, configured by a list, that are converted to a tree -- too much responsability. Keeping in mind @abuzittin gillifirca advice, a good practise is to refactor factory methods in static, public, methods.

public final class BinaryTrees {
    public static List<T> createLeftViewList(BinaryTree<T> tree) { ... }
}

In your code, LeftView had his own tree representation. To better convey the idea of an algorithm, consider start with simple interfaces, followed by the algorithm itself. That way you get better readability and guarantee further reuse in real code.

public interface BinaryTree<T> {
    BinaryTree<T> left();
    BinaryTree<T> right();
    T element();
}

And than we fill it with runnable code.

public class BinaryTree<T> {
    public BinaryTree<T> left() { return left; }
    public BinaryTree<T> right() { return right; }
    public T element() { return element; }

    private BinaryTree<T> left, right;
    private T element;

    public BinaryTree(List<? extends T> bfsRepresentation) {
        final List<? extends T> items;
        items = new ArrayList<>( bfsRepresentation );
        create( items );
    }
    ...
}

The create method assumes that items is always random access list. Well, it is if we consider only the test methods. If it gets called with a very large LinkedList, you will get quadratic performance.

Consider copying the list in an java.util.ArrayList. An array, T[], should do the trick too, and its preferrable.

Last piece of advice, consider parameterized unit tests in JUnit. There's a good example how to use it in their github wiki.


Representing IPv6 addresses with PHP per RFC 5952 I wrote the following function in PHP to represent IPv6 addresses as short as possible:

function ipv6_compress($ip){
    // Shorten first group of zeros
    if(substr($ip, 0, 4) == 0000) $ip = substr_replace($ip, '0', 0, 4);
    // Shorten full groups of zeros
    $ip = str_replace('0000:', '0:', $ip);
    // Remove leading zeros
    $ip = preg_replace('/:0{1,3}(?=\w)/', ':', $ip);
    // Remove longest extra group of zeros per [RFC 5952](http://tools.ietf.org/html/rfc5952)
    if(strpos('::') !== false) return $ip; // But don't if a :: is already present as entered
    $pos = strpos($ip, '0:0:0:0:0:0:0:');
    if($pos !== false) return $ip = substr_replace($ip, '::', $pos, 14);
    $pos = strpos($ip, '0:0:0:0:0:0:');
    if($pos !== false) return $ip = substr_replace($ip, '::', $pos, 12);
    $pos = strpos($ip, '0:0:0:0:0:');
    if($pos !== false) return $ip = substr_replace($ip, '::', $pos, 10);
    $pos = strpos($ip, '0:0:0:0:');
    if($pos !== false) return $ip = substr_replace($ip, '::', $pos, 8);
    $pos = strpos($ip, '0:0:0:');
    if($pos !== false) return substr_replace($ip, '::', $pos, 6);
    $pos = strpos($ip, '0:0:');
    if($pos !== false) return substr_replace($ip, '::', $pos, 4);
    return $ip;
}

I attempt to adhere to RFC 5952.

Do I violate the standard in any way?
Can I do anything more efficiently?

As you can probably tell, I am not concerned with validating the addresses.

You seem to adhere to the RFC from what I can tell. What I feel is missing in your code is just a little more bracing and a way to drop these repeating if-statements.

What do you think of the following for the :: replacing (pseudocode)

const $zero_chain = '0:0:0:0:0:0:0:';

// zero shortening already happened by now
$zero_chain_copy = $zero_chain;
while (strpos($ip, '::') === false && strlen($zero_chain) >= 4) {
    $pos = strpos($ip, $zerochain);
    if ($pos !== false) {
        // early return, because no further shortenings can be applied anyways
        return $ip = substr_replace($ip, ::, $pos, strlen($zero_chain));
    }
    // cut away one '0:' to shorten the chain
    $zero_chain = substr($zerochain, 0, strlen($zero_chain) - 2);
}
return $ip; // no zero-chains at all


Replace tabs with spaces I wrote a program that replaces tabs with spaces. First, I defined a maximum length of an input string in the constant MAX_LIMIT. Next, I declared the procedure detab that takes two arguments: an input string and a maximum length. In the procedure I read characters until encountering EOF. I'm replacing tabs with four spaces and I'm checking if index is equal to limit, and if yes, then I break the loop. Finally, I created a str character array. I called on it the detab procedure and I printed out the value of str in main.

main.c

#include <stdio.h>
#define MAX_LIMIT 1000

void detab(char str[], int limit);

int main(void) {
    char str[MAX_LIMIT];
    detab(str, MAX_LIMIT);
    printf("%s", str);
    return 0;
}

void detab(char str[], int limit) {
    char current;
    int index = 0;
    int stop = 0;
    while ((current = getchar()) != EOF) {
        if (stop) {
            break;
        }
        if (current == '\t') {
            for (int i = 0; i < 4 && !stop; i++) {
                if (index == limit) {
                    stop = 1;
                } else {
                    str[index++] = ' ';
                }
            }
        } else {
            str[index++] = current;
        }
    }
}

How can I improve this program? Is it an optimal solution of this problem?

First of all, it is not exactly doing what is expected of detab. The visual appearance of the detabbed text shall remain the same. You need to insert as many spaces as required to reach the next tab stop, not necessarily 4 of them.

For example, a string ab\tc should become

ab  c

(2 spaces inserted) whereas your code produces

ab    c

detab does not zero-terminate the string. It is an (un)lucky coincidence that the code produces meaningful output.
The limit condition is only tested during the tab expansion. A long sequence of ordinary characters will overrun the buffer.


Bit Flipping Programming Exercise in C The comment at the top was not followed because I printed the contents. I just want an opinion on coding style.

    /*
     * Write a function setbits(x,p,n,y) that returns x with the n bits
     * that begin at position p set to the rightmost n bits of y, leaving
     * the other bits unchanged in the least number of lines.
     *
     * Note: function signatures and curly brackets dont count towards
     * the number of lines. You must also declare your variables, and
     * call the function
     *
     * build with:
     *   gcc -o bit_twiddle -Wall -g -lm ./bit_twiddle.c
     */

    #include <stdio.h>
    #include <math.h>
    #include <limits.h>

    unsigned setbits(unsigned x, unsigned p, unsigned n, unsigned y) {
        x |= (y & ~(~0 << n)) << p;
        size_t s = (int)(log(INT_MAX)/log(2)) + 1;
        printf("An int is %d bits\n", s);
        int mask = pow(2.0, (int)s);
        do {
            ((x & mask) == 0) ? printf("0") : printf("1");
            ((s%4)==0) ? printf(" ") : printf("");
            x <<= 1;
        } while (s--);
        printf("\n");
    }

    void main( ) {
        unsigned retrn=1, begin=3, nbits=3, input=7;
        unsigned x = setbits(retrn, begin, nbits, input);
    }

UPDATE

x |= (y & ~(0 << n)) << p --> x |= (y & ~(~0 << n)) << p

I think your code does not exactly what the comment wants. In line:

x |= (y & ~(0 << n)) << p;

0 << n is 0. If you want n rightmost bits of y you may use a mask like (1<<n)-1 (which is n 1 bits).
Now on your code style, when you are coding a bit manipulating task, you'd better do it completely with bitwise operators (at least use other functions very few). Here in this code, using log and pow can be avoided. If you want to get number of bits in an integer type, you can use sizeof, like below:

size_t s = sizeof(int) << 3;

here << 3 is equal to * 8, because each byte definitely has 8 bits (it works on integer types of any size, just replace int with any other type like short or long long).

Now instead of using pow(2, s) you can write 1 << s :)

And about your printing, you can replace s%4 with s&3 (think about it). And I prefer shifting down mask instead of shifting up x (here  it doesn't make any difference, but when you work with signed values it does, because sign will be preserved in left shifts). And for reducing number of lines for printing, you may use for loop.


Multiple Formats: Reading Working Routine From File It is my job to store the workout routine for a given individual in a multimap<string, Exercise*> using an overloaded extraction operator in main. I wanted to know if my code for said operator can be simplified and made more intuitive. There are three classes, but I have only shown Routines; the others seem trivial to include. Implementations will be done in the header file. I will be extracting information from the following file (routine.txt):

Donald Duck,2
Chin-ups,30
Lat Pulldown,20,90
David Ash, 2
Push-ups,5
Wrist curl,10, 2

The format is name,numberOfExercises followed by numberOfExcercises exercises. Each exercise involves weights or does not. A weighted exercise is formatted as exerName,reps,weight. A non-weighted exercise is formatted as exerName,reps. The inclusion of a space after a comma is not a mistake. main contains the following:

int main() {
   Routines routines;
   string name;
   ifstream inf("routines.csv");

   inf >> routines;
}

The routines class has the following:

#include <fstream>
#include <map>
#include "exercise.h"
#include "weightedexercise.h"

using namespace std;

class Routines
{
  multimap<string, Exercise*> routineMap;

public:
  friend istream& operator>>(istream& is, Routines& rout)
  {
    string name, line;
    char comma;
    int numEx, rep, weight = 0;
    // numEx is number of exercises.
    // name is name of person. line is the exercise information.

    getline(is, name, ',');

    do
    {
      is >> numEx;
      getline(is, line);

      for(int i = 0; i < numEx; i++)
      {
        getline(is, line, ',');
        is >> rep;
        is.get(comma);
        if(comma == ',')
          is >> weight;

        if(weight)
        {
          rout.routineMap.insert(multimap<string,
          Exercise *>::value_type(name, new WeightedExercise(line, rep, weight)));
          getline(is, line);
        }
        else
        {
          rout.routineMap.insert(multimap<string,
          Exercise *>::value_type(name, new Exercise(line, rep)));
        }

        weight = 0;
      }
    } while(getline(is, name, ','));
  }
};

Avoid pointers

In this case, there seems to be no particularly good reason for your map to store pointers to the data instead of just storing the data directly. It'll simplify your code a bit if you just have it store something like a vector<exercise>. Alternatively, you could consider using a std::multimap<std::string, Exercise> (which has a similar effect).

Note that many people are likely to give advice more like: "avoid raw pointers". While that's good advice in itself, I prefer to avoid (explicit use of) pointers entirely if possible. If you find that you can't avoid using a pointer, then use a smart pointer. But if you can avoid using any sort of pointer at all, so much the better.

Distribution of responsibilities

You've defined an Exercise class (or struct), but all the responsibility for reading an Exercise from a file has been assigned to Routines. At least IMO, it would be better if an Exercise knew how to extract itself from a file, and a Routine just made use of that to read the exercises in a routine.

map data type

Right now, your Routines is basically just a map with an overload of operator>> to extract data and populate that map from the file. We don't need to define a new class just for that purpose though. map already uses a template to define the type of data it stores--we can just use that (and give it a name, if we like):

using routine = std::pair<std::string, std::vector<Exercise> >;

Putting those together we get extraction operators for routine and exercise that look something like this:

std::istream &operator>>(std::istream &in, Exercise &e) {
    std::string temp;
    std::getline(in, temp);
    std::istringstream is(temp);
    std::getline(is, e.name, ',');
    is >> e.reps;
    if (is.peek() == ',') {
        is.ignore(1);
        is >> e.weight;
    }
    else
        e.weight = 0;
    return in;
}

std::istream &operator>>(std::istream &is, routine &r) {
    std::getline(is, r.first, ',');
    int count;
    is >> count;
    is.ignore(1024, '\n');
    for (int i=0; i<count; i++) {
        exercise e;
        is >> e;
        r.second.push_back(e);
    }
    return is;
}

So now we've restricted the messiness of reading an exercise correctly to a somewhat smaller area. A Routine just reads a name and a some number of Exercises.

With those in place, initializing a map of routines from a file can look something like this:

std::ifstream in("routines.csv");

std::map<std::string, std::vector<exercise>> routines {
    std::istream_iterator<routine>(in), {} };

Of course, if you really want to define your own complete class for Routine, you can do that--but I'd still keep the responsibility for reading an Exercise in an extractor specifically for an Exercise.


Compute e using Euler's expansion Given the following task:

  Exercise 1.38.  In 1737, the Swiss
  mathematician Leonhard Euler published
  a memoir De Fractionibus Continuis,
  which included a continued fraction
  expansion for e - 2, where e is the
  base of the natural logarithms. In
  this fraction, the Ni are all 1, and
  the Di are successively 1, 2, 1, 1, 4,
  1, 1, 6, 1, 1, 8, .... Write a program
  that uses your cont-frac procedure
  from exercise 1.37 to approximate e,
  based on Euler's expansion.

I wrote this solution:

(define (i-cont-frac n d k)
  (define (iterate result i)
    (define (next n) (- n 1))
    (if (= i 0) result
        (iterate (/ (n i)
                    (+ (d i)
                    result)) (next i))))
  (iterate 0 k))

(define (divisible? a b) (= (remainder a b) 0))
(define (compute-e k)
  (define (n i) 1)
  (define (d i) (cond ((= i 1) 1)
                      ((= i 2) 2)
                      ((divisible? (- i 1) 3)
                       (* (/ (- i 1) 3.0) 2.0))
                      (else 1.0)))
  (+ 2 (cont-frac n d k)))

What do you think?

Your definition of d is not quite right.  It produces incorrect results for some values of i and it has two special cases which are not necessary.  A possible definition is:

  (define (d i)
    (cond
      ((divisible? (+ i 1) 3)
       (* (/ (+ i 1) 3) 2))
      (else 1)))

This is because i is 1-indexed (series begins with i = 1).  If you want a decimal instead of a fraction as your result, change the multiplier to 2.0 and the else value to 1.0.

Another way to write d is:

  (define (d i)
    (let
        ((d-value (* (/ (+ i 1) 3) 2)))
      (if (integer? d-value) d-value 1)))


Spring configuration without XML is this reality? We're doing a website for Cardshifter, and now I've converted the plain html files and css into a Spring MVC web application. Here is the typical configuration classes that I used in most of the project I work with.

package com.cardshifter.io.web.config;

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.ComponentScan;
import org.springframework.context.annotation.Configuration;
import org.springframework.web.servlet.config.annotation.EnableWebMvc;
import org.springframework.web.servlet.config.annotation.ResourceHandlerRegistry;
import org.springframework.web.servlet.config.annotation.WebMvcConfigurerAdapter;
import org.thymeleaf.spring4.SpringTemplateEngine;
import org.thymeleaf.spring4.view.ThymeleafViewResolver;
import org.thymeleaf.templateresolver.ServletContextTemplateResolver;

@Configuration
@EnableWebMvc
@ComponentScan(basePackages = { "com.cardshifter.io.web" })
public class ApplicationConfig extends WebMvcConfigurerAdapter {

    @Bean
    ServletContextTemplateResolver templateResolver() {
        ServletContextTemplateResolver templateResolver = new ServletContextTemplateResolver();
        templateResolver.setPrefix("/WEB-INF/");
        templateResolver.setSuffix(".html");
        templateResolver.setTemplateMode("HTML5");
        templateResolver.setOrder(10);
        return templateResolver;
    }

    @Bean
    SpringTemplateEngine templateEngine() {
        SpringTemplateEngine templateEngine = new SpringTemplateEngine();
        templateEngine.setTemplateResolver(templateResolver());
        return templateEngine;
    }

    @Bean
    ThymeleafViewResolver viewResolver() {
        ThymeleafViewResolver resolver = new ThymeleafViewResolver();
        resolver.setOrder(1);
        resolver.setTemplateEngine(templateEngine());
        resolver.setViewNames(new String[] { "*" });
        return resolver;
    }

    @Override
    public void addResourceHandlers(ResourceHandlerRegistry registry) {
        registry
            .addResourceHandler("/css/**","/images/**")
            .addResourceLocations("/css/", "/images/");
    }

}

This is the class that define the view engines and a bunch of other stuff.

package com.cardshifter.io.web.config;

import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.ServletRegistration.Dynamic;

import org.springframework.web.WebApplicationInitializer;
import org.springframework.web.context.ContextLoaderListener;
import org.springframework.web.context.support.AnnotationConfigWebApplicationContext;
import org.springframework.web.servlet.DispatcherServlet;
import org.springframework.web.util.Log4jConfigListener;

import com.cardshifter.io.web.config.ApplicationConfig;

public class AppInitializer  implements WebApplicationInitializer {

    public void onStartup(ServletContext servletContext) throws ServletException {

        // Create the 'root' Spring application context
        AnnotationConfigWebApplicationContext rootContext = new AnnotationConfigWebApplicationContext();
        rootContext.register(ApplicationConfig.class);

        // pour log4j
        servletContext.addListener(new Log4jConfigListener());

        // Register and map the dispatcher servlet
        Dynamic dispatcherSpring = servletContext.addServlet("io", new DispatcherServlet(
                rootContext));
        dispatcherSpring.setLoadOnStartup(1);
        dispatcherSpring.addMapping("/");

        servletContext.addListener(new ContextLoaderListener(rootContext));
    }

}

That class is the one responsible to expose the application to the servlet without a good old web.xml.

What I'm wondering is, is there something more I would need to add in order to make the configuration easier? I always have difficulty to set the resources (css, js, etc.) in the webapp, is what I'm doing the "right" way?

You definitely want to look into Spring Boot. It can significantly reduce Spring configuration + has a lot of features that help to with deployment to production. Think about it as "convention over configuration" wrapper for Spring framework.

If you would follow Spring Boot conventions, you may need to shift some resources to different folders, but I believe you would be able to eliminate your spring configuration rapidly/completely.

If you decide not to use Spring Boot:

You may want to use AbstractAnnotationConfigDispatcherServletInitializer instead of WebApplicationInitializer, because I don't observe any unusual servet configuration. It may look like this:

import org.springframework.web.servlet.support.AbstractAnnotationConfigDispatcherServletInitializer;

public class AppInitializer extends
        AbstractAnnotationConfigDispatcherServletInitializer {

    @Override
    protected Class<?>[] getRootConfigClasses() {
        return null;
    }

    @Override
    protected Class<?>[] getServletConfigClasses() {
        return new Class[]{ApplicationConfig.class};
    }

    @Override
    protected String[] getServletMappings() {
        return new String[]{"/"};
    }

}


Draw an arc based on a given percentage I wanted to create a class that would allow me to draw an arc representing a percentage of progress, similar to the activity view on the Apple watch. There are various examples which I found helpful on the web and my output is a combination of many of these.

I have posted my code so that it works and renders in a Xcode playground. Could someone have a look over it and advise if i'm doing anything I shouldn't, or going wrong anywhere.

import UIKit
import XCPlayground

class CircleProgressView: UIView {

    var startPoint: CGFloat = 0
    var color: UIColor = UIColor.yellowColor()
    var trackColor: UIColor = UIColor.grayColor()
    var trackWidth: CGFloat = 1
    var fillPercentage: CGFloat = 100

    override init(frame: CGRect) {

        super.init(frame: frame)
        self.backgroundColor = UIColor.clearColor()

    } // init

    required init(coder aDecoder: NSCoder) {

        super.init(coder: aDecoder)
        self.backgroundColor = UIColor.clearColor()

    } // init

    private func getGraphStartAndEndPointsInRadians() -> (graphStartingPoint: CGFloat, graphEndingPoint: CGFloat) {

        // make sure our starting point is at least 0 and less than 100
        if ( 0 > self.startPoint ) {
            self.startPoint = 0
        } else if ( 100 < self.startPoint ) {
            self.startPoint = 100
        } // if

        // make sure our fill percentage is at least 0 and less than 100
        if ( 0 > self.fillPercentage ) {
            self.fillPercentage = 0
        } else if ( 100 < self.fillPercentage ) {
            self.fillPercentage = 100
        } // if

        // we take 25% off the starting point, so that a zero starting point
        // begins at the top of the circle instead of the right side...
        self.startPoint = self.startPoint - 25

        // we calculate a true fill percentage as we need to account
        // for the potential difference in starting points
        let trueFillPercentage = self.fillPercentage + self.startPoint

        let π: CGFloat = CGFloat(M_PI)

        // now we can calculate our start and end points in radians
        let startPoint: CGFloat = ((2 * π) / 100) * (CGFloat(self.startPoint))
        let endPoint: CGFloat = ((2 * π) / 100) * (CGFloat(trueFillPercentage))

        return(startPoint, endPoint)

    } // func

    override func drawRect(rect: CGRect) {

        // first we want to find the centerpoint and the radius of our rect
        let center: CGPoint = CGPointMake(CGRectGetMidX(rect), CGRectGetMidY(rect))
        let radius: CGFloat = rect.width / 2

        // make sure our track width is at least 1
        if ( 1 > self.trackWidth) {
            self.trackWidth = 1
        } // if

        // and our track width cannot be greater than the radius of our circle
        if ( radius < self.trackWidth ) {
            self.trackWidth = radius
        } // if

        // we need our graph starting and ending points
        let (graphStartingPoint, graphEndingPoint) = self.getGraphStartAndEndPointsInRadians()

        // now we need to first draw the track...
        var trackPath = UIBezierPath(arcCenter: center, radius: radius - (trackWidth / 2), startAngle: graphStartingPoint, endAngle: 2.0 * CGFloat(M_PI), clockwise: true)
        trackPath.lineWidth = trackWidth
        self.trackColor.setStroke()
        trackPath.stroke()

        // now we can draw the progress arc
        var percentagePath = UIBezierPath(arcCenter: center, radius: radius - (trackWidth / 2), startAngle: graphStartingPoint, endAngle: graphEndingPoint, clockwise: true)
        percentagePath.lineWidth = trackWidth
        percentagePath.lineCapStyle = kCGLineCapRound
        self.color.setStroke()
        percentagePath.stroke()

        return

    } // func

} // class

var circleProgress: CircleProgressView = CircleProgressView()
circleProgress.frame = CGRectMake(0 , 0, 200, 200)

circleProgress.trackWidth = 20

circleProgress.startPoint = 0
circleProgress.fillPercentage = 75

XCPShowView("Container View", circleProgress)

I want to focus on just the getGraphStartAndEndPointsInRadians() method of your class, as this is the area I see the most problems with a cursory glance.

First of all, from the method name alone, I can tell it does too much.  The word "and" should very rarely be used in method names.  It's usage is usually a sign that a method does too much.  And this one does.

First thing we can do is refactor out the percent-to-radians conversions into their own method.

func percentToRadians(percent: CGFloat) -> CGFloat {
    return (2 * CGFloat(M_PI)) / 100) * percent
}

Next, this logic has no business in the method:

    // make sure our starting point is at least 0 and less than 100
    if ( 0 > self.startPoint ) {
        self.startPoint = 0
    } else if ( 100 < self.startPoint ) {
        self.startPoint = 100
    } // if

    // make sure our fill percentage is at least 0 and less than 100
    if ( 0 > self.fillPercentage ) {
        self.fillPercentage = 0
    } else if ( 100 < self.fillPercentage ) {
        self.fillPercentage = 100
    } // if

Instead, this logic should go somewhere more closely related to the actual setting of these variables.  Like in the didSet method.

A method with get in the name definitely shouldn't have side effects like this.  (And we really don't use get in method names either.)

As an aside, all of your variables should have didSet methods that assure that the view is redrawn when their values are changed.

So, we're agreed that we'll move side-effecting logic out of this method, and that we'll refactor the percent-to-radian logic out, now we just need to split the methods.  We need one for the start point and one for the end point.  Why don't we do these as computer properties?

private var startingDrawPoint: CGFloat {
    return percentToRadians(CGFloat(self.startPoint) - 25)
}

private var endingDrawPoint: CGFloat {
    return percentToRadians(CGFloat(self.fillPercentage - self.startPoint) - 25)
}


HackerRank Left rotation code using Java 8 I tried to solve this Left Rotation problem on HackerRank. My code works out for most test cases, but for some it doesn't, probably because they have large number of data. So there seems to be some optimization or performance issue in my code, and I can't really figure out how to fix it. Any help will be greatly appreciated :)

public class Solution {

    private static final Scanner scanner = new Scanner(System.in);

    public static void main(String[] args) {
        String[] nd = scanner.nextLine().split(" ");
        int c;
        int n = Integer.parseInt(nd[0]);

        int d = Integer.parseInt(nd[1]);

        int[] a = new int[n];

        int arr[] = new int[n];

        String[] aItems = scanner.nextLine().split(" ");
        scanner.skip("(\r\n|[\n\r\u2028\u2029\u0085])?");

        for (int i = 0; i < n; i++) {
            int aItem = Integer.parseInt(aItems[i]);
            a[i] = aItem;
        }

        scanner.close();

        for (int k=1;k<=d;k++) {
            for (int j=0;j<n;j++) {
                if (j==n-1)
                { c=0;}
                else
                { c=j+1;}
                arr[j]=a[c];
            }
            System.arraycopy(arr, 0, a, 0, n);
        }

        for (int j=0;j<n;j++) {
            System.out.print(arr[j]+ " ");
        }
    }
}

String[] nd = scanner.nextLine().split(" ");
        int c;
        int n = Integer.parseInt(nd[0]);

        int d = Integer.parseInt(nd[1]);

First, why declare c here?  You don't use it until much later.  So let's take it out of here and define it closer to its first use.  I would also rename it, as I find c insufficiently descriptive.

Second, why use nextLine this way?  Consider

        int numbersCount = scanner.nextInt();
        int rotationCount = scanner.nextInt();

I don't know that this is any faster, but it's more direct in code.

I would also add

        rotationCount %= numbersCount;

You could do this in the declaration, but I find it more readable separate.

What this does is remove any superfluous rotations.  If we are rotating numbersCount times exactly, we're already done.  Because that's just the original array.  By taking the modulus or remainder, we're getting rid of all those full cycle rotations.  Since that's just wasted time, this purely improves those large rotations at the expense of an extra division for all rotations.

        int[] numbers = new int[numbersCount];
        for (int i = 0; i < numbers.length; i++) {
            numbers[i] = scanner.nextInt();
        }

Again, I wouldn't use nextLine here.  The scanner is perfectly capable of this.  If you want to read line by line, consider going to something like a BufferedReader that works that way.

You have

                if (j==n-1)
                { c=0;}
                else
                { c=j+1;}
                arr[j]=a[c];

So we finally find c.  It's the index into the old array.  But it could have been more readably written

                int originalIndex = (j + 1 == numbers.length) ? 0 : j + 1;

or

                int originalIndex = j + 1;
                if (originalIndex == numbers.length) {
                    originalIndex = 0;
                }

And whatever you do, please don't do things like {c=0;}.  It's hardly any shorter.  It's just different.  Unless different indicates something, it's not helpful.

But I would actually skip the rotation.  Instead, break the output into two pieces:

        for (int i = rotationCount; i < numbers.length; i++) {
            System.out.print(numbers[i] + " ");
        }

        for (int i = 0; i < rotationCount; i++) {
            System.out.print(numbers[i] + " ");
        }

This will display the output as rotated without rotating.

Or just read it rotated.

        int rotationPoint = numbers.length - rotationCount;
        for (int i = rotationPoint; i < numbers.length; i++) {
            numbers[i] = scanner.nextInt();
        }

        for (int i = 0; i < rotationPoint; i++) {
            numbers[i] = scanner.nextInt();
        }

Then the output could be something like

        System.out.println(String.join(" ", numbers));

There's no need to rotate at all, much less to rotate one at a time.

class Solution {

    public static void main(String[] args) {
        System.out.println(String.join(" ", readInputRotated());
    }

    public static int[] readInputRotated() {
        try (Scanner scanner = new Scanner(System.in)) {
            int[] rotated = new int[scanner.nextInt()];

            int rotationPoint = rotated.length - scanner.nextInt();
            for (int i = rotationPoint; i < rotated.length; i++) {
                rotated[i] = scanner.nextInt();
            }

            for (int i = 0; i < rotationPoint; i++) {
                rotated[i] = scanner.nextInt();
            }

            return rotated;
        }
    }

}

This also uses the try-with-resources form to manage the Scanner lifecycle without explicitly calling close.

Your original code was \$\mathcal{O}(mn)\$ where \$m\$ and \$n\$ are the number of rotations and the input size.  This is \$\mathcal{O}(n)\$ (because you read the input) and the "rotation" is \$\mathcal{O}(1)\$.


Getting LinkedIn details Each field is prone to throw an exception if not found. If one element not found, then the next should be checked. I have put each element in a separate try/catch. Is this a correct approach or can it be optimized?

def getLinkedinDetails(self, gearman_worker, gearman_job):
    driver = webdriver.PhantomJS('/usr/local/bin/phantomjs')
    user_input = gearman_job.data
    user_input_list = user_input.split()
    if len(user_input_list) == 2:
        first = user_input_list[0]
        last = user_input_list[1]
    elif len(user_input_list) == 1:
        first = user_input_list[0]
        last = ''
    print "https://www.linkedin.com/pub/dir/?first="+first+"&last="+last
    driver.get("https://www.linkedin.com/pub/dir/?first="+first+"&last="+last)
    driver.set_window_size(1124, 850)
    try:
        wait = WebDriverWait(driver, 15)
        wait.until(EC.visibility_of_element_located((By.CLASS_NAME, "content-wrapper")))
    except:
        "print timed out"
    final_result = []
    for s in driver.find_elements_by_class_name('vcard'):
        result = {}
        try:
            name = s.find_element_by_class_name('given-name').text
            result['name'] = name
            surname = s.find_element_by_class_name('family-name').text
            result['surname'] = surname
            fullname = name + ' ' + surname
            if user_input.strip() == user_input.strip():
                pass
            else:
                continue
            title = s.find_element_by_class_name('title').text
            result['title'] = title
            location = s.find_element_by_class_name('location').text
            result['location'] = location
            profile_url = s.find_element_by_tag_name('a').get_attribute('href')
            result['profile_url'] = profile_url
        except:
            pass
        try:
            industry = s.find_element_by_class_name('industry').text
            result['industry'] = industry
        except:
            pass
        try:
            current = s.find_element_by_class_name('current-content').text
            result['current'] = current
        except:
            pass
        try:
            past = s.find_element_by_class_name('past-content').text
            result['past'] = past
        except:
            pass
        try:
            education = s.find_element_by_class_name('education-content').text
            result['education'] = education
        except:
            pass
        try:
            img = s.find_element_by_class_name('photo').get_attribute('src')
            result['img'] = img
        except:
            pass
        print result

Proper error handling

Never, ever do something like this:

try:
    ...
except:
    ...

It's a horrible idea, for a couple of reasons:

It will catch every error, even the ones you don't want to catch.
Rare, nasty errors like a SystemError, will be caught, and ignored as well. Stuff like NameErrors, and even SyntaxErrors will be caught.
It's not very clear to the reader of your code, what you're trying to prevent.

For more info, see this Stackoverflow question.

Proper error logging

You have a lot of except: passes in your code. It's never helpful to the user, or even the programmer in some cases, if you don't get output when an error is caught. You could use the logging module, or write error output to a file, while informing the user in some way, that something went wrong.

Style

From looking at your code, you only appear to have two style violations:

Function names should be in snake_case.
Lines should be limited to 79/80 characters.

If you want to be sure, you can always check PEP8, Python's official style guide, to ensure that everything is correctly styled.


Hdfs count command tweak I am using below command to assign a value, Is there a alternative way to get by tweaking the code
var=hdfs dfs -ls /hdfs_path/ | grep col=2020_03 | awk -F' ' '{system("hdfs dfs -count "$8)}' | awk -F' ' '{if ($3 != 0) print $4}' | awk -F'=' '{print $2}'|sort -nr | uniq | head -1

Basically i am need to find the latest month value that holds data in hdfs directory. For eg
hdfs_path/col=2020_03_21/data.part - this has no data
    hdfs_path/col=2020_03_20/data.part - this has data
    hdfs_path/col=2020_03_19/data.part - this has data

hence output should be val = 2020_03_20

Just some comments on the last part of the command.
Since you don't use any of the extra features of uniq, you can drop replace
sort -nr | uniq
with
sort -nr -u

I don't see a reason to sort in reverse, and then take the first line with head -1, when you could instead use tail -1.
sort -nu | tail -1

While outputting duplicated lines to tail might take some time if there is a lot of duplication, since you only actually want the final line, you can probably drop the -u flag in sort. If you wanted the most recent 3 dates, you'd need to add it back in.


Node.js/Express middleware to relay requests to various APIs I am creating a nodejs middleware server that will handle api transactions from a frontend and relay them to various api's (internal and external). The primary goal is to hide api keys from the frontend. It can also consolidate requests that may require 2 or more endpoints, and modify data structures, thus simplifying our frontend code.

I am hoping for feedback on overall design/structure which will allow us to create a project that will be flexible as we add more apis while keeping it DRY

URLs, keys, etc. are stored in a .env file

index.js

require('dotenv').load();
const express = require('express'),
  app = express(),
  request = require("./request").request,
  bodyParser = require('body-parser');

app.use(bodyParser.json());

//returns all locations
app.get("/locations", (req, res) => {
  let URL = '...';
  request({
    method: 'get',
    url: URL,
    api: 'apiName'
  }).then((resp) => {
    res.json(resp);
  })
});

//send password reset link
app.post("/resetemail", (req, res) => {
  let URL = '...';
  let DATA = req.body;

  request({
    method: 'post',
    url: URL,
    api: 'apiName',
    data: DATA
  }).then((resp) => {
    res.json(resp);
  })
});

app.listen(3000);

request.js

module.exports.request = function (options) {

  const client = require('./' + options.api).instance;
  const handleError = require('./' + options.api).handleError;
  const onSuccess = function (response) {
    console.log('Request Successful!', response);
    return response.data;
  }

  return client(options)
    .then(onSuccess)
    .catch(handleError);
}

apiName.js

const axios = require('axios');
const axiosInstance = axios.create({
  headers: { "token": process.env.API_TOKEN },
  baseURL: process.env.API_BASE_URL,
  timeout: 2000
});

const handleError = function (error) {
  console.error('Request Failed:', error.config);

  if (error.response) {
    console.error('Status:', error.response.status);
    console.error('Data:', error.response.data);
    console.error('Headers:', error.response.headers);

  } else {
    console.error('Error Message:', error.message);
  }

  return Promise.reject(error.response || error.message);
}

module.exports = {
  instance: axiosInstance,
  handleError: handleError
}

I think you need to add some middle-wares that helps to run your API code after that middle-ware with proper structuring way.

I would suggest the NPM Module express-app-generator

The advantages of it include:

Code Management in clean way.
Structured Routing.
Add Multiple Middle-Wares in filters array.
Create CRUD API's with REST or CRUD Keyword.