Simple console directive, revised using provider I recently created my first directive based on jQuery plugin and I asked for review here: Angular console like window - first directive

I've edited my directive so now instead of $rootScope it uses provider.
Below is my code:

(function() {
  'use strict';

  angular
    .module('misiu.providers', [])
    .provider('simpleConsole', function() {

      var events = [];
      var separator = '::'; //default

      //public methods
      this.$get = function() {
        return {
          addLog: function(data) {
            addEvent('log', 'Log' + separator + data);
          },
          addError: function(data) {
            addEvent('error', 'Error' + separator + data);
          },
          clear: function() {
            events.length = 0;
          },
          events: events
        };
      };

      //private method
      var addEvent = function(type, data) {
        if (events.length > 0 && events[0].type === type && events[0].data === data) {
          events[0].count++;
        } else {
          events.unshift({
            type: type,
            data: data,
            count: 1
          });
        }
      };

      //this is available only in config
      this.setSeparator = function(value) {
        separator = value;
      };
    });

  angular
    .module('misiu.directives', ['misiu.providers'])
    .directive('simpleConsole', ['simpleConsole', function(simpleConsole) {
      return {
        restrict: 'E',
        template: [
          '
',
          '
',
          '{{event.data}}1" class="count">{{event.count}}',
          '
',
          '
'
        ].join(''),
        link: function($scope, element) {
          $scope.events = simpleConsole.events;
        }
      };
    }]);
})();

and Plunker to see results.

I'd like to ask for review. Especially that provider part, because I'm not sure if all functions in it are correctly declared - private methods should be private, setSeparator should be available only in config section and addLog,addError should be the only two public methods available inside controllers and directives.

Any other general tips are welcome.

Just quick comments, as so far your code looks good

ng-class can receive an ng-model variable, the class being displayed matching the ng-model content, so you can directly state

 ng-class="{event.type}"

