Find and return the earliest occurring word from a string which contains the most repeating letters I'd like feedback on my solution to the outlined programming challenge (medium level). I've tried it fast in any way I know how to, but, what might be a more efficient and/or pythonic solution?

  [Letter Count II]
  Have the function LetterCount(str) take the str parameter being passed and
  return the first word with the greatest number of repeated letters. For
  example: "Today, is the greatest day ever!" should return greatest because it
  has 2 e's (and 2 t's) and it comes before ever which also has 2 e's. If
  there are no words with repeating letters return -1. Words will be separated
  by spaces.

import doctest
import logging
import timeit

from collections import Counter
from string import ascii_letters

def letter_count(s: str):
    """Return the word in s with the largest number of repeating letters

    >>> letter_count(s="Today, is the greatest day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the >>>>greatest day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the greatest >>>> day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the greatest >>>> day ever xx!")
    'greatest'
    >>> letter_count(s="")
    -1
    >>> letter_count(s="abcd efg hijk")
    -1
    """
    if s == "":
        return -1

    word_counters = dict((word, Counter(word)) for word in s.split(" "))

    # tuple of (word(str), most common letter frequency(int))
    most_repeating_word = ("", 0)

    for word, counter in word_counters.items():

        # most_common format = (letter(str), freq(int))
        for most_common in counter.most_common():

            if most_common[0] not in ascii_letters:
                continue

            if most_common[1] > most_repeating_word[1]:
                most_repeating_word = (word, most_common[1])
                break

    if most_repeating_word[1] <= 1:
        return -1

    return "".join(l for l in most_repeating_word[0] if l in ascii_letters)

if __name__ == "__main__":
    doctest.testmod()
    print(timeit.timeit("letter_count(s='!!!Today, is the greatest >>>> day ever!')",
                  setup=("from __main__ import letter_count; from collections "
                         "import Counter; from string import ascii_letters"),
                  number=10000))

The core problem can be solved a lot quicker using the built-in max and making a generator that produces the maximum count of repeated letters. Note that max already takes the first occurrence of the maximum, as required by the challenge, but we need to take care to not compare the words lexicographically, as I did in a previous version, by using a key function. You should probably add testcases for this, such as "free greatest" -> "free" and "greatest free" -> "greatest".

from collections import Counter
from operator import itemgetter

def letter_count(s):
    words = s.split(" ")
    most_common_letters = (Counter(word).most_common(1)[0][1] for word in words)
    return max(zip(most_common_letters, words), key=itemgetter(0))[1]

This does not handle any of the edge cases, though. It fails if the most common letter is not an ASCII character, but we can easily get around this by removing all of those right at the start. I also added your check against the empty string, but improving it to exit early if there is nothing left after filtering for ASCII characters, including the case that only spaces are left. I also split on all whitespace, not just single spaces, because this way multiple spaces are joined and all other whitespace characters have already been removed. I also use if not s instead of if s == "", because empty sequences are falsey and it sounds more like English this way.

from collections import Counter
from string import ascii_letters
from operator import itemgetter

ASCII_LETTERS = set(ascii_letters) | {" "}

def letter_count(s: str):
    """Return the word in s with the largest number of repeating letters

    >>> letter_count(s="Today, is the greatest day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the >>>>greatest day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the greatest >>>> day ever!")
    'greatest'
    >>> letter_count(s="!!!Today, is the greatest >>>> day ever xx!")
    'greatest'
    >>> letter_count(s="Today, is thee greatest day ever!")
    'thee'
    >>> letter_count(s="!!! >>>>")
    -1
    >>> letter_count(s="")
    -1
    >>> letter_count(s="abcd efg hijk")
    -1
    """
    s = "".join(c for c in s if c in ASCII_LETTERS)
    if not s.replace(" ", ""):
        return -1
    words = s.split()
    most_common_letters = (Counter(word).most_common(1)[0][1] for word in words)
    best_word = max(zip(most_common_letters, words), key=itemgetter(0))
    return best_word[1] if best_word[0] > 1 else -1

Note that the challenge design is not the best. Normally I would recommend against returning differing types from a function unless absolutely necessary. Having it return a string in most cases and sometimes an int can be dangerous. Better to at least return None, if you absolutely need a special value, or even better, just let the caller deal with an exception.

Otherwise your code looks quite alright. You have a docstring, tests and a main guard. I must say that your comments don't help me a lot, I personally prefer plain English or, even better, self-explanatory variable names. But naming things is hard, and you have already done a good enough job at that.


Singleton manager/service classes with mockable dependencies in swift In our swift project, there are multiple service classes that encapsulate functions, mostly network-api calls. They implement a protocol, and depend on other service classes, e.g. an authorization service or a network facade.

Their instances should only exist once, hence they are designed as singletons, with a private constructor and a single point to access, like so:

static let defaultInstance = MyManagerImplementation(networkFacade: MyApiFacade())

In order to write unit tests for each service class, i want to mock the dependencies. Sadly this is not possible in the code above.

So i re-wrote the initialization like this.

let networkFacade: MyApiFacade

private static var instance: MyManagerImplementation?

static func defaultInstance(apiFacade apiFacade: MyApiFacade = RestApiImplementation.defaultInstance) -> MyManager {
    if instance == nil {
        instance = MyManagerImplementation(networkFacade: apiFacade)
    }
    return instance!
}

init(networkFacade: MyApiFacade) {
    self.networkFacade = networkFacade
}

Which should be the classic singleton pattern, as i know it from other languages.

The problem i have with this is that swift seemingly wants to avoid this way of defining a singleton. It is way longer than the single-line-singleton, which is considered best practice in many posts i found on the web.

So, would it make sense to you to define these service-classes completely static? Or would you go with another solution or the solution above?

Your singleton pattern leaves a lot to be desired.

It is not thread safe.
It makes use of the crash operator (!)
The class can be initialized any number of times--this isn't a true singleton
The fact that defaultInstance is a function which can be passed a parameter can lead to misleading results.

Thread-safety
The simplest way to make singletons thread safe in Swift is pretty straight-foward.
class MySingleton {
    private init(){}
    static let sharedInstance = MySingleton()
}

But given we want our singleton to potentially have different types, we need a slightly more complicated pattern.
Without account for any of the other points I want to address, the simplest solution looks like this:
static func defaultInstance(apiFacade apiFacade: MyApiFacade = RestApiImplementation.defaultInstance) -> MyManager {

    struct Static {
        static var onceToken: dispatch_once_t = 0
        static var instance: MyManagerImplementation?
    }

    dispatch_once(&Static.onceToken) {
        Static.instance = MyManagerImplementation(networkFacade: apiFacade)
    }

    return Static.instance!
}

The Crash Operator
Force unwrapping optional variables always leaves us with the potential to crash.  Even in a case like this where crashing probably is the correct behavior (because for the variable to be nil is presumably impossible), we still deserve better diagnostic information in the case of the crash.
By simply changing the last line from return Static.instance! to this:
guard let instance = Static.instance else {
    fatalError("Unable to obtain reference to MyManager singleton instance.")
}

return instance

We're doing the exact same thing, but in the case of a crash, we get better diagnostic information.
Importantly, if for some reason in the future, the MyManagerImplementation(networkFacade:) constructor were ever turned into a failable initializer, your app is all of the sudden going to start crashing without Xcode providing any warnings here.
And if left unchanged, you'll have no diagnostic information beyond:

fatal error: Unexpectedly found nil while unwrapping an optional

So... a few extra lines can be very helpful.

