jQuery code repetition and MVC A while ago I started working on a project, which does things mostly with jQuery and PHP. I had some problems with my structure and started using CodeIgniter which uses an MVC pattern. So the PHP part is all good now.

I am just a little worried about my jQuery code. It looks bloated and I feel like there is room for improvement. The problem is that the code is very simple and I don't know how to actually improve it.

My code looks like this:

$(function() {
    var baseURL = "index.php/";

    /* *
     * Fill the  list with all available objects
     */
        $(document).ready(function() {
            $.ajax({
                type: "POST",
                url: baseURL+"Objects/getList",
                dataType: "json",
                data: "",
                cache: false,
                success: function(data) {
                    $('#object-list').html();

                    var objects= "";
                    for (var i = 0; i < data.length; i++) {
                        objects+= "<option value='" + data.id +"'>" + data[i].name + "</option>";
                    }

                    $('#object-list').html(options);
                }
            });
        });

    /* *
     * Authenticate and login using a username and password
     */
        $('#login').click(function(event) {
            var username = $("#login-username").val();
            var password = $("#login-password").val();

            $.ajax({
                type: "POST",
                url: baseURL+"User/authenticateUser",
                dataType: "json",
                data: "username="+ username +"&password=" + password,
                success: function(data) {
                    var success = data.result;

                    if (!success) {
                        $("#login-window-inner-message").html("The credentials provided were not found. Please try again.");
                    } else {
                        $('#login-window').remove();
                        LoadGame();
                    }
                }
            });
        });

    /* *
     * Run all functions part of the startup procedure
     */
        function LoadGame() {
            UpdateNavigation();
            DisplayUserInfo();
        }

    /* *
     * Update the action screen
     */
        function UpdateActions() {
             $.ajax({
                type: "POST",
                url: baseURL+"Location/getOptions",
                dataType: "json",
                data: "",
                success: function(data){
                    $("#action1").html(typeof data.option1 != 'undefined' ? data.option1.title : "
");
                    $("#action2").html(typeof data.option2 != 'undefined' ? data.option2.title : "
");
                    $("#action3").html(typeof data.option3 != 'undefined' ? data.option3.title : "
");
                }
            });
        }

    /* *
     * Display the description of the current zone
     */
        function DisplayZoneDescription() {
             $.ajax({
                type: "POST",
                url: baseURL+"Location/getZoneDesciption",
                dataType: "json",
                data: "",
                success: function(data){
                    $("div#message-log").html("
" + data.description + "
");
                }
            });
        }
});

That's not the entire code (so some called functions might not be there), which would be too long to post here. But the gist of it is that I have about a hundred "hooks" to HTML elements which all fire an Ajax function which returns data after which the screen is updated.

Some things that come to mind:

In an MVC environment, should the Ajax data part be in the URL?
I will be splitting up the functions into different files an the production environment.
The main problem I'm having with this is that it just looks like it's hanging loosely together, and doesn't fit my structured MVC environment/code.

You say you got a hundred of these? And possibly longer?

I suggest you invest on learning these tools:

A client-side MV* framework like Backbone. That way, your code will be modular and structured.
As for your UI, I suggest you go with Mustache for a logicless templating solution.
An automation tool like Grunt, which runs for you the most repetitive tasks like merging files, moving files unit tests etc.

I also suggest you watch this video from Nicholas Zakas regarding scalability and architecture. Though not all points will be applicable, at least you get the idea how to loosely couple your code.

As for the questions:

  In an MVC environment, should the Ajax data part be in the url?

If you were to use MVC, that would be handled in the Models and Routers. Depending on the framework, some handle it for you. Just give them the proper routing and parameters, and you are good to go. No "low-level" code.

  I will be splitting up the functions into different files an the production environment.

It's only in development that you actually split them so they make sense. In production, you actually merge and minify them. Check out jQuery. Their source is split into parts during development, but end up with a squished file during deployment.

The usual procedure (via Grunt tasks) is:

jsbeautifier - Beautify code, so that the linter will be happy
JShint - Checks for syntax errors, readability issues, possible break areas.
Qunit - Unit testing for all your functions. to check if it works as expected.
concat - Join all files into one. Lessens HTTP requests.
minify - shrinks your concatenated scripts, to lessen download size.

And you end up with one power-packed nKB file (where n is the size of course).

  The main problem I'm having with this is that it just looks like it's hanging loosely together, and doesn't fit my structured MVC environment/code.

I really suggest you invest your time in your toolset. I gave suggestions above based on experience and taste. There are other tools out there, like other MV* frameworks, other template engines. You might even want to learn SASS or LESS for CSS.

Although I encourage you to learn the tools, which will lead you to ultimately abandon your old code, here are a few tips though:

The index.php/ part of the url can be removed via url-rewriting on the server. As far as I know, there's a solution posted somewhere in the CodeIgniter forums, which can be searched. That way, your code will always start right after the first / of the domain.
The data parameter of $.ajax() can accept objects:

data: {
  username : username,
  password : password
}

These could be avoided using templating:

$("#login-window-inner-message").html("The credentials provided were not found. Please try again.");


Project Euler #14 - Longest Collatz sequence I'm attempting some Project Euler problems in an attempt to learn Python.  While I can get the correct answer quite easily, my programs are quite slow. Project Euler, if doing it correctly, specifies that each program should run in <1 minute. Some of mine take 5-10 mins.

Here is one example.

x=1;
terms=0;
maxTerms=0;
maxTermsNum=0;
for x in range(1,1000000):
    i=x;
    while (i!=1):
        terms+=1;
        if (i%2==0):
            i/=2
        else:
            i=3*i+1
    if (terms>maxTerms):
        maxTerms=terms;
        maxTermsNum=x;
        print(x);
    terms=0;
print("Biggest one: is %s." %maxTermsNum)

It produces the correct answer, but it takes a long long time. How can I speed this up?  This is also in 32-bit Windows.

While many of the Project Euler problems can be solved by brute force, this often isn't the fastest way to do them. Usually there is some kind of algorithmic insight that you can apply that will make your solution faster. This kind of insight is independent of programming language - simply running your code through a hypothetical "go-fast compiler" still won't match the speed of the correct algorithm.

In your case, here's a hint. Many of the sequences you calculate look the same as ones you've previously done. How could you use that information to speed up your algorithm?


Find matching dictionary words given a Scrabble tray This is a question I was asked in an interview, below is a cleaned-up copy of the answer I gave. Apparently this answer was not satisfactory. How can it be improved?

Question: Given a dictionary of words (a text file with 100000+ entries) and a list of n letters with possible repeats (i.e. a Scrabble tray), return the list of words which can be formed from some or all of the letters in the tray.

function scrabble(dictionary, tray) {
  return dictionary.filter(w => isWordInTray(tray, w));
}

function isWordInTray(tray, word) {
  // build multiset of letters in tray
  let counts = {};
  for (const letter of tray) {
    if (counts[letter] === undefined) {
      counts[letter] = 1;
    } else {
      counts[letter]++;
    }
  }

  // take letters from the word and decrement tray count
  for (const letter of word) {
    if (counts[letter] > 0) {
      counts[letter]--;
    } else {
      return false;
    }
  }
  return true;
}

//---------------

// dictionary (full dictionary contains 178691 entries)
const dict = ['AA', 'ABSORBABILITIES', 'AD', 'ADD', 'BAD', 'DAD', 'FOO']; // ...

// test case
const exampleTray = ['D', 'D', 'A'];
console.log(scrabble(dict, exampleTray));

// expected correct answer (in any order)
// [ 'AD', 'ADD', 'DAD' ]

Caveats

The interviewer said that the problem was "kind of like Scrabble", this was the only mention of Scrabble. The size of the tray was given simply as n (it was described as a list of letters, not a tray).
The full dictionary was given to me as a text file, it is sorted and has 178691 entries. I've included a minimal subset which works with the test case I was given in the interview.
counts is a hashtable but I could have used a 26-element array instead to get O(1) inserts and lookups which improves the worst case from O(n), but n = 26, so it's not a big deal.
I'm not looking for micro-optimizations for special cases: I'm only seeking to decrease the big-O worst case complexity.

Alternative Approaches

These are the other approaches which spring to mind:

Approach 2: Generate all words

The opposite approach is to generate all words from the tiles and look them up in the dictionary, but if we treat the tray as a multiset of letters then the number of words we will generate is a multiset permutation and that's just for the case where len(word) = len(tiles) without considering all the shorter words which can be formed.

The size of the tray was given as n, while in the game of Scrabble it's at most 7 (news to me as I don't play Scrabble), the interviewer never gave this restriction, so presumably max(n) = max(len(word) in dict) which is "ABSORBABILITIES" at 15 letters. That's going to be a huge search space. I asked what n was and he said "anything".

If my understanding is correct, the worst case for multiset permutations is when each letter in the tray is unique, because this is simply the number of permutations which is n!. Again, this doesn't account for the need to also find words shorter than the tray length.

7! = 5040, so for an actual Scrabble tray, generating all words is feasible, but at 9! we've generated more words than are in our dictionary and by 15! there's over a trillion.

Approach 3: Use a trie?

When I see words being looked up in a dictionary, I think of a trie (prefix tree). What I can't see is what it would offer in this case, especially w.r.t big-O worst case complexity.

Is there something I'm totally missing?

Big-O

Assuming that all words are of length w and the tray is also that length, the dictionary is of length n, and there are no anagrams; the complexity should be O(n*w).

(This also assumes that counts is replaced with an array as mentioned above.)

The way to use a trie here is to sort the letters of each dictionary word:

[ AD, ADD, DAD ] becomes A > D [ ad ] > D [ add, dad ]

Then walk the trie and stop descending when isWordInTray returns false.

Worst-case complexity is unchanged (arguably worse, since sorting time is not linear); actual runtime is greatly improved.


Checking against a specific sequence in python I'm trying to learn python and I'm modeling a simple dice game that a friend and I invented.

On any one turn of the game, you must roll a specific combination to stay in the game.

The particular rule I'm trying to model is this:

Given a previous roll (where the dice are contiguous) make another contiguous roll either above the previous roll or before it. 6 and 1 are considered contiguous. Outside of being contiguous the order of the dice does not matter...

some examples

  existing roll (3, 4) valid subsequent rolls: (1, 2) (5, 6)

  existing roll (5, 6) valid subsequent rolls: (1, 2) (3, 4)

  existing roll (6, 1) valid subsequent rolls: (2, 3) (4, 5)

I have written the below python3 code to deal with this aspect of the game. However, being new to python, I'd like to know how experienced programmers would deal with this problem. Is there a smarter way to do this programmatically without all the sorting and comparisons and having to make the two arbitrary evaluations? All comments are welcome, readability, efficiency etc. Thanks for your time.

arguments:

prev = previous/ existing roll

curr = current roll to check

sides = number of sides of the dice

Program:

from itertools import cycle, islice

def chkStk(prev, curr, sides):
    def srt(a):
        return sorted(a, reverse=True if (min(a), max(a)) == (1, sides)
                      else False)

    def cmp(a):
        return a == list(x for x in islice(cycle(range(1, sides+1)),
                                           a[0]-1, a[0]+len(a)-1))
    curr = srt(curr)
    prev = srt(prev)

    return cmp(prev+curr) or cmp(curr+prev)

# examples
print(chkStk([3, 4], [1, 2], 6))
print(chkStk([3, 4], [5, 6], 6))

print(chkStk([5, 6], [1, 2], 6))
print(chkStk([5, 6], [3, 4], 6))

print(chkStk([6, 1], [2, 3], 6))
print(chkStk([6, 1], [4, 5], 6))

# counter examples
print(chkStk([6, 1], [6, 1], 6))
print(chkStk([1, 1], [1, 1], 6))
print(chkStk([1, 2], [4, 5], 6))

output:

True
True
True
True
True
True
False
False
False
[Finished in 0.1s]

Specification

I'm having trouble understanding the problem. I think you roll two dice at once, and the results must be distinct and adjacent to each other and to the previous roll.

Comments

Your question includes some explanation before the program:

  I'm modeling a simple dice game that a friend and I invented.

  On any one turn of the game, you must roll a specific combination to stay in the game.

  The particular rule I'm trying to model is this:

  Given a previous roll (where the dice are contiguous) make another contiguous roll either above the previous roll or before it. 6 and 1 are considered contiguous. Outside of being contiguous the order of the dice does not matter...

This should be part of the program! It could be a docstring at the beginning of the file.

  arguments:

  prev = previous/ existing roll

  curr = current roll to check

  sides = number of sides of the dice

These should also be part of the program! They should be part of chkStk's docstring, or better yet, included in the argument names. prev and curr could be called previous_roll and current_roll.

srt returns its argument in order, unless it contains both bounds, in which case it's reversed. This is surprising, so it requires an explanation in a comment or docstring.

Names

All three function names are inscrutably short.

srt sorts its argument (which should be a two-element list) in a cyclic order. So it could be called cyclic_order.

srt's argument a is a 2-die roll (i.e. a pair), so it should be called pair or roll.

cmp checks whether its argument is a contiguous ascending sequence (in the same cyclic order). So it could be called contiguous or is_contiguous or is_ascending or even in_order.

cmp's argument a is a list of (1-die) rolls, so it should be called rolls.

chkStk checks whether curr is a valid roll after prev, so it should be called something like valid_roll or is_valid_roll or `

(It's confusing to have roll mean a pair of 1-die rolls, so maybe the whole program should switch to something consistent, such as "roll" for one die and "pair" for two dice.)

Small simplifications

True if boolean_expression else False can be simplified to just boolean_expression.

[debatable] (min(a), max(a)) == (1, sides) is short, but most people are accustomed to reading min(a) == 1 and max(a) == sides.

Even better: since 1 and sides are the minimum and maximum values possible, you can skip the min and max and just check whether the values are present: 1 in a and sides in a.

list(x for x in foo) can be simplified to just list(foo).

Simpler ways

In cmp, instead of building a sequence in cyclic order and comparing to it, it might be simpler to check that each successive pair is in cyclic order.

There are easier ways to solve this problem.

If my restatement of the problem above ("the results must be adjacent to each other and to the previous roll") is correct, it can be easily turned into a program. You can simply check each part (possibly in a helper function):

whether two dice of a pair are adjacent to each other
whether two pairs are adjacent to each other

You can do both of these without any list operations.

Tests

Instead of printing out results for you to check, your test cases can check them for you! The simplest way to do this is with assert:

assert chkStk([5, 6], [1, 2], 6)
assert not chkStk([5, 6], [1, 4], 6)
assert not chkStk([6, 1], [2, 1], 6), 'overlapping pairs should fail'

This won't print anything unless the test fails. You can include an optional error message.


Beginner code for MadLibs game I am working through Automate the Boring Stuff. How can I make this code cleaner/better?

#madlibs.py : ABS Chapter 8 Project
#Make mad libs using madlibtemplate.txt file that will prompt user for parts of speech
#Will return new file with replacements called madlib.txt

import re, os

#Create template.txt file if doesn't exist

print("Let's play MadLibs! \n Note: Your current working directory is ", os.getcwd())
if os.path.exists('madlibtemplate.txt') == False:
    print("No template file found. Using default 'madlibtemplate.txt'")
    template = open('madlibtemplate.txt', 'w+')
    template.write("""The ADJECTIVE girl walked to the NOUN and then VERB.
    A nearby NOUN was upset by this.""")
else:
    print("Using your 'madlibtemplate.txt' file:")
    template = open('madlibtemplate.txt', 'r')

#Extract text from template file & turn into list of words

template.seek(0) #reset pointer to beginning of file
text = (template.read()).lower() #make lowercase to match parts of speech

wordregex = re.compile(r'\w+|\s|\S')
words = re.findall(wordregex, text)

#Find and replace parts of speech

for i, word in enumerate(words):
    for part_of_speech in ['adjective', 'noun', 'adverb', 'verb', 'interjection']:
        if word == part_of_speech:
            print('Enter a(n) %s ' % part_of_speech)
            words[i] = input()

madlib_text_unformatted = ''.join(words)

#Capitalize first letter in each sentence (Function found on stackoverflow)

def sentence_case(text):
    # Split into sentences. Therefore, find all text that ends
    # with punctuation followed by white space or end of string.
    sentences = re.findall('[^.!?]+[.!?](?:\s|\Z)', text)

    # Capitalize the first letter of each sentence
    sentences = [x[0].upper() + x[1:] for x in sentences]

    # Combine sentences
    return ''.join(sentences)

madlib_text = sentence_case(madlib_text_unformatted)

#Print result and save to a new file

print('Here we go... \n')
print(madlib_text)

madlib = open('madlib.txt', 'w')
madlib.write(madlib_text)
madlib.close()
template.close()

print("\n This has been saved to file 'madlib.txt'")

You could use a bit less comments, most of the code is self-explnatory
You could also use a bit more split of the code into functions, e.g. a function to create a default file
This is a full script, so it's nice to have the if __name__ == "__main__": condition to call the main function
If you use the """ to have a multi-line string, whatever you put will be there, including newlines and tabs. You probably want to split the string by using " on separate lines (see later).
Also, if you concatenate that way, you're losing the \n, it's better to use format

Revised code:

#madlibs.py : ABS Chapter 8 Project
#Make mad libs using madlibtemplate.txt file that will prompt user for parts of speech
#Will return new file with replacements called madlib.txt

import re, os

def generate_default_template():
    with open('madlibtemplate.txt', 'w+') as template:
        template.write("The ADJECTIVE girl walked to the NOUN and then VERB. "
                        "A nearby NOUN was upset by this.")

def capitalize_sentences(text):
    sentences = re.findall('[^.!?]+[.!?](?:\s|\Z)', text)
    capitalized_sentences = [sentence.capitalize() for sentence in sentences]
    return ''.join(capitalized_sentences)

def main():
    print("Let's play MadLibs!\nNote: Your current working directory is {0}".format(os.getcwd()))
    if not os.path.exists('madlibtemplate.txt'):
        print("No template file found. Using default 'madlibtemplate.txt'")
        generate_default_template()
    else:
        print("Using your 'madlibtemplate.txt' file:")

    with open('madlibtemplate.txt', 'r') as template:
        template.seek(0)
        text = (template.read()).lower()

    wordregex = re.compile(r'\w+|\s|\S')
    words = re.findall(wordregex, text)

    for i, word in enumerate(words):
        if word in {'adjective', 'noun', 'adverb', 'verb', 'interjection'}:
            print('Enter a(n) %s ' % word)
            words[i] = input()

    madlib_text_unformatted = ''.join(words)
    madlib_text = capitalize_sentences(madlib_text_unformatted)

    print('Here we go... \n')
    print(madlib_text)

    with open('madlib.txt', 'w') as madlib:
        madlib.write(madlib_text)

    print("\nThis has been saved to file 'madlib.txt'")

if __name__ == "__main__":
    main()

If you want to capitalize a word, there's the... well, capitalize function for that.
When opening files it's nice to use the with keyword, so they'll be closed automatically.
You don't need to do a loop when checking the parts of speech, just use the in operator.


"contains" function for STL containers I have made a simple program that tests if the given element exists in any STL container. The program tests if the container has a find member function. It will use it if it does. Otherwise, it will call the STL find function instead. I would like to know how I can improve it further?

#include <iostream>
#include <algorithm>
#include <utility>
#include <map>
#include <set>
#include <vector>
#include <array>
#include <type_traits>

template <typename C> decltype(std::declval<C>().find(0), std::true_type{}) test(int);
template <typename C> std::false_type test(...);
template <typename C> using has_find = decltype(test<C>(0));

template <bool B, typename T, typename F>
std::enable_if_t<std::integral_constant<bool, B>::value, T>
conditional(T&& t, F&&)
{
    return std::forward<T>(t);
}

template <bool B, typename T, typename F>
std::enable_if_t<!std::integral_constant<bool, B>::value, F>
conditional(T&&, F&& f)
{
    return std::forward<F>(f);
}

template <typename C, typename T>
auto contains(const C& container, const T& key)
{
    static auto first = [] (auto&& c, auto&& k)
    {
        return c.end() != c.find(k);
    };

    static auto second = [] (auto&& c, auto&& k)
    {
        return c.end() != std::find(c.begin(), c.end(), k);
    };

    auto op = conditional<has_find<C>::value>(first, second);

    return op(container, key);
}

int main()
{
    std::cout << std::boolalpha;

    std::array<int, 3> a = {{ 1, 2, 3 }};
    std::cout << contains(a, 0) << "\n";
    std::cout << contains(a, 1) << "\n\n";

    std::vector<int> v = { 1, 2, 3 };
    std::cout << contains(v, 0) << "\n";
    std::cout << contains(v, 1) << "\n\n";

    std::set<int> s = { 1, 2, 3 };
    std::cout << contains(s, 0) << "\n";
    std::cout << contains(s, 1) << "\n\n";

    std::map<int, int> m = { { 1, 1}, { 2, 2}, { 3, 3} };
    std::cout << contains(m, 0) << "\n";
    std::cout << contains(m, 1) << "\n";
}

If you like new features, and even experimental features, you can make your code a lot cleaner.

Concepts

A lot of those arcane SFINAE techniques will be obsolete once concepts are out, and concepts are already available in an experimental state with the last versions of gcc (enable them with the -fconcepts option):

template <typename Container, typename Value>
concept bool HasFindFunction = requires(Container c, Value v) {
    c.find(v);
};

... and that's it for your trait.

if constexpr

To specialize the contains function, you can rely on C++17 if constexpr. It's a compile-time if, in which only the chosen branch has to be well-formed. So, here's what you could write:

template <typename Container, typename Value>
auto find(Container&& cont, Value&& val) {
    if constexpr (HasFindFunction<Container, Value>) {
        std::cout << "member find\n";
        return cont.find(val);
        }
    else {
        std::cout << "algorithm find\n";
        return std::find(std::begin(cont), std::end(cont), val);
    }
}

I replace contains by find, because I find it a pity to make a hard won information (the item position) go to waste. You can then write contains on top of it.

Complete working example (g++ prog.cc -Wall -Wextra -std=gnu++2a -fconcepts)

#include <iostream>
#include <vector>
#include <map>
#include <algorithm>

template <typename Container, typename Value>
concept bool HasFindFunction = requires(Container c, Value v) {
    c.find(v);
};

template <typename Container, typename Value>
auto find(Container&& cont, Value&& val) {
    if constexpr (HasFindFunction<Container, Value>) {
        std::cout << "member find\n";
        return cont.find(val);
        }
    else {
        std::cout << "algorithm find\n";
        return std::find(std::begin(cont), std::end(cont), val);
    }
}

template <typename Container, typename Value>
auto contains(Container&& c, Value&& v) {
    return std::end(c) != find(c, std::forward<Value>(v));
}

int main() {
    std::map<int, int> map;
    std::vector<int> vector;

    find(map, 5);
    contains(vector, 5);
}


Case insensitive sub-string search I found myself in a need of a function that checks whether a string is a sub-string of another string, in a case-insensitive manner. I found that there's no standard function to do this, so I wrote my own.

Please review for bugs / improvements:

char *istrstr(char *str, const char *substr)
{
    size_t n = 0;
    while (*str) {
        while (str[n] && substr[n]) {
            if (tolower(str[n]) != tolower(substr[n])) {
                break;
            }
            n++;
        }
        if (!substr[n]) {
            return str;
        }
        else if (!str[n]) {
            return substr[n] ? str : NULL;
        }
        n = 0;
        str++;
    }
    return NULL;
}

Both arguments should be const char*. It's a search, it shouldn't be able to change str by accident.

Let's have a look at its use, though. Does it really work like strstr?

puts(istrstr("example", "please")); // prints "ple"

istrstr's behaviour completely differs from strstr. The latter will return NULL when the second string isn't completely in the first one. istrstr's name is therefore misleading and should be called istringoverlap or similar.

Unless you're going for performance, I suggest you to go for simplicity and write an additional helper:

/** @brief checks whether @a prefix is a prefix of @a str.
 *
 * @param str    a null-terminated string
 * @param prefix a null-terminated string
 * @returns 1 if @a prefix is a prefix of @a str
 * @returns 0 otherwise
 *
 * The check is case-insensitive.
 *
 * This function yields undefined behavior if tolower() yields undefined
 * behavior for any character in either str or prefix. If @a str
 * or @a prefix is not a null-terminated string, the behavior will be
 * undefined.
*/
int is_iprefix(const char * str, const char * prefix) {
    while(*str && *prefix) {
        if(tolower(*str) != tolower(*prefix)) {
            return 0;
        }
        prefix++;
        str++;
    }
    return (*prefix == '\0');
}

Why would I recommend to write another function? With only seven lines it's easy to check whether we got that right. We can re-use this function if we want to, and we can use it as a building block for our now istrstr variant:

const char * istrstr(const char * str, const char * substr) {
    while(*str) {
        if(is_iprefix(str,substr)) {
            return str;
        } else {            /* not really necessary, since return exits */
            str++;
        }
    }
    return NULL;
}

However, that's not going for maximum performance. If we're really interested in more performance, we have to stop as soon as str is shorter than substr. We can either use strlen and count down, e.g.

const char * istrstr(const char * str, const char * substr) {
    int diff = strlen(str) - strlen(substr);
    while(*str && diff-- >= 0) {
        if(is_iprefix(str,substr)) {
            return str;
        } else {
            str++;
        }
    }
    return NULL;
}

or we can inline is_iprefix and check whether str runs out of characters, but that's left as an exercise. Your code is almost there, by the way, you just need to fix the aforementioned bug.


Hide and show menu if button clicked and change the icon I wrote simple jQuery code and I want suggestions as to how I can do it better and shorter.

Basically the following code is used to change the menu icon on each click event and also hide and display menu accordingly. If I did it correctly, then fine, but is there any way to make the code shorter and faster?

if ( $('.site-topbar').length ) {
    // prepend menu icon
    $('.nav-topbar').prepend('');

    // hide navigation menu
    $('.nav-topbar').find('#menu-topbar-menu').hide();

    if ( $('.responsive-menu').length ) {

        // prepare click event
        $('.responsive-menu').click(function() {

            $(this).find('.ion-android-menu').attr('class', function(){

                // find class='ion-android-menu' exists
                // then add class='ion-ios-close-empty' if button clicked
                if ( $(this).attr('class') == 'ion-android-menu' ) {
                    $(this).addClass('ion-ios-close-empty');

                    // display menu
                    $('.nav-topbar').find('#menu-topbar-menu').show(800);

                } else {

                    // remove class="ion-ios-close-empty" if condition return false
                    $(this).removeClass('ion-ios-close-empty');

                    // hide menu
                    $('.nav-topbar').find('#menu-topbar-menu').hide(800);
                }
            });
        });
    }
}

Caching Variables and selectors

Whenever selecting an element on the page, if you are going to select it multiple times, try to set the jQuery element as a variable first, and reuse the same variable.

$('.nav-topbar').prepend('

');
...
$('.nav-topbar').find('#menu-topbar-menu').hide();

can become:

var $navBar = $('.nav-topbar');
$navBar.prepend('

');
...
$navBar.find('#menu-topbar-menu').hide();

Caching variables is more performant and can be much easier to read.

You shouldn't have to find by id

IDs are meant to only exist once on the page. By calling .find({selector}), you are effectively saying "Starting from my current element, lets search down the DOM tree until I find something that matches my {selector}"

$('.nav-topbar').find('#menu-topbar-menu').hide();

should be the same as:

$('#menu-topbar-menu').hide();`

If it isn't, then your DOM should be corrected.

== and Type Coercion

As a general rule of thumb, you should always opt for === over == (and conversely opt for !== over !=).

a == b basically means does a sort of equal b. It doesn't take into account the variable's type. "1" == 1 passes as true (even though one is clearly a string, and the other clearly a number).

a === b means does a and b match the same type, and does the value equal each other. In this case, "1" === 1 will return false (because of the difference in type.)

I know this might sound small and nit-picky, but in reality it really isn't. Using == will lead up to unexpected behavior that can be hard to spot.

Additionally, === is faster than ==. === will stop trying to compare two variables if they do not have a matching type.

Simplify Conditional

I want to highlight the following conditional:

if ( $(this).attr('class') == 'ion-android-menu' ) {
    $(this).addClass('ion-ios-close-empty');

    // display menu
    $('.nav-topbar').find('#menu-topbar-menu').show(800);

} else {

    // remove class="ion-ios-close-empty" if condition return false
    $(this).removeClass('ion-ios-close-empty');

    // hide menu
    $('.nav-topbar').find('#menu-topbar-menu').hide(800);
}

You can simplify this conditional to:

var isButtonClicked = $(this).hasClass('ion-android-menu');
$(this).toggleClass('ion-ios-close-empty', isButtonClicked);
$('#menu-topbar-menu').toggle(800);

(Note I strongly recommend using hasClass(...) over .attr('class') == '...', you'll run into less problems in the future this way.

Event Binding

You should try to get into the habit of using $('.parent-class').on('click', '{selector}', function(){}) instead of `$({selector}).click(function(){}).

.click() applies an event handler for every instance of {selector}, whereas the .on() will apply a delegate event handler to .parent-class, and listen for any {selector} within .parent-class. The difference is making 1 event handler compared to making many.

Right here, there shouldn't be too much of a difference. We are assuming that there is only 1 .responsive-menu. But, as you can imagine, using .on() can give huge performance benefits over .click directly.

But, most importantly, by using $({selector}).click(), you are binding the click event to {selector}. Should you remove {selector} and re-add a new {selector} in its place, the click event will be lost.  This is not the case with applying the .on() to a parent element.

Overall approach

Hiding and showing menus has been done a lot, and to be honest, you are applying a lot of javascript for something that can be done mostly with css. I'm not aware of your DOM, however you can move the toggle menu behavior (and even the animation) to your CSS and just have a simple click event handler that fires a toggleClass event.

Example:

    var $wrapper = $('.nav-wrappers');
    $wrapper.on('click', '.responsive-menu', function(){
        $wrapper.toggleClass('showTopBar');
    });
.nav-wrappers .menu {
  background: #ccc;
  padding: 0.5em;
  text-align:center;
  transition: all .5s ease;
  overflow:hidden;
}

.responsive-menu {
  cursor:pointer;
}
.responsive-menu:hover {
  background: #ffa
}

/** Animation Stuff **/

.menu-topbar-menu {
  opacity: 0;
  padding: 0;
  height: 0;
}
.nav-wrappers.showTopBar .menu-topbar-menu {
  opacity: 1;
  height: 1em;
}

Top Menu

Responsive Menu

Hope that helps!


Extension methods for ReaderWriterLockSlim In current project I'm using a lot ReaderWriterLockSlim for synchronizing reading and writing values. Handling try { EnterReadLock(); .... } finaly { ExitReadLock(); } every time I need access value seems to me as copying code. So I created extension methods for ReaderWriterLockSlim which makes me coding much easer and faster (and reusable i think).

However I would like know, if you found some disadvantages in the extension methods I missed or forgot.
There are three extension methods:

ReadOnly<T>(this ReaderWriterLockSlim self, Func<T> readFunc) // calls EnterReadLock()
Read<T>(this ReaderWriterLockSlim self, Func<T> readFunc) // calls EnterUpgradeableReadLock()
Write(this ReaderWriterLockSlim self, Action writeAction) // calls EnterWriteLock()
Write<T>(this ReaderWriterLockSlim self, Func<T> writeAction) // calls EnterWriteLock()

There is code of ReadOnly<T>(...) method. All methods looks similar.

public static T ReadOnly<T>(this ReaderWriterLockSlim self, Func<T> readFunc) {
    if (object.ReferenceEquals(self, null)) { throw new ArgumentNullException("self"); }
    if (object.ReferenceEquals(readFunc, null)) { throw new ArgumentNullException("readFunc"); }

    T result;

    // flag, if lock was entered
    bool lockEntered = false;
    try {
        // I don't want the ThreadAbortException during claiming the lock
        Thread.BeginCriticalRegion();
        try {
            self.EnterReadLock();
            lockEntered = true;
        }
        finally {
            Thread.EndCriticalRegion();
        }

        result = readFunc();
    }
    finally {
        // I don't want the ThreadAbortException during releasing the lock
        Thread.BeginCriticalRegion();
        try {
            if (lockEntered) {
                self.ExitReadLock();
            }
        }
        finally {
            Thread.EndCriticalRegion();
        }
    }

    return result;
}

It looks pretty good. I can't see anything less than great.
I'd just add that to your code:

    private static void Critical(Action criticalAction)
    {
        try
        {
            Thread.BeginCriticalRegion();
            criticalAction();
        }
        finally
        {
            Thread.EndCriticalRegion();
        }
    }

And i used the IsWriteLockHeld, IsReadLockHeld, etc, before exiting the lock.


Three PHP database queries to manage accounts I have a database connection PHP file which has a lot of queries to the database. Here are 3 of the functions that I am using:

public function deleteAccount($token)
{
    $sql1 =
        "DELETE FROM $this->table_user
         WHERE $this->key_token = ?;";

    $stmt1 = $this->connection->prepare($sql1);
    $stmt1->bind_param("s", $token);
    $stmt1->execute();
    $result = $stmt1->affected_rows;
    $stmt1->close();
    return ($result >= 1);
}

public function getAccountDetails($token)
{
    $sql1 =
        "SELECT $this->key_name, $this->key_username, $this->key_email
         FROM $this->table_user
         WHERE $this->key_token = ?;";
    $stmt1 = $this->connection->prepare($sql1);
    $stmt1->bind_param("s", $token);
    $stmt1->execute();
    $stmt1->bind_result($name, $username, $email);
    $stmt1->fetch();
    $stmt1->close();
    return
        array(
            "name" => $name,
            "username" => $username,
            "email" => $email
        );
}

public function resetPassword($password, $hash, $email)
{
    $sql1 =
        "UPDATE $this->table_user
         SET $this->key_hash = ?
         WHERE $this->key_email = ? AND $this->key_passwordhash = ?;";

    $stmt1 = $this->connection->prepare($sql1);
    $passhash = password_hash($password, $this->algorithm);
    $stmt1->bind_param("sss", $passhash, $email, $hash);
    $stmt1->execute();
    $result = ($stmt1->affected_rows == 1);
    $stmt1->close();
    return $result;
}

To me this seems like a lot of code duplication and I find myself doing a lot of copying and pasting which is a big sign that it is very duplicated but I am struggling to see how I can refactor this code so that for every sql query I perform it does not require so many lines of code.

Common functionality refactoring

public function deleteAccount($token)
{
    $sql1 =
        "DELETE FROM $this->table_user
         WHERE $this->key_token = ?;";

    $stmt1 = $this->connection->prepare($sql1);
    $stmt1->bind_param("s", $token);
    $stmt1->execute();
    $result = $stmt1->affected_rows;
    $stmt1->close();
    return ($result >= 1);
}

You could change this to something like

private function modify($sql, array $parameters = [])
{
    $stmt = $this->connection->prepare($sql);

    if (!empty($parameters))
    {
        // ensure $parameters are passed by reference
        $parameters_by_reference = [];
        for ($i = 0, $n = count($parameters); $i < $n; $i++)
        {
            $parameters_by_reference[] =& $parameters[$i];
        }

        call_user_func_array([$stmt, bind_param], $parameters_by_reference);
    }

    $stmt->execute();
    $result = $stmt->affected_rows;
    $stmt->close();

    return $result;
}

public function deleteAccount($token)
{
    $sql = <<<EOSQL
DELETE FROM $this->table_user
  WHERE $this->key_token = ?
EOSQL;

    return ($this->modify($sql, ['s', $token]) >= 1);
}

Now it only takes two or three statements to set up the SQL query.  The repeated part is all inside the modify function.  Note that modify can handle any of INSERT, UPDATE, or DELETE.

I prefer the heredoc as being better for multiline strings.

I don't like numbered variables, particularly if there's only one of them.

I prefer to use single quotes for PHP strings that do not have any variable interpolation in them.

You do not need to pass a ; when executing a single statement programmatically.

Be careful about $this->table_user and $this->key_token.  These should NOT contain any user input.  Otherwise, this would be subject to SQL injection.

You can then change

public function resetPassword($password, $hash, $email)
{
    $sql1 =
        "UPDATE $this->table_user
         SET $this->key_hash = ?
         WHERE $this->key_email = ? AND $this->key_passwordhash = ?;";

    $stmt1 = $this->connection->prepare($sql1);
    $passhash = password_hash($password, $this->algorithm);
    $stmt1->bind_param("sss", $passhash, $email, $hash);
    $stmt1->execute();
    $result = ($stmt1->affected_rows == 1);
    $stmt1->close();
    return $result;
}

to

public function resetPassword($password, $hash, $email)
{
    $sql = <<<EOSQL
UPDATE $this->table_user
  SET $this->key_hash = ?
  WHERE $this->key_email = ? AND $this->key_passwordhash = ?
EOSQL;

    $passhash = password_hash($password, $this->algorithm);

    return ($this->modify($sql, 'sss', $passhash, $email, $hash) == 1);
}

PDO

Note that if you switch to PDO, then modify could be simpler:

private function modify($sql, array $parameters = [])
{
    $stmt = $this->connection->prepare($sql);

    foreach ($parameters as $parameter => $value)
    {
        $stmt->bindParam($parameter, $value);
    }

    $stmt->execute();

    return $stmt->rowCount();
}

And you would call it like

    return ($this->modify($sql, [1 => $passhash, 2 => $email, 3 => $hash]) == 1);
    return ($this->modify($sql, [1 => $token]) >= 1);

Or you could use named parameters, but that requires modifying the SQL string as well.


JavaScript Armstrong Number Validator (ES6) I have the following function which takes in an input and validates if that input is an Armstrong number or not:

  An Armstrong Number is a number such that the sum of the cubes of its digits is equal to the number itself. For example, 371 is an Armstrong number since 3 ** 3 + 7 ** 3 + 1 ** 3 = 371.

Here's a link to the program on codepen.

const isArmstrong = input => {
  const expandedInputExpression = `
    ${input[0]} ** 3 + ${input[1]} ** 3 +
    ${input[2]} ** 3`;

  const expandedInputValue = Array.from(input)
    .map(digit => digit)
    .reduce((total, currentDigit) => total + currentDigit ** 3, 0);

  if (expandedInputValue === parseInt(input)) {
    console.log(`${input} is an Armstrong number since ${expandedInputExpression} = ${input}`);
    return true;
  } else {
    console.log(`
    ${input} is not an Armstrong number because ${expandedInputExpression}( ${expandedInputValue}) is not equal to ${input}`);
    return false;
  }
};

Is there any way to refactor and simplify the function?

Being called isArmstrong, it should just return a boolean value, and not have side effects of setting any sort of DOM. That can be done in a separate function by the caller.
expandedInputValue can probably have a better name. Maybe digitTotal or digitCubeTotal. But the current name isn't helpful.
I think this is a no-op: .map(digit => digit)


HTML form enable one list of options or another I'm trying to display a form where a user can select a specific item from a list or a specific category of items from another list. I want them to select one or the other, not both.

I followed this question which solves how to handle this in the backend.

I still need to figure out how to properly show this in the view.
I have the following sample which does what I would like to achieve, but I'm wondering if this is the way to go:

$('.formGroup input:checkbox').on('click', function() {
  var id = $(this).prop('id');
  var groups = $(this).parents('.formGroup').siblings('.formGroup');
  var thisGroup = $(this).parents('.formGroup');

  $('select', thisGroup).prop('disabled', false);

  $.each(groups, function(index, item) {
    var checkbox = $('input:checkbox', item);
    var select = $('select', item);

    checkbox.prop('checked', false);
    select.val(-1).prop('disabled', true);
  })
})

    <input type="checkbox" value="" id="Option1">Select an Item
  <select disabled="disabled">
    <option value=""></option>
    <option value="1">Item 1</option>
    <option value="2">Item 2</option>
    <option value="3">Item 3</option>
  </select>

    <input type="checkbox" value="" id="Option2">Or select a Category
  <select disabled="disabled">
    <option value=""></option>
    <option value="1">Category 1</option>
    <option value="2">Category 2</option>
    <option value="3">Category 3</option>
  </select>

Possible problem of the current solution

If you check one of the checkboxes, it gets activated and the select-element becomes enabled. If you click the same checkbox again, you only uncheck it, but the select-element stays enabled. I'd say there are two possibilities here:

disable the select-element as well
don't let the user uncheck the input-element

In the following examples I went with the first option.
As requested in the comments, I've updated the review to work for the second option.

Reduce the number of DOM queries and cache selectors

You didn't say how many of those groups are present or how often a user is going to make this decision. But you do a lot of the same DOM queries in your event handler over and over again.

At first let's store the "group-selector" in a string, to make it re-usebale. Then select all groups:

const selector = '[data-form-group]';
const groups = $(selector);

You can stick to classes if you like. I tend to use data-*-attributes to couple JavaScript.

As we also often need the input- and the select-element in each group, let's store them as well:

groups.each(function() {
    const group = $(this);
    group.data('checkbox', group.find('input'));
    group.data('select', group.find('select'));
});

Now we can access the form elements anytime, without re-querying the DOM.

The actual event handler

Attach the event handler to each input-element, the checkboxes radio-buttons. Then test, whether the current group is "enabled", i.e. the input-element is checked:

If it is checked already

do nothing and simply return

If it is not checked

enable the select-element
run through all other groups and disable them

The final code could look like this:

const selector = '[data-form-group]';
const groups = $(selector);

groups.each(function() {
  const group = $(this);
  group.data('toggle', group.find('input'));
  group.data('options', group.find('select'));

  group.data('toggle').on('change', function() {
    const selectedGroup = $(this).closest(selector);

    if (!selectedGroup.data('toggle').is(':checked')) {
      return;
    }

    selectedGroup.data('options').prop('disabled', false);

    groups.each(function() {
      const current = $(this);

      if (selectedGroup[0] === current[0]) {
        return;
      }

      current.data('toggle').prop('checked', false);
      current.data('options').prop('disabled', true);
    });
  });
});

Try it yourself


My own functions for working with files I wrote the functions for opening a file, getting a file size and getting a file contents. I would welcome any suggestions to improve my code.

You might think I'm over-commenting the code. But I do it mostly just for the visual separation of the code parts. I hope to know your opinion about it.

P. S. English isn't my native language, but I try to write all code comments in English. So I would welcome any corrections also!

/*
===============
open_file
===============
*/
FILE *open_file(char const * restrict name, char const * restrict mode) {
    /* Debug: checking the input arguments. */
    assert(name != NULL);
    assert(mode != NULL);

    /* Opening the file and checking if an error occurs. */
    FILE *file = fopen(name, mode);
    if (file == NULL) {
        LOG("Error: the file \"%s\" couldn't be opened (in the \"%s\" mode). "
            "The reason: \"%s\".\n", name, mode, strerror(errno));
    }

    return file;
}

/*
===============
get_file_size
===============
*/
long get_file_size(FILE *file) {
    /* Debug: checking the input arguments. */
    assert(file != NULL);

    /* Setting the file position to the ending of the file. */
    fseek(file, 0, SEEK_END);

    /* Getting the size of the file. */
    const long size = ftell(file);
    if (size == -1) {
        LOG("Error: can't get the current value of the file position. The "
            "reason: \"%s\".\n", strerror(errno));
    }

    /* Setting the file position to the beginning of the file. */
    rewind(file);
    return size;
}

/*
===============
get_file_contents
===============
*/
char *get_file_contents(FILE *file) {
    /* Debug: checking the input arguments. */
    assert(file != NULL);

    /* Getting the size of the file. */
    long const size = get_file_size(file);
    if (size == -1)
        return NULL;

    /* Allocating memory for the contents of the file. */
    char * const contents = calloc(size + 1, 1);
    if (contents == NULL) {
        LOG("Error: can't allocate %ld bytes for a contents of a file. "
            "The reason: \"%s\".\n", size, strerror(errno));
    }

    /* Reading the contents of the file and checking if an error occurs. */
    size_t const read_size = fread(contents, 1, size, file);
    if (read_size < (size_t) size) {
        LOG("Warning: the size of elements successfully read (%zu bytes) is "
            "less than the size of the file (%ld bytes). It also may occurs "
            "if the file uses the CR+LF line endings (it is the Microsoft "
            "Windows system usually).\n", read_size, size);
    }

    return contents;
}

I must admit that I didn't went through all of your code and have not even compiled it. Some ideas for improvement:

open_file:

I would change your function from

FILE *open_file(char const * restrict name, char const * restrict mode)

to something like this

int open_file(const char *name, const char *modes, FILE **file)

I prefer using a return value that indicates a successful function execution for functions that can fail. By using this you don't have to remember that the function is not successful if the return values is NULL. You will always get a FAILURE return for each function. Inside the function you could use something like this:

int open_file(const char *name, const char *modes, FILE **file) {
    if (name == NULL) { goto fail; }
    if (modes == NULL) { goto fail; }
    *file = fopen(name, modes);
    if (*file == NULL) { goto fail; }
    return SUCCESS;
fail:
    return FAILURE;
}

Even if someone tells you using goto would be bad. I absolutely love it for this situation. The main reason against goto is, that it can create code that is hard to understand. But in this case to me there is no clearer version due to the usage of goto.

If that is still not readable enough for you, you can create macros for error-checking. In these macros you could also integrate printing or logging error messages. For example for developement you could switch on printing error messages and for production code compile it without any error messages if the user shall not see them.

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

#define SUCCESS EXIT_SUCCESS
#define FAILURE EXIT_FAILURE

#define ERRTEST_ARG_NULL(arg, tag) \
    do { \
        if (!(arg)) { \
            printf("%s:%d: In function '%s': error: %s: ERROR_ARG_NULL\n", __FILE__, __LINE__, __func__, tag); \
            goto fail; \
        } \
    } while (0)

int open_file(const char *name, const char *modes, FILE **file) {
    ERRTEST_ARG_NULL(name, "name");
    ERRTEST_ARG_NULL(modes, "modes");
    *file = fopen(name, modes);
    ERRTEST_ARG_NULL(*file, "*file");
    return SUCCESS;
fail:
    return FAILURE;
}

int main(void) {
    FILE *file;
    printf("%d\n", open_file("C:/pathToFile/file.txt", "rb", &file));
    getchar();
    return 0;
}

assert

I would go with the above solution for error-checking because you are able to continue with you program even if an error occured. Think of an user that tries to open a file but uses a wrong file name. Using 'assert' your program would stop working from the first problem.

fread

'fread' will fail for loading bigger files than 1GB (at least on my machine). You can still use it for bigger files by reading them sequentially in chunks with sizes smaller than 1GB and then adding them at the right spot in your allocated array.

+1 for introducing restrict to me. That was new to me.


blacklist with wildcard using Lucene I have a table that contains information about users, which I deny loading a specific web page.

Each record contains 4 columns: referrer, ip, userAgent & email.

Each column can contain a wild card (i.e. an asterisk *), which make me block a group.

How could performance be improved with this code?

import org.apache.lucene.analysis.standard.StandardAnalyzer;
import org.apache.lucene.document.Document;
import org.apache.lucene.document.Field;
import org.apache.lucene.document.StringField;
import org.apache.lucene.index.DirectoryReader;
import org.apache.lucene.index.IndexWriter;
import org.apache.lucene.index.IndexWriterConfig;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.BooleanClause;
import org.apache.lucene.search.BooleanQuery;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.TermQuery;
import org.apache.lucene.store.RAMDirectory;

import java.io.IOException;

class BlacklistLucene {

    private static final String MATCH_ALL = "*";
    private static IndexSearcher cache;

    private enum Fields {
        referrer, ip, userAgent, email
    }

    private static Document getDocument(String referrer, String ip, String userAgent, String email) {
        Document doc = new Document();
        doc.add(getStringField(referrer, Fields.referrer.name()));
        doc.add(getStringField(ip, Fields.ip.name()));
        doc.add(getStringField(userAgent, Fields.userAgent.name()));
        doc.add(getStringField(email, Fields.email.name()));
        return doc;
    }

    private static StringField getStringField(String val, String field) {
        return new StringField(field, val, Field.Store.NO);
    }

    private static BooleanQuery createQuery(String referrer, String ip, String userAgent, String email) {
        return new BooleanQuery.Builder()
                .add(createBooleanQuery(Fields.referrer.name(), referrer), BooleanClause.Occur.FILTER)
                .add(createBooleanQuery(Fields.ip.name(), ip), BooleanClause.Occur.FILTER)
                .add(createBooleanQuery(Fields.userAgent.name(), userAgent), BooleanClause.Occur.FILTER)
                .add(createBooleanQuery(Fields.email.name(), email), BooleanClause.Occur.FILTER)
                .build();
    }

    private static BooleanQuery createBooleanQuery(String key, String value) {
        return new BooleanQuery.Builder()
                .add(new TermQuery(new Term(key, value)), BooleanClause.Occur.SHOULD)
                .add(new TermQuery(new Term(key, MATCH_ALL)), BooleanClause.Occur.SHOULD)
                .build();
    }

    private static boolean isBlacklisted(String ref, String ip, String ue, String email) throws IOException {
        BooleanQuery query = createQuery(ref, ip, ue, email);
        return cache.search(query, 1).totalHits > 0;
    }

    public static void main(String[] args) throws Exception {

        RAMDirectory directory = new RAMDirectory();
        IndexWriter writer = new IndexWriter(directory, new IndexWriterConfig(new StandardAnalyzer()));
        writer.addDocument(getDocument("ref1", "127.0.0.ip1", "Mozilla UserAgent1", "email.com"));
        writer.addDocument(getDocument("ref2", "127.0.0.ip2", "Mozilla UserAgent2", "*"));
        writer.close();
        DirectoryReader reader = DirectoryReader.open(directory);
        cache = new IndexSearcher(reader);

        System.out.println(isBlacklisted("ref1", "127.0.0.ip1", "Mozilla UserAgent1", "email.com")); // true
        System.out.println(isBlacklisted("r2.com", "127.0.0.4", "Mozilla", "yahoo.com")); // false
        System.out.println(isBlacklisted("ref2", "127.0.0.ip2", "Mozilla UserAgent2", "this is ignored")); // true
        System.out.println(isBlacklisted("*", "127.0.0.ip2", "Mozilla UserAgent2", "*")); // true
    }
}

private enum Fields {
    referrer, ip, userAgent, email

It is customary to use UPPERCASE for enum members.

private static Document getDocument(String referrer, String ip, String userAgent, String email) {

I wouldn't mind seeing a javadoc somewhere that explains what a referrer URL looks like. It can be quite detailed, including ? query terms, but I imagine you strip it down to page or FQDN level, or perhaps even to 2nd-level domain, which .co.uk and others add interesting wrinkles to.

Similarly, I expect ip looks like "10.0.1.2", but some folks prefer the sort order they get from normalizing to "010.000.001.002" or compact hex "0A000102". And the UA might be case-smashed or truncated, and (at least the 2nd half of) the email might be case-smashed, as well.

Some folks choose to blacklist a prefix rather than a /32 individual IP, e.g. matching first three bytes for a /24, or masking for a /20. If you could typically combine 512 IPs into a /23 prefix, your DB would shrink by orders of magnitude, making cache hits go up and random I/Os go down, supporting higher query rates.

I see no evidence that your server answers IPv6 clients. Handling prefixes would be even more important in that environment.

Kudos for following Webster, despite history.
Oh, actually, wait! Does Fields.referrer.name() even work? Or does it return one more "r" than headers arriving off-the-wire have?
Do tests succeed?

Thank you for adding the getStringField() helper - it improves readability.
I wonder if we could achieve even greater conciseness by putting it in the Fields class?

            .add(new TermQuery(new Term(key, value)), BooleanClause.Occur.SHOULD)

I'm reading BooleanClause.Occur :

  SHOULD.
  Use this operator for clauses that should appear in the matching documents. For a BooleanQuery with no MUST clauses one or more SHOULD clauses must match a document for the BooleanQuery to match.

I wouldn't mind seeing a comment explaining why SHOULD works better here. The simplest thing to do seems like MUST - then the Gentle Reader wouldn't be puzzling out what other interactions there might be, what might override it.

private static boolean isBlacklisted(String ref, String ip, String ue, String email) throws IOException {

The ue identifier is very odd. A more natural choice would be ua.

    RAMDirectory directory = new RAMDirectory();

Rather than invent a name for this temp, you might have just buried it in the IndexWriter() call. Then I don't have to worry about other interactions further down in the code. Code with fewer variables in-scope is code that is more easily read at a glance.

    cache = new IndexSearcher(reader);

I would have preferred to see this as a static initializer, rather than deferring until main runs, to make your public API easier to use and so more reusable. Static initializers run at class load time, which can threaten to introduce cyclic dependencies among your own classes, but here the app and the library are well separated so I don't have that concern.

    System.out.println(isBlacklisted("ref1", "127.0.0.ip1", "Mozilla UserAgent1", "email.com")); // true
    System.out.println(isBlacklisted("r2.com", "127.0.0.4", "Mozilla", "yahoo.com")); // false

Well! Now we finally get down to brass tacks, answering some doubts I voiced above. Ok, first thing, it's nice to see the test results printed, but it would so much nicer to have a self-evaluating test, e.g. assertTrue in JUnit.

POLA violation: I was not expecting that "ref1" might be a plausible valid input. Yes, I understand you're making up test inputs. But that's not even an FQDN. Maybe I should understand the substring "hoo" would match "yahoo.com"? When I saw referrer I was kind of mislead into thinking in RFC terms, rather than grep -F terms. At some point you'll blacklist "oo.com" and then see interesting interactions.

Similarly "ip1" was unexpected, and while "Mozilla" makes perfect sense the longer UAs are very test oriented, given that matching a structured user-agent is complex in its own right. Consider pre-processing "equivalent" UAs down to a single-word identifier before handing them to Lucene.

Test coverage seems light. At a bare minimum you probably want four or eight tests, each with three wildcards, so you can focus on individual field behavior.

Test cases are an opportunity to educate folks about your public API, just as javadoc offers an opportunity to educate.  Good test data can help to reveal your intent.

It's hard to comment on performance when it isn't clear what semantics you support (e.g. substring vs. prefix-match) and what Lucene features you considered using to support performant queries. Add some benchmark results, and contrast e.g. "Lucene answers 20 query/second" with a requirement of "peak web traffic is 10 hit/sec" or "is 30 hit/sec" so we can see any gap. Mention Winchester vs. SSD, and available RAM. It would be useful for IddoE to create a separate Answer posting that describes any recent work with this code.

Caching is a big deal for performance, so you will need a representative workload, either synthetic or gathered from production. Issuing 20 high-entropy queries each second is very different from 20 predictable queries.

Lucene is very nice, and can be fast. In createQuery() it's not obvious to me that trusting Lucene to optimize MATCH_ALL will yield fastest results. You could just suppress such terms, so Lucene considers between 1 and 4 fields according to how many MATCH_ALL's you used.


Naive Bayes Classifier in C# I took the code from internet and tried to simplify it. So, what can I do?

How can I simplify my code more to make it easily understandable?

public class NaiveBayesAlgorithm
{
    private static List<Training> trainingDataSet = new List<Training>();
    private static Dictionary<string, HeightWeightSize> gaussianDataSet = new Dictionary<string, HeightWeightSize>();
    private static Dictionary<string, Gaussian> gaussianList = new Dictionary<string, Gaussian>();

    //takes a table
    public static void Train(List<Training> trainingData)
    {
        //adding the training data to the dataset
        trainingDataSet = trainingData;

        foreach(Training t in trainingDataSet)
        {
            if (!gaussianDataSet.ContainsKey(t.Class))
            {
                gaussianDataSet.Add(t.Class, new HeightWeightSize());
            }

            gaussianDataSet[t.Class].Height.Add(t.Height);
            gaussianDataSet[t.Class].Weight.Add(t.Weight);
            gaussianDataSet[t.Class].FootSize.Add(t.FootSize);
        }

        foreach(string key in gaussianDataSet.Keys)
        {
            Gaussian gaussian = new Gaussian();
            gaussian.Sex = key;
            gaussian.Count = gaussianDataSet[key].Height.Count;

            gaussian.HeightMean = Statistics.Mean(gaussianDataSet[key].Height);
            gaussian.WeightMean = Statistics.Mean(gaussianDataSet[key].Weight);
            gaussian.FootSizeMean = Statistics.Mean(gaussianDataSet[key].FootSize);

            gaussian.HeightVariance = Statistics.Variance(gaussianDataSet[key].Height);
            gaussian.WeightVariance = Statistics.Variance(gaussianDataSet[key].Weight);
            gaussian.FootSizeVariance = Statistics.Variance(gaussianDataSet[key].FootSize);

            gaussianList.Add(key, gaussian);
        }

        string str2 = string.Empty;
    }

    public static string Classify(double[] obj)
    {
        double maxScore = 0;
        string className = string.Empty;

        foreach( string key in gaussianList.Keys)
        {
            gaussianList[key].HeightNormalDist = Statistics.NormalDist(obj[0], gaussianList[key].HeightMean, gaussianList[key].HeightVariance);
            gaussianList[key].WeightNormalDist = Statistics.NormalDist(obj[1], gaussianList[key].WeightMean, gaussianList[key].WeightVariance);
            gaussianList[key].FootSizeNormalDist = Statistics.NormalDist(obj[2], gaussianList[key].FootSizeMean, gaussianList[key].FootSizeVariance);

            gaussianList[key].CombinedNormalDist = gaussianList[key].HeightNormalDist * gaussianList[key].WeightNormalDist * gaussianList[key].FootSizeNormalDist;
            gaussianList[key].UltimateScore = gaussianList[key].CombinedNormalDist * 0.5;

            if(maxScore < gaussianList[key].UltimateScore)
            {
                maxScore = gaussianList[key].UltimateScore;
                className = key;
            }
        }

        return className;
    }
}

HeightWeightSize

This doesn't make a good class name. What if you add another property? You'd need to change its name again.

I guess Person or something alike would be much better.

Gaussian gaussian = new Gaussian();
gaussian.Sex = key;
gaussian.Count = gaussianDataSet[key].Height.Count;

gaussian.HeightMean = Statistics.Mean(gaussianDataSet[key].Height);
gaussian.WeightMean = Statistics.Mean(gaussianDataSet[key].Weight);
gaussian.FootSizeMean = Statistics.Mean(gaussianDataSet[key].FootSize);

gaussian.HeightVariance = Statistics.Variance(gaussianDataSet[key].Height);
gaussian.WeightVariance = Statistics.Variance(gaussianDataSet[key].Weight);
gaussian.FootSizeVariance = Statistics.Variance(gaussianDataSet[key].FootSize);

You could simplify this by putting it inside the Gaussian class.

class Gaussian
{
    public Gaussian(string sex, int count, double height, double weight, double footSize)
    {
        HeightMean = Statistics.Mean(Height);
        // ...
    }

    public double HeightMean { get; }
    // ...
}

Adding it to a dictionary would then can be reduced to:

gaussianList.Add(key, new Gaussian(
    sex,
    gaussianDataSet[key].Count
    gaussianDataSet[key].Height,
    ...,
    ...)
);

There is still too much redundancy and you could improve more then the above but I don't know how the HeightWeightSize type looks like.


Database query inside a method of a class and return data I'm trying to find out what is the right approach to return DB values from a class. This is what I came up with:

class Category {

  public $result;
  public $errors;

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

  public function getCategoryNames () {

    $sql = "SELECT category_id, category_name FROM category";

    try {
      $query =  $this->pdo->prepare( $sql );

      if( $query->execute( ) ) {
        if( $query->rowCount() ) {
          $this->result = $query->fetchAll();
          return true;
        }
        return false;
      }
      return false;
    } catch ( PDOException $pe ) {
      trigger_error( 'SQL Error' . $pe->getMessage() );
    }
  }

  public function getUserDetails($userid) {
    $sql = "SELECT * FROM users WHERE user_id = :user_id";

    try {
      $query =  $this->pdo->prepare( $sql );

      if( $query->execute( array( 'user_id' => $userid ) ) ) {
        if( $query->rowCount() ) {
          $this->result = $query->fetch(PDO::FETCH_ASSOC);
          return true;
        }
        return false;
      }
      return false;
    } catch ( PDOException $pe ) {
      trigger_error( 'SQL Error' . $pe->getMessage() );
    }
  }

$category = new Category($pdo);
$category->getCategoryNames();
print_r($result);
$category->getUserDetails('1');
print_r($result);

Is this approach good? Or what is the proper way to do this?

Let me first say that any answer you get here is going to by biased by each person's personal preference. What you're doing isn't dangerous or breaking any rules, so it's 'fine' as far as that's concerned. So with that in mind I'll give you my opinion on a few things to consider, and you can decide from there.

Returning true and false to me is a bit old-school and doesn't take advantage of php's flexible variable type handling. I typically return FALSE if there's an error, and return the results if not. The calling code can then just do a if($result !== FALSE) and you're good to go.

The other thing to consider is the format of your return data.

One method is to dump it into some predefined objects. This is a very clean approach that makes code reliable and easy to read, but it isn't always that easy to implement. I would only do this if you're going to be creating a large and complex project.

If you've got a simple project, then returning arrays is the fastest and easiest way to go. It's a little less reliable if your database is likely to change, so watch out for that. I typically massage my arrays in the method prior to returning them. Since in your example you're breaking out specific operations, you can do custom data formatting in each. This is encouraged since it will make your calling code much easier to create, read, and maintain.

Lastly I wanted to talk about your error handling. What I prefer to do is return FALSE on error only, and return data every other time. You've chosen to return FALSE if no results are found which makes a bit of sense as it will simplify the calling code. The change I would suggest you make is to remove your trigger_error method and instead set the error message to a class variable that can be retrieved by your calling code. Your should then return FALSE after the catch. This will allow your code to fail more gracefully and also give you more control in your calling code as to how to handle the error (think MVC).

Of course I am making some assumptions as to what trigger_error does. I see you have an $errors variable so perhaps you're already on that track. If you are then I recommend always returning FALSE from the method after an error, and adding hasErrors and getErrors methods to your class.


Show cents only if price contains them I have to show price of an item. If the price is only in dollars, it should look like an integer. But if it contains cents, it should look like double value and always have two symbols after decimal separator:

  2 550$
  2 550,55$
  2 550,50$

I decided to use Decimal format for this task, but I don't know how to properly handle the cents.

Here is my solution:

public static String moneyFormat(double amount, String currency) {
    DecimalFormatSymbols dfSymbols = new DecimalFormatSymbols();
    dfSymbols.setDecimalSeparator(',');
    dfSymbols.setGroupingSeparator(' ');
    dfSymbols.setCurrencySymbol(currency);
    DecimalFormat df;
    if (isInteger(amount)) {
        df = new DecimalFormat("###,###,##0 \u00A4", dfSymbols);
    } else {
        df = new DecimalFormat("###,###,##0.00 \u00A4", dfSymbols);
    }
    df.setGroupingUsed(true);
    return df.format(amount);
}

private static boolean isInteger(double amount) {
    return  !Double.isInfinite(amount) && (amount == Math.floor(amount));
}

The amount comes from server in Double, therefore, probably, we can have some errors in case if 2$ will be represented as 1,999(9)$ but I am not sure how to test for such errors.

As an alternative to passing two patterns I can use             df.setMaximumFractionDigits();:

 df = new DecimalFormat("###,###,##0.00 \u00A4", dfSymbols);
if (isInteger(amount)) {
    df.setMaximumFractionDigits(0);
} else {
    df.setMaximumFractionDigits(2);
}

But the question stays the same: can I handle integers in some other, cleaner way?

Update

Unfortunately, changing output from server to String or other types is not an option. And I forgot to mention, that it for Android. I don't use classes of platform, so I haven't added it to tags.

I know that you receive double, but you shouldn't get your system "infected" by double prices. This is just wrong. Convert them once you received them.

Instead of comparing your Doubles to find the Integer, you could also just work with the String and replace the zeros. This will also work with your double values and is a valid approach if you just want to render the number.

import java.math.BigDecimal;
import java.text.DecimalFormat;
import java.text.DecimalFormatSymbols;

public class Main
{
    public static void main(String... args)
    {
        System.out.println(format(new BigDecimal("2.1111")));
        System.out.println(format(new BigDecimal("5454542.1111")));
        System.out.println(format(new BigDecimal("5454542.1")));
        System.out.println(format(new BigDecimal("5454542")));
    }

    private static String format(BigDecimal price)
    {
        return createFormat().format(price).replace(",00", "");
    }

    private static DecimalFormat createFormat()
    {
        DecimalFormatSymbols dfSymbols = new DecimalFormatSymbols();
        dfSymbols.setDecimalSeparator(',');
        dfSymbols.setGroupingSeparator(' ');
        final String currency = "$";
        dfSymbols.setCurrencySymbol(currency);
        return new DecimalFormat("###,###,##0.00 \u00A4", dfSymbols);
    }
}

This will give you:

2,11 $
5 454 542,11 $
5 454 542,10 $
5 454 542 $


Finding the duplicate with an array with 1,000,000 integers between 1 and 1,000,000 I recently had an interview and got to phase 2 which is a coding assessment. One of the questions was:

  Given an array with 1,000,000 integers between 1 and 1,000,000, one integer is in the array twice. Find the duplicate.

I failed the interview and was wondering what improvements do I need.

/// <summary>
/// Assuming that there is no space constraint,
/// I used a HashSet to find the duplicate.
/// If the duplicate is found, assign that number as the return value
///
/// </summary>
/// <param name="givenArray">1,000,000 sized array containing ints between 1 - 1000000</param>
/// <returns>The duplicate number</returns>
public static int findDuplicate(int[] givenArray)
{
    // This hashset will be filled with non duplicated numbers.
    HashSet<int> nonDuplicateContainer = new HashSet<int>();
    int duplicate = -1;

    for (int i = 0; i < givenArray.Length; i++)
    {
        if (!nonDuplicateContainer.Add(givenArray[i])) {
            duplicate = givenArray[i];
            break;
        }
    }

    return duplicate;
}

Test:

/// <summary>
/// Testing with the correct duplicated number
/// </summary>
[TestMethod()]
public void FindDuplicateTestCorrectGivenDuplicate()
{
    int[] testArray = InitializeOneMillionSizedArray(106500);

    int testDuplicateNumber = BlueWolfSolutions.findDuplicate(testArray);

    Assert.AreEqual(106500, testDuplicateNumber);
}

/// <summary>
/// Testing with the incorrect duplicated number
/// </summary>
[TestMethod()]
public void FindDuplicateTestWrongGivenDuplicate()
{
    int[] testArray = InitializeOneMillionSizedArray(453234);

    int testDuplicateNumber = BlueWolfSolutions.findDuplicate(testArray);

    Assert.AreNotEqual(25, testDuplicateNumber);
}

There is already a good answer that describes how you could improve your tests, however, I would like to offer another solution.  I had a question that was similar to this on an interview (well, I solved it using a similar method to the one I gave here).

When the interviewer asked this question, I think they wanted you to find a solution that is not only \$\mathcal{O}(n)\$ time but also \$\mathcal{O}(1)\$ space complexity.  Based on the description you gave, there is one number that appears twice but there also must be one number that does not appear.

The sum of the first \$n\$ integers is \$\frac{n(n + 1)}{2}\$, and the sum of their squares is \$\frac{n(n + 1) (2n + 1)}{6} \$.

Let

\$sn\$ = 500000500000 be the sum of all of the numbers from 1 to 1000000
\$sn2\$ = 333333833333500000 be the sum of their squares
\$sn'\$ be the sum of the elements of the array
\$sn2'\$ be the sum of their squares

Then there are numbers \$d\$ (double) and \$m\$ (missing) such that

$$sn' + m - d = sn$$
$$sn2' + m^2 - d^2 = sn2$$

Solving the first equation for \$m\$ and substituting into the second, we get

$$sn2' + (sn + d - sn')^2  - d^2 = sn2$$

Expand and collect like terms

$$sn2' + (sn - sn')^2 + 2 (sn - sn')d = sn2$$

Thus

$$d = \dfrac{(sn2 - sn2') - (sn - sn')^2}{2 (sn - sn')}$$

Here is a version of findDuplicate that uses this rule (note that I have never used C# so there may be a couple of mistakes, and I did not put in comments since you've already read the explanation of how I got the formula):

/// <summary>
/// Find the duplicate number in a length n array where all elements are numbers
/// from 1 to n, all appearing once except for one duplicate (and consequently one
/// missing).
///
/// </summary>
/// <param name="givenArray">n sized array containing ints between 1 - n inclusive</param>
/// <returns>The duplicate number</returns>
public static int findDuplicate(int[] givenArray)
{
    ulong n = givenArray.Length, dsn, dsn2;
    dsn = n*(n + 1)/2;
    dsn2 = dsn*(2*n + 1)/3;
    for(ulong i = 0; i < n; ++i){
        dsn -= givenArray[i];
        dsn2 -= givenArray[i]*givenArray[i];
    }
    return (dsn2 - dsn*dsn)/(2*dsn);
}

Note that this method of finding extra or missing numbers in a list that is otherwise a range can be extended simply by adding equations for higher powers (cubes and beyond).


Decorate instance method with (arbitrary) meta data in python Requirements

What I need: attach meta information to methods, such that

it is 'easy' to retrieve all such methods for a given class instance
methods can still be called 'in a normal way', e.g. obj.method()
meta-data is accessible from the decorated method, e.g. obj.method.data
IDEs (PyCharm in particular) do not produce any warnings or errors (and, if possible, IDE support, e.g. auto-completion, should be able to handle the annotation)

Additionally, I would like the code to be readable/intuitive (not necessarily the super classes, though), generally robust and 'bug-free'. I accept the limitation that my decorators need to be the most 'outer' decorator for the automatic collection to take place.

From my point of view, overcoming function/method transformation while still exposing an arbitrary object type (not a function type -- thinking of this, maybe subclassing a FunctionType might be another idea?) is the hardest challenge.

What do you think of the following three solutions? Is there something I did miss?

Code

class MethodDecoratorWithIfInCall(object):
    def __init__(self):
        self._func = None

    def __call__(self, *args, **kwargs):
        if self._func is None:
            assert len(args) == 1 and len(kwargs) == 0
            self._func = args[0]
            return self
        else:
            return self._func(*args, **kwargs)

    def __get__(self, *args, **kwargs):
        # update func reference to method object
        self._func = self._func.__get__(*args, **kwargs)
        return self

class MacroWithIfInCall(MethodDecoratorWithIfInCall):
    def __init__(self, name):
        super(MacroWithIfInCall, self).__init__()
        self.name = name

class MethodDecoratorWithExplicitDecorate(object):
    def __init__(self, *args, **kwargs):
        # wildcard parameters to satisfy PyCharm
        self._func = None

    def __call__(self, *args, **kwargs):
        return self._func(*args, **kwargs)

    def __get__(self, *args, **kwargs):
        # update func reference to method object
        self._func = self._func.__get__(*args, **kwargs)
        return self

    def _decorate(self):
        def _set_func(func):
            self._func = func
            return self
        return _set_func

    @classmethod
    def decorate(cls, *args, **kwargs):
        obj = cls(*args, **kwargs)
        return obj._decorate()

class MacroWithExplicitDecorate(MethodDecoratorWithExplicitDecorate):
    def __init__(self, name):
        super(MacroWithExplicitDecorate, self).__init__()
        self.name = name

class MacroWithoutSuperclass(object):
    def __init__(self, func, name):
        self.func = func
        self.name = name

    def __get__(self, *args, **kwargs):
        # update func reference to method object
        self.func = self.func.__get__(*args, **kwargs)
        return self

    def __call__(self, *args, **kwargs):
        return self.func(*args, **kwargs)

    @staticmethod
    def decorate(name):
        return lambda func: MacroWithoutSuperclass(func, name)

class Shell:
    def __init__(self):
        macros = [macro for macro in map(self.__getattribute__, dir(self))
                  if isinstance(macro, (MacroWithIfInCall, MacroWithExplicitDecorate, MacroWithoutSuperclass))]

        for macro in macros:
            print(macro.name, macro())

    @MacroWithIfInCall(name="macro-with-if-in-call")
    def macro_add_1(self):
        return "called"

    @MacroWithExplicitDecorate.decorate(name="macro-with-explicit-decorate")
    def macro_add_2(self):
        return "called"

    @MacroWithoutSuperclass.decorate(name="macro-without-superclass")
    def macro_add_3(self):
        return "called"

if __name__ == '__main__':
    shell = Shell()

Output

macro-with-if-in-call called
macro-with-explicit-decorate called
macro-without-superclass called

From your three approaches, I think MacroWithoutSuperclass is probably the cleanest. I wanted to just comment a bit, but it turned out I had too much to say... Thus, here a few remarks followed by a maybe more intuitive solution as an inspiration (it's probably not perfect).

None of your methods supports arbitrary meta data, in case that is a requirement, but only name. In the following I assume that you in fact want to add arbitrary data.

You are assigning meta-data by name, so that e.g. macro.name is accessible. Is that a requirement? Or would macro.meta['name'] be enough? Below I will present both options, but it is not really clear from you question, only implicitly from your code.

All of your three methods seem in general very complicated.
For example

if self._func is None:
    assert len(args) == 1 and len(kwargs) == 0
    self._func = args[0]
    return self
else:
    return self._func(*args, **kwargs)

is quite complicated and should have some more explanations.

The inheritance

class MacroWithIfInCall(MethodDecoratorWithIfInCall):
    def __init__(self, name):
        super(MacroWithIfInCall, self).__init__()
        self.name = name

is not needed (the same applies to MacroWithExplicitDecorate), instead you can assign the name in MethodDecoratorWithIfInCall's init function – exactly as you did in the MacroWithoutSuperclass. Which is basically why I think a combination of the MacroWithIfInCall (the __call__ function) and the MacroWithoutSuperclass would be the best solution using your approach.

I don't understand why you need wildcard parameters (# wildcard parameters to satisfy PyCharm) for one class, but not for the others, you shouldn't need them in either case.

Anyways, I think you can fare much better with an even simpler decorator, as I will explain below.

I will try to go along your list and provide some comments to answer your requirements.

  it is 'easy' to retrieve all such methods for a given class instance

This is sufficiently easy using inspect.getmembers:

import inspect

types = (MacroWithIfInCall, MacroWithExplicitDecorate, MacroWithoutSuperclass)
macros = inspect.getmembers(Shell, lambda m: isinstance(m, types))

Note however, that macros is now a list containing tuples of the form (name, function). Dependending on your use case you might have to use any of the following lines:

names = [macro[0] for macro in macros]  # Get only the names
funcs = [macro[1] for macro in macros]  # Get only the functions
names, funcs = zip(*macros)  # Get names and functions in separate lists

  methods can still be called 'in a normal way', e.g. obj.method()

This is fine, and will be the case for the classic decorator which follows this form:

import functools

def decorator(func):

    @functools.wraps(func)  # Handles the docs and names properly
    def wrapper(*args, **kwargs):
        # Do something before the call

        result = func(*args, **kwargs)

        # Do something after the call

        return result

    return wrapper

In fact, for your problem slightly less complex, as you don't need to perform any tasks on execution but only when declaring the function, so in fact you can return the original function:

def meta(func):
    func.meta = 'meta data here'  # Assign constant metadata
    return func

  meta-data is accessible from the decorated method, e.g. obj.method.data

As mentioned above, I am not sure whether you mean that all meta data is contained in data or that data is just one part, and name, age, abc would be others.

To have this, you could now just assign these values by extending the decorator meta(func) I presented above. Remember that in Python, functions are just objects (search for "User-defined functions"):

  Function objects also support getting and setting arbitrary attributes, which can be used, for example, to attach metadata to functions. Regular attribute dot-notation is used to get and set such attributes.

This means, that instead of having your complex wrapper classes storing the meta data and the function you can just store the meta data at the function itself.

So the decorator meta needs to have a way to specify the meta data. The common way is building another function around it, which eventually returns the decorator.

def meta(**meta_data):
    """Attaches meta information to a method."""

    def _attach_meta(func):
        func.meta = meta_data
        return func

    return _attach_meta

To support the dot-notation (and not the meta-dictionary) the line func.meta = meta_data could to be changed to

for k, v in meta_data.items():
    setattr(func, k, v)
func.has_meta = True  # flag to use for inspect.getmembers' predicate

I would use the dictionary version unless you have a very strict set of meta attributes, as then accessing data using function.meta.get(KEY, DEFAULT_VALUE) becomes fairly useful.

  IDEs (PyCharm in particular) do not produce any warnings or errors (and, if possible, IDE support, e.g. auto-completion, should be able to handle the annotation)

I can't say anything about this, but I think it should work as you expect... I use YCM and am too lazy to install PyCharm now.

  Additionally, I would like the code to be readable/intuitive (not necessarily the super classes, though)

This should be sufficiently satisfied with the presented solution, although it is also lacking some (in-code) documentation.

  generally robust and 'bug-free'.

There can always be name clashes etc., and my solution does not have the strong typing your solution has. But for bug-free-ness, it is possible to add some tests, making it less buggy.

  I accept the limitation that my decorators need to be the most 'outer' decorator for the automatic collection to take place.

This should still be the case, I fear. But I haven't tried to solve this.

Full working example

import inspect

def meta(**meta_data):
    """Attaches meta information to a method."""

    def _attach_meta(func):
        func.meta = meta_data
        return func

    return _attach_meta

def has_meta(func):
    """Predicate to check for meta data."""
    return hasattr(func, 'meta')

class SomeAnnotated:
    @meta(a=1, b=2)
    def __call__(self):
        """The call method."""
        return 'Hello'

    @meta(x=3, y=4)
    def annotated(self, abc):
        """The annotated method."""
        return abc

    def not_annotated(self):
        """The not annotated method."""
        return 'Not annotated'

print('Meta information is available for:')
for name, func in inspect.getmembers(SomeAnnotated, has_meta):
    print(f'{func.__qualname__}: {func.meta} -- {func.__doc__}')

Expected output:

Meta information is available for:
SomeAnnotated.__call__: {'a': 1, 'b': 2} -- The call method.
SomeAnnotated.annotated: {'x': 3, 'y': 4} -- The annotated method.

Edit: As per comment here is a solution more closely related to your idea around a non-data descriptor (i.e. using __get__):

import inspect
import functools

class MetaAnnotated(functools.partial):
    pass

class MetaAnnotator:
    def __init__(self, func, **meta_data):
        self.func = func
        self.meta = meta_data

    def __get__(self, obj, type=None):
        annotated_func = functools.wraps(self.func)(MetaAnnotated(self.func, obj))
        annotated_func.meta = self.meta
        return annotated_func

def meta(**meta_data):
    """Attaches meta information to a method."""

    def _attach_meta(func):
        return MetaAnnotator(func, **meta_data)

    return _attach_meta

def has_meta(func):
    """Predicate to check for meta data."""
    return isinstance(func, MetaAnnotated)

The above solution uses functools for two purposes: first, to retain the documentation etc (wraps). Second, and more importantly, to create a partial function which is called with our instance as self. In a step-by-step solution we would do this as follows (each block represents a change to understand the code, not a sequence of commands):

# annotated_func behaves the same as func, but will
# always use "obj" as its first argument.
annotated_func = functools.partial(func, obj)

# We can inherit from functools.partial to have a tagging class:
class MetaAnnotated(functools.partial): pass
annotated_func = MetaAnnotated(func, obj)

# Keep the doc string from func:
class MetaAnnotated(functools.partial): pass
annotated_func = functools.wraps(func)(MetaAnnotated(func, obj))

This way, no __call__ is needed, neither is a staticmethod: as soon as the function we decorated using MetaAnnotated is searched for on the original object, the properly wrapped function is returned (instead of the MetaAnnotator!).

This behaves almost the same as the solution without typing, but it does not have the func.__qualname__ needed in my test prints, so we need to change this. I also added a "proof" that it's properly callable:

print('Meta information is available for:')
for name, func in inspect.getmembers(SomeAnnotated, has_meta):
    print(f'{name}: {func.meta} -- {func.__doc__}')

instance = SomeAnnotated()
print(instance())
print(instance.annotated(123))

I think this is a nice synthesis between your original ideas about using the descriptors and my ideas of having a very plain and straight forward decorator. All the "magic" happens in one place (the MetaAnnotator), the MetaAnnotated class transparently tags functions, and there is a very neat API: a user just needs to call @meta and can forget about everything else.


Creating an enumerable by calling a function returning enumerables The purpose of the application is very simple.
PlantList is a object containing a list of PlantConfiguration.

Each PlantConfiguration has two schedules: HourlySchedule and DailySchedule, as well as a TimeZone.

The program reads a PlantList object, stored as a JSON file in the settings, and returns an enumerable of Schedule, containing all the HourlySchedules, and the DailySchedule if it is midnight in a plant's timezone.

In addition to general comments about the code, I am looking for a way to improve the loop. Indeed, using the Concat feature to smash together all schedules as they come feels sub-optimal, and I figure this is a good occasion to learn about a new pattern.

public class Program
    {
        private readonly static log4net.ILog log = LogHelper.GetLogger();

        public static void Main()
        {
            var container = new UnityContainer();
            container.RegisterType<IPlantRepository, JsonPlantRepository>();

            IPlantRepository serverRepository = container.Resolve<IPlantRepository>();
            var plantList = serverRepository.GetPlants(MySettings.Default.Servers);

            if (MySettings.Default.Disabled == true)
            {
                log.Fatal("Script disabled");
                return;
            }

            UpdateValues(plantList);
        }

        private static void UpdateValues(PlantList plantList)
        {
            DateTime utcTopHourDate = GetUtcTopHourDate();
            IEnumerable<KpiSchedule> schedulesToUpdate = new List<KpiSchedule>();
            foreach(PlantConfiguration plant in plantList.Plants)
            {
                schedulesToUpdate = schedulesToUpdate.Concat(GetScheduleList(utcTopHourDate,plant));
            }
            //... Do stuff with the schedule list
        }

        private static IEnumerable<KpiSchedule> GetScheduleList(DateTime utcTime, PlantConfiguration plant)
        {
            var localDate = TimeZoneInfo.ConvertTimeFromUtc(utcTime, server.TimeZone);
            List<KpiSchedule> returnList = new List<KpiSchedule>
            {
                plant.HourlySchedule
            };

            if (localDate.Hour == 0)
            {
                returnList.Add(plant.DailySchedule);
            }

            return returnList;
        }

        private static DateTime GetUtcTopHourDate(int ForceHour = -1)
        {
            return new DateTime(
                DateTime.UtcNow.Year,
                DateTime.UtcNow.Month,
                DateTime.UtcNow.Day,
                DateTime.UtcNow.Hour,
                0,
                0,
                DateTimeKind.Utc
            );
        }
    }
}

There is no need to create lists and add items to the lists when working with enumerables; yield is your new best friend.

As written, GetScheduleList() is messy to unit test so we can push the condition up a level and then use yield

private static IEnumerable<KpiSchedule> GetScheduleList(PlantConfiguration plant, bool includeDaily)
{
    yield return plant.HourlySchedule;
    if (includeDaily)
    {
        yield return plant.DailySchedule;
    }
}

To aggregrate the schedules for each plant we can use SelectMany().

private static IEnumerable<KpiSchedule> GetAllSchedules(IEnumerable<PlantConfiguration> allPlants, bool includeDaily)
{
    return allPlants.SelectMany(p => GetScheduleList(p, includeDaily));
}

We can now revise UpdateValues() to something like

private static void UpdateValues(PlantList plantList)
{
    DateTime utcTopHourDate = GetUtcTopHourDate();
    var includeDaily = CheckIncludeDaily(utcTopHourDate);

    var schedulesToUpdate = GetAllSchedules(plantList.Plants, includeDaily);
    //... Do stuff with the schedule list

}

or even

private static void UpdateValues(PlantList plantList)
{
    DateTime utcTopHourDate = GetUtcTopHourDate();
    var includeDaily = CheckIncludeDaily(utcTopHourDate);

    var schedulesToUpdate = plantList.Plants.SelectMany(p => GetScheduleList(p, includeDaily));
    //... Do stuff with the schedule list
}


Moving files to an output directory, with instructions on missing files I have some files such as .\image_1_01.bmp that will be created elsewhere. I am then moving these files to an output directory. If any are missing I inform the user that it's missing and instruct them to check the logs.

Given how similar all the code is, can I make this code shorter?

if not os.path.exists(".\image_1_01.bmp"):
    print('image_1_01.bmp not generated\n' 'for more info check the log file' )
    return 1
else:
    shutil.move(".\image_1_01.bmp", output_directory)

if not os.path.exists(".\image_1_01.raw"):
    print('image_1_01.raw not generated\n' 'for mor info check the log file' )
    return 1
else:
    shutil.move(".\image_1_01.raw", output_directory)

if not os.path.exists(".\image_1_01_hist.csv"):
    print('image_1_01_hist.csv not generated\n' 'for mor info check the log file' )
    return 1
else:
    shutil.move(".\image_1_01_hist.csv", output_directory)

if not os.path.exists(".\image_1_02.bmp"):
    print('image_1_02.bmp not generated\n' 'for more info check the log file' )
    return 1
else:
    shutil.move(".\image_1_02.bmp", output_directory)

if not os.path.exists(".\image_1_02.raw"):
    print('image_1_02.raw not generated\n' 'for mor info check the log file' )
    return 1
else:
    shutil.move(".\image_1_02.raw", output_directory)

if not os.path.exists(".\image_1_02_hist.csv"):
    print('image_1_02_hist.csv not generated\n' 'for mor info check the log file' )
    return 1
else:
    shutil.move(".\image_1_02_hist.csv", output_directory)

if not os.path.exists(".\scan_stats.csv"):
    print('scan_stats.csv not generated\n' 'for mor info check the log file' )
    return 1
else:
    shutil.move(".\scan_stats.csv", output_directory)

if retcode != 0:
    sys.exit(1)
else:
    print("Test case is not suitable for your configuration")
    sys.exit(2)

if __name__ == '__main__':
    exit( generic() )`

You are right in that most of your code is very similar, except for the file that you are checking. Since only a small part of the code changes, and the same actions are made at every step, this asks for a loop that iterates over the different files that you want to check:

paths_to_check = [".\image_1_01.bmp", ".\image_1_01.raw", ".\image_1_01_hist.csv", ".\image_1_02.bmp", ".\image_1_02.raw", ".\image_1_02_hist.csv", ".\scan_stats.csv"]

for p in paths_to_check:
    if not os.path.exists(p):
        print(p + 'not generated\n' 'for mor info check the log file' )
        return 1
    else:
        shutil.move(p, output_directory)