Maybe you want to extract the logic to check if the event exists to some functions with proper naming

    if (events.length > 0 && isFirstEvent(events, type, data)) {

Mine is probably not the best example but it will help to have an idea what's happening

Plus extracting the html of the directive to a template will help readability and maintainability

Hope it helps!


Convert text file to html? I have a script that converts a text file to html. The text file is an output of a program (needle). The function takes the text file as an input. I am aware the script is awful. I use this function in Django. I would like to improve it. Can you help me with it?. Here is the result file from the script.

def write_html(result):
    sequence_1 = ''
    sequence_2 = ''
    identity = ''
    similarity = ''
    gaps = ''
    html_string = ''
    with open("/Users/catuf/Desktop/needle_align.txt", "w") as e:
        e.write('{% extends "database/base_table.html" %}' + "\n")
        e.write('\n')
        e.write("{% block content %}"+"\n")
        table1 = '''
        <!DOCTYPE html>

            Query Sequence
            Matched Sequence
            Identity
            Similarity
            Gaps

        '''

        table2 = """

        """
        for line in result.splitlines():
            #s = html.unescape(lines)
            #print(line)
            if line.startswith('# 1:'):
                line = line.split()
                sequence_1 = line[2]
            elif line.startswith('# 2:'):
                line = line.split()
                #print(line)
                sequence_2 = line[2]

            elif line.startswith('# Identity:'):
                identity = re.search(r"\d{1,3}\.\d*\%", line)
                identity = identity.group()
                identity = identity.replace('%', '')
                #identity = float(identity)* 100
                #identity = str(identity)

            elif line.startswith('# Similarity:'):
                similarity = re.search(r"\d{1,3}\.\d*\%", line)
                similarity = similarity.group()
                similarity = similarity.replace('%', '')

            elif line.startswith('# Gaps:'):
                line = line.split()
                gaps = line[2]

            elif line.startswith('#'):
                pass
            else:
                s = "
" + line + "
"
                html_string += s

        e.write(table1)
        e.write("" + sequence_1 + '	' + '\n')
        e.write("" + sequence_2 + '	'+ '\n')
        e.write(""+ identity + "%" + '	'+ '\n')
        e.write(""+ similarity+ "%" +'	'+ '\n')
        e.write(""+ gaps+ '	'+ '\n')
        e.write(table2)
        e.write(html_string)
        e.write("" + "\n")
        e.write("{% endblock content %}"+"\n")

Functions

Right now, write_html manages both parsing and writing to a file. Separate this function into two functions, perhaps parse_data and write_data.

String Formatting

This

s = "
" + line + "
"
e.write("" + sequence_1 + '	' + '\n')

can be written like this

s = f"
{line}>
"
e.write(f"{sequence_1}	\n")

The f"" allows you to directly implement variables into your strings.

Simplification

This

elif line.startswith('# Gaps:'):
    line = line.split()
    gaps = line[2]

can be written like this

elif line.startswith('# Gaps:'):
    gaps = line.split()[2]

The same with these two:

if line.startswith('# 1:'):
    sequence_1 = line.split()[2]
elif line.startswith('# 2:'):
    sequence_2 = line.split()[2]

Variable Assignments

This

sequence_1 = ''
sequence_2 = ''
identity = ''
similarity = ''
gaps = ''
html_string = ''

can be written like this

sequence_1, sequence_2, identity, gaps, similarity, html_string = '', '', '', '', '', ''

Type Hints

You can use type hints to make it clear what is being accepted as parameters, and what is being returned by the function.

From this

def write_html(result):

to this

def write_html(result: str) -> None:

Docstrings

You should include a docstring at the beginning of every function, method, class, and module you write. This will allow documentation to identify what these are supposed to do.

def write_html(result: str) -> None:
    """
    (Description about this method here)
    """
    ... code here ...


CodeEval Challenge: Remove specified characters from a string Question

  Write a program which removes specific characters from a string.
  The first argument is a path to a file. The file contains the source
  strings and the characters that need to be scrubbed. Each source
  string and characters you need to scrub are delimited by comma.

  Input sample:

  how are you, abc
  hello world, def

  Output sample:

  how re you
  hllo worl

I have used regex to eliminate the characters.

public class Main{

    public static void main(String[] args) throws IOException {
        File file = new File(args[0]);
        BufferedReader br = new BufferedReader(new FileReader(file));
            String s;
            while ((s = br.readLine()) != null) {
                s = s.trim();
                String arr[]=s.split(",\\s");
                String pat="([^"+arr[1]+"])";
                Pattern p=Pattern.compile(pat);
                Matcher m=p.matcher(arr[0]);
                while(m.find())
                {
                    System.out.print(m.group(0));
                }
                System.out.println();
            }
            br.close();
        }

    }

The above code passes all the test cases.
Is this the most optimized code or can it be improved further?

You can extract out the requirement into its own method:

private static String removeCharacters(String input, String excludedLetters) {
    // ...
}

And since you have to split from a single String first, you can use a wrapper method in addition to the one above:

private static String removeCharacters(String line) {
    String[] parts = // ...
    return removeCharacters(parts[0], parts[1]);
}

If you are on Java 8, you can use a combination of try-with-resources and Files.lines() to Streamline the processing (pun intended):

try (Stream<String> lines = Files.lines(Paths.get(/* ... */))) {
    lines.map(Main::removeCharacters).forEach(System.out::println);
}


Extract equally from X queues to one queue with Y elements I need to implement a system of queueing that extracts elements equally from x queues to one single queue with y positions.

Let's say I have 5 queues, each with a number of elements and I need to extract elements from each of the 5 queues to build a new queue containing the extracted elements until the new queue is full.

For example:

5 queues to a queue with 100 positions. I need to extract 20 elements from each queue. Ok, but if some of the 5 input queues have less than 20 elements, the resulting queue will have some empty elements.
51 queues to a queue with 100 positions. I need to extract 1 element from each queue. The input queues have for sure at least 1 element. But the resulting queue will have 49 empty positions. So I'll need to iterate the input queues to extract 1 element from each one, until I reach the max 100 positions of the resulting queue.

I need this to implement a system of sending queues to send massive SMSs form web for our customers. In those example, the 51 queues are 51 different SMS, inside each of these queues there are all the mobile numbers to send the sms to.

I can send only $this->QueueMax sms every 2 minutes, so I have a script that takes a number of mobile phones from all sms to send and send all sms in parallel.

I've implemented this PHP script to manage the final queue: at the end, $smsToSend will tell me how many mobile numbers to extract for each SMS in the 'limit' field.

Is that alright? Will the script work fine?

But most of all, is there a less-complex method of doing this?

    $query = 'SELECT sms_id, COUNT(DISTINCT mobile) AS tot_mobile ';
    $query.= 'FROM sms_queue ';
    $query.= 'GROUP BY sms_id ';
    $this->Dbh->query($query);

    $smsToSend = array();
    while($this->Dbh->next()) {
        $smsToSend[$this->Dbh->Record['sms_id']] = array(
            'tot' => $this->Dbh->Record['tot_mobile'],
            'limit' => 0,
        );
    }

    /* The queue to send is empty */
    if( empty($smsToSend) ) {
        $this->IsSending = false;
        return false;
    }

    /* Total phone numbers to take from each sms queue */
            $totSmsToSend = count($smsToSend);

    /* There are more sms to send than empty positions in the send queue; send only the firsts "QueueMax" sms */
    if( $totSmsToSend > $this->QueueMax ) {
        $smsToSend = array_slice($smsToSend, 0, $this->QueueMax, true);
        $totSmsToSend = $this->QueueMax;
    }

    $totMobilesFromEachSms = (int)floor($this->QueueMax / $totSmsToSend);
    foreach( $smsToSend as $smsID => $info ) {
        $smsToSend[$smsID]['limit'] = ($totMobilesFromEachSms >= $info['tot']) ? $info['tot'] : $totMobilesFromEachSms;
    }

           /* If there are some empty positions, iterate $smsToSend and take 1 mobile number foreach sms 'till covering all empty positions */
    if( ($totMobilesFromEachSms * $totSmsToSend) < $this->QueueMax ) {
        $i = $totSmsToSend + 1;
        while(true) {
            $check = false;

            foreach($smsToSend as $smsID => $info) {
                if( $info['limit'] >= $info['tot'] ) {
                    continue;
                }

                $check = true;

                $smsToSend[$smsID]['limit']++;
                $i++;

                if( $i >= $this->QueueMax ) {
                    break 2;
                }
            }

            /* Each sms has all its phone numbers correctly placed inside the send queue*/
                            if( !$check ) {
                break;
            }
        }
    }

First off, you're code looks nice, but it can be improved in places:

I don't see why:

        $totSmsToSend = count($smsToSend);

Is indented so far, you should fix that. There's quite a few elements that have been over indented.

Your constant use of the word Sms like so, seems a bit silly, as it as an acronym, you should stick to SMS or sms but don't mix cases.
$tot, You shouldn't leave out two characters, just for the purpose of slimming when it makes it much less readable.
Your code seems really forced, so I think it could be improved in the method it goes about doing.

Your solution really just sounds like basic division:

Y / X, rounded down to a whole number
For leftover positions in Y queue, transfer X(1), X(2), ... X(X) and go through until you fill up the queue.


Check whether string of braces, brackets, and parentheses is balanced The Task

is taken from codewars:

  Write a function that takes a string of braces, and determines if the
  order of the braces is valid. It should return true if the string is
  valid, and false if it's invalid.

  All input strings will be nonempty, and will only consist of
  parentheses, brackets and curly braces: ()[]{}.

  What is considered Valid? A string of braces is considered valid if
  all braces are matched with the correct brace.

  Examples

  (){}[]   =>  True
  ([{}])   =>  True
  (}       =>  False
  [(])     =>  False
  [({})](] =>  False

My solution

const areBracesBalanced = brc => {
  const brace = Object.freeze({
    "(": [],
    "[": [],
    "{": [],
  });

  const removeBrace = b => brace[b].splice(-1, 1);
  const getLastBraceIndex = b => brace[b][brace[b].length - 1]
  const braceExists = b => !brace[b].length;
  const isBraceBeforeClosed = (before, current) => braceExists(before) || getLastBraceIndex(before) < getLastBraceIndex(current);

  const braceIsBalanced = (b, i) => {
    switch (b) {
      case "(":
      case "[":
      case "{":
        return brace[b].push(i);
      case ")":
        return isBraceBeforeClosed("[", "(") &&
               isBraceBeforeClosed("{", "(") &&
               removeBrace("(");
      case "]":
        return isBraceBeforeClosed("(", "[") &&
               isBraceBeforeClosed("{", "[") &&
               removeBrace("[");
      case "}":
        return isBraceBeforeClosed("(", "{") &&
               isBraceBeforeClosed("[", "{") &&
               removeBrace("{");
      default:
        // be a good code...
    }
  };

  return[...brc]
    .every(braceIsBalanced) && !Object.values(brace).flat().length;
}

Your solution is complicated due to the use of three separate stacks:

For each closing character, you have to cross-validate the two other types of delimiters.  (If you had to handle a fourth type of matching characters, such as < and >, it would be even messier.)
At the end, you need to write !Object.values(brace).flat().length to verify that all of the stacks have been cleared.

I also think that the use of Object.freeze() is overkill.

Your removeBrace() helper can use Array.prototype.pop() instead.

Suggested solution

Keep all of the state in one combined stack.

const areBracesBalanced = brs => {
  let expectCloseStack = [];
  return [...brs].every(b => {
    switch (b) {
      case "(": return expectCloseStack.push(")");
      case "[": return expectCloseStack.push("]");
      case "{": return expectCloseStack.push("}");

      case ")":
      case "]":
      case "}":
        return expectCloseStack.pop() === b;
    }
  }) && !expectCloseStack.length;
};

console.log(areBracesBalanced("[({})](]"));


Unity3D like component system I am currently working on my own little game engine and thought about implementing a simplified version of Unitys component system where you can attach objects of derived Components to an GameObject. Later you can request specific Components to use their functionality. Below is a stripped down (but still working) version of what I think about implementing.

#include <iostream>
#include <vector>

class GameObject;

class Component
{
private:
    GameObject * parent;

public:
    Component(GameObject * parent = nullptr)
    {
        this->parent = parent;
    }

    virtual ~Component() { parent = nullptr; }

    GameObject* GetParent() { return parent; }

    void SetParent(GameObject* parent) { this->parent = parent; }

    virtual std::string What() { return "Basic Component"; }
};

class SoundSource : public Component
{
private:
    // some sound specific members

public:
    SoundSource(GameObject * parent = nullptr) : Component(parent) {}

    virtual ~SoundSource() { }

    std::string What() { return "SoundSource Component"; }
};

class GameObject
{
private:
    std::vector<Component*> m_Components;

public:
    GameObject() :
        m_Components()
    {}

    ~GameObject()
    {
        for (size_t i = 0; i < m_Components.size(); ++i)
            if (m_Components[i]->GetParent() == this) delete m_Components[i];
        m_Components.clear();
    }

    void AddComponent(Component* comp)
    {
        if (comp->GetParent() == nullptr)
        {
            comp->SetParent(this);
        }
        m_Components.push_back(comp);
    }

    template <class component_type>
    component_type* GetComponent()
    {
        for (auto comp : m_Components)
        {
            component_type* c =  dynamic_cast<component_type*>(comp);
            if (c) return c;
        }
        return nullptr;
    }
};

int main()
{
    GameObject go;
    go.AddComponent(new SoundSource());
    go.AddComponent(new Component());

    SoundSource *s = go.GetComponent<SoundSource>();
    if (s) std::cout << s->What() << std::endl;
}

My initial plan for GetComponent<>() was to use the name of typeid to compare the component types, however since all components need to derive from the same base I think that using a dynamic_cast is the better option in this case. However I have never worked with dynamic casting before and I am not sure whether the way I use it is safe, so please tell me is it?

Also if you see any points for improvements I would appreciate your opinions.

I'm not a C++ programmer, but there are a few things that I notice that I feel are important in any language.

Be Consistent:

While you'll find many people that are passionate about different naming conventions and coding styles, one thing that they'll all agree on is to at least be consistent in your choice. There are a few inconsistencies in your code:

You declare a private member of your Component class named parent, and go on to introduce Hungarian notation in your GameObject class with a member named m_Components.
You declare a pointer with a space on either side of the asterisk: Component * parent; and go on with this style in your constructors, but you also connect the asterisk to the type in some other cases: GameObject* GetParent() and SetParent(GameObject* parent).
In your GameObject destructor, you have a for with a single-line body and choose to omit curly braces, but in it's AddComponent member function, you have an if with a single-line body but keep the curly-braces. If you're going to omit curly-braces in the former, you may as well omit them in the latter.
If you insist on omitting the curly-braces, the former could also be written in a more readable manner:

for (size_t i = 0; i < m_Components.size(); ++i)
    if (m_Components[i]->GetParent() == this)
        delete m_Components[i];

Also:

virtual std::string What() { return "Basic Component"; } "What" is not a very descriptive identifier. Consider replacing it with something along the lines of "GetName()"
The dynamic cast:

component_type* c = dynamic_cast<component_type*>(comp);
    if (c) return c;

I've often seen dynamic_cast in this form, which I find more readable:

if (component_type* c = dynamic_cast<component_type*>(comp))
    return c;

You use std::vector for your collection of Components. Any time where you loop through the elements of an std::vector in order to find a single element, you should consider using std::map. Using data structures like these to locate a Value object using a Key object will be much faster than looping through a vector, because it will take the same amount of time regardless of how many items are in the collection. This approach would result in a much faster GetComponent() method

I'm not sure if this is the approach used to implement GetComponent in Unity, but I know that it's advised against using GetComponent in the Update method in Unity because of how slow it is. If you used an std::map to implement your GetComponent method, it would be a marginal difference compared to doing the lookup in advance and storing the result (which is currently the recommended approach in Unity).

That's the only real feedback I have on your object-oriented design. For the most part, it seems like a pretty accurate C++ representation of Unity's component system.


Dynamic programming with Fibonacci I have written the following code using a dynamic programming technique.  Can I use ArrayList here?  Please let me know if I can improve this code.

package com.java.fib;

import java.math.BigInteger;
import java.util.HashMap;

public class Fibonaci {

    public static void main(String[] args) {
        System.out.println(" number ");
        long startTime = System.currentTimeMillis();
        HashMap<Integer, BigInteger> memoized = new HashMap<Integer, BigInteger>();
         fibonanci(220, memoized);
        System.out.println(" Total Time "
                + (System.currentTimeMillis() - startTime));
    }

    private static BigInteger fibonanci(int n, HashMap<Integer, BigInteger> memoized) {
        if (memoized.containsKey(n)) {
            return memoized.get(n);
        }
        if (n <= 0) {
             return BigInteger.ZERO;
        }
        if (n <= 2) {
            return BigInteger.ONE;
        } else {
            BigInteger  febonani = fibonanci(n - 1, memoized).add (fibonanci(n - 2, memoized));
            System.out.println(" febonani " + febonani);
                memoized.put(n, febonani);
            return febonani;
        }
    }
}

A few things:

Naming:

It's neither Fibonaci, nor febonani, nor fibonanci it's fibonacci. Please get your names to reflect what you're actually talking about and not some disfigured mutation of it :(

memoized OTOH is a relatively nice name, I'd probably prefer memoizedFibonacciNumbers, but that's a thing of preference

Calculating:

quoting wikipedia:

  By definition, the first two numbers in the Fibonacci sequence are \$1\$ and \$1\$, or \$0\$ and \$1\$, depending on the chosen starting point of the sequence, and each subsequent number is the sum of the previous two.

You on the other hand say:

  The value of Fibonacci for \$n <= 0\$ is \$0\$, and for \$n <= 2\$ is \$1\$, and each subsequent number is the sum of the previous two.

Wikipedia also mentions "negative Fibonacci numbers". Your definition does not match the actual Fibonacci sequence definition :(

instead your fibonacci method should look like this, if you exactly follow the rules for only positive fibonacci numbers:

private static BigInteger fibonacci(Integer n, HashMap<Integer, BigInteger> memoized) {
    if (n < 0) {
       throw new IllegalArgumentException("We assume the positive Fibonacci sequence only");
    }
    if (memoized.containsKey(n)) {
        return memoized.get(n);
    }
    if (n == 0) {
        memoized.put(n, BigInteger.ZERO);
        return memoized.get(n);
    }
    if (n == 1) {
        memoized.put(n, BigInteger.ONE);
        return memoized.get(n);
    }
    BigInteger fibonacci = fibonacci(n-1, memoized).add(fibonacci(n-2, memoized));
    memoized.put(n, finbonacci);
    return fibonacci;
}

Printing is slow:

I see you benchmarking your code. Please keep in mind, that writing something to the Console is quite slow. Your measured execution time is not matching the calculation time.

You should remove the System.out.println() in your fibonacci method.

Conditionals

Also you maybe have already realized, I removed the else in the fibonacci method.

This is because if your code reaches that area, all other branches have returned early, and the else is quite useless.


Mastermind game in Python I am a C#/.Net developer and have decided it is time to branch out and really learn Python. I started with a simple implementation of a Mastermind-esque game. Any comments about the code would be greatly appreciated. I already know the gameplay isn't perfect, but it was just a quick attempt. I also am aware that there isn't much user input checking, but again, quick attempt.

import random

class Game(object):
    def run(self):
        guesses = []
        game_states = []
        length = int(input("Choose length of code to break: "))
        code = [random.randint(0, 9) for i in range(length)]

        def take_guess():
            correct_guess_length = False
            while not correct_guess_length:
                guess = input("Guess a number of length {0}: ".format(length))
                if len(guess) != length:
                    print("Guesses must be a number of length {0}.".format(length))
                else:
                    correct_guess_length = True
            return guess

        def check_guess(guess):
            return all([x == y for x, y in zip(guess, code)])

        def evaluate_guess(guess):
            new_state = []
            for pos, number in enumerate(guess):
                if code[pos] == number:
                    new_state.append(str(number))
                elif number in code:
                    new_state.append("O")
                else:
                    new_state.append("X")
            game_states.append(''.join([c for c in new_state]))

        def print_game_state():
            print()
            for guess, state in zip(guesses, game_states):
                print("{0}\t{1}".format(guess, state))

        correct = False
        while not correct:
            guess = take_guess()
            guesses.append(guess)
            guess_as_ints = [int(c) for c in guess]
            if not check_guess(guess_as_ints):
                evaluate_guess(guess_as_ints)
                print_game_state()
            else:
                print("You guessed the code was {0}! Congratulations!".format(''.join([str(i) for i in code])))
                correct = True

if __name__ == '__main__':
    game = Game()
    game.run()

Organization
As Simon points out, you shouldn't put the list of functions inside of run. It doesn't make much sense. Move them out into the class.
Use self
self in python is equivalent to this in C# and many other languages.
This:
    guesses = []
    game_states = []
    length = int(input("Choose length of code to break: "))
    code = [random.randint(0, 9) for i in range(length)]

Should be this:
def __init__(self):
    self.guesses = []
    self.game_states = []
    self.code = [] # Add this later when you define length.

(Also, I would keep length as a local variable defined in run and then refer to len(self.code) instead.)
I would advise you look at how Python classes work.
You can replace random.randint(0, 9) with random.randrange(10).
Furthermore, once you pull the functions out of run, things like:
def take_guess():

become:
def take_guess(self):

Naming
        correct_guess_length = False

I expect a varaible with the name of length have some sort of integer (maybe real?) value. A boolean? Not often.
You don't really need this anyway!
            else:
                correct_guess_length = True

Can just be:
            else:
                break

I would probably just get rid of that too and say:
            else:
                return guess

Same thing goes for:
    correct = False
    while not correct:
        ...

You can get rid of these and use a break statement instead.
randrange
    code = [random.randint(0, 9) for i in range(length)]

Is pretty good except, randrange is a little more specialized for this:
    code = [random.randrange(10) for _ in range(length)]

(Also, when you disregard a variable the convention is to use _, not i.)
__str__
Since you rap everything in a game object, maybe you should give it a string representation overloading the __str__ function, and just print out the string representation of itself. Move print_game_state to __str__.


Split camel cased/snake cased String I want to implement a method which, given some camelcased or underscored String, will return a list of separate words that make up this String.
Examples:

ISomeCamelCasedString -> {I, Some, Camel, Cased, String}
UNDERSCORED_STRING -> {UNDERSCORED, STRING}
camelCased_and_UNDERSCORED -> {camel, Cased, and, UNDERSCORED}

My approach to solve this is as follows: I add a space character between each two words that should be separated and then divide them into a list using StringTokenizer.

public static List<String> split(String string) {
    StringBuilder separatedWords = new StringBuilder();

    for (int i=0; i<string.length(); i++) {
        char c = string.charAt(i);

        if (i > 0) {
            char previousC = string.charAt(i-1);

            if ((!Character.isLowerCase(c) && !Character.isUpperCase(previousC)) //  UpperCamelCase, UPPER_DASHED
                    || !Character.isLetterOrDigit(previousC)                    // lower_dashed
                    || (i < string.length() - 1) && Character.isLowerCase(string.charAt(i+1))  && Character.isUpperCase(c)  ) // IAttribute
            {
                separatedWords.append(" ");
            }
        }
        if (Character.isLetterOrDigit(c)) {
            separatedWords.append(c);
        }
    }

    List<String> tokens = new ArrayList<String>();

    StringTokenizer tokenizer = new StringTokenizer(separatedWords.toString());

    while(tokenizer.hasMoreTokens()) {
        tokens.add(tokenizer.nextToken());
    }
    return tokens;
}

As you can see, it looks quite complicated. How can I improve this?

My first impression:

The implementation looks long and complicated for something that sounds simple
You've provided some example cases to verify correctness, which is a very good thing

My first reaction is to embody the example test cases in proper unit tests, and then replace the implementation with a different approach, and make the tests I broke work again.

The unit tests, straightforward from your examples:

@Test
public void testCamelCased() {
    assertEquals(Arrays.asList("I", "Some", "Camel", "Cased", "String"), split("ISomeCamelCasedString"));
}

@Test
public void testSnakeCased() {
    assertEquals(Arrays.asList("UNDERSCORED", "STRING"), split("UNDERSCORED_STRING"));
}

@Test
public void testMixed() {
    assertEquals(Arrays.asList("camel", "Cased", "and", "UNDERSCORED"), split("camelCased_and_UNDERSCORED"));
}

Next, I was thinking that this can be simplified by splitting on some clever regex. I'm no regex wiz, but there are a lot of those on Stack Overflow, and I found a suitable thread about splitting camelCase to its words, here. Adapting it to also split on underscores was relatively easy, though probably not perfect, resulting in this:

private static final String RE_CAMELCASE_OR_UNDERSCORE =
        "(?<!(^|[A-Z]))(?=[A-Z])|(?<!^)(?=[A-Z][a-z])|_";

public static List<String> split(String string) {
    List<String> words = new ArrayList<String>();
    for (String word : string.split(RE_CAMELCASE_OR_UNDERSCORE)) {
        if (!word.isEmpty()) {
            words.add(word);
        }
    }
    return words;
}

This is probably not perfect, but a lot shorter than the original, and easier to understand how it works. If somebody can figure out how to get RE_CAMELCASE_OR_UNDERSCORE so that it doesn't produce empty elements, then the method can be shortened to simply:

return Arrays.asList(string.split(RE_CAMELCASE_OR_UNDERSCORE));

PS: this_is_usually_called_snake_cased, not "underscored".


Splits up two comma separated items, if they exist This method splits up two comma separated items, if they exist, and increments the amount if the are already in the list; otherwise add the item to the list

I want to know if this code can be optimized some more. Any suggestions?

The method below works 100% fine but I feel like it is not neatly done.

private void SeperateJoinedDisposables(IEnumerable<Models.Container> joinedDisposables)
{
    if (!joinedDisposables.Any())
        return;

    foreach (var item in joinedDisposables.ToList())
    {
        var amount = item.Amount;
        var container = item.Container;

        var disposable1 = container.Substring(0, container.IndexOf(","));
        var disposable2 = container.Substring(container.IndexOf(",") + 1);

        DisposablesModel.Containers.Remove(item);

        var disposableCointians1 = DisposablesModel.Containers.FirstOrDefault(x => x.Container.ToUpper().Contains(disposable1.ToUpper()));
        var disposableCointians2 = DisposablesModel.Containers.FirstOrDefault(x => x.Container.ToUpper().Contains(disposable2.ToUpper()));

        if (disposableCointians1 != null)
            disposableCointians1.Amount = (int.Parse(disposableCointians1.Amount) + int.Parse(amount)).ToString();
        else
            DisposablesModel.Containers.Add(new Models.Container { Amount = amount, Container = disposable1 });

        if (disposableCointians2 != null)
            disposableCointians2.Amount = (int.Parse(disposableCointians2.Amount) + int.Parse(amount)).ToString();
        else
            DisposablesModel.Containers.Add(new Models.Container { Amount = amount, Container = disposable2 });
    }
}

Here is my model class

    public class DisposablesModel
    {
        public string Barcode { get; set; }
        public string RushColor { get; set; }
        public string RushDescription { get; set; }
        public string Pollution { get; set; }
        public int NumberOfSamples { get; set; }
        public List<Container> Containers { get; set; }
        public bool IsSubcontracted { get; set; }
    }

    public class Container
    {
        public string Amount { get; set; }
        public string Container { get; set; }
    }

Some quick remarks:

The class "Container" has a property named "Container"? That is... confusing, to say the least. Hence you having to name it item when iterating through it, which is unclear (and thus bad).
"Cointians" has two typos.
If you need to have a case insensitive Contains(), there are better ways to do it than use ToUpper(): https://stackoverflow.com/a/54686459/648075
Do not copy-paste logic when it is the same for two cases; instead move it to a method and call it.
Why is the property Amount a string instead of an int? Your code constantly needs to converts strings to ints and vice versa; why not use the proper types instead and avoid all that?
Is the Container of a Container always a comma-separated string of two items? Why then not make it a List<string> Names?

Most impotantly: due to the bad names, I have trouble figuring out what your code actually does.

That joinedDisposables is a list of Containers is unclear, and due to "Container" being both the name of a class as well as one of its properties the code becomes even more unclear.

I have the impression that once your Container class has a "proper" structure, much of this code can be replaced by simple LINQ queries; it's just that the bad names and the bad structures obfuscate so much it makes things hard to grasp.

To me it looks like public List<Container> Containers { get; set; } should be a Dictionary<string, int>, and that what you pass to SeperateJoinedDisposables should be a List<string> where each string is a comma separated list. And then your code would be something like this:

Dictionary<string, int> Containers = new Dictionary<string, int>();

private void Parse(List<string> commaSeparatedValues)
{
    foreach (var commaSeparatedValue in commaSeparatedValues)
    {
        foreach (var value in commaSeparatedValue.Split(','))
        {
            var normalisedValue = value.ToLower();

            if (!Containers.TryGetValue(normalisedValue, out int count))
            {
                count = 0;
            }

            count++;
            Containers[normalisedValue] = count;
        }
    }
}

To me this code expresses far more clearly the intent of what you're trying to do -- if I correctly understand the purpose of your code, of course.

But then I don't know where IEnumerable<Models.Container> joinedDisposables comes from and how it is constructed and why it is constructed that way, so I can only assume a "better" solution based on limited knowledge.


Balanced parenthesis in Ruby I'm solving the "Balanced Parenthesis" problem in Ruby, and I came up with this solution.

def balanced?(string)
  return false if string.length.odd?

  pairs = { '{' => '}', '[' => ']', '(' => ')' }

  string.chars.each_with_object([]) do |bracket, stack|
    if pairs.keys.include?(bracket)
      stack << bracket
    elsif pairs.values.include?(bracket)
      return false unless pairs[stack.pop] == bracket
    else
      return false
    end
  end

  true
end

The first check is for the length of the string: If it's odd, the can't be balanced.

I then iterate over the chars of the string:

If I find an opening bracket, I add it to an array.
If I find a closing bracket, I remove the last element from the array and check if the brackets are a pair.
If I find neither an opening or a closing bracket, the string must be invalid.

Are there any edge cases I'm missing? Also, this doesn't seem efficient: First, there's an added dictionary. Second, there is a linear search on each iteration to check either the keys or the values of the dictionary. There's an \$O(n)\$ on the array resulting from the string, as well, but I'm not sure if we can avoid this.

To eliminate nested loop you may apply regex checking for invalid symbol from the start -- it would be O(n) + O(n) = O(n):

def balanced? string
  return false if string.length.odd?
  return false if string =~ /[^\[\]\(\)\{\}]/

  pairs = { '{' => '}', '[' => ']', '(' => ')' }

  stack = []
  string.chars do |bracket|
    if expectation = pairs[bracket]
      stack << expectation
    else
      return false unless stack.pop == bracket
    end
  end

  stack.empty?
end


Error-checking macro to jump to specified label given a failing statement I'm starting a new C projet and I want to have the cleanest and shortest error checking code possible.

To that aim, I often use two things :

functions return 0, false or NULL on error
I try to use small functions with one cleanup label "fail" or "error"

So I thought about using a macro to simplify the code even more. If I choose to use it, this pattern will appear in almost every function so I expect that people reading the code will become accustomed to it very quickly.

So, is it a viable solution ? Here is an example snippet :

#define try( stmt , error_label ) if (!(stmt)) { goto error_label; }

// this function creates a new image database
ImageDatabase*
db_create(uint32_t max_images,
          struct resolution resolutions[NB_RESOLUTIONS])
{
    ImageDatabase* db;
    try( db = allocate_empty_db(), error);
    memcpy(db->resolutions, resolutions, NB_RESOLUTIONS * sizeof(struct resolution));
    db->max_nb_images = max_images;
    try( db->metadatas = calloc(max_images, sizeof(struct metadata)) , error);
    try( finalize_creation(db) , error);
    return db;

error:
    free(db->metadatas);
    free(db);
    return NULL;
}

EDIT: Here is an alternative without the try macro... but it's not as pretty. Also note that in this example the code is simple enough to avoid a goto, but it's not always the case.

ImageDatabase*
db_create(uint32_t max_images,
          struct resolution resolutions[NB_RESOLUTIONS])
{
    ImageDatabase* db = allocate_empty_db();
    if (db) {
        memcpy(db->resolutions, resolutions, NB_RESOLUTIONS * sizeof(struct resolution));
        db->max_nb_images = max_images;
        db->metadatas = calloc(max_images, sizeof(struct metadata)) , error);
        if (db->metadata == NULL
           || !finalize_creation(db))
        {
            free(db->metadatas);
            free(db);
            db = NULL;
        }
    }
    return db;
}

NULL dereference

If allocate_empty_db() returns NULL, it will jump to the error handling code, which will cause a NULL dereference when it tries to free(db->metadatas).  You will need to guard that line like this:

if (db)
    free(db->metadatas);

The macro

I'm not exactly a fan of the macro.  First, calling it try makes me think your code is java, and then when I realize it isn't, I'm more confused.  Second, if your label is always going to be the same in every function, then you can omit the second argument of the macro and just hardcode it to goto error.  That way the macro can look smaller and the reader doesn't have to parse the extra comma and argument (which look like part of the body you are try-ing).

Also, the macro should be a do while instead of an if.  You could get in trouble with code like this:

if (condition)
    try(foo(), error);
else
    bar();

Here, your macro will cause the above to expand to:

if (condition)
    if (!foo()) { goto error; };
else
    bar();

Which is actually a syntax error because of the extra semicolon but is also close to making the else pair with the wrong if.  The macro should look like this instead:

#define try( stmt , error_label )              \
        do {                                   \
            if (!(stmt)) { goto error_label; } \
        } while(0)

This do while(0) construction is a common trick when writing C macros.  It allows a multi-statement macro to be used in an if statement like the one in my example.  See this Stackoverflow question for more info.


Creating range of numeric values based on array of strings Basically I have an array of strings. In this array, a range separator word might exist. For this example, I'm going to use the word to as range separator. We can assume that the range separator is not the first or last entry in the array. If we find the range separator we need to get the entry before and the entry after the separator, compare the two entries that we can assume are safe to parse as integer and find the character that differs between them. We then need to create new entries by counting up the number that is different, and return an IEnumerable<string> containing the new entries. We can also assume that only one character will differ between the start of the range and the end of the range.

For example, if the array contains these values:

1102
to
1602

we need to return

1202
1302
1402
1502

And here's my code:

public static IEnumerable<string> GetRangeSegments(string[] segments, string rangeIndicator)
{
    var rangeIndicatorIndex = Array.IndexOf(segments, rangeIndicator);
    var rangeSegments = new string[] { };

    if (rangeIndicatorIndex < 0)
    {
        return rangeSegments;
    }

    var rangeStart = segments[rangeIndicatorIndex - 1];
    var rangeEnd = segments[rangeIndicatorIndex + 1];

    var startChars = rangeStart.ToCharArray();
    var endChars = rangeEnd.ToCharArray();

    if (startChars.Length != endChars.Length)
    {
        return rangeSegments;
    }

    var diffIndex = -1;

    for (var i = 0; i < startChars.Length; i++)
    {
        if (startChars[i] == endChars[i])
        {
            continue;
        }

        diffIndex = i;
        break;
    }

    if (diffIndex == -1)
    {
        return rangeSegments;
    }

    var start = int.Parse(startChars[diffIndex].ToString()) + 1;
    var end = int.Parse(endChars[diffIndex].ToString());

    var numbers = Enumerable.Range(start, end - start);
    return rangeSegments.Concat(numbers.Select(number => rangeStart.Remove(diffIndex, 1).Insert(diffIndex, number.ToString())));
}

If a method returns an IEnumerable<T> we expect an deffered execution using the yield keyword.

To make this happen and have a valid parameter validation we need another method which calls the method which is yielding.

So let us add one

public static IEnumerable<string> GetRangeSegments(string[] segments, string rangeIndicator)
{
    if (segments==null) { throw new ArgumentNullException(nameof(segments)); }
    if (rangeIndicator == null) { throw new ArgumentNullException(nameof(rangeIndicator)); }
    if (string.IsNullOrWhiteSpace(rangeIndicator)) { throw new ArgumentException("May not be empty.", nameof(rangeIndicator)); }

    return GetSegments(segments, rangeIndicator);
}

All the if statements will be executed and if the code comes to return it will immediately return because it knows about the yieldings in the GetSegments() method.

Using yield makes the empty string[] rangeSegments superflous because we just yield break. If you call ToArray() on the result of these yield break you get an empty array.

Because you can access the chars of a string by using its index you don't need ToCharArray().

If there isn't by some mistake either rangeStart[diffIndex] or rangeEnd[diffIndex] a digit the code blows up at the call to int.Parse(). A better way would be to use int.TryParse() and if this doesn't succeed you could yield break.

But if we e.g do int start = rangeStart[diffIndex] + 1; then the ascii value of that char will be casted to an int which we then can check if 47 < start < 58 and if not we yield break here.

The return statmenet in your method is just unreadable, hence hard to maintain as well. To "replace" a single char inside a string we can just use a StringBuilder.

Summing all up lead to

public static IEnumerable<string> GetRangeSegments(string[] segments, string rangeIndicator)
{
    if (segments == null) { throw new ArgumentNullException(nameof(segments)); }
    if (rangeIndicator == null) { throw new ArgumentNullException(nameof(rangeIndicator)); }
    if (string.IsNullOrWhiteSpace(rangeIndicator)) { throw new ArgumentException("May not be empty.", nameof(rangeIndicator)); }
    return GetSegments(segments, rangeIndicator);
}

private const int ascii0 = 48;
private const int ascii9 = 57;
private static IEnumerable<string> GetSegments(string[] segments, string rangeIndicator)
{
    var rangeIndicatorIndex = Array.IndexOf(segments, rangeIndicator);

    if (rangeIndicatorIndex < 0 || rangeIndicatorIndex == segments.Length - 1)
    {
        yield break;
    }

    var rangeStart = segments[rangeIndicatorIndex - 1];
    var rangeEnd = segments[rangeIndicatorIndex + 1];

    if (rangeStart.Length != rangeEnd.Length)
    {
        yield break;
    }

    var diffIndex = GetFirstDifferentCharPosition(rangeStart, rangeEnd);
    if (diffIndex == -1)
    {
        yield break;
    }

    var start = rangeStart[diffIndex] + 1;
    if (start < ascii0 || start > ascii9) { yield break; }

    var end = rangeEnd[diffIndex];
    if (end < ascii0 || end > ascii9) { yield break; }

    if (end < start) { yield break; }

    StringBuilder sb = new StringBuilder(rangeStart);
    for (int i = start; i < end; i++)
    {
        sb[diffIndex] = (char)i;
        yield return sb.ToString();
    }

}
private static int GetFirstDifferentCharPosition(string first, string second)
{
    var diffIndex = -1;

    for (var i = 0; i < first.Length; i++)
    {
        if (first[i] != second[i])
        {
            diffIndex = i;
            break;
        }
    }
    return diffIndex;
}


Signup page in HTML/PHP I've written some PHP code that connects to a database and saves input entered by the user at our signup page. I would like some feedback before I present this code to my colleagues, as I intend on using this code for our website:

Security
Use of PDO vs other connection methods
Variable Naming
PHP Conventions
HTML Conventions
Anything else

sign_up.html

<!DOCTYPE html>

        <meta charset="UTF-8">
        <title>Sign Up</title>
        <link rel="stylesheet" type="text/css" href="styles/main.css">
        <link rel="stylesheet" type="text/css" href="styles/dropdown.css">
        <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css">

            <form action="sign_up.php" method="post">
                <input id="form_input"
                       type="text"
                       placeholder="First Name"
                       name="firstname">

                <input id="form_input"
                       type="text"
                       placeholder="Last Name"
                       name="lastname">

                <input id="form_input"
                       type="text"
                       placeholder="Age"
                       name="age">

                <input id="form_input"
                       type="text"
                       placeholder="Email"
                       name="email">

                <input id="form_input"
                       type="password"
                       placeholder="Password"
                       name="password">

                <button id="signup_button" type="submit" disabled>Sign Up</button>
            </form>

sign_up.php

<?php

include 'config/database.php';

try {

    $conn = new PDO("mysql:host=$DB_HOST;dbname=Database", $DB_USER, $DB_PASSWORD);
    $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    $firstname = filter_var($_POST["firstname"], FILTER_SANITIZE_STRING);
    $lastname = filter_var($_POST["lastname"], FILTER_SANITIZE_STRING);
    $email = filter_var($_POST["email"], FILTER_SANITIZE_EMAIL);
    $password = filter_var($_POST["password"], FILTER_SANITIZE_STRING);

    #No need to hash name and email, can be used to check duplicates
    $password_hash = password_hash($password, PASSWORD_DEFAULT);

    $stmt = $conn->prepare("INSERT INTO Users (FirstName, LastName, Email, Password) VALUES (:firstname, :lastname, :email, :password)");
    $stmt->bindParam(':firstname', $firstname);
    $stmt->bindParam(':lastname', $lastname);
    $stmt->bindParam(':email', $email);
    $stmt->bindParam(':password', $password_hash);
    $stmt->execute();

    header("Location: signed_up.html");

} catch(PDOException $e) {

    echo "Connection failed: " . $e->getMessage();

}

?>

I see multiple instances of id="form_input". These attributes need to change to class or each id attribute value should be altered to be unique.
Instead of using
 tags to style your input fields on separate lines, use css.  By setting all of the elements in the form as display:block;, you can eliminate the excess markup -- a cleaner alternative.  CSS should be the first technique to create spacing and layout stylings; when you find yourself creating spaces with the spacebar or with
, try to achieve the same effect with display, margin, padding, etc.
11 reasons why placeholders are problematic -- I can appreciate that you intend to design a clean, minimalist layout, but I don't think I support non-labeled fields and your project should maintain a consistent theme considering that your project may eventually need to employ more forms.
In addition to the previous bullet point about UX, I will urge you to add some client-side validation to your form.  These character limitations will be up to you.  I assume empties.js is doing something that enables the submit button, but I can't see it.  If empties.js is as functionally narrow a file as it's name suggests, you may want to build more robust processes in there.
Rather than sanitizing server-side and unconditionally inserting into the database, you should be catching overtly "bad" submissions and denying access.
If a submitted password contains html markup, that is no concern of yours (no need to sanitize) because you will never, ever be printing that value -- ever.  More clearly: never alter someone's submitted password characters.
You must never, ever present the raw error message to the end user.  See @YCS's hyperlink.  Especially with registration forms, you need to find the correct balance between informing the registrant why the submission was denied, yet not compromising the security of your project.
Finally, login/registration systems are widely available now.  If you are doing this as an educational exercise, then carry on.  If you don't want to reinvent the wheel, just use a pre-existing script from a security-minded author.


Rob Pike's Golang presentation excercise of day 1 I'm following this presentation. At the very end there's an exercise about solving Fibonacci and says that instead of addition, make the operation setteable by a function.

Is the following is a good solution? How close might it be to what Rob Pike would have written?

package main

import "fmt"

func add(a, b int) int {
    return a + b
}

func subtract(a, b int) int {
    return a - b
}

func power(a, b int) int {
    return a ^ b
}

// fib returns a function that returns
// successive Fibonacci numbers.
func fib(op func(int, int) int) func() int {
    n0, n1 := 0, 1
    return func() int {
        n0, n1 = n1, op(n0, n1)
        return n0
    }
}

func main() {
    f := fib(add)
    // Function calls are evaluated left-to-right.
    fmt.Println(f(), f(), f(), f(), f(), f(), f())

    f = fib(subtract)
    // Function calls are evaluated left-to-right.
    fmt.Println(f(), f(), f(), f(), f(), f(), f())

    f = fib(power)
    // Function calls are evaluated left-to-right.
    fmt.Println(f(), f(), f(), f(), f(), f(), f())
}

This seems like a good solution except for one thing:

func power(a, b int) int {
    return a ^ b
}

The ^ operator doesn't do what you think it does. It's a bitwise xor operator. There is no power operator in Go, there's only math.Pow for float64s.

On a side note, I personally would create a type for operators:

type Op func(int, int) int

It's easier to type in function definitions and allows you to extend it with methods like String().


Project Euler 23: Non Abundant Sums Here is my solution to Project Euler 23: Non Abundant Sums. I know I'm doing the Divisors function brute force, but I can't seem to get it to work any other way. Any improvements are welcome.

#include <iostream>
#include <vector>

//Calculates divisors
void Divisors(unsigned number, std::vector<unsigned> &result){
    for(unsigned a = 1; a < number; a++){
        if(number % a == 0){
            result.push_back(a);
        }
    }
}
//Calls for divisors and adds them
unsigned SumOfDivisors(unsigned number){
    unsigned result = 0;
    std::vector<unsigned> divisors;
    Divisors(number, divisors);
    for(unsigned a = 0; a < divisors.size(); a++){
        if(divisors[a] < number)
            result += divisors[a];
    }
    return result;
}
//Calls calculates the sum of divisors to see if it is Abundant
bool IsAbundant(unsigned number){
    if(SumOfDivisors(number)> number){
        return true;
    }
    else{
        return false;
    }
}
//pushes back abundant numbers
void Abundant(unsigned limit, std::vector<unsigned> &abunNumbers){
    for(unsigned a = 1; a < limit; a++){
        if(IsAbundant(a)){
            abunNumbers.push_back(a);
        }
    }
}

//Find's all number's that are the sum of two abundant numbers
void SumOfAbundant(unsigned limit, std::vector<unsigned> const& abunNumbers, std::vector<bool> &sumAbun){
    for(unsigned a = 0; a < abunNumbers.size(); a++){
        for(unsigned b = a; b < abunNumbers.size(); b++){
            if(abunNumbers[a]+abunNumbers[b] <= limit){
                sumAbun[abunNumbers[a]+abunNumbers[b]] = true;
            }
        else{
            break;
        }
        }
    }
}

int main(){
    const unsigned limit = 28123;
    unsigned result = 0;

    std::vector<unsigned>abunNumbers;
    Abundant(limit, abunNumbers);

    std::vector<bool>sumAbun(limit,false);
    SumOfAbundant(limit, abunNumbers, sumAbun);

    for(unsigned a = 0 ; a < sumAbun.size() ; a++){
        if(!sumAbun[a]){
        result += a;
        }
    }
    std::cout << result;
}

I would recommend the following changes:

Math is your friend. Factorize the number into prime powers (a precalculated table of primes is of immence value for a lot of Project Euler problems), and use a formula from this article.
Memoize. As soon as you identify a single (prime) divisor of a number, and take the quotient, notice that the quotient was already inspected and factorized. A vector of tuples (prime, power) representing a factorization may also help a lot.

There could be other optimizations, but these two seem to be enough.


Reduce number of parameters of the same type I'm looking at a class diagram and it shows a method like this:

SavePrintOptions(bool,bool,bool,bool,bool,bool,bool,bool,bool);

where each of the parameters match up with save options (checkboxes) in the UI.  How could I refactor this signature to reduce the number of parameters?

That is a real code smell and should be refactored.
How should any mortal soul unterstand, what this code does?

Why not using a simple object?

public class{

    public bool Whateveryouwant { get; set; }
    public bool Whateveryouwantelse { get; set; }
}

The advantage:

1) your parameter shrunk to just one