Not a true singleton
Because your initializer is perfectly accessible outside of the file, this is not a true singleton.  You do have a shared instance available, but other instances can be instantiated.
And perhaps importantly, if you compare your original implementation to the one posted above with the nested struct, even though your instance property is private, it has to be declared as a var (let wouldn't work), and that means it is possible to change the reference.
This is important to note because you can't stop users from capturing a reference to the original instance and continuing to make calls on it even after you've changed the value the singleton returns.

Misleading Results
Your defaultInstance function accepts and argument, and that argument has a default value of RestApiImplementation.defaultInstance.  This works for allowing your first time instantiation to choose how the singleton is set up.
But... we're going to get misleading results if the first time we called it we passed one sort of implementation, and then later we tried calling it with the other type of implementation.  Your code doesn't change the implementation, and despite me calling it requesting it be set up with Foo, you return me a Bar.
Perhaps better would be a method to initialize the singleton with a particular value?  And then you have to decide what happens if initialize is called more than once.  On the second and later calls, you must either reinitialize with the new value or you should do something to indicate that the action the user requested wasn't able to be performed.  So perhaps, you should throw an error here.


Going from IQueryable> to IEnumerable I have a "GetPersonsPerDepartment()" method returning a IQueryable<IEnumerable<Person>> while I'm trying to get a simple List<Person> or IEnumerable<Person> with all the persons from every department. I wrote a simple method GetAllPersons() that solves this with a loop but I feel I must be missing a simple command to get the same result:

private List<Person> GetAllPersons()
{
    List<Person> allPersons = new List<Person>();

    foreach(var Persons in GetPersonsPerDepartment())
    {
        allPersons.AddRange(Persons);
    }

    return allPersons;
}

private IQueryable<IEnumerable<Person>> GetPersonsPerDepartment()
{
    IQueryable<IEnumerable<Person>> Persons;

    using (var context = MySource.GetPersonsContext())
    {
        var library = "Personlist";
        var PersonFolderList = context.Web.Lists.GetByTitle(library);
        context.Load<List>(PersonFolderList);

        var PersonFolderListItems = PersonFolderList.GetItems(new CamlQuery());
        context.Load<ListItemCollection>(PersonFolderListItems);

        context.ExecuteQuery();

        Persons = PersonFolderListItems.Where(x => x.FileSystemObjectType == FileSystemObjectType.Folder).Select(x => GetPersonsFromFolderListItem(x, context, PersonFolderList));
    }

    return Persons;
}

Use SelectMany. As in, allPersons.SelectMany(s => s);. This flattens the sequence for you.


Creating a numeric phone keypad I'm writing a Telephone class and it has a method called getDigits which takes in a String parameter and returns the number that would appear on your phone if you typed in those letters the old fashioned way.

Example: Typing in "CAT" would return 228.

I wrote the following code and it works but I wanted to know if anyone else out there had better ideas to do it/ more sophisticated ones. I'm learning about data abstraction/enums and maybe that's a clue to how my professor wanted me to approach this but I'm stumped.

public int getDigits(String test){
        String result = "";
        String param = test.toUpperCase();
        for(int i = 0; i<param.length(); i++){
            String s = Character.toString(param.charAt(i));
            if(s.equals("A") || s.equals("B") || s.equals("C")){
                result += 2;
            }

            else if(s.equals("D") || s.equals("E") || s.equals("F")){
                result += 3;
            }

            else if(s.equals("G") || s.equals("H") || s.equals("I")){
                result += 4;
            }

            else if(s.equals("J") || s.equals("K") || s.equals("L")){
                result += 5;
            }

            else if(s.equals("M") || s.equals("N") || s.equals("O")){
                result += 6;
            }

            else if(s.equals("P") || s.equals("Q") || s.equals("R") || s.equals("S")){
                result += 7;
            }
            else if(s.equals("T") || s.equals("U") || s.equals("V")){
                result += 8;
            }

            else if(s.equals("W") || s.equals("X") || s.equals("Y") || s.equals("Z")){
                result += 9;
            }
        }
        return Integer.parseInt(result);
    }

The text-to-key problem is one that has multiple solutions. The Map solution is a good one, there's nothing wrong with it. There are two other algorithms that are good too. First, though, please convert the char-to-key to a function. Having such a big loop is a problem.

public static int charToKey(char c) {
    ... do something to return key ....
}

A switch statement is similar to your cascading if's, except it is neater, and faster:

switch (c) {
    case 'A': case 'B': case 'C':
        return 2;
    case 'D': case 'E': case 'F':
        return 3;
    ....
}

Note that I abuse the layout of the switch to have multiple chars on a line.... I sometimes do that for structured inputs like this. Also, note that a case block with a return statement does not need a break - that's really nice when putting switches in to functions - just return, don't break.

A second alternative to a Map, is a mathematical function. The math one is interesting.... because it is fast. It's not that readable, though.....

Consider your keys, most keys have 3 letters, but 7 and 9 have 4 letters. If we treat Z as a special case, we can think of 9 as having just 3, which leaves only key 7 with 4 letters. Now, if we lay those letters on to a number line, where each letter occupies just... 0.32 of a span (we choose 0.32 because 4 of them is just slightly less than 1), and we organize that P is aligned with 7.0 ..... and we also take the integer value of the result... we have:

    double span = 0.32;
    double offset = 7.0 - span * ('P' - 'A');
    for (char c = 'A'; c <= 'Z'; c++) {
        double val = offset + span * (c - 'A');
        int key = (int)val;
        System.out.printf("%s -> %.3f ==> %d\n", c, val, key);
    }

and that produces:

A -> 2.200 ==> 2
B -> 2.520 ==> 2
C -> 2.840 ==> 2
D -> 3.160 ==> 3
E -> 3.480 ==> 3
F -> 3.800 ==> 3
G -> 4.120 ==> 4
H -> 4.440 ==> 4
I -> 4.760 ==> 4
J -> 5.080 ==> 5
K -> 5.400 ==> 5
L -> 5.720 ==> 5
M -> 6.040 ==> 6
N -> 6.360 ==> 6
O -> 6.680 ==> 6
P -> 7.000 ==> 7
Q -> 7.320 ==> 7
R -> 7.640 ==> 7
S -> 7.960 ==> 7
T -> 8.280 ==> 8
U -> 8.600 ==> 8
V -> 8.920 ==> 8
W -> 9.240 ==> 9
X -> 9.560 ==> 9
Y -> 9.880 ==> 9
Z -> 10.200 ==> 10

Note how all the characters have valid values, except Z. Now, there's a trick with that... which is as ugly as anything, but it works really well.... key -= key/10 - subtract an integral 10'th of the value from itself.

What does this mean? Well, it means that your function can be (for valid input):

public static int charToKey(char c) {
    int key = (int)(2.2 + (c - 'A') * 0.32);
    return key - key / 10;
}

If you want to validate the input, something like:

char uc = Character.toUpperCase(c) - 'A';
if (uc < 0 || uc >= 26) {
    return 0;
}
int key = (int)(2.20 + uc * 0.32);
return key - key / 10;

Update

Since this became a benchmarking session comparing various options, and since a new option came up in discussions with @chillworld, it seems an update is in order.

First up, I was reminded that this:

int length = text.length();
for (int i = 0; i < length; i++) {....

is faster than:

for (int i = 0; i < text.length(); i++) {....

I checked that, and, in fact, it is true. I consistently get the following benchmark times:

Task Keys -> Calc: (Unit: MILLISECONDS)
  Count    :     1000      Average  :   0.2914
  Fastest  :   0.2749      Slowest  :   2.3255
  95Pctile :   0.3327      99Pctile :   0.4448
  TimeBlock : 0.315 0.285 0.292 0.285 0.284 0.280 0.293 0.283 0.299 0.297
  Histogram :   999     0     0     1

Task Keys -> Cached: (Unit: MILLISECONDS)
  Count    :     1000      Average  :   0.2802
  Fastest  :   0.2665      Slowest  :   1.9090
  95Pctile :   0.3212      99Pctile :   0.4237
  TimeBlock : 0.298 0.275 0.284 0.276 0.276 0.269 0.284 0.274 0.283 0.284
  Histogram :   999     0     1

Where the different code paths are just:

public static int calced(String text) {
    int result = 0;
    for (int i = 0; i < text.length(); i++) {
        result *= 10;
        result += charToKey(text.charAt(i));
    }
    return result;
}

public static int cached(String text) {
    int result = 0;
    final int len = text.length();
    for (int i = 0; i < len; i++) {
        result *= 10;
        result += charToKey(text.charAt(i));
    }
    return result;
}

"Caching" the String length has a small (1% in this case) improvement in performance.

More interestingly, the idea @Chillworld had of creating a lookup array for each relevant character is much, much faster. His idea started off as:

private static final int[] cache = new int[91];
static {
    cache[65] = 2;
    cache[66] = 2;
    cache[67] = 2;
    cache[68] = 3;
    cache[69] = 3;
    cache[70] = 3;
    ....

Essentially, build an array with a direct 1-to-1 mapping between the char value, and the phone key number.

Until this point, the fastest transform was by doing a calculation, but comparing that calculation with:

private static int cached(String text) {
    int result = 0;
    for (int i = 0; i < text.length(); i++) {
        result *= 10;
        result += cache[text.charAt(i)];
    }
    return result;
}

Note the direct cache[text.charAt(i)] call. The cached performance (compared with the calc performance), is:

Task Keys -> Calc: (Unit: MILLISECONDS)
  Count    :     1000      Average  :   0.2914
  Fastest  :   0.2749      Slowest  :   2.3255
  95Pctile :   0.3327      99Pctile :   0.4448
  TimeBlock : 0.315 0.285 0.292 0.285 0.284 0.280 0.293 0.283 0.299 0.297
  Histogram :   999     0     0     1

Task Keys -> rawcache: (Unit: MILLISECONDS)
  Count    :     1000      Average  :   0.1425
  Fastest  :   0.1328      Slowest  :   1.7173
  95Pctile :   0.1633      99Pctile :   0.2393
  TimeBlock : 0.159 0.142 0.142 0.139 0.138 0.138 0.144 0.137 0.142 0.144
  Histogram :   998     1     0     1

Essentially it runs in half the time.

Validated

Additional thoughts about how I would make the lookup process neat, and how I would include other ideas in my answer as well as other answers, lead me to revised code with the following features:

fast by using a lookup
handles upper case, and lower case characters
handles the raw digits too (like 1800MyPhone)
returns an int array, not an int:

allowing for leading 0 values
allowing for longer-than-9-digit numbers.

strips off unrecognized characters

In essence, the following number 1(800)-My-Phone will translate to [1, 8, 0, 0, 6, 9, 7, 4, 6, 6, 3]

private static int switchKey(char c) {
    switch (Character.toUpperCase(c)) {
        case '0':
            return 0;
        case '1':
            return 1;
        case '2': case 'A': case 'B': case 'C':
            return 2;
        case '3': case 'D': case 'E': case 'F':
            return 3;
        case '4': case 'G': case 'H': case 'I':
            return 4;
        case '5': case 'J': case 'K': case 'L':
            return 5;
        case '6': case 'M': case 'N': case 'O':
            return 6;
        case '7': case 'P': case 'Q': case 'R': case 'S':
            return 7;
        case '8': case 'T': case 'U': case 'V':
            return 8;
        case '9': case 'W': case 'X': case 'Y': case 'Z':
            return 9;

    }
    return -1;
}

private static final int[] KEY_LOOKUP = buildLookup("ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789");

private static int[] buildLookup(String candidates) {
    int max = -1;
    for (char c : candidates.toCharArray()) {
        max = Math.max(max,
                Math.max(Character.toLowerCase(c), Character.toUpperCase(c)));
    }
    int[] lookup = new int[max + 1];
    Arrays.fill(lookup, -1);
    for (char c : candidates.toCharArray()) {
        lookup[Character.toLowerCase(c)] = switchKey(c);
        lookup[Character.toUpperCase(c)] = switchKey(c);
    }
    return lookup;
}

/**
 * Map a phone key character to a phone key number. Return -1 for invalid
 * input characters.
 *
 * @param ch
 *            The character to convert.
 * @return the corresponding number (0 though 9), or -1 for invalid numbers
 */
public static final int lookupKey(final char ch) {
    return ch >= KEY_LOOKUP.length ? -1 : KEY_LOOKUP[ch];
}

/**
 * Parse an input string for characters that are valid phone-key characters,
 * and return the corresponding phone digits as an array.
 *
 * @param text
 *            the text to parse for phone numbers
 * @return the valid phone digits from the input text.
 */
public static int[] parsePhoneNumber(String text) {
    final int length = text.length();
    int[] result = new int[length];
    int size = 0;
    for (int i = 0; i < length; i++) {
        int key = lookupKey(text.charAt(i));
        if (key >= 0) {
            result[size++] = key;
        }
    }
    if (size == length) {
        return result;
    }
    return Arrays.copyOf(result, size);
}


Analysis of the most common and salient words in a text I've been trying to get more understanding and experience in the Natural Language Processing space and, to get some more practice, decided to attempt to make a simple high-level analysis of the "Industrial Society and its Future" (aka "Unabomber Manifesto").

Here is what I am trying to do:

download the text of the Manifesto from the Washington Post website
preprocess it using nltk:

tokenize into words
remove the non-alpha words
remove English stop words
lemmatize the tokens

count most commonly used words
use the TF-IDF model from gensim library to compute the most popular and most rarely used / important words

The code:

from bs4 import BeautifulSoup

from gensim.models.tfidfmodel import TfidfModel
from gensim.corpora.dictionary import Dictionary

from nltk import word_tokenize, Counter, sent_tokenize
from nltk.stem import WordNetLemmatizer
from nltk.corpus import stopwords

import requests

ENGLISH_STOPS = set(stopwords.words('english'))

def retrieve_text(url):
    """Downloads HTML content from a URL, parses with an HTML parser and returns text only."""
    response = requests.get(url)

    soup = BeautifulSoup(response.content, "html.parser")
    return soup.get_text()

def preprocess(text):
    """Pre-processes the text, splits into tokens that are lower-cased, filtered and lemmatized."""
    tokens = word_tokenize(text)

    # lowering
    lower_tokens = [token.lower() for token in tokens]

    # Retain alphabetic words: alpha_only
    alpha_only = [t for t in lower_tokens if t.isalpha()]

    # Remove all stop words: no_stops
    no_stops = [t for t in alpha_only if t not in ENGLISH_STOPS]

    # Lemmatize all tokens into a new list: lemmatized
    wordnet_lemmatizer = WordNetLemmatizer()
    lemmatized = [wordnet_lemmatizer.lemmatize(t) for t in no_stops]

    return lemmatized

def tf_idf(text):
    """
    Computes the TF-IDF of a text - using every sentence as a separate "document".
    Returns a list of tuples with words and weights.
    """
    texts = [preprocess(sentence) for sentence in sent_tokenize(text)]

    dictionary = Dictionary(texts)
    corpus = [dictionary.doc2bow(text) for text in texts]

    tfidf = TfidfModel(corpus)
    corpus_tfidf = tfidf[corpus]

    tfidf_weights = {dictionary.get(id): value
                     for doc in corpus_tfidf
                     for id, value in doc}
    sorted_tfidf_weights = sorted(tfidf_weights.items(), key=lambda w: w[1])

    return sorted_tfidf_weights

if __name__ == '__main__':
    content = retrieve_text("http://www.washingtonpost.com/wp-srv/national/longterm/unabomber/manifesto.text.htm")

    tokens = preprocess(content)
    token_counter = Counter(tokens)

    most_common = token_counter.most_common(10)
    tf_idf_results = tf_idf(content)
    popular_terms, rare_terms = tf_idf_results[:10], tf_idf_results[-10:]

    # print out words only (without counts and weights)
    keys = lambda x: next(zip(*x))
    print(f"Most common words: {keys(most_common)}")
    print(f"Most popular terms: {keys(popular_terms)}")
    print(f"Most unique/important terms: {keys(rare_terms)}")

Prints:

Most common words: ('society', 'system', 'people', 'power', 'would', 'one', 'human', 'technology', 'leftist', 'need')
Most popular terms: ('society', 'people', 'freedom', 'whole', 'human', 'lead', 'system', 'necessity', 'process', 'use')
Most unique/important terms: ('license', 'simplification', 'personnel', 'carried', 'crossroad', 'eminent', 'exactly', 'paramount', 'danger', 'virtue')

I am still learning and would like my code to be reviewed. Would appreciate any feedback about the code quality, performance or any other possible improvements.

Also, I am not 100% sure the TF-IDF model is applied in the most appropriate way. Currently, I am treating each sentence as a separate document. Is this a good approach to detect the most "important"/"valuable" terms/words, or should I use other articles/posts as documents for the corpus?

First, I will start with PEP 8 specifications. The PEP 8 analysis shows the following:

E501:17:80:line too long (94 > 79 characters)
E501:25:80:line too long (99 > 79 characters)
E501:46:80:line too long (82 > 79 characters)
E501:66:80:line too long (114 > 79 characters)
E731:76:5:do not assign a lambda expression, use a def
W292:79:62:no newline at end of file

If I am not incorrect, one of the things that you are trying is to calculate the frequency distribution of words, namely the most common words and most rare words. I tried to re-write your code like viz.

from bs4 import BeautifulSoup
from nltk import FreqDist, re

import requests

def retrieve_text(url):
    """Downloads HTML content from a URL, parses with an HTML parser and returns text only."""
    response = requests.get(url)

    soup = BeautifulSoup(response.content, "html.parser")
    return soup.get_text()

if __name__ == '__main__':

    content = retrieve_text("http://www.washingtonpost.com/wp-srv/national/longterm/unabomber/manifesto.text.htm")
    wordList = re.sub("[^\w]", " ", content).split()
    fdist = FreqDist(wordList)

    # print out words only (without counts and weights)
    print(fdist.most_common(20))

As you see in the output, I am obviously not skipping connectorwords, like the,  of etc.:

[('the', 1735), ('of', 1251), ('to', 1078), ('a', 736), ('and', 719), ('that', 657), ('is', 611), ('in', 546), ('be', 412), ('for', 351), ('it', 319), ('or', 290), ('are', 281), ('have', 246), ('society', 239), ('not', 235), ('will', 233), ('as', 230), ('by', 229), ('they', 224)]

but, the execution was much faster and I did not have to spend a huge time downloading the gensim library. Off course if you need something as sophisticated as gensim, I have no substitute, else for frequency, this maybe much faster and you can easily write a method to remove connector words.


Very basic Integer queue in C I've never written a queue before and literally just took the concepts and tried to code something up. I decided to write a basic integer queue and as an example/driver I use a queue of size 5 which implements insert(), delete(), and display().

Please critique these implementations and comment if there should be any additions or modifications. The reason for the simplicity is that I'm mostly concerned that I got the concepts correct at this point. This is not the most robust queue by any means.

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

#define ERR(msg) fprintf(stderr, "%s\n", msg)
#define QUEUE_SIZE 5

static size_t const front = 0;
static size_t rear = 0;

int queue[QUEUE_SIZE];

int insert(int num);
int delete(void);
void display(void);

int main(void)
{
    insert(5);
    insert(8);
    insert(58);
    insert(9);
    insert(10);
    display();
    delete();
    display();
    delete();
    display();
    delete();
    display();
    delete();
    display();

    return EXIT_SUCCESS;
}

int insert(int num)
{
    if((rear + 1) > QUEUE_SIZE)
    {
        ERR("Queue full, cannot insert item.");
        return -1;
    }
    queue[rear++] = num;
    return 0;
}

int delete(void) // Deletes the front one
{
    if(rear == front)
    {
        ERR("Cannot delete from empty queue.");
        return -1;
    }

    size_t i;

    for(i = front; (i+1) < QUEUE_SIZE; i++)
    {
        queue[i] = queue[i+1];
    }
    rear--;
    return 0;
}

void display(void)
{
    size_t i;
    for(i = front; i < rear; i++)
    {
        printf("%d\t", queue[i]);
    }
    putchar('\n');
    return;
}

The delete function is not right. It is very non-optimal to shift every item by one index after each deletion. Imagine the queue has 1 million items and the deletion of front element is needed.

From Wikipedia:

  Fixed length arrays are limited in capacity, but it is not true that
  items need to be copied towards the head of the queue. The simple
  trick of turning the array into a closed circle and letting the head
  and tail drift around endlessly in that circle makes it unnecessary to
  ever move items stored in the array. If n is the size of the array,
  then computing indices modulo n will turn the array into a circle.

So, you should use a Circular buffer.

This video has a good explanation of queue and circular buffer: Data structures: Array implementation of Queue.


Insert an array[4] to an array[8] (C++, SSE) I have this code to get audio output levels in dB to an array (peak_dB[8]) to be used in real time peakmeter:
#include <emmintrin.h>

float channelsPeak[8] = { 0 };
float peak_dB[8] = { 0 };

__m128 log2_sse(__m128 x) {
  // https://www.kvraudio.com/forum/viewtopic.php?p=7524831#p7524831
  // 12-13ulp
  const __m128 c0 = _mm_set1_ps(1.011593342e+01f);
  const __m128 c1 = _mm_set1_ps(1.929443550e+01f);
  const __m128 d0 = _mm_set1_ps(2.095932245e+00f);
  const __m128 d1 = _mm_set1_ps(1.266638851e+01f);
  const __m128 d2 = _mm_set1_ps(6.316540241e+00f);
  const __m128 one = _mm_set1_ps(1.0f);
  const __m128 multi = _mm_set1_ps(1.41421356237f);
  const __m128i mantissa_mask = _mm_set1_epi32((1 << 23) - 1);
  __m128i x_i = _mm_castps_si128(x);
  __m128i spl_exp = _mm_castps_si128(_mm_mul_ps(x, multi));
  spl_exp = _mm_sub_epi32(spl_exp, _mm_castps_si128(one));
  spl_exp = _mm_andnot_si128(mantissa_mask, spl_exp);
  __m128 spl_mantissa = _mm_castsi128_ps(_mm_sub_epi32(x_i, spl_exp));
  spl_exp = _mm_srai_epi32(spl_exp, 23);
  __m128 log2_exponent = _mm_cvtepi32_ps(spl_exp);
  __m128 num = spl_mantissa;
  num = _mm_add_ps(num, c1);
  num = _mm_mul_ps(num, spl_mantissa);
  num = _mm_add_ps(num, c0);
  num = _mm_mul_ps(num, _mm_sub_ps(spl_mantissa, one));
  __m128 denom = d2;
  denom = _mm_mul_ps(denom, spl_mantissa);
  denom = _mm_add_ps(denom, d1);
  denom = _mm_mul_ps(denom, spl_mantissa);
  denom = _mm_add_ps(denom, d0);
  __m128 res = _mm_div_ps(num, denom);
  res = _mm_add_ps(log2_exponent, res);
  return res;
}
__m128 lin2db(__m128 x) {
  const __m128 convert_10 = _mm_set1_ps(6.02059991328f);
  return _mm_mul_ps(log2_sse(x), convert_10);
}

float getPeaks_dB(int cCount) { // cCount = enabled channels (1...8)
    __m128 s1 = _mm_setzero_ps();

    s1 = _mm_set_ps(channelsPeak[3], channelsPeak[2], channelsPeak[1], channelsPeak[0]); //channels 1-4
    s1 = lin2db(s1);
    _mm_store_ps(peak_dB, s1);

    if(cCount > 4){
        float t2[4] = { 0 };
        __m128 s2 = _mm_setzero_ps();
        s2 = _mm_set_ps(channelsPeak[7], channelsPeak[6], channelsPeak[5], channelsPeak[4]); // channels 5-8
        s2 = lin2db(s2);

        _mm_store_ps(t2, s2);
        for (int i = 4; i < 8; i++){peak_dB[i] = t2[i-4];}
    }
    return 0;
}

where float channelsPeak[0..7] array is storage for linear levels (0.0f..1.0f) of  eight channels read from audio rendering device (one GetChannelsPeakValues() call), peak_dB is array of eight elements to hold this value in dB format (to be used in some later calculations and textual representation) and lin2db is 20log10(x) approximation (faster (because of lower accuracy) than std::log10) implemented using SSE intrinsics.
Q: Are there other (better) ways to insert data from s2 into last four elements of peak_dB (AVX excluded)?
I'm using VS2013 and, by Compiler Explorer, compiler seem to improve for-loop as used in code now but, as the 1st part in peak_dB stored with _mm_store_ps(peak_dB, s1) looks there much simpler, just wondering if there's a way doing it without for-loop.

Storing

Are there other (better) ways to insert data from s2 into last four elements of peak_dB (AVX excluded)?

Yes, actually you already used that way: it's _mm_store_ps. For example:
    s2 = lin2db(s2);
    _mm_store_ps(peak_dB + 4, s2);

Maybe you prefer &peak_dB[4] instead of peak_dB + 4, that works just fine too.
_mm_store_ps takes a pointer to wherever you want to store the data, that pointer does not have to point to the start of an array.
Loading
An other problem here is the use of _mm_set_ps. Though it accepts variable arguments, it is mostly meant for constant arguments, and good code is far from guaranteed if it is used differently. In the code on Godbolt, you can see the effect. Here I removed the code from lin2db from it that got "interleaved" into it:
    movss   xmm1, DWORD PTR float * channelsPeak+12
    movss   xmm0, DWORD PTR float * channelsPeak+8
    movss   xmm2, DWORD PTR float * channelsPeak+4
    movss   xmm4, DWORD PTR float * channelsPeak
    unpcklps xmm2, xmm1
    unpcklps xmm4, xmm0
    unpcklps xmm4, xmm2

And this is seen for the other similar instance of _mm_set_ps as well:
    movss   xmm1, DWORD PTR float * channelsPeak+28
    movss   xmm0, DWORD PTR float * channelsPeak+24
    movss   xmm2, DWORD PTR float * channelsPeak+20
    movss   xmm3, DWORD PTR float * channelsPeak+16
    unpcklps xmm3, xmm0
    unpcklps xmm2, xmm1
    unpcklps xmm3, xmm2

Avoid this pattern, try to use _mm_load(u)_ps if possible. That's easy in this case:
s1 = _mm_loadu_ps(channelsPeak);
// later
s2 = _mm_loadu_ps(channelsPeak + 4);

Zeroing
Initializing local variables of type __m128 like this is fine:
__m128 s1 = _mm_setzero_ps();

There's no serious problem with that, but it does not do anything useful in this code, it's just redundant. Of course if the value of zero is used as an input to something, then it should be done. But here it can just as well be skipped, declaring the variable only when you have a value to assign to it, for example:
__m128 s1 = _mm_loadu_ps(channelsPeak);


bash script - sed - template file processing Using a bash script to process a file and pass it to AWS CLI. The script runs inside a docker container.

I'm using sed to do template string replace. When MY_VALUE contains special chars. (eg: MY_VALUE="http://aaa.com") the sed command fails.

This makes me question if sed is the right tool for this job, don't have a lot of control on the values that come in.

Because the template is a JSON file, I considered trying JQ or Python (something like this).

I haven't used bash or Unix tools a lot so I would like to hear your thoughts on:

Is sed the right tool for this type of job?
What else would you use?
What do you think about the script?

Simplified template (JSON)

{
    "networkMode": "bridge",
    "name": "[Placeholder.A]"
}

Script

#!/bin/bash

MY_VALUE=${myvalue}
rm -f processed.json

cat /app/template.json | \
sed s/\\[Placeholder.A]/${MY_VALUE}/ > /app/processed.json

aws cli command -file processed.json

You are parsing a JSON file using some UNIX tools. This is risky, since those can have so many edge cases that you will end up refactoring over and over.

For example, you mention that sed s/\\[Placeholder.A]/${MY_VALUE}/ is giving errors when $MY_VALUE has slashes. This is normal, since this is how it gets expanded:

sed s/\[Placeholder.A]/${MY_VALUE}/
                           ↓
sed s/\[Placeholder.A]/http://aaa.com/
#    ^                ^     ^^       ^

See? Too many slashes for a simple sed 's/find/replace/, thus it fails. You can solve this by using another separator:

$ sed 's/\[Placeholder.A\]/a/' file
{
    "networkMode": "bridge",
    "name": "a"
}
$ MY_VALUE="http://aaa.com"
$ sed "s/\[Placeholder.A\]/$MY_VALUE/" file
sed: -e expression #1, char 27: unknown option to `s'

# Let's use ? as a separator, for example
$ sed "s?\[Placeholder.A\]?$MY_VALUE?" file
{
    "networkMode": "bridge",
    "name": "http://aaa.com"
}

Instead, use jq, a command-line JSON processor. Given your file:

$ cat file
{
    "networkMode": "bridge",
    "name": "[Placeholder.A]"
}

See how easy is to replace the value in the "name" key:

$ jq '.name="a"' file
{
  "networkMode": "bridge",
  "name": "a"
}

If you want to use a variable then you will need to do some quotes-opening-closing:

$ MY_VALUE="http://aaa.com"
$ jq '.name="'$MY_VALUE'"' file
{
  "networkMode": "bridge",
  "name": "http://aaa.com"
}


Mutex helper for interprocess sychronization of accessing a network drive I have two windows services working in parallel. Each of them is using a network drive that is connected and disconected dynamically by each one as required. They do it by executing net use command. (This is necessary because there've been issues with automatic reconnection when the server is rebooted. Not it's running stable.)

However, from time to time both services overrun and one of them fails to connect the drive because it's already connected by the other one.

To address this problem I thought I use a named Mutex to prevent both services trying to use that network location at the same time.

I didn't want to think about mutexes each time I want to synchronize that access so I created this small helper encapsulating a named Mutex.

public static class InterprocessSynchronizer
{
    public static async Task<T> TryExecuteAsync<T>(string resourceName, Func<CancellationToken, Task<T>> func, TimeSpan timeout, CancellationToken cancellationToken)
    {
        using (var mutex = new Mutex(initiallyOwned: false, name: resourceName))
        {
            if (mutex.WaitOne(timeout))
            {
                try
                {
                    return await func(cancellationToken);
                }
                finally
                {
                    mutex.ReleaseMutex();
                }
            }
        }
        return default(T);
    }
}

I tested it with two threads and it succeeded. I used Task.Factory.StartNew here for test purposes (mutexes require different threads) but I'm not going to use it later when they will be running in two different processes anyway.

[TestClass]
public class InterprocessSynchronizerTest
{
    [TestMethod]
    public async Task TryExecuteAsync_TwoTasks_Synchronized()
    {
        var task1 = Task.Factory.StartNew(async () => await InterprocessSynchronizer.TryExecuteAsync("foo", token =>
        {
           Thread.Sleep(2000);
           return Task.FromResult("abc");
        }, TimeSpan.FromMilliseconds(-1), CancellationToken.None));

        var task2 = Task.Factory.StartNew(async () => await InterprocessSynchronizer.TryExecuteAsync("foo", token =>
        {
            Thread.Sleep(3000);
            return Task.FromResult("xyz");
        }, TimeSpan.FromMilliseconds(-1), CancellationToken.None));

        var stopwatch = Stopwatch.StartNew();

        var result = await Task.WhenAll(task1, task2);

        stopwatch.Stop();

        Assert.AreEqual("abc", task1.Result.Result);
        Assert.AreEqual("xyz", task2.Result.Result);

        Assert.IsTrue(stopwatch.ElapsedMilliseconds >= 5000);
    }
}

Is this solution acceptable or am I doing something terribly wrong?

Unfortunately you are in fact doing something wrong, but maybe not terribly! Because you are accessing the Mutex either side of an await, there is no guarantee that the same thread will be accessing it each time. The question and answer here have more detail. This is only a problem if your SynchronizationContext doesn't guarantee async calls will return to the same thread (as I suspect is the case when running your tests). If you only call this code from the UI thread of a WPF app for example, you won't have any issues. The code breaks when used in a console app however.

public class Program
{
    public static async Task Main()
    {
        using (var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)))
        {
            await Task.WhenAll(
                InterprocessSynchronizer.TryExecuteAsync("resource", c => SwitchThreads("FIRST", c), TimeSpan.FromSeconds(60), cts.Token),
                InterprocessSynchronizer.TryExecuteAsync("resource", c => SwitchThreads("SECOND", c), TimeSpan.FromSeconds(60), cts.Token)
            );
        }
    }

    private static async Task<int> SwitchThreads(string name, CancellationToken arg)
    {
        Console.WriteLine($"{name} ON THREAD ID {Thread.CurrentThread.ManagedThreadId}");
        await Task.Delay(1000, arg);
        Console.WriteLine($"{name} NOW ON THREAD ID {Thread.CurrentThread.ManagedThreadId}");
        return 0;
    }
}

  Object synchronization method was called from an unsynchronized block of code

If you encounter this error some of the suggesting in the afformentioned SO question could help.


Determining whether a loop iterated at least one element in generator function I have a case in which I need to determine whether a for loop yielded at least one element:

@property
def source_customers(self):
    """Yields source customers"""
    for config in RealEstatesCustomization.select().where(
            RealEstatesCustomization.customer == self.cid):
        yield config.source_customer
    # Yield self.customer iff database
    # query returned no results
    try:
        config
    except UnboundLocalError:
        yield self.customer

Is this a pythonic way to go or shall I rather use a flag for that like so:

@property
def source_customers(self):
    """Yields source customers"""
    yielded = False
    for config in RealEstatesCustomization.select().where(
            RealEstatesCustomization.customer == self.cid):
        yield config.source_customer
        yielded = True
    # Yield self.customer iff database
    # query returned no results
    if not yielded:
        yield self.customer

What is preferable?

Separate Your Concerns

The function at the moment does two things: finds matching customers and checks that it's nonempty. Let's instead add a default checker:

from itertools import tee

def iter_or_default(iterable, dflt):
    orig, test = tee(iter(iterable))
    try:
        next(test)
        # if we're still here, the iterable is non-empty
        yield from orig
    except StopIteration:
        # iterable was empty
        yield dflt

And now, we just use that:

@property
def source_customers(self):
    """Yields source customers, or the instance customer if none found"""

    source_customers = (c.source_customer for c in
         RealEstatesCustomization.select().where(
            RealEstatesCustomization.customer == self.cid))
    yield from iter_or_default(source_customers, self.customer)


Displaying Video file from storage with MediaStore in android Java I Used MediaStore to scan and display media files from my storage using below code
private void displayAllVideos() {
    Uri uri;
    Cursor cursor;
    VideoAdapter videoAdapter;
    int column_index_data,thum;

    String absolutePathThumb;
    uri = MediaStore.Video.Media.EXTERNAL_CONTENT_URI;
    String[] projection = {MediaStore.MediaColumns.DATA, MediaStore.Video.Media.BUCKET_DISPLAY_NAME, MediaStore.Video.Media._ID,MediaStore.Video.Thumbnails.DATA};

    final String orderBy = MediaStore.Video.Media.DATE_ADDED + " DESC";
    cursor = getApplicationContext().getContentResolver().query(uri,projection,null,null,orderBy);
    column_index_data = cursor.getColumnIndexOrThrow(MediaStore.MediaColumns.DATA);
    thum = cursor.getColumnIndexOrThrow(MediaStore.Video.Thumbnails.DATA);

    while (cursor.moveToNext()){
        Log.d("VIDEO", cursor.getString(0));
        absolutePathThumb = cursor.getString(column_index_data);
        Uri thumbUri = Uri.fromFile(new File(absolutePathThumb));
        //String cursorThumb = cursor.getString(thum);
        String fileName = FilenameUtils.getBaseName(absolutePathThumb);
        String extension = FilenameUtils.getExtension(absolutePathThumb);
        String duration = getDuration(absolutePathThumb);
        VideoModel videoModel = new VideoModel();
        videoModel.setDuration(duration);
        videoModel.setVideo_uri(thumbUri.toString());
        videoModel.setVideo_path(absolutePathThumb);
        videoModel.setVideo_name(fileName);
        videoModel.setVideo_thumb(cursor.getString(thum));
        if (extension!=null){
            videoModel.setVideo_extension(extension);
        }else {
            videoModel.setVideo_extension("mp4");
        }
        if (duration!=null){
            videoModel.setDuration(duration);
        }else {
            videoModel.setDuration("00:00");
        }
        arrayListVideos.add(videoModel);
    }

with above code i can able to get and display video file, Is there anything i need to improve in my code.
Your help in need thanks in advance.android

There are no big issues with your method, but I noticed something that can be improved:
Variable declaration
In Java is not common to declare the variables at the beginning of the method:
Uri uri;
Cursor cursor;
VideoAdapter videoAdapter;
int column_index_data,thum;

String absolutePathThumb;

Declare and initialize variables in the same place, it's easier to read and maintain.
By the way, videoAdapter is declared but never used, that's another reason why you should avoid doing this.
Query results not used
Here you configure the projection:
String[] projection = {MediaStore.MediaColumns.DATA, MediaStore.Video.Media.BUCKET_DISPLAY_NAME, MediaStore.Video.Media._ID,MediaStore.Video.Thumbnails.DATA};

But after the query you never use the results for MediaStore.Video.Media.BUCKET_DISPLAY_NAME and MediaStore.Video.Media._ID.
Close the resources
The Cursor object which is returned from the query is a resouce that needs to be closed:
cursor = getApplicationContext().getContentResolver().query(uri,projection,null,null,orderBy);

To close it automatically use the try-with-resources statement:
try (Cursor cursor = getApplicationContext().getContentResolver().query(uri,projection,null,null,orderBy)) {
// ...
}

Camelcase instead of underscore
From the Java Naming Convention:

Methods should be verbs, in mixed case with the first letter
lowercase, with the first letter of each internal word capitalized.

So instead of videoModel.setVideo_uri() use videoModel.setVideoUri() or even better videoModel.setUri(). Similarly for the other methods.
Duplicated assignment
videoModel.setDuration(duration) is called three times. Compute the value of duration first and add it to videoModel only once.
Ternary operator instead of if-else
Sometime using the ternary operator is better for readability. For example instead of:
if (extension!=null){
    videoModel.setVideo_extension(extension);
}else {
    videoModel.setVideo_extension("mp4");
}

You can get the same result with:
String extension = extension != null ? extension : "mp4";
videoModel.setVideo_extension(extension);

Documentation
In the link you posted there is an example that is very similar to your use case. Most of my suggestions are already mentioned there, read it carefully.


Config class - follow-up Original question: Basic Config Class

One problem I know about is Initialize() not being threadsafe, which I am sorting as we speak, so please disregard that.

using log4net;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;

namespace Apple.Application.Base.Config
{
    internal class AppleConfig
    {
        /// <summary>
        /// List of configuration items
        /// </summary>
        private Dictionary<string, string> _configItems;

        /// <summary>
        /// Config file info
        /// </summary>
        private readonly FileInfo _configFile;

        /// <summary>
        /// Log4net logger.
        /// </summary>
        private readonly ILog _log;

        /// <summary>
        /// Checks if class has been initialized.
        /// </summary>
        private readonly bool _initialized;

        /// <summary>
        /// AppleConfig constructor.
        /// </summary>
        /// <param name="fileP=path"></param>
        public AppleConfig(string filepath)
        {
            if (_initialized)
                return;

            _configItems = new Dictionary<string, string>();
            _configFile = new FileInfo(filepath);
            _log = LogManager.GetLogger(typeof(AppleConfig));
            _initialized = Initialize();
        }

        /// <summary>
        /// Initializes AppleConfig
        /// </summary>
        private bool Initialize()
        {
            try
            {
                foreach (string line in File.ReadLines(_configFile.FullName).Where(IsConfigurationLine))
                {
                    var splittedLine = line.Split('=');

                    String key = splittedLine[0];
                    String value = splittedLine[1];

                    _configItems[key] = value;
                }

                return true;
            }
            catch (Exception exception)
            {
                _log.Error(exception); // assuming overload that takes an exception.
                return false;
            }
        }

        /// <summary>
        /// Checks for valid config line
        /// </summary>
        /// <param name="Line">String to check</param>
        /// <returns></returns>
        private bool IsConfigurationLine(string line)
        {
            return !line.StartsWith("#") && line.Contains("=");
        }

        /// <summary>
        /// Returns a config items value by its key.
        /// </summary>
        /// <param name="Key">Config Key</param>
        /// <returns>Config Value</returns>
        public string GetConfigElement(string key)
        {
            if (!IsInitialized)
                return null;

            return _configItems[key];
        }

        /// <summary>
        /// Checks if class has been initialized.
        /// </summary>
        public bool IsInitialized
        {
            get { return _initialized; }
        }

        /// <summary>
        /// Refreshes config
        /// </summary>
        public void RefreshConfig()
        {
            if (!_initialized)
                return;

            Initialize();
        }
    }
}

Merging some of the suggestions you've received so far this is what I think it should look like (I removed the comments for this review):

internal class AppleConfig
{
    private Dictionary<string, string> _configItems = new Dictionary<string, string>();

    private readonly string _fileName;

    private readonly ILog _log = LogManager.GetLogger(typeof(AppleConfig));

    public AppleConfig(string fileName)
    {
        _fileName = fileName;

        // first time we want it to work
        if(!LoadConfig())
        {
            throw new Exception("Initalization failed.");
        }
    }

    public string this[string key]
    {
        // this might throw an exception if key not found
        get { return _configItems[key]; }
    }

    public bool IsInitialized
    {
        get { return _configItems != null; }
    }

    public bool LoadConfig()
    {
        try
        {
            _configItems =
                File.ReadLines(_fileName)
                    .Where(IsConfigurationLine)
                    .Select(line => line.Split('='))
                    .ToDictionary(line => line[0], line => line[1]);

            return true;
        }
        catch (Exception exception)
        {
            _log.Error(exception); // assuming overload that takes an exception.
            return false;
        }
    }

    private bool IsConfigurationLine(string line)
    {
        return !line.StartsWith("#") && line.Contains("=");
    }
}

You don't need the _isInitialized field, you can check the dictionary if it's not null
You don't need the big loop but just a short linq query (by @Konrad Morawski)
You don't need the Initialize method and all the crazy logic ;-) but a simple LoadConfig method that you call in the constructor and later if you want to load it again
You don't need the FileInfo class but just a simple string for the path
There's still a lot that might be improved but it all depends how you are going to use it and whether you need all the improvements at all: will someone else use it? Do you need/want meaningful exceptions (I think it's always hard to implement them if you want them to make it possible to find an error right away).
After all I think the IsInitialized property is also not necessary, it doesn't have any usage. The object cannot actually be in an un-initialized state beacause it would be useless then so I would say it can be removed without losing anything. The load method either worked and everything is fine or it didn't and the constuructor throws an exception.


Generating character permutations I want to generate a 'dictionary' containing all 8 character permutations of upper-case letters such that the output file looks like:

AAAAAAAA
AAAAAAAB
AAAAAAAC
...
ZZZZZZZZ

I came up with this solution that uses the product method of itertools:

from itertools import product

per = product('ABCDEFGHIJKLMNOPQRSTUVWXYZ', repeat=8)

f = open('myfile', 'w')

p = ""

for p in per:
    p = "".join(p)
    f.write(p + "\n")

f.close()

I know there's 208,827,064,576 (> 200 billion) possible permutations so it's going to take time no matter what, but how can I optimise this?

Not much to do there, maybe use with ... as and inline everything:

from itertools import product

with open('myfile', 'w') as file:
    for p in product('ABCDEFGHIJKLMNOPQRSTUVWXYZ', repeat=8):
        file.write("".join(p) + "\n")

I'm sure there is a clever way to generate the characters as well. Edit: ah yes, you could replace 'A...Z' with map(chr, range(65, 91)) although
that seems rather cryptic.

Now why would you want that though? It seems that using that generator and
staying in Python would be more than enough instead of generating a static
file.


LeetCode: Maximum XOR of Two Numbers in an Array Please review how clear my code is and also comment on performance.

  Given a non-empty array of numbers, a0, a1, a2, … , an-1, where 0 ≤ ai
  < 2^31.

  Find the maximum result of ai XOR aj, where 0 ≤ i, j < n.

  Could you do this in O(n) runtime?

  Example:

  Input: [3, 10, 5, 25, 2, 8]

  Output: 28

  Explanation: The maximum result is 5 ^ 25 = 28.

LeetCode

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace TrieQuestions
{
    /// <summary>
    /// https://leetcode.com/explore/learn/card/trie/149/practical-application-ii/1057/
    /// </summary>
    [TestClass]
    public class FindMaxXorInArray
    {
        [TestMethod]
        public void XorTrieTreeTest()
        {
            int[] nums = {3, 10, 5, 25, 2, 8};
            Assert.AreEqual(28,FindMaximumXOR(nums));
        }

        //xor mean if the 0^0 == 0 and 1^1 == 0
        // so if we want maximum we want the maximum opposites
        public int FindMaximumXOR(int[] nums)
        {
            XorTrieTree tree = new XorTrieTree();
            tree.Insert(nums);
            return tree.GetMax(nums);
        }
    }

    public class XorTrieTree
    {
        private XorTreeNode _root;

        public XorTrieTree()
        {
            _root = new XorTreeNode();
        }
        /// <summary>
        /// for each one of the numbers we find we flags are set on with right shit all of the 32 bits
        /// and bitwise AND to understand if the bit is set on or off
        /// </summary>
        /// <param name="nums"></param>
        public void Insert(int[] nums)
        {
            foreach (var num in nums)
            {
                XorTreeNode cur = _root;
                for (int i = 31; i >= 0; i--)
                {
                    int bit = (num >> i) & 1;
                    if (cur.children[bit] == null)
                    {
                        cur.children[bit] = new XorTreeNode();
                    }
                    cur = cur.children[bit];
                }
            }
        }

        /// <summary>
        /// for each one of the numbers we try to understand which bits are set on.
        /// if the the bit is set on then we search for the NOT bit of that,
        /// we add the NOT bit to the temp xorValue variable, because the prefix of those two numbers are xor-ed
        /// and continue to the NOT bit next node.
        /// at the end we do max of current max and xorValue
        /// </summary>
        /// <param name="nums"></param>
        /// <returns></returns>
        public int GetMax(int[] nums)
        {
            int max = int.MinValue;
            foreach (var num in nums)
            {
                XorTreeNode cur = _root;
                int xorValue = 0;
                for (int i = 31; i >= 0; i--)
                {
                    int bit = (num >> i) & 1;
                    if (cur.children[bit == 1 ? 0 : 1] != null)
                    {
                        xorValue += (1 << i);
                        cur = cur.children[bit == 1 ? 0 : 1];
                    }
                    else
                    {
                        cur = cur.children[bit];
                    }
                }
                max = Math.Max(xorValue, max);
            }

            return max;
        }
    }

    //the root has 2 nodes  for 0 and 1
    public class XorTreeNode
    {
        public int Val;
        public XorTreeNode[] children;
        public XorTreeNode()
        {
            children = new XorTreeNode[2];
        }
    }
}

In XorTreeNode you're not using the field Val

public void Insert(int[] nums)
public int GetMax(int[] nums)

Having two methods that must take the same data to operate on is hazardous and should be avoided. Either provide the data to the constructor or you should only have one public method with the data set as argument:

public int FindMaxXor(int[] nums) { Insert(nums); return GetMax(nums) }

You can invert a bit in the following way:

bit ^ 1

It can be used to simplify this:

                int bit = (num >> i) & 1;
                if (cur.children[bit == 1 ? 0 : 1] != null)
                {
                    xorValue += (1 << i);
                    cur = cur.children[bit == 1 ? 0 : 1];
                }
                else
                {
                    cur = cur.children[bit];
                }

to

        int bit = ((num >> i) & 1) ^ 1;
        if (cur.children[bit] != null)
        {
          xorValue += (1 << i);
          cur = cur.children[bit];
        }
        else
        {
          cur = cur.children[bit ^ 1];
        }

You can insert numbers and search for max in the same operation, because every new number only need to compare with already existing numbers.

So you can change GetMax(...) to:

private int GetMax(int num)
{
  XorTreeNode cur = _root;
  int xorValue = 0;
  for (int i = _numBits; cur != null && i >= 0; i--)
  {
    int bit = ((num >> i) & 1) ^ 1;
    if (cur.children[bit] != null)
    {
      xorValue += (1 << i);
      cur = cur.children[bit];
    }
    else
    {
      cur = cur.children[bit ^ 1];
    }
  }

  return xorValue;
}

and Insert(...) to:

public int FindMaxXor(int[] nums)
{
  int result = int.MinValue;

  foreach (var num in nums)
  {
    result = Math.Max(result, GetMax(num));

    XorTreeNode cur = _root;
    for (int i = _numBits; i >= 0; i--)
    {
      int bit = (num >> i) & 1;
      if (cur.children[bit] == null)
      {
        cur.children[bit] = new XorTreeNode();
      }
      cur = cur.children[bit];
    }
  }

  return result;
}

where _numBits is defined as a class const field:

private const int _numBits = 31;

If you know that the nums array contains only small values there might be a significant performance improvement in finding the leftmost significant bit:

  int max = nums.Max();
  while (max > 0)
  {
    _numBits++;
    max >>= 1;
  }

in the start of the Insert(...) method. _numBits should then not be const and initialized to zero obviously.
If the numbers span over the entire int+ domain this may slow the entire process.


Node module to fetch/post API data I'm fairly new to JavaScript and Node and am wondering if I'm on the right track.

Module file:

'use strict';

var chalk = require('chalk');
var config = require('../config');
var moment = require('moment');
var rp = require('request-promise');

var todayDay = moment().format('dddd'); // ["Monday"]"
var todayDate = moment().format('DD').replace(/^0+/, ''); // "1"
var todayMonth = moment().format('MMMM'); // ["January"]

rp({
  method: 'GET',
  uri: config.api.uri + '/tasks',
  qs: {
    access_token: config.api.accessToken,
    project_id: config.api.projectId,
    page_size: 5
  },
  json: true
})
  .then(function(res) {
    console.log(res);
    var data = res.data;

    data.forEach(function(val) {

      // day is today or date and month is today
      if (val.custom_fields.custom_353.indexOf(todayDay) > -1 ||
        val.custom_fields.custom_356.indexOf(todayMonth) > -1 && val.custom_fields.custom_358 == todayDate) {

        // post
        rp({
          method: 'POST',
          uri: config.api.uri + '/incidents',
          qs: {
            access_token: config.api.accessToken
          },
          body: {
            item: {
              name: val.name,
              description: val.description,
              due_date: moment().toISOString(),
              assigned_to: {
                type: val.assigned_to.type,
                id: val.assigned_to.id
              },
              project: {
                id: config.api.projectId
              },
              custom_fields: {
                custom_361: val.custom_fields.custom_384, // sequence
                custom_352: val.custom_fields.custom_270, // time
                custom_354: val.custom_fields.custom_269, // procedure
                custom_277: val.custom_fields.custom_183, // service group
                custom_278: val.custom_fields.custom_184 // service
              }
            }
          },
          json: true
        })
          .then(function(res) {
            console.log(chalk.green(val.name));
          })
          .catch(function(err) {
            console.log(chalk.red(err));
          });
      } else {
        console.log(chalk.grey(val.name));
      }
    });
  })
  .catch(function(err) {
    console.log(chalk.red(err));
  });

Config file:

'use strict';

var env = process.env.NODE_ENV || 'development';
var config = {};

switch (env) {
  case 'development':
    config = require('../env/development/config.json');
    break;
  case 'production':
    config = require('../env/production/config.json');
    break;
};

module.exports = config;

var todayDate = moment().format('DD').replace(/^0+/, ''); // "1"

Looks like you're taking a zero-padded day of month here... and then stripping off the leading zero.

You should use moment().format('D'), it will give you day of month without a leading zero.

Aside from that... you shouldn't use multiple instances of moment(). What if the time changes between the two lines of code? You might get things like 31 February.

var todayMoment = moment();

var todayDay = todayMoment.format('dddd'); // ["Monday"]
var todayDate = todayMoment.format('D'); // "1"
var todayMonth = todayMoment.format('MMMM'); // ["January"]

Like so, both problems are fixed.


User search controller I am using the ruby sunspot gem to create a search users function on my site.
It works fine and the users controller for the index page where there is a search form is this

def index
 if params[:search].nil?
  @users = nil
 else
  @search = User.search do
    fulltext params[:search]
  end
  @users = @search.results.paginate(page: params[:page], :per_page => 10)
 end
end

on the index page, I do this

  <% if @users %>
    <ul class="users followers">
      <%= render @users %>
      <%= will_paginate @users%>
    </ul>
  <% end %>

My question is this. I think setting @users to nil looks and feels wrong. I want the index page to show nothing but my search form if nothing is put in the field. There must be some easy Ruby way to express this. By the way I am fairly new to Ruby on Rails.

How about using the ternary operator and assigning the result to the variable?

def index
  @users = params[:search].nil? ? nil : User
           .search { fulltext params[:search] }.results
           .paginate(page: params[:page], per_page: 10)
end


To Do List using basic JS I learned javascript from some tutorials and as per advice, I thought of creating some task by my self. I wrote the following code and as I expected I saw the answer. If someone can assist me with how to code better? (The below code will create a table and update rows with input data which user enter)

// Javascript
var input = document.getElementById("input");
var btn   = document.getElementById("btn");
var table = document.getElementById("table");

btn.addEventListener('click', function(){
    var updateTask = input.value;
    // Now create row and add all the inputs value to a table
    var row = table.insertRow(-1);
    var cell1 = row.insertCell(0);
    var cell2 = row.insertCell(1);

    var allRows = table.querySelectorAll("tr");

    cell2.innerHTML = updateTask;
    // Update the Number/Order
    var rowLength = allRows.length;
    if (rowLength > 0){
        cell1.innerHTML = rowLength - 1;
    }
});

To Do List

    <input id="input" class="input" type="text" placeholder="Enter here!!!" onfocus="this.value=' ' ">
    <input id="btn" type="submit" value="ADD">

Number	Task

Querying for row elements
The original code queries for elements with tag name tr:

var allRows = table.querySelectorAll("tr");

Just as the code utilizes table methods like insertRow(), the property rows could be used instead:
var allRows = table.rows;

That way there is no DOM query each time the button is clicked.
If there was in fact a need to get all elements with tag name tr, the method Element.getElementsByTagName() could be used, and since it "returns a live HTMLCollection of elements"1 that could happen outside the click handler (e.g. where the other elements fetched by Id are stored). For more information about the differences refer to answers to this post from 2010 which (at the time) claimed “querySelectorAll("a") is a shocking 98% slower than getElementsByTagName("a")”, as well as answers to What is the difference between querySelector and getElementsByTagName? like Quentin's answer plus this jsPerf test.

inline onfocus handler vs Javascript
The markup contains the text input element with the onfocus attribute:

 <input id="input" class="input" type="text" placeholder="Enter here!!!" onfocus="this.value=' ' ">

Instead of specifying the focus handler in the markup, it can be added in the JavaScript. That way there is a separation between the business logic and the markup. One way to do this is to use Element.addEventListener() to add a callback function whenever an event occurs. See the example code below:
input.addEventListener('focus', function(event) {
    input.value = ' '; // could also use event.target instead of input
});

That way the logic of clearing the value is separated from the markup.


Reverse Polish Notation Evaluation in Java I would like to hear feedback about my code both in term of improving efficiency if possible or using other data structures. Also go ahead and let me know about code styles/patterns.

/**
 * Created by mona on 5/26/16.
 */
import java.util.Arrays;
import java.util.Stack;
import java.util.HashSet;
public class EvaluateReversePolishNotation {
    //["2", "1", "+", "3", "*"] -> ((2 + 1) * 3) -> 9
    public static int evaluateReversePolishNotation(String[] tokens) throws ArithmeticException {
        Stack<Integer> stk = new Stack<>();
        HashSet<String> ops = new HashSet<>(Arrays.asList("+","-","*","/"));
        for (int i=0; i<tokens.length; i++) {
            if (!ops.contains(tokens[i])) {
                stk.push(Integer.parseInt(tokens[i]));
            }
            else {
                int secondOperand = stk.pop();
                int firstOperand = stk.pop();
                switch (tokens[i]) {
                    case "+":
                        stk.push(firstOperand+secondOperand);
                        break;
                    case "-":
                        stk.push(firstOperand-secondOperand);
                        break;
                    case "*":
                        stk.push(firstOperand*secondOperand);
                        break;
                    case "/":
                        if (secondOperand == 0) {
                            throw new ArithmeticException("Second operand can't be zero in division");
                        }
                        stk.push(firstOperand/secondOperand);
                        break;
                    default:
                        break;
                }
            }
        }
        return stk.pop();
    }

    public static void main(String[] args) {
      String[] revPolishNotations ={"2","1", "+", "3", "*"};
      System.out.println(evaluateReversePolishNotation(revPolishNotations));
    }
}

public class EvaluateReversePolishNotation {

As a general rule, name classes and objects as nouns.  Verbs are used for methods.  I personally like PostfixCalculator, but if you want these words, consider:

public class ReversePolishNotationEvaluator {

Now it's a noun.

        HashSet<String> ops = new HashSet<>(Arrays.asList("+","-","*","/"));

I would mark down for this when evaluating code.

        Set<String> ops = new HashSet<>(Arrays.asList("+", "-", "*", "/"));

Unless you are specifically using some method that only exists in that specific implementation, I would always recommend using the interface as the type.

I prefer a little whitespace in my lists.

Note that you don't actually need this.

        HashSet<String> ops = new HashSet<>(Arrays.asList("+","-","*","/"));
        for (int i=0; i<tokens.length; i++) {
            if (!ops.contains(tokens[i])) {
                stk.push(Integer.parseInt(tokens[i]));
            }
            else {
                int secondOperand = stk.pop();
                int firstOperand = stk.pop();
                switch (tokens[i]) {
                    case "+":
                        stk.push(firstOperand+secondOperand);
                        break;
                    case "-":
                        stk.push(firstOperand-secondOperand);
                        break;
                    case "*":
                        stk.push(firstOperand*secondOperand);
                        break;
                    case "/":
                        if (secondOperand == 0) {
                            throw new ArithmeticException("Second operand can't be zero in division");
                        }
                        stk.push(firstOperand/secondOperand);
                        break;
                    default:
                        break;
                }
            }
        }

It's enough to write

        for (String token : tokens) {
            stk.push(processToken(stk, token));
        }

and separately

    public static String processToken(Stack stack, String token) throws ArithmeticException {
        switch (token) {
            case "+":
                return stack.pop() + stack.pop();
            case "-":
                Integer subtrahend = stack.pop();
                return stack.pop() - subtrahend;
            case "*":
                return stack.pop() * stack.pop();
            case "/":
                Integer divisor = stack.pop();
                if (divisor == 0) {
                    throw new ArithmeticException("Divisor can't be zero");
                }

                return stack.pop() / divisor;
            default:
                return Integer.parseInt(token);
        }
    }

The advantage of this is that you only write each operator down once.  So adding or removing an operator only needs to be done in one place.  In the original code, you had to change in two places.  This can lead to odd behavior if you make changes.

The separate method saves having to rewrite stk.push( constantly.  Yes, we write return a lot, but we no longer need to break.

I would prefer the for each form or the advanced for loop to a C-style for loop anytime you don't use the index variable for anything other than indexing the collection.

You also might consider having a PostfixCalculator class with an object field of a Stack.  Then you wouldn't have to pass the Stack around.  Obviously the two methods would no longer be static in that case.

It's not clear to me that you gain anything by the check for division by zero.  If you actually divide by zero, it will throw an ArithmeticException anyway.  Why do so manually?  Normally it would be because you wanted to add something to the error message or log something prior to throwing the exception.  Or to keep going after handling the exception.  But you don't do any of that here.  So why the extra code?  If there's a reason, put it in a comment so that no one undoes what you did.

It would make more sense to me to catch the exception for the stack being empty.  That's likely to happen due to bad input.  If you catch it, you can add additional information about the error.  In particular, what token failed.

Similarly, you might want to handle the situation where the stack is not empty.  If you have more than one value left on the stack after processing all the tokens, then there was a mistake somewhere.

Note that the JavaDoc for Stack says to use Deque instead.


Deleting nodes having greater value on right side Problem Summary:

  Given a singly linked list, remove all the nodes which have a greater value on right side.

  Example: The list 12->15->10->11->5->6->2->3->NULL should be changed
  to 15->11->6->3->NULL. Note that 12, 10, 5 and 2 have been deleted
  because there is a greater value on the right side.

Full problem statement

My logic follows Method 2 mentioned in the solution to that problem.

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

struct node
{
    int data;
    struct node* next;
};

void printList (struct node* head)
{
    struct node* current;
    for (current = head; current != NULL; current = current->next)
        printf ("%d\n",current->data);
}

void appendNode (struct node** headRef, int data)
{
    struct node* newNode = malloc(sizeof(struct node));

    newNode->data = data;
    newNode->next = *headRef;
    *headRef = newNode;
}

void reverseList (struct node** head)
{
    struct node* past = NULL;
    struct node* present;
    struct node* future;

    for (present = *head; present != NULL; )
    {
        future = present->next;
        present->next = past;
        past = present;
        present = future;
    }
        *head = past;
}

void removeGreater (struct node** head)
{
    struct node* current;
    struct node* prev;
    int max;

    if (*head != NULL)
    {
        max = (*head)->data;
        prev = *head;
        for (current = (*head)->next; current != NULL;)
        {

            if (current->data >= max)
            {
                max = current->data;
                prev = current;
                current = current->next;
            }
            else
            {
                if (current->next != NULL)
                {
                    struct node* temp = current->next;
                    current->data = temp->data;
                    current->next = temp->next;
                    free (temp);
                }
                else
                {
                    prev->next = NULL;
                    free (current);
                    break;
                }
            }
        }
    }
}

int main(void)
{

    struct node* head = NULL;
    struct node* tail;
    struct node* current;

    int input,num,num1;

    printf ("Enter no. of inputs \n");
    scanf ("%d",&num);
    num1 = num;
    printf ("\n");
    printf ("Enter the numbers\n");

    do
    {
        scanf("%d",&input);
        if (head == NULL)
        {
            appendNode (&head, input);
            tail = head;
        }
        else
        {
            appendNode (&tail->next, input);
            tail = tail->next;
        }
        num--;
    } while (num > 0);

    printf ("\n Before Reversing :\n");
    printList (head);

    reverseList(&head);

    printf ("\n After Reversing :\n");
    printList (head);

    removeGreater (&head);
    printf ("\n After Removing :\n");
    printList (head);

    reverseList(&head);
    printf ("\n Final Output :\n");
    printList (head);

    return 0;
}

In appendNode(), you have to provide a pointer to node to sizeof() and typecast its return value to struct node*:

void appendNode (struct node** headRef, int data)
{
    struct node* newNode = (struct node*)malloc(sizeof(struct node*));

    newNode->data = data;
    newNode->next = *headRef;
    *headRef = newNode;
}


Clojure function composition: logging results of an action performed by a "pure" function I'm wondering how I could compose this program better allow it to grow more functionality without constant refactoring.

This is my code sample for Hacker School, so I'm less married to the results but instead am looking to further my understanding of Clojure and functional programming.

Basically, my battle simulator needs to update the state of the battle participants, while also making those actions known to the console where I represent the state of the battle. Every time I add a feature, I seem to be rewriting everything, so I know I'm structuring this wrong.

How would you arrange these functions (specifically, logfight and battleturn to work well together without being intrinsically tied? Right now I carry around the pertinent info (:lastswing) in the atom, but that feels wrong.

Feel free to mention anything else that bugs you in the code.

(ns rpg)

(def characters (atom
                 {:player {:name "Fred" :hp 100 :att 5 :dex 10 :alive? true :lastswing 0}
                  :creature1 {:name "Badman" :hp 99 :att 4 :dex 7 :alive? true :lastswing 0}}))

(defn hit? [dex]
  (> 5 (* dex (rand 1))))

(defn swing [att]
  (int (* att (+ 1 (rand 1)))))

(defn damage [hp swing]
  (- hp swing))

(defn attack [att dex hp]
  (if (hit? dex)
    (damage hp (swing att))
    hp))

(defn logfight [aname dname aswing dhp]
  (if (> aswing 0)
    (println (str aname " hits " dname " for " aswing ". " dname " has " dhp " hp left."))
    (println (str aname " missed!"))))

(defn battleturn [thechars a d]
  (let [attacker (a thechars)
        defender (d thechars)
        att (:att attacker)
        dex (:dex attacker)
        hp (:hp defender)
        result (attack att dex hp)
        lastswing (- hp result)]
    (if (:alive? defender)
      (if (<= result 0)
         (do
           (println (str (:name defender) " died!"))
           (-> thechars
           (assoc-in [d :alive?] false)))
         (do
           (logfight (:name attacker) (:name defender) lastswing hp)
           (-> thechars
           (assoc-in [d :hp] result)
           (assoc-in [a :lastswing] lastswing))))
      (do
        (println (str (:name defender) " is already dead!"))
        thechars))))

(defn fight [target]
  ((swap! characters battleturn :player target)
   (swap! characters battleturn target :player)))

;; The UI
(fight :creature1)

Let the side effects happen and return nil
The way you have your fight function has the potential to throw an exception when you try to use it. Notice how you start with two opening parentheses, (( -- this should stick out like a sore thumb to any Lisper, and it's especially rare in Clojure. Keep in mind that because Clojure is a Lisp, it attempts to call the first thing in any S-expression as a function. In this case, (swap! characters battleturn :player target) is the first thing in the single S-expression that constitutes the body of the fight function. That means that whatever that expression evaluates to, Clojure will try to call it as a function, using whatever follows (in this case, (swap! characters battleturn target :player)) as an argument. You don't really want that, though. You just want the side effects from using swap! twice, once for :player target and once for target :player, and then you don't care what it returns. There are a handful of special forms in Clojure that have what's called "implicit do syntax," which means you can have more than one S-expression in the body. defn is one of those forms, so all you have to do to avoid throwing an exception is take out a set of parentheses:
(defn fight [target]
  (swap! characters battleturn :player target)
  (swap! characters battleturn target :player))

Incidentally, this will also greatly simplify the way your battleturn function is structured, because you don't have to worry about returning thechars at the end of every conditional branch. You're working with a mutable collection (the atom characters), so the key thing to remember is that it doesn't matter what you're returning. This is backwards from the way things usually work in Clojure with its functional programming paradigm, but that's the whole point -- your program is utilizing a more object-oriented style, so it's all about mutating values rather than returning something. In this case, you're mutating things like the characters' HP.
Maybe each character should be an atom (also, destructuring)
On that note, I think it would make more sense to represent each character as its own, separate atom, since the character itself is what you are modifying. Maybe try something like this:
(def player     (atom {:name "Fred" :hp 100 :att 5 :dex 10 :alive? true :last-swing 0}))
(def creature-1 (atom {:name "Badman" :hp 99 :att 4 :dex 7 :alive? true :last-swing 0}))

(By the way, this is just a nitpicky thing, but the idiomatic thing to do in Clojure is to hyphenate multi-word symbol names, e.g. battle-turn instead of battleturn, etc.)
Then adjust your functions so that they are referring to the player and the creature directly by their symbols, rather than reaching into characters to get them. This also means you can take thechars out of your battleturn function entirely, which simplifies things:
(defn battle-turn [attacker defender]
  (let [{:keys [att dex]}   @attacker
        {:keys [hp alive?]} @defender
        result              (attack att dex hp)
        last-swing          (- hp result)]
    ...

I'll pause here and point out that I shortened your let statement in a couple of ways:

You no longer need to bind attacker and defender to (a thechars) and (d thechars), since we got rid of the characters collection altogether and we're just passing in the atoms for each character to the function explicitly. Notice that we now have to dereference attacker and defender using @, as they are atoms.

I used destructuring to simplify the process of grabbing fields from attacker and defender. Basically what we're saying is "bind the values for :att and :dex from @attacker to att and dex", and the same for :hp and :alive? from @defender.

Prefer cond over nested ifs
Another key point is that it's generally better to use a cond statement than nested if statements when you have several branches of conditional logic like you have here. It can really make your code easier to read if you structure it right. Consider this:
(defn battle-turn [attacker defender]
  (let [{:keys [att dex]}   @attacker
        {:keys [hp alive?]} @defender
        result              (attack att dex hp)
        last-swing          (- hp result)]
    (cond
      (not alive?)
      (printf "%s is already dead!\n" (:name @defender))

      (<= result 0)
      (do
        (printf "%s died!\n" (:name @defender))
        (swap! defender assoc :alive? false))

      :else
      (do
        (log-fight (:name @attacker) (:name @defender) last-swing hp)
        (swap! defender assoc :hp result)
        (swap! attacker assoc :last-swing last-swing)))))

The cond statement here makes it clearer that there are only 3 possible outcomes, and in what order they are considered.
printf/format is awesome
You may have noticed that I replaced your use of (println (str ...)) with (printf ...). printf is just a shortcut for (print (format ..., so we're really talking about str vs. format. To be fair, there's nothing wrong with using str, and in fact, it's often shorter than its format equivalent, particularly when you're not dropping a whole lot of things into the string. But I still prefer format, because I think it's a lot more readable, and when you use printf, it's often shorter than the equivalent (print (str ... expression. I don't think there's an "idiomatic" preference of one vs. the other, but I just thought I'd throw that out there for your consideration. Consider:
(println (str aname " hits " dname " for " aswing ". " dname " has " dhp " hp left."))

vs.
(printf "%s hits %s for %d. %s has %d left.\n" aname dname aswing dname dhp)


FizzBuzz with debugging output This is a pretty simple FizzBuzz variant. I'm more interested in my use of C (this is my first bit of C code) than I am in my actual FizzBuzz implementation, although I'm always open to suggestions.

In particular I'm pretty certain that my string manipulations are horrific, but I don't really know where to start improving.

I've also added in a little debug utility that I've created, with the purpose of implementing some stuff with header files, as those are completely new to me. All it does is toggle whether a print method will do anything, based on a global flag; I'm not sure if this is actually useful, or if I'm overlooking a better equivalent.

FizzBuzz Source

#include <string.h>
#include "debug_util.c"

void evaluate(char*, int, int, char*);
const int MAX_STRING_LEN = 17;
const int MAX_WORD_LEN = 7;
const int MAX_DIGIT_COUNT_LEN = 4;
const bool LOCAL_DEBUG_FLAG = false;

int main()
{
    DEBUG = LOCAL_DEBUG_FLAG;
    int i;
    char output[MAX_STRING_LEN];
    char intVal[MAX_DIGIT_COUNT_LEN];

    printDebug("Initial Loop Start\n");

    for (i = 1; i <= 100; i++)
    {
        printDebug("Setting initial output to null\n\n");
        /* Set first value in array to null termination,
         * effectively clearing the array,
         * if it is only read as a sequential string */
        output[0] = '\0';

        snprintf(intVal, MAX_DIGIT_COUNT_LEN,"%d",i);

        evaluate(output, 3, i, "Fizz");
        evaluate(output, 4, i, "Buzz");
        evaluate(output, 5, i, "Sizzle");

        output[sizeof(output)] = '\0';

        if (output[0] == '\0')
        {
            strncat(output, intVal, MAX_DIGIT_COUNT_LEN);
        }

        printf("%s\n", output);
    }
}

void evaluate(char *buf, int factor, int value, char *result)
{
    printDebug("Evaluating || Factor: %d, Value: %d, Success Result: %s\n", factor, value, result);
    char temp[MAX_WORD_LEN];

    strncpy(temp, ((value % factor == 0) ? result : ""), MAX_WORD_LEN -1);

    printDebug("Evaluating complete || Result: \"%s\"\n\n", temp);

    //Check array is not empty
    if (value % factor == 0)
    {
        strncat(buf, temp, MAX_STRING_LEN -1);
    }
}

Debug Util Header

#ifndef C_EXAMPLES_DEBUG_UTIL_H
#define C_EXAMPLES_DEBUG_UTIL_H

#ifndef _STDIO_H_
#include <stdio.h>
#endif

#include <zconf.h>

#ifndef _STDBOOL_H
#include <stdbool.h>
#endif

extern bool DEBUG;

#endif //C_EXAMPLES_DEBUG_UTIL_H

Debug Util C Source

#include "debug_util.h"

bool DEBUG = false;

//Only prints if the global DEBUG flag has been set to true
//Replicates code from printf()
int printDebug(const char *format, ...)
{
    int done = 0;
    if (DEBUG)
    {
        va_list arg;

        va_start (arg, format);
        done = vfprintf (stdout, format, arg);
        va_end (arg);
    }

    return done;
}

#includeing source files

A very big issue - especially if you're planning on writing a library:

You include a C source file to use printDebug. You shouldn't do that to avoid possible name collisions between the #includeing file and the #included file, as all of the advantages you had from the header guards are absent when you import the source file itself instead of the header.

The header represents the public interface of the code, so always include that to avoid name collision issues and not depend on implementation details, like internal functions (not that you do that now, but for the future).

The problem with doing this right now would be that you have not put your function declarations in the header. Declare printDebug in the header (I guess you already know how to do this, as you've already done it for evaluate), and you're ready to use it from your main source - just include debug_util.h instead of debug_util.c.

Weird and missing #includes

What is that #include <zconf.h> doing in debug_util.h? as far as I could find out, it's for the compression library zlib, which you don't seem to be using. Also, you should #include <stdarg.h> in debug_util.h to get standards-compliant behavior and compiler portability for the va_* macros you use in debug_util.c.

You should #include <stdio.h> in your main file too, as you're using printf. The header guard is in debug_util.h to save you from the multiple-include problem.

Style and Convention

Your coding style is fine, but you should use int main(void) to indicate that main takes no parameters, as main() indicates (counterintuitively) that main
 can take any number of parameters.
return 0; at the end of main is only optional in C99 mode and above, so you might want to document that.
ALL_CAPS names are generally reserved for macro definitions - use normal variable naming for constants.
DEBUG as declared in debug_util.c and debug_util.h could be const bool - don't assign it in debug_util.c and just leave it for the user to do it.
Or you could use a DEBUG macro definition and #ifdef/#ifndef checks to do this, as every debug library does by convention; using #ifdef checks means you can even choose to leave out the compilation of printDebug in release builds (it can be an empty function in release builds given its conditional definition, which would cause the call to printDebug to be optimized away.

An example of this idea:

debug_util.h

#ifndef DEBUG_UTIL_H
#define DEBUG_UTIL_H

#ifndef _STDIO_H_
#include <stdio.h>
#endif

#ifndef _STDBOOL_H
#include <stdbool.h>
#endif

int printDebug(const char*, ...);

#endif //DEBUG_UTIL_H

debug_util.c

#include "debug_util.h"

//Only prints if the global DEBUG flag has been set to true
//Replicates code from printf()
int printDebug(const char *format, ...)
{
#ifdef DEBUG
     int done = 0;
     va_list arg;

     va_start (arg, format);
     done = vfprintf (stdout, format, arg);
     va_end (arg);
#else
     return 1;
#endif
}

In the main code, just do a #define DEBUG 1 after #include "debug_util.h", or pass the -DDEBUG=1 option to gcc when compiling the main code.


Sum an array of video lengths in JavaScript into HH:MM:SS I am trying to learn functional programming in JavaScript, and saw a little task on Twitter that I wanted to have a go at:

var durations = ["12:38", "6:36", "9:03", "8:34", "5:02", "6:54", "13:22", "4:41", "8:36", "21:58", "3:06", "10:46", "10:13", "12:54",
"14:00", "11:03", "16:03", "10:52", "24:53", "10:03", "11:49",
"15:47", "3:19", "2:06", "5:47", "1:03", "5:29", "5:47", "26:39"];

// Above is a list of video lengths, how long are all the videos together?
// write a function that will take the above array of string durations and convert it hours/mins/seconds
// You can use any JS you want - loops/map/reduce/etc...

It would be great to have some feedback on how this could be refined further or improved.

var hours = 0, minutes = 0, seconds = 0;

function sumArrayIndex(array, i, separator) {
  return array
    .map(x => x.split(separator)
      .map(c => { return parseInt(c) })
    )
    .map(x => { return x[i]; })
    .reduce((x, y) => { return x += y }, 0);
}

function minToSec(m) {
  return m * 60
}

function tallyTime(s) {
  var hrs = 0;
  var min = Math.floor(s / 60);
  var sec = parseInt(s % 60, 16);

  if (min > 59) {
    hrs = Math.floor(min / 60);
    min = min % 60;
  }

  hours += hrs;
  minutes += min;
  seconds += sec;
}

function outputTime() {
  return hours + ':' + minutes + ':' + seconds;
}

var tally = tallyTime(minToSec(sumArrayIndex(durations, 0, ':')) + sumArrayIndex(durations, 1, ':'));

console.log(outputTime(tally));

The global hours, minutes, seconds are icky. Many of the functions you've defined access and change them, in essence causing side-effects.

Besides, this seems over-engineered. The sumArrayIndex function is a strange mix of specific and generic. It's specific because it expects an array of strings with a delimiter and stuff that can parsed to integers and summed. But it's generic in that it stops there. It's already built for this very specific task, but doesn't go all the way. Instead you have to call it twice (and it has to split everything twice, etc.).

You have 3 tasks here:

convert each time code to the smallest common unit (i.e. seconds),
calculate the total,
format it as HH:MM:SS.

Your sumArrayIndex does some of the first two, rather than just one of them, or all of both.

Also having a function like minToSec is a little much. It's literally just a multiplication. In this context, I'd say that 60 doesn't even count as a magic number. So no reason to wrap it in a function; just multiply.

There also some bugginess: Should the final total be, say, 2:00:01, your code will output "2:0:1" because it doesn't pad the numbers.

And lastly, you're parsing seconds as a base16 number in tallyTime? I don't know why you're using parseInt on something that's already an int, and I certainly don't know why you're expecting it to be base16.

Anyway, here's my (very different) take:

function sumDurations(durations) {
  return durations.reduce((sum, string) => {
    var mins, secs;
    [mins, secs] = string.split(":").slice(-2).map(n => parseInt(n, 10));
    return sum + mins * 60 + secs;
  }, 0);
}

function formatDuration(duration) {
  function pad(number) {
    return `${number}`.slice(-2);
  }

  let hours = duration / 3600 | 0;
  let minutes = duration % 3600 / 60 | 0;
  let seconds = duration % 60;
  let minsSecs = `${pad(minutes)}:${pad(seconds)}`;

  return hours > 0 ? `${hours}:${minsSecs}` : minsSecs;
}

Some notes:

It's two functions for the sake of clarity, though they could be combined or be placed inside an outer function to hide that implementation detail (like pad is nested inside formatDuration).
I've combined steps 1 & 2 in sumDurations.
I'm using some array destructuring to pull out the minutes and seconds after splitting the time code string.
Like your code, you'll get wrong results if the time code contains hours as well. Mine ignores them, yours treats them as minutes - and the minutes as seconds. Fixing this is an exercise left to the reader.
I'm using the | 0 trick, which does the same as Math.floor (for positive numbers that fit in 32-bit integers anyway).
I'm using ES6 string templates/interpolation to build the final string.