2) if you name the properties (better than above) anybody else knows, what options were chosen.


Floyd's cycle-finding algorithm I am writing a piece of code for some students and I came up with the following implementation of Floyd's algorithm for finding cycles in linked lists.
I was wondering if there are ways I can improve what I have so far such that the result is

more C++ idiomatic
does not contain dumb mistakes associated with the generic code

template <typename T>
struct Node {
    T val;
    Node* next;
    Node() = default;
    Node(T x)
        : val(x)
        , next(nullptr)
    {
    }
};

template <typename T>
Node<T>* detect_cycle_constant_time(Node<T>* head)
{
    Node<T> *n1, *n2;
    n1 = n2 = head;

    while (n1 && n2) {
        n1 = n1->next;
        n2 = n2->next;
        if (n2)
            n2 = n2->next;
        else
            break;

        if (n1 == n2)
            break;
    }

    // second phase of Floyds's algorithm
    if (n1 == n2) {
        n2 = head;
        while (n1 != n2) {
            n1 = n1->next;
            n2 = n2->next;
        }
        return n1;
    }
    return nullptr;
}

General
I found no "dumb mistakes associated with generic code". Good job.
Still, there are some points which can be improved.
(Aren't there always?)

The first point is encapsulation.
Letting nodes float around free, instead of keeping them in their dedicated container, or (rarely) evacuating them to a handle for re-insertion, is not recommended.
Admittedly, a properly written container won't suddenly sprout a cycle, so there you are.
Also, it would detract from the point of your code, so let's leave it alone this time.
Node
Node's default ctor won't initialize its members. The fix is easy:
Just add in-class initializers.
Doing so even allows you to simplify your second constructor, removing the second mem-initializer.
That is, if Node should even have anything but its data members. I personally prefer to have my Node's not getting in the way when implementing my containers.
Floyd's algorithm
The name detect_cycle_constant_time() is a bald-faced lie. The algorithm needs linear time in the number of nodes.
Doing an early return would simplify your code. As you don't allocate any resources, there goes the only argument against. Doing data-flow analysis is much more involved.
There are more loops than the humble while-loop. Sometimes, they are even useful.
Using auto would slightly simplify your local variable definitions.
Whether to use class or typename for template arguments is a matter of taste. All else being equal, I prefer brevity.
Modified Code
template <class T>
struct Node {
    T val = T();
    Node* next = nullptr;
    Node() = default;
    Node(T x) : val(x) {}
};

template <class T>
Node<T>* detect_cycle_floyd(Node<T>* head)
{
    auto n1 = n2 = head;

    do {
        if (!n2)
            return n2;
        n1 = n1->next;
        n2 = n2->next;
        if (!n2)
            return n2;
        n2 = n2->next;
    } while (n1 != n2);

    // second phase of Floyds's algorithm
    for (n2 = head; n1 != n2; n2 = n2->next)
        n1 = n1->next;
    return n1;
}


Adding and Retrieving Players in a simple Pre-Flop Poker Hud with Local Storage I am building a browser based Pre-Flop HUD that for now just tracks VPIP, PFR and number of hands played for an opponent (https://pokercopilot.com/poker-statistics/vpip-pfr). I am currently using local storage to store my data. In the future I will use a better persistence strategy, add more features, and find a way to de-couple the user interface without just putting it in the functions I have now. I used an object literal approach without classes right now to try out some basic decomposition with simple functions for practice. Right now the only functionality I have is adding a player to Local Storage and getting all players from Local Storage.

Any suggestions for improvement are welcome.
But I do have a couple questions:

Is there a benefit to using Classes?  Right now I don't see the benefit of having a handle, like, player1 = new Player(arg,arg,etc...), player2 = new Player(arg,arg, etc...)...
Is there a better way to store to Local Storage? I don't think storing each player with their own key and having a ton of entries is a good idea.
In my function addPlayer() should I be calling savePlayersToStorage in my localStorageUtils.js?

Main.js file = entry point for my program

    /**********************************
 * This is the program driver, that
 * will call other functions
 * *******************************/

import { addPlayer } from "./player.js";
import {
  savePlayersToStorage,
  getPlayersFromStorage,
} from "./LocalStorageUtilsObjectLiteral.js";

function main() {
  let newPlayer = "Doug Polk";
  const localStorageKey = "stored_players";
  let players = getPlayersFromStorage(localStorageKey); //array of player objects in localStorage
  players.push(addPlayer(newPlayer));
  savePlayersToStorage(localStorageKey, JSON.stringify(players));
}

main();

Player.js addPlayer creates and returns an object literal

function addPlayer(name) {
  let player = {
    uuid: Date.now(),
    name: name,
    vpipActionCount: 0, //The number of times a player voluntarily put money into the pot Pre-Flop, whether calling or raising.
    pfrActionCount: 0, //The number of times a player raised pre-flop, this includes 3-bets.
    vpipPercentage: 0, //The percentage of hands a player has played, can be between 0 and 100%
    pfrPercentage: 0, //The percentage of hands a player has raised pre-flop, can be between 0 and 100%, but can never be higher than vpipPercentage
    totalHandsTracked: 0, //The number of hands tracked for a player

    callAction: function () {
      this.totalHands++;
      this.vpipActionCount++;
    },
    raiseAction: function () {
      this.totalHands++;
      this.pfrActionCount++;
    },
    foldAction: function () {
      this.totalHands++;
    },
    calculateVpipPercentage: function () {
      this.vpip = (this.vpipActionCount * 100) / this.totalHands;
    },
    calculatePfrPercentage: function () {
      this.pfrPercentage = (this.pfrActionCount * 100) / this.totalHands;
    },
    getVpipPercentage: function () {
      return this.vpipPercentage;
    },
    getPfrPercentage: function () {
      return this.pfrPercentage;
    },
  };
  return player;
}

export { addPlayer };

LocalStorageUtils.js Functions to get and save players to Local Storage

function savePlayersToStorage(localStorageKey, playersArray) {
  if (localStorage.getItem(localStorageKey === null)) {
    localStorage.setItem(localStorageKey);
  }
  localStorage.setItem(localStorageKey, playersArray);
}

function getPlayersFromStorage(localStorageKey) {
  if (localStorage.getItem(localStorageKey === null)) {
    localStorage.setItem(localStorageKey);
  }
  let players = JSON.parse(localStorage.getItem(localStorageKey));

  return players;
}

export { savePlayersToStorage, getPlayersFromStorage };

To Your Questions

Is there a benefit to using Classes? Right now I don't see the benefit of having a handle, like, player1 = new Player(arg,arg,etc...), player2 = new Player(arg,arg, etc...)...

Maybe there are benefits, but I can't think of any serious ones now.
The current version is similar to the factory pattern, where the creation of an object is abstracted by a factory - in this case addPlayer:
const player = addPlayer('Harry Potter');

Is there a better way to store to Local Storage? I don't think storing each player with their own key and having a ton of entries is a good idea.

I think storing all players under one key is sufficient and the easiest to manage. But you can get the best of both worlds:
Currently you are saving an array of players. Instead you could save a map where the key is a unique attribute of the player (maybe the name or an id) and the player itself. The benefit would be that you do not need to loop through an array to find a player:
function getPlayersFromStorage(key) {
  /* ... read from storage ... */;
  /* return has the form:
     {
        playername: player
     }
  */
}

const players = getPlayersFromStorage(localStorageKey);
const harryPotter = players["Harry Potter"];

This is only necessary if you need to find players directly.

In my function addPlayer() should I be calling savePlayersToStorage in my localStorageUtils.js?

It depends. Currently the method name addPlayer does not represent what the method does. When I first read the name I thought: "Ah ok, this methods put a player in some collection or maybe the local storage". But then I read the following line and get confused:

players.push(addPlayer(newPlayer));

A better name for addPlayer would be createPlayer and I would not call savePlayersToStorage in it, to have smaller and side effect free functions.
Change Persistenz

I am currently using local storage to store my data. In the future I will use a better persistence strategy

Currently, only the main.js depends on localStorage, and knows the implementation details, which would make it easier to change it in the future. However, to make it easier to change, especially as the code grows and more files depend on the persistence implementation, it should be abstracted as much as possible:
class LocalStorage {
    getAll() { /* ... */ }
    getByName() { /* ... */ }
    save()  { /* ... */ }
}

class OtherStorage {
    getAll() { /* ... */ }
    getByName() { /* ... */ }
    save()  { /* ... */ }
}

When your different storage strategies share the same interface they are easy interchange able:
// Main.js

const storage = new LocalStorage('my-storage-key');
# const storage = new OtherStorage(/* dependencies */);

function main() {
    const players = storage.getAllPlayers();
    const harryPotter = storage.getPlayerByName('Harry Potter');

    const newPlayer = createPlayer('Ron Weasley');
    const storage.save(newPlayer);
}

Read from Store

function getPlayersFromStorage(localStorageKey) {
 if (localStorage.getItem(localStorageKey === null)) {
   localStorage.setItem(localStorageKey);
 }
 let players = JSON.parse(localStorage.getItem(localStorageKey));

 return players;
}

When reading from the store we set an item if the key does not exists with localStorage.setItem(localStorageKey). To set an item is not the responsibility for reading data. Instead we could add some logic which is related for reading. For example if there is stored data under the given key:
function getPlayersFromStorage(localStorageKey) {
    const fromStore = localStorage.getItem(localStorageKey);

    if (fromSore) {
        return [];
    }

    return JSON.parse(fromStore);
}


Call WebAPI then parse part of json result this is a basic web api call using HttpClient and parse some part of the result, this was working as expected, but the part inside the if (response.IsSuccessStatusCode) doesnt seems right. It feels like it can be simplified, but I don't know how.

static async Task<List<Hero>> GetHero(string url, string urlParameters)
{
    HttpClient client = new HttpClient();
    client.BaseAddress = new Uri(url);
    client.DefaultRequestHeaders.Accept.Clear();
    client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

    using (var response = client.GetAsync(urlParameters).Result)
    {
        if (response.IsSuccessStatusCode)
        {
            var heroes = new List<Hero>();
            var jsonResult = await response.Content.ReadAsStringAsync();
            var deserializedjsonResult = JObject.Parse(jsonResult);
            var resultHeroes = deserializedjsonResult["result"]["heroes"].ToList();

            resultHeroes.ForEach(x => heroes.Add(x.ToObject<Hero>()));

            return heroes;
        }
    }

    return null;
}

the api that I was calling will return something in this format

{
  "result": {
    "heroes": [
      {
        ...
      },
      {
        ...
      }
    ],
    "villains": [
      {
        ...
      },
      {
        ...
      }
    ],
    "extras": [
      {
        ...
      },
      {
        ...
      }
    ],
  }
}

HttpClient client = new HttpClient();

This needs to be disposed.

using (var response = client.GetAsync(urlParameters).Result)

Why don't you await this like you do with other awaitable APIs?

static async Task<List<Hero>> GetHero(string url, string urlParameters)
{
    ..
    return null;
}

Methods returning collections should return an empty collection instead of null this means you should

return Task.FromResult(new List<Hero>());

As far as the if block is concerned you don't need the heros variable. With LINQ can create the list inplace:

if (response.IsSuccessStatusCode)
{
    var jsonResult = await response.Content.ReadAsStringAsync();
    var deserializedjsonResult = JObject.Parse(jsonResult);
    var resultHeroes = deserializedjsonResult["result"]["heroes"];
    return resultHeroes.Select(x => x.ToObject<Hero>()).ToList();
}


Writing null-safe code to set form values I will ask my question using the following example:

HTML

<div>
    <input id="ABC" placeholder="abc"></input>
    <textarea id="XYZ" placeholder="xyz"></textarea>
</div>

JS Code

var valArray = [{id: "ABC", value: "INPUT"}, {id: "UNKNOWN", value: ""}, {id: "XYZ", value: "TEXTAREA"}]

function process(arr) {
    for(var i=0; i<arr.length; i++) {
        var elem = document.getElementById(arr[i].id);
        if (elem != null) {
            elem.value = arr[i].value;
        }
    }
}

process(valArray);

It works fine, but now I want to do it without an explicit if statement.

How about the following code? Is it a proper JavaScript way to handle such situations?

function process(arr) {
    for(var i=0; i<arr.length; i++) {
        (document.getElementById(arr[i].id) || 0).value = arr[i].value;
    }
}

From Programming JavaScript Applications by Eric Elliott- Chapter 3. Objects:

  Even primitive types get the object treatment when you refer to them
  with the property access notations. They get automatically wrapped
  with an object so that you can call their prototype methods.

  Primitive types behave like objects when you use the property access
  notations, but you can't assign new properties to them. Primitives get
  wrapped with an object temporarily, and then that object is
  immediately thrown away. Any attempt to assign values to properties
  will seem to succeed, but subsequent attempts to access that new
  property will fail.

In general I saw a lot of JavaScript code where || <default or alternative value> construct was used to protect against null.

So I thought, that the above described behavior would make primitives ideal candidates for "throw-away" objects.

If this is so convoluted, what about using Object:

        (document.getElementById(arr[i].id) || Object).value = arr[i].value;

That was my original idea but it seems to pollute the environment.

(document.getElementById(arr[i].id) || 0).value = arr[i].value;

In this code there is a pointless assignment when the element is not found.
In this case, the 0 gets wrapped into a temporary object,
and the assignment will work, but it will be pointless,
it's result will be thrown away.
This assignment is merely a side effect of the shortcut,
it has no purpose of its own.

Every operation in a program should be intentional, purposeful, and this shortcut violates that. As such, I consider this a dirty hack. Don't use it.

When in doubt,
go for the more intuitive and readable solution.
The first example in your post is slightly longer,
but it's simple and clear: there's no question about what will happen.

The second way,
although shorter,
may not be obvious for everyone.

Keep in mind that code is read far more times than it’s written.
It's better to favor a technique that's clear and easy to understand to something "clever" but potentially confusing / obscure.

Additional excellent recommendation by @GameAlchemist, verbatim:

  1) for optimisation purposes, the if is faster. and more importantly 2) a piece of code should be as much self-documenting as possible. Even if (element) is better written if (element != null) (which makes obvious the defensive check against getElementById returning null)


Find the in-order successor of a given node in a binary search tree I have an implementation of an algorithm to return the in-order successor of a given node in a binary search tree. We are allowed to assume that each node in the BST has a reference to its parent node. The in-order successor of a node is the node with the next largest key. So, if we received a node with a value of 5, and the tree contained a node with the value 6, we should return the node with the value 6.

My implementation differs slightly from most I've seen online. Most solutions, when given an input node with no right child, iterate back up the tree until the node is the left child of its parent and then returns the parent. I iterate back up the tree until the node's value is greater than our input node's value and then return that node. I think this is equivalent and results in a little more compact and easier to follow implementation. Let me know if I'm overlooking something with this implementation.

def successor( node, targetValue ):
    if node.right:
        node = node.right
        while node.left:
            node = node.left
    else:
        while node:
            node = node.parent
            if node.value > targetValue:
                break

    return node

For idiomatic Python:

You should not put spaces between the brackets and the values in function calls.
You should use snake_case for variables.

You've said you have concerns over finding the parent node that is the ancestor. Lets say we have the tree:

   A
 B
C D

Since that all left nodes are smaller than the current value in binary trees we know that \$B < A\$, \$C < B\$ and \$D < A\$. We also know that the right nodes are larger than the current node, and so \$B < D\$. And so we know \$C < B < D < A\$.

From this we can see that logically if node.parent.left == node is the same as if node.parent.value > target.

The benefit to the former is you don't have to create target.

Your implementation is awkward, successor should always be called duplicating the input.

successor(node, node.value)

If you don't then you'll have problems. Say \$A = 1\$, and \$\text{node} = D\$. The successor to \$D\$ is \$A\$, however successor(node, 1) will incorrectly return \$B\$.

To fix this just define target_value in successor.

def successor(node):
    if node.right:
        node = node.right
        while node.left:
            node = node.left
    else:
        target_value = node.value
        while node:
            node = node.parent
            if node.value > target_value:
                break
    return node

If you follow what you've seen online you can reduce the amount of lines of code:

def successor(node):
    if node.right:
        node = node.right
        while node.left:
            node = node.left
    else:
        while node != node.parent.left:
            node = node.parent
        node = node.parent
    return node

It should be noted that if there is no successor in the tree all implementations will raise an AttributeError.

This is rather confusing, and so you may want to change it to a different error.

def successor(node):
    try:
        if node.right:
            node = node.right
            while node.left:
                node = node.left
        else:
            while node != node.parent.left:
                node = node.parent
            node = node.parent
        return node
    except AttributeError:
        raise ValueError('No successor exists.') from None


A Java Iterable over multiple arrays GlueArrays is used for iterating over an array of arrays, and it looks like this:

Main.java

import java.util.Iterator;
import java.util.NoSuchElementException;

public class Main {

    public static class GlueArrays<T> implements Iterable<T> {

        private final T[][] arrays;

        public GlueArrays(T[]... arrays) {
            this.arrays = arrays;
        }

        @Override
        public Iterator<T> iterator() {
            return new GlueArrayIterator(arrays);
        }

        private static class GlueArrayIterator<T> implements Iterator<T> {

            private final T[][] arrays;
            private int arrayIndex = 0;
            private int localIndex = 0;

            GlueArrayIterator(T[][] arrays) {
                this.arrays = arrays;
            }

            @Override
            public boolean hasNext() {
                if (arrayIndex == arrays.length) {
                    return false;
                }

                return arrayIndex < arrays.length ||
                       localIndex < arrays[arrayIndex].length;
            }

            @Override
            public T next() {
                if (!hasNext()) {
                    throw new NoSuchElementException("Nothing to iterate.");
                }

                T returnValue = arrays[arrayIndex][localIndex];

                if (localIndex < arrays[arrayIndex].length) {
                    localIndex++;

                    if (localIndex == arrays[arrayIndex].length) {
                        localIndex = 0;
                        arrayIndex++;
                    }
                }

                return returnValue;
            }
        }
    }
    public static void main(String[] args) {
        Number[] arr1 = {1, 2, 3};
        Number[] arr2 = {4f, 5f, 6f};
        Number[] arr3 = {7.0, 8.0, 9.0};

        for (Number number : new GlueArrays<>(arr1, arr2, arr3)) {
            System.out.println(number);
        }
    }
}

As always, any critique is welcome.

First off, this is buggy. If one of the input arrays is empty, you'll get an ArrayIndexOutOfBoundsException - and even if you correct that error, the iterator will stop if it encounters an empty array.

Consider using proper tests with multiple scenarios, especially with edge cases, in order to catch errors like that.

Then I get two warnings from Eclipse (other compilers and their settings may differ, but I believe these are "common" warnings, that normally would be raised). You should never have warnings. Code should preferably be adjusted or rewritten to avoid them, or at the very least (if you have a good reason to keep the warning) annotated with @SuppressWarnings and commented on with those reasons.

1) Due to the use of generics together with variable arguments the GlueArrays constructor should have the annotation @SafeVarArgs.

2) The instantiation of the iterator in iterate() is lacking its type parameters. It should be

return new GlueArrayIterator<T>(arrays);

or, if you are at least using Java 1.7, just

return new GlueArrayIterator<>(arrays);


Singly Linked List in C++ I've written a singly linked list in C++. Would appreciate reviews on code efficiency and best practices.

#ifndef SLIST_HPP
#define SLIST_HPP

#include <stdexcept>
#include <iostream>

template <typename T> struct node{
    node(T data):data(data),next(nullptr){}
    T data;
    node<T> * next;
};

template< typename T> class slist{
    node<T>* head;
    int size;
public:
    slist(node<T>* head):head(head), size(0){}

    slist(const slist<T>& rhs){

     node<T>* temp = rhs.getHead();

    node<T>* current = new node<T>(temp->data);
    while(temp != nullptr){

        current->next = new node<T>(temp->next->data);
        temp = temp->next;
        current = current->next;
    }
}
    ~slist(){
        if(head == nullptr) return;

        while(head != nullptr){
            node<T>* current = head;
            head = head->next;
            delete(current);
        }
    }
    slist& operator= (const slist& rhs){

    node<T>* temp = rhs.getHead();
    try{
        if(temp == nullptr) throw std::runtime_error("Can't assign null");
    }catch(std::runtime_error& e){
        std::cout<<e.what()<<std::endl;
    }

    node<T>* current = head;
    while(temp != nullptr){

        temp = temp->next;
        current = current->next;
        current->next = new node<T>(temp->next->data);
    }

}
    node<T>* getHead()const {
        return head;
    }

    void add(node<T>* p, T item){
        if(p == nullptr) return;
        node<T>* current = new node<T>(item);
        node<T>* temp = p->next;
        p->next = current;
        current->next = temp;
        size++;

    }
    void insertFront(T item){
        node<T>* p = new node<T>(item);
        if(head == nullptr){
            p = head;
            size++;
            return;
        }
        p->next = head;
        head = p;
        size++;
    }

    void insertBack(T item){
        node<T>* p = new node<T>(item);
        node<T>* current = head;
        while(current->next != nullptr){
            current = current->next;
        }
        current->next = p;
        size++;
    }

    void remove(T item){
        bool check = false;

        node<T>* current = head;
        try {
            while(current != nullptr){
                if(current->data == item) check = true;
                current = current->next;
            }
            if(!check){
                throw std::runtime_error("Item not in list");

            }
        }catch(std::runtime_error& e){
            std::cout<<e.what()<<std::endl;
            exit(-1);
        }

        current = head;
        while(current != nullptr){
           if(current->next->data == item){
               node<T>* temp = current->next;
               current->next = current->next->next;
               delete(temp);
               break;
           }
            current = current->next;
        }
        size--;

    }
    int getSize () const {
        return size;
    }

    void printList(){
        node<T>* current = head;
        while(current != nullptr){
            if(current->next != nullptr){
                std::cout<<current->data<<"->";
            }else{
                std::cout<<current->data<<std::endl;
            }
            current = current->next;
        }
    }
};
#endif //SLIST_HPP

You're reimplementing std::forward_list, so a good place to start would be the API documentation for forward_list. That'll show you a lot of deficiencies in your current approach:

Naming. For example, getSize() should be size(), so that you can write generic algorithms that work with your class in addition to std::list, std::vector, etc (all of which have a size() method).
Encapsulation. You provide a public member function getHead() that returns a raw pointer to an internal node. What do you expect the user to be able to do with a node<T>*?
Composability. Your remove member function causes an exit(-1) on failure. It would be much much better to have remove throw an exception, and then allow the caller to catch and handle the exception — possibly with an exit, but possibly in some other way.
Support common C++ idioms. Your list class doesn't provide begin() and end(), so it's not iterable; e.g. I can't say for (auto&& element : myList) { std::cout << element; }. If you provide iteration, then I can write this loop, so I won't need your printList member function anymore.

Finally, one more naming nit: As a member function, printList could just be named print. myList.print() is slightly better (shorter and less repetitive) than myList.printList(). And again it enables generic programming:

// You could overload this printTwice function for each container type:

void printTwice(const List& l)
{
    l.printList();
    l.printList();
}

void printTwice(const Vector& l)
{
    l.printVector();
    l.printVector();
}

// ^ But yuck. Prefer genericity:

template<class Container>
void printTwice(const Container& c)
{
    c.print();
    c.print();
}

// ^ In fact, prefer to use non-member functions for anything that
// doesn't require special access to the innards of the class:

template<class Container>
void printTwice(const Container& c)
{
    print(c);
    print(c);
}

// ^ In fact, prefer to spell the verb "print" in the standard way[*]:

template<class Container>
void printTwice(const Container& c)
{
    std::cout << c << std::endl;
    std::cout << c << std::endl;
}

[* – unless you're really concerned about efficiency, in which case you should probably stay away from iostreams; but at that point you're not doing generic programming anymore anyway]


Wrapping postback functions to call custom code at execution Here is my solution for this stackoverflow question.

It is designed to return an onbeforeunload message when the user leaves the page (excluding postbacks). It does this by logging a timestamp each time a .NET control causes a postback (either full or partial using update panels). When this happens, if allowedWaitTime has elapsed, amessage will be returned.

Are there any best or better practices missing from this code and does anyone have an opinion on my style (albeit a small sample)?

(function ($) {

    if (typeof $ !== 'function') {
        throw new Error('jQuery required');
    }

    /* The time in milliseconds to allow between a function call and
       showing the onbeforeunload message. */
    var allowedWaitTime = 100,

        timeStamp = new Date().getTime(),

        // Each function to override
        baseFuncs = {
            __doPostBack: this.__doPostBack,
            WebForm_DoPostBackWithOptions: this.WebForm_DoPostBackWithOptions
        };

    // Set timeStamp when each baseFunc is called
    for (var baseFunc in baseFuncs) {
        (function (func) {
            this[func] = function () {
                var baseFunc = baseFuncs[func];
                timeStamp = new Date().getTime();
                if (typeof baseFunc === 'function') {
                    baseFunc.apply(arguments.callee, arguments);
                }
            }
        })(baseFunc);
    }

    /* Form submit buttons don't call __doPostBack so we'll set timeStamp
       manually on click. */
    $('input[type="submit"]').click(function () {
        timeStamp = new Date().getTime();
    });

    $(this).on('beforeunload', function (e) {

        // Only return string if allowedWaitTime has elapsed
        if (e.timeStamp - timeStamp > allowedWaitTime) {
            return 'message';
        }
    });
}).call(window, jQuery);

Interesting question,

I think the biggest thing is that callee is going the way of the Dodo bird.

JsHint does not like your code because you are using for( .. in .. ) without filtering properties, which is fine since you create the object with Object Notation, and I assume you did not modify the Object prototype. You are also creating functions/closures in a loop. Are you sure you need those closures there?

Other than well commented, and easy to follow, something that I would not mind using in one of my projects.


C++ bare getter setter with dispatch I am trying to achieve a getter/setter behaviour that will reduce code in C++. The interface I would like to achieve is for example:

MyClass a;
a.velocity = 15; // Will dispatch a member function of MyClass responsible for setting "velocity".
someFloatFunc( a.velocity ); // Will implicitly convert to the argument type of someFloatFunc if necessary

I have come up with the following. Suggestions, comments, gotchas, are welcomed.

#include <iostream>
#include <functional>

#include <cassert>

using namespace std;

template <class T>
class encapsulate
{
public:
    typedef T                               value_type;
    typedef std::function<void( const T& )> setter_type;

    // Disable things like auto b = a.v;. Access value like: int b = a.v;
    encapsulate( const encapsulate& )=delete;

    encapsulate( T&in, setter_type fin ):
        v( in ),
        f(fin)
    {

    }

    encapsulate &operator= ( const T&o )
    {
        f( o );
        return *this;
    }

    operator T() const { return v; }

private:
    T           &v;
    setter_type f;
};

#define GETSET( TYPE, VARPUBLIC, VARPRIVATE, CLASS, SETTER ) \
    encapsulate<TYPE> VARPUBLIC {VARPRIVATE, std::bind(&CLASS::SETTER,this,std::placeholders::_1 ) };\
    private:  \
    TYPE VARPRIVATE; \
    public:

#define GETSETDEFAULT( TYPE, VARPUBLIC, VARPRIVATE, CLASS, SETTER, DEF ) \
    encapsulate<TYPE> VARPUBLIC {VARPRIVATE, std::bind(&CLASS::SETTER,this,std::placeholders::_1 ) };\
    private:  \
    TYPE VARPRIVATE = DEF; \
    public:

template <class T>
class A
{
public:

    GETSET( T, v, v_, A, setValue )
    void setValue( const T &other )
    {
        cout << "Setting value..." << endl;
        v_ = other;
    }

    GETSETDEFAULT( double, g, g_, A, setGrayLevel, 0 )
    void setGrayLevel( const double &gl )
    {
        if ( gl < 0 )
        {
            g_ = 0;
            return;
        }

        if ( gl > 1 )
        {
            g_ = 1;
            return;
        }

        g_ = gl;
    }
};

const double testval = 215.33;

//Implicit conversion test
void printInt( const int &c )
{
    std::cout << "PrintInt: " << c << std::endl;
    assert( c == int(testval));
}

int main()
{
    A<double> a;

    a.v = testval;
    std::cout << "a.v: " << a.v << std::endl;

    //auto b = a.v; // Disallowed
    double b = a.v;

    assert( a.v == b);

    b= testval-22;
    assert( a.v != b);

    printInt( a.v );

    return 0;
}

A big issue with your solution is that you have to repeat yourself a lot, even when you have a macro to help you. I've come up with an alternative that doesn't use macros at all, and reduces the amount of repetition, although it still is more verbose that I would like. It uses the following class, which is similar to your class encapsulate, except that it doesn't store a pointer to a setter function, but instead has virtual getter and setter functions that can be overridden in a derived class:

template<typename T>
class property
{
protected:
    T v;

    virtual const T &get(void) const {
        return v;
    }

    virtual void set(const T &o) {
        v = o;
    }

public:
    property() {}
    property(const T &v_) { set(v_); }
    property &operator=(const int &o) { set(o); return *this; }
    operator T() const { return get(); }
};

So now you can declare and use a property variable this way:

property<int> foo;
foo = 42;

When you assign a value to foo, it will call operator=(), which in turn will call the function set(). The trick is that since this is a virtual function, you can create a class or struct that inherits property<int>, and override the set() function. You can make use of the fact that you can declare a class or struct within another class or struct, and that you don't have to give them a name. It looks a bit weird at first, but it looks like this:

struct: property<int> {      // declare a nameless struct that inherits from property<>
    void set(const int &o) { // override the set() function
        ...;
    }
} bar;                       // declare a variable named foo with this type

The main problem is that while a bare property variable works perfectly fine:

property<int> foo;
foo = 42;

The moment you inherit from it, the derived class has an implicitly defined assignment operator which hides the one in the base class. To get the one from the base class back, you have to add using property::operator=; to the body of the derived class. The same goes for the constructor that takes a value.

Here is your class A converted to use class property:

template<typename T>
class A
{
public:
    struct: property<T> {
        using property::operator=; // required for assigning

        void set(const T &other) {
            std::cout << "Setting value..." << endl;
            property::set(other);
        }
    } v;

    struct: property<double> {
        using property::operator=; // required for assigning
        using property::property;  // required for initialization to explicit value

        void set(const T &gl) {
            if (gl < 0)
               property::set(0);
            else if (gl > 1)
               property::set(1);
            else
               property::set(gl);
        }
    } g = 0;
};

Another option that avoids having to add the using statements is to make the derived property struct hidden, and add a base property reference to it that is public, like so:

class example {
    struct: property<int> {
        void set(const int &o) { ... }
    } foo_;

public:
    property<int> &foo = foo_;
};

When assigning a value to foo, it doesn't know it's actually a derived class, so it will directly call operator= from the base class, which is what we want.

Perhaps the above class property can be combined with a macro that hides the verbosity. For example:

#define PROPERTY(type, setter) \
struct: property<type> { \
    using property::property; \
    using property::operator=; \
    void set(const T &value) { property::set(setter(value)); } \
}

This way, you can provide the name of a function that acts as the setter, or even a lambda. For example:

class B {
    static float invert(float x) {
        return -x;
    }

public:
    PROPERTY(float, invert) foo;
    PROPERTY(int, [](int v){ return v * v; }) bar = 0;
};

The drawback of this macro is that you can't easily use non-static member functions as getters and setters.