Handling DB transaction in closure I think DB transaction should be as small as possible, and close it soon.

So I wrote a helper function like 'using' statement in C#.

function transaction($conn, $closure) {
    $conn->beginTransaction();
    try {
        $closure($conn);
        $conn->commit();
    } catch (Exception $e) {
        $conn->rollback();
        throw $e;
    }
}

$conn = new PDO('mysql:....');
$conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
transaction($conn, function($conn) {
    $conn->query('insert ....');
    $conn->query('insert ....');
});

but with this pattern:

I have to capture manually any variables with use()
Stack trace will be bit complicated

or, should I write try/catch and begin/commit in every transaction?

There's a couple of things I'd recommend you change about your current code, and there's an alternative to your using a closure's use construct to pass in other variables.

The first thing I'd do, is to change your function's signatures to type-hint the expected types for each argument, ie change the transaction function to look something more like this:

function transaction(PDO $conn, callable $closure)
{}

Optionally, replacing the callable type-hint with Closure. The upside of callable is that old-school callable constructs will be accepted, too (array($object, 'someMethod') or even ucfirst), the downside is, of course that functions like ucfirst are accepted, but clearly not a valid function in this context.

Anyway, apart from using use to pass in additional variables, you could add a third argument to the transaction function, and type-hint an array:

function transaction(PDO $conn, callable $closure, array $arguments = [])
{}

Then, simply invoke the $closure argument using call_user_func_array to pass the arguments that need to be passed:

array_unshift($conn, $arguments);//prepend $conn to the arguments
call_user_func_array($closure, $arguments);

And you're there... Of course, if you find that array_unshift call clumsy looking (which it kind of is), you can just assume the $arguments array to contain the PDO connection from the off, in which case:

function transaction(callable $callback, array $arguments)
{
    if (!$arguments[0] instanceof PDO) {
        throw new InvalidArgumentException('arguments should contain PDO instance');
    }
    $conn = $arguments[0];
    try {
        $conn->beginTransaction();
        call_user_func_array($callback, $arguments);
        $conn->commit();
    } catch (Exception $e) {
        $conn->rollBack();
        throw $e;
    }
}

Again, there is a downside to this approach, the main disadvantages, IMO, are:

Overhead (a PHP closure is syntactic sugar, it's actually creating a new instance of the Closure class.
The $arguments array will often be a bit too complex for its own good, leaving you with code that actually calls your transaction function to decide on the actual logic -> every call can look different, and often will contain SQL strings, fetch calls and the like. Ugly code is harder to maintain...
Like you said: the stack trace looses some of its relevance, making debugging harder.

The last two issues (code that is harder to maintain + harder to debug) are, to me, severe problems, so much so, that I'd recommend not to use the transaction function, but instead write the try-catch blocks where and when I need them.


Program to check if in line are doubled words I made program to open a text file and count the number of lines without doubled words. This number is printed in the end.

This is an example text file:

sayndz zfxlkl attjtww cti sokkmty brx fhh suelqbp
xmuf znkhaes pggrlp zia znkhaes znkhaes
nti rxr bogebb zdwrin
sryookh unrudn zrkz jxhrdo zrkz
bssqn wbmdc rigc zketu ketichh enkixg bmdwc stnsdf jnz mqovwg ixgken

I've already made a program, and it looks like that program works. But I'm aware that in programming if something works it doesn't mean that program is made properly.

My code:

class SkyphrasesValidation(object):
    def get_text_file(self):
        file = open('C:/Users/PC/Documents/skychallenge_skyphrase_input.txt', 'r')
        return file

    def lines_list(self):
        text = self.get_text_file()
        line_list = text.readlines()
        return [line.split() for line in line_list]

    def phrases_validation(self):
        validated_phrases = 0
        for line in self.lines_list():
            new_line = []
            for word in line:
                exam = line.count(word)
                if exam > 1:
                    new_line.append(0)
                else:
                    new_line.append(1)
            if 0 in new_line:
                validated_phrases += 0
            else:
                validated_phrases += 1
        return validated_phrases

    def __str__(self):
        return str(self.phrases_validation())

text = SkyphrasesValidation()

print(text)

Is my logic good and is this program is well-made? Or maybe it looks like poop and I could write this more clearly?

Here are some suggestions:

Keep file handlers in one function.

First off, the get_text_file() and lines_list() methods:

def get_text_file(self):
    file = open('C:/Users/PC/Documents/skychallenge_skyphrase_input.txt', 'r')
    return file

def lines_list(self):
    text = self.get_text_file()
    line_list = text.readlines()
    return [line.split() for line in line_list]

It's generally a bad idea to pass files outside of a function... it poses some questions: Who will close the file? When? And how? Instead, open and close it in the same function so that the file can be contained.
Since the lines_list() method is only reading text off from get_text_file() (and nothing else), it may be better to simply write one function which reads handles and reads lines off the file. (The splitting of lines, line.split(), can be done in phrases_validation().)

With these points in mind, we could define get_text_lines() as:

def get_text_lines(self):
    with open('C:/Users/PC/Documents/skychallenge_skyphrase_input.txt', 'r') as file:
        line_list = file.readlines()

    return line_list

Notice that the use of the with statement. This automatically closes the file when the block-scope is exited.

Using the above function implies modifying phrases_validation():

def phrases_validation(self):
    # ...
    for line in get_text_lines():   # call function here
        # ...
        words = line.split()        # split lines here
        for word in words:          # iterate through split line here
            # ...

The Validation Algorithm

I've tried annotating the meat of your phrases_validation:

# iterate through lines...
for line in get_text_lines():

    new_line = []  # container to store 1s and 0s

    # iterate through words...
    words = line.split()
    for word in words:

        exam = line.count(word)

        # append 0 if word is duplicated
        if exam > 1:
            new_line.append(0)

        # append 1 if word is not duplicated
        else:
            new_line.append(1)

    # increment validated_phrases if no duplicates
    if 0 in new_line:
        validated_phrases += 0
    else:
        validated_phrases += 1

Some general suggestions:

Opt for readability. Since only 0 and 1 are being used, this implies a binary domain, and thus can be substituted with False and True respectively.
validated_phrases += 0 This can be removed since nothing is added. Simply do

if 0 not in new_line:
    validated_phrases += 1

Some possible improvements to the algorithm:

Using the any built-in function.

The any built-in function takes a list and tests for truth. If any of the elements are Truthy, any returns True, else False. Your inner loop can make use of the any function:

for line in get_text_lines():
    words = line.split()

    has_duplicates = any(words.count(word) > 1 for word in words)
    if not has_duplicates:
        validated_phrases += 1

Here, a comprehension was used: words.count(word) > 1 for word in words.

This generates an iterable of True/False values: True if words.count(word) > 1 and False otherwise. We then check whether there are any duplicates and if not, increment validated_phrases.

(To keep track of the number of duplicates, you can use the sum function instead. This will add the True values, counting the number of duplicates.)

Using the set class.

Indeed, there are a couple more ways to check for duplicates. list.count has a time-complexity with an upper-bound of \$O(n)\$ (with \$n\$ being the length of the list), i.e. it searches the entire list, which may be slow when the list grows to sizes of thousands or millions.

One other way to check for duplicates would be to obtain a container of unique words, and check whether it has the same length as the list of words in the line. We can use set to obtain the unique words.

for line in get_text_lines():
    words = line.split()

    unique_words = set(words)
    has_duplicates = len(unique_words) != len(words)
    if not has_duplicates:
        validated_phrases += 1,

I'm not entirely sure about the complexity of set(words) but the len function has a time complexity of \$O(1)\$ which can be really fast in the long run.

Using the collections.Counter class.

Another way that might be of interest would be to use Counter from the collections module.

for line in get_text_lines():
    words = line.split()

    counter = Counter(words)
    has_duplicates = any(counter[word] != 1 for word in counter)
    if not has_duplicates:
        validated_phrases += 1

(For this, remember to add from collections import Counter at the top of your script.)

Try not to rely on __str__.

In your code, the main algorithm, phrases_validation(), is only called when str(text) or text__str__() is called (this is implicitly called by print(text)).

It's creative... but prefer to directly call phrases_validation():

validator = SkyphrasesValidation()
print(validator.phrases_validation())

(And call str() on it if needed.) This is more explicit to readers that you're calling the phrases_validation() method.

Generally, the __str__ magic method should be overloaded when you're trying to display a class' members contents. Unfortunately, SkyphrasesValidation doesn't have any members.

The variable name text can be interpreted in several ways. Is it a text file? A string?  Consider a more descriptive name instead. (validator was used in the example above.)

Class?

The class could have been substituted with one/two functions. But sticking with a class... since the self argument is rarely used, consider making the methods static or instead, place get_text_lines() outside of the class and pass the lines of text to phrases_validation() as an argument.

Hope this helps!


Using Redis to store word occurences I am doing a project where I am analyzing a group of Tweets.

What I need to do is find the occurrence of each individual word. I am fairly new to Redis, so I am not quite sure that my solution is optimal. I am storing a key value pair for each word. The key is a word, and the value is the occurrence count. I then use a list to keep track of every word that goes into the cache.

$cache = new Redis;
$words = array('foo', 'bar', 'baz');

foreach ($words as $word) {
    if ($cache->exists($word) {
        $cache->incr($word);
    } else {
        $cache->put($word, 1);
        $cache->lpush('words', $word);
    }
}

$data = array();
$list = $cache->lrange('words', 0, -1);

foreach ($list as $word) {
    $data[$word] = $cache->get($word);
}

asort($data);

Your code is alright, really. However, by your using redis, I suspect you're after a fast way to store tiny bits of data. The varname $cache somewhat confirms my suspicion.
If  this is the case, your using lrange might not be the best choice, nor is exists really meaningful: INCR creates a new key, if required, and it returns the new value of that key in every case, so you can ditch the use of put, and simplify your first loop to:

foreach ($words as $word)
{
    if ($cache->incr($word) == 1)
        $cache->lpush('words', $word);
}

Both these functions are atomic, so I don't think you can much improve on that.

lrange has a time-complexity of O(S+N) according to the documentation. Given that you're just fetching the entire words collection, I'd opt for the O(n) alternative SMEMBERS.
If you are going to use lrange, if you want to use pagination, for exampe, I'd consider using SCARD first, to determine the total number of members of your words collection.

Anyway, here's how I'd write the second loop:

$list = $cache->smembers('words');
foreach ($list as $word)
{
    $data[$word] = $cache->get($word);
}

I'll look into using redis itself a bit more, because it's quite easy to do away with that words collection, too:

foreach ($words as $word)
    $cache->incr($word);
$data = array();
$list = $cache->keys('*');//get all keys in collection
foreach ($list as $word)
    $data[$word] = $cache->get($word);

But this would imply the entire database contains nothing but words, which isn't what you want, probably. If I find the time, I'll attempt to put together a function or script that simply iterates over that words collection, and performs something like this

EVAL "return {KEYS[1], redis.call('get', KEYS[1])}" 1 <a word>

for each word, returning a series of redis objects that look like this:

1) <a word>
2) 123

where the 2) actually is the occurance count

Unrelated
I notice you're constructing an object of the Redis class. This, to me, tells me you're using a library other than the official one. The PRedis class was, last time I checked, the preferred one. Perhaps consider changing to that lib, and see if it works for you. Anyway, look into the libs with a star next to their name :)


Mutually exclusive expandable panels (needs to work in IE9 and above) I have a function which essentially opens and closes content associated to a panel. Only one panel can be open at any time.

Here is my JavaScript, along with a typical HTML sample that it could work with:

ts = {};

// Panels
ts.panel = (function() {

  function panels() {

    'use strict';

      var panel   = document.querySelectorAll('.panels-item');
      var button  = document.querySelectorAll('.panels-button');
      var content = document.querySelectorAll('.panels-content');

      // Reset all states
      function reset() {

        for (var i = 0; i < panel.length; i++) {
          button[i].setAttribute('aria-expanded', false);
          content[i].setAttribute('aria-hidden', true);
          content[i].className = 'panels-content js-hidden';
        }

      }

      Array.prototype.forEach.call(panel, function(el, i) {

        var button  = el.querySelector('.panels-button');
        var content = el.querySelector('.panels-content');

        // Buttons
        button.setAttribute('aria-expanded', false);
        button.setAttribute('aria-controls', content.getAttribute('id'));

        // Content
        content.setAttribute('aria-hidden', true);

        function togglePanel() {

          if (content.getAttribute('aria-hidden') === 'true') {

            reset();

            button.setAttribute('aria-expanded', true);
            content.setAttribute('aria-hidden', false);
            content.className = 'panels-content';

          } else {

            button.setAttribute('aria-expanded', false);
            content.setAttribute('aria-hidden', true);
            content.className = 'panels-content js-hidden';

          }

        }

        // Hook up the event listener
        button.addEventListener('click', togglePanel);

      });

    }

    return {
      panels: panels
    };

})();

ts.panel.panels();
* {
  margin: 0;
  padding: 0;
}

.panels {
  display: flex;
  flex-wrap: wrap;
}

.panels-item {
  border-radius: 24px;
  display: flex;
  flex-basis: 25%;
  flex-direction: column;
}

.panels-button {
  background-color: blue;
  border-radius: 24px;
  color: white;
  padding: 24px;
}

.panels-content {
  background-color: #ccc;
  padding: 24px;

}

.panels-title {
  color: blue;
  text-decoration: underline;
}

.js-hidden {
  display: none;
}

  <button class="panels-button">1</button>

1

  <button class="panels-button">2</button>

2

  <button class="panels-button">3</button>

3

  <button class="panels-button">4</button>

4

It does work, but I'm not confident with it. It would be good to have some suggestions.

Overall, the code looks pretty good. Definitely much better than many other JS examples in CodeReview. A few minor points can be made though.

DRY - Don't repeat yourself

At the very minimum, you can reuse some code by encapsulating it into a function.

You can define setPanelVisibilityState which will be responsible for the details of showing/hiding a section. The decision on whether a button/panel should be visible or not remains in the control of the caller...

Variable naming

Similarly, I'm unsure if the selectors '.panels-button' and '.panels-content' more than in one place...

I personally am a big enemy of variables like i, j, c, and so on. It's not so difficult to spell it out: panelIndex.

IE is a beast

As far as IE page renderer goes, I can't provide much feedback because I don't deal with it.

function togglePanel() {
    const showContent = content.getAttribute('aria-hidden') === 'true';
    reset();
    setPanelVisibilityState(false, button, content);
}

function reset() {
    for (var panelIndex = 0; panelIndex < panel.length; panelIndex++)
        setPanelVisibilityState(false, button[panelIndex], content[panelIndex]);
}

function setPanelVisibilityState(isVisible, button, content) {
    button.setAttribute('aria-expanded', isVisible);
    content.setAttribute('aria-hidden', !isVisible);
    content.className = isVisible ? 'panels-content' : 'panels-content js-hidden';
}


Factorial using trampolines I've translated this Java code into Delphi code:

unit Trampoline;

interface

type
  ITrampoline<T> = interface
    function Get: T;
    function Run: ITrampoline<T>;
    function Execute: T;
  end;

  TTrampoline<T> = class(TInterfacedObject, ITrampoline<T>)
  public
    function Get: T; virtual;
    function Run: ITrampoline<T>; virtual;
    function Execute: T; virtual; final;
  end;

  TFactorial = class(TTrampoline<integer>)
  private
    fsum, fn: integer;
  public
    constructor Create(n, sum: integer);
    class function Factorial(n, sum: integer): ITrampoline<Integer>; static;
    function run: ITrampoline<Integer>; override;
  end;

implementation

uses System.Generics.Defaults;

function TTrampoline<T>.Run: ITrampoline<T>;
begin
  Result:= nil;
end;

function TTrampoline<T>.Get: T;
begin
  Result:= default (T);
end;

function TTrampoline<T>.Execute: T;
var
  Comparer: IEqualityComparer<T>;
  Trampoline: ITrampoline<T>;
begin
  Trampoline:= Self;
  Comparer:= TEqualityComparer<T>.Default;
  while Comparer.Equals(Trampoline.get, default (T)) do begin
    Trampoline:= Trampoline.run;
  end;
  Result:= Trampoline.get;
end;

type
  TDoneFactorial = class(TFactorial)
    function get: integer; override;
  end;

constructor TFactorial.Create(n, sum: integer);
begin
  fn:= n;
  fsum:= sum;
end;

class function TFactorial.Factorial(n, sum: integer): ITrampoline<Integer>;
begin
  if (n <= 1) then begin
    Result:= TDoneFactorial.Create(n, sum);
  end else begin
    Result:= TFactorial.Create(n, sum);
  end;
end;

function TDoneFactorial.get: integer;
begin
  Result:= fsum;
end;

function TFactorial.run: ITrampoline<Integer>;
begin
  Result:= Factorial(fn - 1, fsum * fn);
end;

end.

Usage:

procedure TForm5.Button1Click(Sender: TObject);
var
  Outcome: Integer;
begin
  Outcome:= TFactorial.Factorial(StrToInt(Edit1.Text), 1).Execute;
  Label2.Caption:= IntToStr(OutCome);
end;

It runs nicely, in all the iterations of the execute method the stack pointer does not move and hence it will not generate a stack overflow no matter how many deep the recursion goes.

It is rather inefficient though because of:

The implicit try - finally guarding the interface variable
The constant creation of new structures on the heap.

Is there a more efficient way to do this?

Never mind the fact that factorials are a stupid example for recursion.  I know recursion is only really useful with trees.

Recursion can make for elegant code, but it can also make for very slow code. Calculating Fibonacci numbers and Factorials are both classic examples where the recursive algorithm is simple to implement, but slow beyond the first few (and very small) input values.

I don't know Delphi, so I quickly implemented this in VB6, but you should be able to consider it psuedo-code.

Function Factorial(x As Integer) As Long
    Dim result As Long
    Dim startIndex As Integer

    result = 1
    For startIndex = x To 1 Step -1
        result = result * startIndex
    Next

    Factorial = result
End Function


Getting the index of a character within the alphabet I've made this function to map alphabetic coordinates to their corresponding ordinal number.

var exec = document.querySelector('#exec');

// Maps alphabetic characters to their index
//   within the alphabet.
// --------------------------------------------
// @param { string || array } chars - String
//  consisting of a single alphabetic character
//  or an array which elements are single
//  alphabetic characters.
// @throws Error in case of invalid
//  parameter ( == not a single alphabetic
//  character ).
// --------------------------------------------
// @returns { array } with numbers. The indexes
//  of the characters within the alphabet.
//
// ------- Usage examples ----------
// getIndexInAlphabet('i')); // Result : [9]
// getIndexInAlphabet(['a', 'B', 'c'])[1] // 2
// getIndexInAlphabet(['a', 'G', 'z'])); // [1, 7, 26]

function getIndexInAlphabet( chars ) {
  var alphabet = [ 'a', 'b', 'c', 'd', 'e',
                   'f', 'g', 'h', 'i', 'j',
                   'k', 'l', 'm', 'n', 'o',
                   'p', 'q', 'r', 's', 't',
                   'u', 'v', 'w', 'x', 'y',
                   'z' ];

  if (!Array.isArray(chars)) {
    if (typeof chars === 'string') {
      let tmp = [];
      tmp.push(chars);

      chars = tmp;
    } else {
      throw new Error(
        'Parameter invalid because not of type string.');
    }
  }

  chars.forEach(function(item, i) {
    if (typeof item !== 'string') {
      throw new Error('Element ' + i +
                      ' invalid because not of type string.');
    }
  });

  return chars.map(function(char, i) {
    var ret = alphabet.indexOf(char.toLowerCase()) + 1;

    if (ret === 0) {
      throw new Error('Element ' + i + ' invalid because' +
                      ' not an alphabetic character.');
    }

    return ret;
  });
}

// -- From here on : Just testing ...
exec.addEventListener('click', function() {
  try {
    console.log(getIndexInAlphabet(['a', 'B', 'c'])[1])
    console.log(getIndexInAlphabet('i'));
    console.log(getIndexInAlphabet(['a', 'G', 'z']));

    var charStr = ['a', 'b', 'c', 'd', 'e', 'f'];
    var indexes = getIndexInAlphabet(charStr);
    var charMap = {};

    charStr.forEach(function(char, i) {
      charMap[char] = indexes[i];
    });

    console.log(charMap.f);
  } catch (e) {
    console.log(e.message);
    console.log(e.stack);
  }
});
.wrap {
  width: 800px;
  margin: 50px auto;
}

    Exec!

The alphabet would be simpler if it were written as a string, and it should be const.  I'd also name it ALPHABET to emphasize that it is constant.

Using three statements to stuff a string into an array is too cumbersome.  In my opinion, there is not much value in verifying that a non-array parameter is a string, since you'll validate each element of the array anyway.  Does it really matter that the error message is slightly different?

The validation of each array element can be rolled into the map callback.  In keeping with JavaScript's duck-typing philosophy, I would check that it has a .toLowerCase() method, rather than that it is of type string.

I find it weird that the function returns an array even if the input is a single character.  I suggest renaming the function to getIndexesInAlphabet to make it clear that it returns an array.

function getIndexesInAlphabet(chars) {
  const ALPHABET = 'abcdefghijklmnopqrstuvwxyz'.split('');

  if (!Array.isArray(chars)) {
    chars = [chars];
  }

  return chars.map(function(char, i) {
    if (!char.toLowerCase) {
      throw new Error('Element ' + i +
                      ' invalid because it is not a string.');
    }

    var ret = ALPHABET.indexOf(char.toLowerCase()) + 1;

    if (ret === 0) {
      throw new Error('Element ' + i + ' invalid because' +
                      ' not an alphabetic character.');
    }

    return ret;
  });
}

Also consider whether you really want to accept arrays at all.  Unless you plan to be able to support spreadsheet-style coordinates (e.g. AA to represent column 27), every coordinate will be a single character.  You could rationalize the interface by accepting only strings:

// Maps alphabetic characters to their 1-based index within the alphabet.
// --------------------------------------------
// @param { string } chars - String
//  consisting of a one or more alphabetic characters.
// @throws Error if the input contains a non-alphabetic character.
// --------------------------------------------
// @returns { array } with 1-based indexes. The indexes
//  of the characters within the alphabet.
//
// ------- Usage examples ----------
// getIndexesInAlphabet('i'); // Result : [9]
// getIndexesInAlphabet('aBc')[1] // 2
// getIndexesInAlphabet('aGz'); // [1, 7, 26]
function getIndexesInAlphabet(chars) {
  const ALPHABET = 'abcdefghijklmnopqrstuvwxyz';

  return chars.split('').map(function(char, i) {
    var index = ALPHABET.indexOf(char.toLowerCase());
    if (index < 0) {
      throw new Error(char + 'is not a valid alphabetic character.');
    }
    return index + 1;
  });
}

If what is passed is not a string, it would be standard JavaScript programming practice not to bother trying to make sense of it or throwing a custom error.


Square root image filter tool in Python I have this short Python script for making images brighter:

import skimage.io
import math
import sys

def square_root_filter(input_image_file_name, output_image_file_name):
        image_data = skimage.io.imread(input_image_file_name)
        image_data_height = len(image_data)
        image_data_width  = len(image_data[0])

        for row_index in range(image_data_height):
                for column_index in range(image_data_width):
                        channel = image_data[row_index][column_index]

                        r = float(channel[0])
                        g = float(channel[1])
                        b = float(channel[2])

                        r /= 255.0
                        g /= 255.0
                        b /= 255.0

                        r = math.sqrt(r)
                        g = math.sqrt(g)
                        b = math.sqrt(b)

                        channel[0] = int(255 * r)
                        channel[1] = int(255 * g)
                        channel[2] = int(255 * b)

        skimage.io.imsave(output_image_file_name, image_data)

def main():
        square_root_filter(sys.argv[1], sys.argv[2])

if __name__ == "__main__":
        main()

Output

(I tested it only with Python 3.5. Also, it will take some time to do its work.)

I am not a professional Python programmer, so please tell me anything that will make the code better.

PEP 8 specifies four spaces per level of indentation.  Since whitespace matters in Python, this is a pretty strong convention.

You should round() the results to the nearest integer rather than truncating them towards 0.

Each of the three channels is treated identically and independently, so you shouldn't write the same code three times.

def square_root_filter(input_image_file_name, output_image_file_name):
    image_data = skimage.io.imread(input_image_file_name)
    for row_data in image_data:
        for pixel in row_data:
            for channel in range(3):
                pixel[channel] = round(255 * math.sqrt(pixel[channel] / 255))
    skimage.io.imsave(output_image_file_name, image_data)

If you take advantage of the fact that channels is a NumPy array, you can vectorize the calculation.

import numpy as np
import skimage.io

def square_root_filter(input_image_file_name, output_image_file_name):
    image_data = skimage.io.imread(input_image_file_name)
    for row_data in image_data:
        for pixel in row_data:
            pixel[:] = np.rint(255 * (pixel / 255) ** 0.5)
    skimage.io.imsave(output_image_file_name, image_data)


Argument-parsing class in Python to handle a --force flag I am relatively new to Python. I've so far mostly worked with PHP and C#, so most of it is relatively familiar.

I'm making a simple console application that will store a few API calls in a local database through a cronjob. It will have a timeout, so if the program is run too often it won't spam the API and use up all of my free access tokens.

Of course this begs for my favourite flag in any console application: --force. If something doesn't work, try forcing it, right?

Now, my first attempt at adding this was using sys.argv[n] like I would in PHP, though I read that's a rather insecure solution, which could also produce nasty bugs.

So, wanting to better my Python OOP knowledge as well as add a better implementation of CLI arguments, I've now written this:

import argparse

class Arguments:

    def __init__(self):
        self.argument_parser = argparse.ArgumentParser()
        self.argument_parser.add_argument(
            '--force',
            default=False,
            help="Force the program to run, even if the timer returns False.",
            action="store_true"
        )
        self.parsed = self.argument_parser.parse_args()

    @property
    def force_run(self):
        return self.parsed.force

Is this the right way to use classes in Python, and this is a "better" implementation of CLI arguments?

Sidenote: This was written with the assumption I'll be adding more flags in the future, all of which are accessed through the Arguments class.

In my opinion, no, this isn't any better than just making use of the tools that argparse provides out of the box and a class is overkill. argparse.ArgumentParser operates in a very similar way, so it should be just as easy to call it straight-up:

import argparse

parser = argparse.ArgumentParser()
parser.add_argument(
    "--force",
    default=False,
    help="Force the program to run, even if the timer returns False.",
    action="store_true"
)
args = parser.parse_args()
# ...
if args.force:
    # ...


Merge multiple pandas dataframe along with np.where to process values I have a pandas dataframe, df1:
id  level  val
1   l1     0.2
1   l2     0.1

and another dataframe which contains type for l1 level, df2:
id  type
1   A

Similarly, df3 which contains type for l2 level as well as new ids:
id  new_id  type
1   19      A
1   19      B
1   20      A
1   20      B

Now I want type for l1 level from df2. And for l2 level from df3, while also resetting ids and level. This is what I am doing:
df2['level'] = 'l1'
df3['level'] = 'l2'
df1 = pd.merge(df1, df2, how='outer', on=['id','level'])
df1 = pd.merge(df1, df3, how='outer', on=['id','level'], suffixes=["_", ""])
df1['id'] = np.where(df1['new_id'].isnull(), df1['id'], df1['new_id'])
df1['type'] = np.where(df1['type'].isnull(), df1['type_'], df1['type'])
df1['level'] = np.where(df1['level']=='l2', 'l3', df1['level'])
df1 = df1.drop('new_id', 1)
df1 = df1.drop('type_', 1)

Which finally gives me, df1:
id  level  val  type
1   l1     0.2  A
19  l3     0.1  A
19  l3     0.1  B
20  l3     0.1  A
20  l3     0.1  B

Since df1 is a large dataframe, I want to avoid using np.where thrice, i.e., I am looking for a faster approach to get this. Thanks!

keyword arguments
I had to check the documentation to find out what the 1 means in df1.drop('new_id', 1). More clear would be df1.drop('new_id', axis=1), or even better: df1.drop(columns=["type_", "new_id"])
Don't overwrite raw data
You add columns and values to df1, which makes finding problems harder.
With a bit of reshuffling, and using df.join instead of pd.merge, you can make your intent a bit more clear
df6= df1.join(
    df2.assign(level="l1").set_index(["id", "level"]),
    how="outer",
    on=["id", "level"],
).join(
    df3.assign(level="l2").set_index(["id", "level"]),
    how="outer",
    on=["id", "level"],
    lsuffix="_",
)

np.where
If you're looking for null, you can use fillna or combine_first. To replace 'l2' with 'l3', you can use Series.replace.
df7 = df6.assign(
    id=df6["new_id"].fillna(df6["id"]).astype(int),
    type=df6["type_"].fillna(df6["type"]),
    level=df6["level"].replace("l2", "l3")
).drop(columns=["type_", "new_id"])

I don't know whether this approach will be faster, but to me, what happens here is a lot more clear.


Correct 4 bytes out of 65K to match a crc32 checksum I have a corrupted png image. I checked and the crc32 checksum of the first chunk (size ~65k) does not match with the one I get. I am sure of this.

Also, I have identified the 4 bytes that have the wrong value. I am sure of this.

My question: Is there an efficient way to calculate the values of these 4 bytes, in order to match the required crc32 checksum?

I made a program in C to brute force the values, using this CRC32 function

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

// The CRC32 function
uint32_t crc32(uint32_t crc, const void *buf, size_t SIZE_CHUNK);

// The size of the corrupted chunk (TYPE+DATA)
uint32_t SIZE_CHUNK = 65449;

// The CRC32 that has to be matched
uint32_t GOOD_CRC32 = 2784191201;

// The offset of the 4 corrupted bytes
uint32_t OFF[4] = {20121, 20467, 20742, 20893};

int main() {
        uint8_t *buf = (uint8_t *)calloc(SIZE_CHUNK, sizeof(uint8_t));
        uint32_t i, j, k, l, crc = 0;
        FILE *fp;

        fp = fopen("corrupted.png", "r+");
        // Offset to the first `IDAT` chunk of the png file
        fseek(fp, 87, SEEK_SET);
        // We fill `but` with the TYPE+DATA fields of the chunk
        fread(buf, sizeof(uint8_t), SIZE_CHUNK, fp);

        // Bruteforce...
        for (i=0; i<256; i++) {
                buf[OFF[0]] = i;
                for (j=0; j<256; j++) {
                        buf[OFF[1]] = j;
                        for (k=0; k<256; k++) {
                                buf[OFF[2]] = k;
                                for (l=0; l<256; l++) {
                                        buf[OFF[3]] = l;
                                        crc = crc32(0, buf, SIZE_CHUNK);
                                        if (crc == GOOD_CRC32)
                                                printf("Values found! i=%d j=%d k=%d l=%d\n", i,j,k,l);
                                }
                        }
                }
        }
        free(buf);
        fclose(fp);
}

However, this is very slow. If it doesn't speed up, it may take several days. It might be due to the size of the chunk (~65Kb).

I thought about pre-compute the CRC (the part from 0 to OFF[0]-1), but it would still be too long.

  Info:

  The exact size of the chunk is 65449. The corrupted bytes positions
  are between 20k-21k.

Disclaimer: It is not a review, and I am wild guessing.

The DrDobbs article outlines how the partial CRC could be recombined. I recommend to precompute CRCs of all fixed parts of the chunk, and let the GPU handle the rest. GPUs are notoriously good in number crunching, and you have at least 16K+ truly parallel cores on your graphics card. It is \$2^{18}\$ search space per core. Seems feasible.


Is it okay to 'abuse' re-assignment? Because I don't like using \ to break long lines (to comply with PEP8), I tend to do something like this:

message = "There are {} seconds in {} hours."
message = message.format(nhours*3600, nhours)
print(message)

It also makes code cleaner. Is this an okay way of doing things?

It's more than okay, it is nicely readable, has no problems and involves no abuse whatsoever. I sometimes do the same.


Generating two .csv files from named parameters I wrote Clojure code which takes named params and has to generate 2 .csv files as output.

Please review it.

(ns my_cli_clojure.core
  (:gen-class :main true))

(require '[clojure.data.csv :as csv]
     '[clojure.java.io :as io]
     '[me.raynes.fs :as fs]
     '[clojure.tools.cli :refer [cli] :as c])

(defn write_csv_data [& {:keys [data outfile]
                     :or {data ""
                          outfile ""}}]
  (csv/write-csv outfile
                data))

(defn -main
  [& args]
    (let [[options args banner]
  (c/cli args
 ["-d" "-dome" "Dome Identifier" :default "dome1"]
 ["-p" "-principal" "Principal code" :default "kraft"]
 ["-o" "-output-directory" "Output file" :default "."]
 ["-h" "-help" "Show Help" :flag true :default false])]

(when (:help options)
  (println banner)
  (System/exit 0))

 (let [output_directory (:output-directory options)]
(if-not (fs/exists? output_directory)
  (fs/mkdirs output_directory))
(doseq [n (clojure.string/split (:dome options) #",")]
  (let [dome_identifier (first (clojure.string/split n #"-"))]
(with-open [out-file (io/writer (clojure.string/join "/" [output_directory
                                                                  (clojure.string/join "." [dome_identifier "csv"])]))]
(write_csv_data :data [["dome_identifier" "name"]
                       [dome_identifier (first (reverse     (clojure.string/split n #"-")))]] :outfile out-file))

(with-open [dome_principal_file (io/writer (clojure.string/join "/" [output_directory
                                                                           (clojure.string/join "." ["dome_principal" dome_identifier "csv"])]))]
 (write_csv_data :data [(clojure.string/split "dome_identifier principal_code currency_code active latitude longitude" #"\s")
                        [dome_identifier (:principal options) "" "t" "" ""]] :outfile dome_principal_file)))))))

Everything looked pretty good in general, until I got to the (let [output_directory ... part, then your indentation got all wonky! Part of the problem is that there are a couple of really long lines, and it's difficult to keep them short when you're nested so many layers deep into an S-expression. Shortening your lines and making sure that everything lines up from line-to-line in an intuitive way will solve this.

First of all, I believe it is idiomatic to include all your use/require/import statements within the namespace definition, like so:

(ns my_cli_clojure.core
  (:require [clojure.data.csv :as csv]
            [clojure.java.io :as io]
            [me.raynes.fs :as fs]
            [clojure.tools.cli :refer [cli] :as c]
            [clojure.string :as s])
  (:gen-class :main true))

Notice that I added [clojure.string :as s] -- this abbrevation will come in handy in shortening some of the later lines, in which you have a lot of fully-qualified references to clojure.string methods.

Next, just an aesthetic spacing tweak here:

(defn write-csv-data [& {:keys [data outfile] :or {data "", outfile ""}}]
  (csv/write-csv outfile data))

Your code was fine, I just find my version easier to read at a glance. I generally try to avoid using line-breaks whenever the code I'm writing looks sufficiently compact on less lines. When the lines start getting long, I will think more about where it would make the most sense to break them up. (I also renamed write_csv_data to write-csv-data; it's idiomatic in Clojure to name symbols using hyphens instead of underscores)

More spacing adjustments here:

(defn -main [& args]
  (let [[options args banner]
        (c/cli args
               ["-d" "-dome" "Dome Identifier" :default "dome1"]
               ["-p" "-principal" "Principal code" :default "kraft"]
               ["-o" "-output-directory" "Output file" :default "."]
               ["-h" "-help" "Show Help" :flag true :default false])]

Here is how I would tackle the part at the end:

(let [out-dir (:output-directory options)]
  (if-not (fs/exists? out-dir)
    (fs/mkdirs out-dir))
  (doseq [n (s/split (:dome options) #",")]
    (let [dome-id (first (s/split n #"-"))]
      (with-open [out-file (io/writer (str out-dir "/" dome-id ".csv"))]
        (write-csv-data :data [["dome_identifier" "name"]
                               [dome-id (last (s/split n #"-"))]]
                        :outfile out-file))
      (with-open [dome-principal-file
                  (io/writer (str out-dir "/dome_principal." dome-id ".csv"))]
        (write-csv-data :data [["dome_identifier" "principal_code"
                                "currency_code" "active" "latitude" "longitude"]
                               [dome-id (:principal options) "" "t" "" ""]]
                        :outfile dome-principal-file)))))))

Changes:

I shortened a few symbol names, like output_directory => out-dir and dome_identifier => dome-id.
I changed the two parts with the pattern like (s/join "/" [out-dir (s/join "." [dome-id "csv"])]) to (str out-dir "/" dome-id ".csv"), which is more concise and easier to read. Keep it simple!
I simplified (first (reverse (s/split n #"-"))) to (last (s/split n #"-")).
There's a part of your code where you created a list of strings by doing something like this: (s/split "first-string second-string third-string fourth-string" #"\s+"). That's clever, but the downside is that it leaves you with a long string that you can't break in order to shorten your lines. In this case, I think it makes more sense just to type e.g. ["first-string" "second-string" "third-string" "fourth-string"].

Hope that helps!


Learning Javascript OOP with a Collapsing Item Class I'm trying to get into object oriented javascript programming. I have written a class for collapsible elements as a test. These elements all have a certain class and no matter how many elements there are on the page, they should all be collapsible.
It works so far, but I don't think it's very elegant yet. For example, I wonder if the function "checkforCollapsibles" doesn't actually belong in the class? But can a class create instances of itself?
But that's probably not the only thing. I would be very happy if you have any suggestions on how to make this even better, cleaner and more elegant ;-)
--- EDIT: I know you don't need a class for collapsing elements :-)
"use strict";

class bb_Collapsible {
  constructor(link, content) {
    this.link = link;
    this.content = content;
    this.content.classList.toggle("bb-collapsed");
    this.link.addEventListener("click", this);
  }
  handleEvent(event) {
    if (event.type === "click") {
      this.switchStatus();
    }
  }
  switchStatus() {
    this.content.classList.toggle("bb-collapsed");
  }
}

function checkForCollapsibles() {
  const collapsibleLinks = document.querySelectorAll(".bb-collapsible");
  const collapsibleContents = document.querySelectorAll(".bb-collapsible-content");

  if (collapsibleLinks.length > 0) {
    collapsibleLinks.forEach((element, index) => {
      new bb_Collapsible(collapsibleLinks[index], collapsibleContents[index]);
    });
  } else {
    console.log("No collapsible Elements");
  }
}
document.addEventListener("DOMContentLoaded", checkForCollapsibles);

EXTENSION
I used this article for event handling. However, I don't quite understand it yet. In the example, only pass this as the callback. I assume javascript interprets this as the eventHandle function of the respective instance. But what if I have different elements that should trigger different functions? In my example, I could only ask which event was triggered and not which element? Or should I then write switches and get it from the respective event object? but that no longer sounds clean and tidy to me.
https://dev.to/rikschennink/the-fantastically-magical-handleevent-function-1bp4

This seems over-engineered. If really all you need is this.content.classList.toggle("bb-collapsed"); then I would simply go for
class Collapsible {
  constructor(link, content) {
    this.link = link;
    this.content = content;
    this.content.classList.toggle("bb-collapsed");
    this.link.addEventListener("click", e => content.classList.toggle("bb-collapsed"));
  }
}

And from there, there really isn't much of a point in having a class. You could do that straight in your checkForCollapsibles.
Something like this
function wireCollapsibles() {
  const links = document.querySelectorAll(".bb-collapsible");
  const contents = document.querySelectorAll(".bb-collapsible-content");

  links.forEach((link, index) => {
    new bb_Collapsible(collapsibleLinks[index], collapsibleContents[index]);
    link.addEventListener(e => contents[i].classList.toggle("bb-collapsed"));
  });
}

Note that I drop the console.log thing, ideally you dont have log statements in your production code.


Optional base class template to get conditional data members In generic code, I sometimes want to conditionally add a data member to a class template. Writing separate template specializations scales as 2^N for N conditional data members. Absent a static_if feature, I have found the following user-defined class templates empty_base<T> and optional_base<T> useful:

namespace xstd {
namespace block_adl {

template<class>
struct empty_base
{
        empty_base() = default;

        template<class... Args>
        constexpr empty_base(Args&&...) {}
};

template<bool Condition, class T>
using optional_base = std::conditional_t<Condition, T, empty_base<T>>;

}       // namespace block_adl

// deriving from empty_base or optional_base will not make xstd an associated namespace
// this prevents ADL from finding overloads for e.g. begin/end/swap from namespace xstd
using block_adl::empty_base;
using block_adl::optional_base;

}       // namespace xstd

Exploiting the empty base optimization then allows users to write class templates that have optional data members (conditional on user-defined template variable traits Trait1_v and Trait2_v)

template<class T>
class Test
:
    public Base0, // unconditional base class
    public xstd::optional_base<Trait1_v<T>, Base1>,
    public xstd::optional_base<Trait2_v<T>, Base2>
{
    using B1 = xstd::optional_base<Trait1_v<T>, Base1>;
    using B2 = xstd::optional_base<Trait2_v<T>, Base2>;
public:
    Test() = default;

    Test(Arg0 const& a0, Arg1 const& a1, Arg2 const& a2)
    :
        Base0(a0),
        B1(a1),
        B2(a2)
    {}
};

Note that the optional_base variadic constructor allows linear scaling of constructor delegation in the user-defined class template Test. One minor wart is that this constructor does require passing all parameters (but otherwise, 2^N different constructors have to be written). But if any of the traits variables evaluates to false, the corresponding parameter and call to the base constructor will be optimized away. And because optional_base also has a trivial default constructor, POD-ness of user-defined classes is preserved by inheriting from it.

Obviously, over-use of inheritance has to be avoided, but the above design does avoid quite a lot of code duplication. Oh, and I haven't tested this with virtual functions, virtual base classes and what-not. I just use this for assembling value-semantic classes out of conditional building blocks.

Question: what other design / usability issues could there be with the above code?

I only have a few comments. First, you probably want to inherit your bases privately:

template<class T>
class Test
:
    private Base0, // unconditional base class
    private xstd::optional_base<Trait1_v<T>, Base1>,
    private xstd::optional_base<Trait2_v<T>, Base2>
{ };

After all, your design is about adding data members. That's something that you probably don't want to expose to the outside world.

Secondly, what happens if you want to potentially have multiple different members of the same type? That's definitely going to come up and so should be supported. In order to do that, you might want to add some kind of ID to differentiate them and then additionally wrap the non-empty case. That is:

template <class, int >
struct empty_base { ... }; // same as before

template <class T, int >
struct wrapped {
    T val;
};

template <bool Condition, class T, int UniqueID = 0>
using optional_base = std::conditional_t<Condition,
                                         wrapped<T, UniqueID>,
                                         empty_base<T, UniqueID>>;

This will additionally let you use non-class types as optional data members (since you couldn't inherit from, e.g. int). So you can now do something like:

template <class T>
class Test
: private xstd::optional_base<Trait1_v<T>, int, 0>
, private xstd::optional_base<Trait2_v<T>, int, 1>
{ ... };

That adds an inconvenience that you have to explicitly add the sequence 0, 1, ...

You can get around that by making a linear inheritance list of all your optional bases. I'm not sure if it's worth it, but I'll present the idea just as an option. At its core, we have OptionalBasesImpl which will inherit from all the optional bases and provide a getter so that the derived class can actually use them:

template <typename... >
struct typelist { };

template <int, typename... >
struct OptionalBasesImpl;

The root case is empty:

template <int ID>
struct OptionalBasesImpl<ID>
{
    OptionalBasesImpl() = default;
    template <typename... Args>
    OptionalBasesImpl(Args&&... ) { }

    void get(typelist<> ) { }
};

And we recurse over each pair of type trait/type:

template <int ID, typename Cond, typename T, typename... Rest>
struct OptionalBasesImpl<ID, Cond, T, Rest...>
: private xstd::optional_base<Cond::value, T, ID>
, OptionalBasesImpl<ID + 1, Rest...>
{
    using Base = xstd::optional_base<Cond::value, T, ID>;
    using Root = OptionalBasesImpl<ID + 1, Rest...>;

    OptionalBasesImpl() = default;

    template <typename Arg, typename... Args>
    OptionalBasesImpl(Arg&& a0, Args&&... args)
    : Base{std::forward<Arg>(a0)}
    , Root(std::forward<Args>(args)...)
    { }

protected:
    using Root::get;

    // by conditional/type pair
    Base& get(typelist<Cond, T> ) { return *this; }
    Base const& get(typelist<Cond, T> ) const { return *this; }

    // by index
    Base& get(std::integral_constant<int, ID> ) { return *this; }
    Base const& get(std::integral_constant<int, ID> ) const { return *this; }
};

Now we just have the top-level class to start us off at 0:

template <typename... T>
struct OptionalBases : OptionalBasesImpl<0, T...> {
protected:
    using OptionalBasesImpl<0, T...>::OptionalBasesImpl;
    using OptionalBasesImpl<0, T...>::get;
};

which lets you write something like:

template <typename T>
struct Test
: private OptionalBases<std::is_integral<T>, int,
                        std::is_same<T, int>, int>
{
    using Bases = OptionalBases<std::is_integral<T>, int,
                        std::is_same<T, int>, int>;
    using Bases::Bases;

    void printFirst() {
        std::cout << Bases::get(typelist<std::is_integral<T>, int>{}).val << std::endl;
    }

    void printSecond() {
        std::cout << Bases::get(std::integral_constant<int, 1>{}).val << std::endl;
    }
};

int main() {
    Test<int> t{1, 2};
    t.printFirst();
    t.printSecond();
}

That's a lot of extra boilerplate for just avoiding writing a sequence of integers, so YMMV.


Are my one-method (and a couple attribute) classes overkill? I saw a talk by Jack Diederich a few weeks ago titled "Stop Writing Classes" and noticed that some of my code perhaps echoes what he argues against (to the effect of "classes with two methods, one of which is __init__") in my code. For some parser that needed to run through a bunch of data, I created a couple classes that implement a single method to extract the salient information and tack it on to the end.

class DataFrameBuilder(object):
    column_order = None
    def __init__(self):
        self.data = []

    def render(self):
        return pd.DataFrame(self.data, columns=self.column_order)

class BoundsBuilder(DataFrameBuilder):
    data_type = 'bounds'
    column_order = ['bid', 'x_min', 'x_max', 'y_min', 'y_max']

    def append(self, bid, blob):
        centroid_x, centroid_y = zip(*blob['centroid'])
        self.data.append({'bid': bid,
                'x_min': min(centroid_x), 'x_max': max(centroid_x),
                'y_min': min(centroid_y), 'y_max': max(centroid_y),
            })
...

def summarize(experiment):
    builders = [BoundsBuilder(), TerminalsBuilder(), SizeBuilder()]

    for bid, blob in experiment.blobs():
        for builder in builders:
            builder.append(bid, blob)

    return {builder.data_type: builder.render() for builder in builders}

I could do the same thing with a couple of dictionaries to store my results and attributes, along with bare functions I pass said dictionary (or value within it), it might even be a few less lines, so I don't know if I'm over-using classes here or not.

My example here is fairly small, but that's why it's here, because they are my smallest classes and perhaps on the edge of usefulness/overkill.

You are correct - in this case classes are an overkill.

Think of classes as nouns, and methods as verbs. Functions do things, while classes are those actual things.

For instance - compare DataFrameBuilder (noun) with build_data_frame (verb). Which makes more sense? Which describes the business logic better? I think the second, since the focus here on building data, and not on a builder itself.

I think I picked up the idea from Steve Yegge's post "Execution in the Kingdom of Nouns" (great article, you should read it when you have time).


How can I tidy up this servlet parameter check code? One of the servlet in my web app takes parameters in the form www.service.com/servletname/parameter1/parameter2.

The first parameter is required and the second parameter is an integer.
I've made some code to validate the parameters but it looks really really messy. Is there a nicer and cleaner way I can write the following:

Thanks :)

protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {

    resp.setContentType(CONTENT_TYPE);
    PrintWriter out = resp.getWriter();
    int maximumResults = defaultMaxResults;

    // check that parameters were provided
    if (req.getPathInfo() == null)
    {
       out.println( JSONHelper.writeStatus(false, Status.TOO_FEW_ARGS.getMessage()));
        return;
    }

    // seperate parameters
    String[] parameters = req.getPathInfo().split(PARAMETER_SEPERATOR);

    if (parameters.length < MIN_NUM_PARAMETERS)
    {
        out.println( JSONHelper.writeStatus(false, Status.TOO_FEW_ARGS.getMessage()));
        return;
    }

    // validate parameters
    if (parameters.length > MIN_NUM_PARAMETERS)
    {
        try
        {
        maximumResults = Integer.parseInt(parameters[2]);
        } catch (NumberFormatException nfe)
        {
        out.println( JSONHelper.writeStatus(false, Status.INVALID_MAX_RESULTS.getMessage()));
        return;
        }
    }

In my opinion you could use the Regex to solve this

protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {

    resp.setContentType(CONTENT_TYPE);
    PrintWriter out = resp.getWriter();
    int maximumResults = defaultMaxResults;

    // you could create pathPattern as a Class field
    Pattern pathPattern = Pattern.compile("http://www.hostname.com/servletname/([^/]+)(/(\\d+))?$");

    Matcher m = pathPattern.matcher(req.getPathInfo());
    if(m.find()) {
         String param1 = m.group(1);
         String param2 = m.group(2); // NOTE param2 may be null
         // TODO your business here
         return;
    } else {
         // TODO throw your exception here
    }

}

You can change the pathPattern to meet your need. Hope this help. Feel free to response.


Single price grid component (HTML & CSS Project) I would appreciate some feedback on this single price grid component project completed using HTML and CSS.
I am trying to recreate the content shown by this image: https://res.cloudinary.com/practicaldev/image/fetch/s--FzGw2rbZ--/c_limit%2Cf_auto%2Cfl_progressive%2Cq_auto%2Cw_880/https://dev-to-uploads.s3.amazonaws.com/i/3y0bhisv135j7979dk3m.jpg
HTML file

    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <link rel="stylesheet" href="styles.css">
    <title>Price Grid</title>

Join our community

30-day, hassle-free money back guarantee

Gain access to our full library of tutorials along with expert code reviews.

Perfect for any developers who are serious about honing their skills.

Monthly Subscription

$29 per month

Full access for less than $1 a day

            <button>Sign up</button>

Why us?

Tutorials by industry experts

Peer & expert code review

Coding exercises

Access to our GitHub repos

Community forum

Flashcard decks

New videos every week

CSS File
html {
    margin: 0;
    padding: 0;
    font-family: 'Abel', sans-serif;
}

body {
    background-color: rgb(230, 230, 230);
}

.container {
    position: absolute;
    left: 50%;
    top: 50%;
    transform: translate(-50%, -50%);
    width: 800px;
    height: 500px;
    border: 20px solid rgb(20, 148, 20);
    border-radius: 25px;
}

.description {
    padding: 20px;
    text-align: left;
    background-color: rgb(255, 255, 255);
}

.pricing {
    position: relative;
    float: left;
    top: 5.5%;
    height: 55%;
    width: 50%;
    color: white;
    background-color: rgb(43, 179, 177);
    text-align: center;
}

.featuredContent {
    position: relative;
    float: right;
    top: 5.45%;
    color: rgb(255, 255, 255);
    background-color: rgb(74, 190, 189);
    height: 55%;
    width: 50%;
}

.featuredContent h2 {
    text-align: center;
}

.featuredContent ul {
    position: relative;
    list-style: none;
    left: 20%;
}

HTML
Several elements are marked up as headlines, although they certainly aren't ones:

The text "Gain access..." in the description element should be two(!) paragraphs. (Don't use
 to create paragraph-like line breaks.)

Gain access to our full library of tutorials along with expert code reviews.

Perfect for any developers who are serious about honing their skills.

"$29 per month" and "Full access..." in pricing shouldn't be headlines either - especially not h2s after a h3. Also the price ("$29") itself needs to be markuped separately, since it's highlighted compared to the rest of the line. For example:

$29 per month

Full access for less than $1 a day

"Why us?" looks identical and is structurally identical to the headline "Monthly Subscription" in pricing, so it should have the same headline level.

The "button" "Sign up" functionally looks more like a link, than a <button>. (Buttons are primarily used to submit forms, which this is not.)
The class names are too generic and so can collide with other classes used on the site. Instead of container something like price-component would be more appropriate. And for the inner components either use a child/descendant selector in the CSS so that the rules only apply to your component:
.price-component > .description

Or use a CSS naming scheme (such as BEM) to create a unique class names for the inner components such as price-component__description.
CSS
Avoid absolute positioning. Centering the price component like that is only appropriate, if it's only used as a modal dialog, that covers the rest of the content of the page and the screen shot doesn't seem to indicate that. In order to "just" center it, use CSS grid or flex.
Also the use of relative positioning isn't appropriate. On those floats it is strange. It just creates a gap where there shouldn't be one and lets the elements overlap the bottom border. And on the list use either a left-margin or just a general padding on the featuredContent element.
Float is not appropriate for layout any more. This component is the perfect example for a grid layout.
Hard coding the width and height in pixels makes the component unresponsive to the screen and font size. For the max-width use relative units such as rem and don't set the height at all, letting it resize dynamically based on the content size. Also consider an alternative media query based layout for smaller screen sizes where it would be too wide.
Don't center all the text. For one, it contradicts the screen shot, but more importantly centered text is difficult to read.
Example: https://jsfiddle.net/vjr2ogpq/


jQuery date split into day month year I'm changing an HTML datepicker value (#date) into 3 separate values in order to post them to a booking engine. I've managed to get it to work but there is a lot of repetition and the original #date value is also submitted unnecessarily.

The engine takes these parameters:

date_day=27&date_month=9&date_year=2013&nights=3&people=2&ref=558&langClient=eng&expr=EUR`

HTML

 <form id="quick_res" action="#" method="post" name="availability">

                Date :<input type="date" name="date" id="date" placeholder="12/09/13">

                Nights :<input type="number" name="nights" id="nights" placeholder="2">

                Guests :<input type="number" name="people" id="people" placeholder="2">

                <input name="ref" type="hidden" id="ref" value="558" />
                <input name="langClient" type="hidden" id="langClient" value="eng" />

                Currency :
                    <select name="expr" id="expr">
                        <option value="EUR" selected="selected">EURO</option>
                        <option value="USD">US Dollar</option>
                        <option value="ARS">Argentine Peso</option>
                        <option value="AUD">Australian Dollar</option>
                        </select>

                <input type="submit" name="submit" class="secondary button radius text-center" value="Click to Book">

    </form>

jQuery

 $('#date').on('change', function() {
     var new_val = $(this).val().split('-'),
         dateYear = parseInt(new_val[0]),
         dateMonth = parseInt(new_val[1]),
         dateDay = parseInt(new_val[2]);

    $('<input>', {
        type: 'hidden',
        name: 'date_day',
        id: 'date_day',
        value: dateDay

    }).appendTo('#anchor');

    $('<input>', {
        type: 'hidden',
        name: 'date_month',
        id: 'date_month',
        value: dateMonth

    }).appendTo('#anchor');

    $('<input>', {
        type: 'hidden',
        name: 'date_year',
        id: 'date_year',
        value: dateYear

    }).appendTo('#anchor');

 });

The tought process to make your code DRY is almost always the same:

Find the commonalities between blocks of code.
Extract these into an iterable structure.
Loop.

Here we can see that everything is the same except the prefix and the value.

The code could be changed to:

Please note that you should always specify the radix argument when using the parseInt function since 10 is not default in every browser.

$('#date').on('change', function() {
    var newVal = $(this).val().split('-'), //renamed new_val to newVal, always stick to one naming convention
        dateParts = {
            year: parseInt(newVal[0], 10),
            month: parseInt(newVal[1], 10),
            day: parseInt(newVal[2], 10)
        };

    $('#anchor').append($.map(dateParts, function (index, key) {
        var name = 'date_' + key;

        return $('<input>', {
            type: 'hidden',
            name: name,
            id: name,
            value: dateParts[key]
        });
    }));
});

EDIT:

Note that with the above logic, you will end up having duplicate inputs with different values if the user changes the value multiple times. You could solve this by removing the previously added inputs before adding the new ones, but you could also simply add the inputs just prior to form submission by listening to the submit event instead.

Also, if you want to avoid sending #date's value, you can disable that input before submitting; disabled inputs aren't sent to the server.

Finally, you could choose to simply submit the form in AJAX using $.ajax (or any of it's shortcut methods such as $.post). That would have the advantage of making your site slighlty more dynamic and you would not have to append inputs to the form since you can use the data option.


Fluent interface builder I'm working on a fluent interface builder that takes:

an array of strings, each of which becomes a key to an object that is a function that follows the fluent interface pattern
a terminator function which when called breaks the chain and does something else

I'm looking for some advice on how to improve this.

var Twitter = require('twit');

var secrets = require("./secrets/secrets");
var fs = require("fs");
var Twit = require('twit')

    var T = new Twit({
      consumer_key: secrets.consumer_key,
      consumer_secret: secrets.consumer_secret,
      access_token: secrets.access_token,
      access_token_secret: secrets.access_token_secret,
      timeout_ms: 60 * 1000,  // optional HTTP request timeout to apply to all requests.
    });
    function fluentBuilder(options, terminator) {
      let locals = options.reduce((opt, e) => {
        opt[e] = null;
        return opt;
      }, {});
      var methods = options.reduce((opt, e) => {
        opt[e] = (val) => {
          if (val) {
            locals[e] = val;
            return opt;
          } else return locals[e];

        }
        return opt;
      }, {});
      methods.locals = locals;
      methods["execute"] = terminator;
      return methods;
    }

    var geocode = fluentBuilder(["lat", "long"],
      function () {
        var self = this;
        return new Promise(resolve => {
          T.get("geo/reverse_geocode", self.locals, function (err, data, results) {
            resolve(data);
          });
        });
      });
      //calls to lat and long are stored on the self.locals variable which is passed to the Promise callback.
      geocode.lat("34.0406")
        .long("-84.2031")
        .execute()
        .then(data => {
          console.log(data);
        });

This seems like a lot of complexity for limited/no value. Could you not just pass an object literal like {lat: 34, long: 84} to the geocoding function and be done with it, losing like 20 lines of code in the process?

A few other thoughts:

Why are you requiring the same dependency twice under different names?
Typically, javascript programmers like to use initial lowercase camel case.  You have several variables named starting with capital letters which may not be desirable.
You have inconsistent use of let vs. var in fluentBuilder. Why have locals defined with let and methods defined with var when both variables are used within the same scope and there is no "inner" code block to which you are trying to limit visibility of locals.  Either one works exactly the same in this case, but it would be good to be consistent.
Your indentation is inconsistent.
Why does fluentBuilder return methods?


Multiple jQuery onClick events I have a ton of jQuery onClick events. onClick I hide/show different UI elements. I was wondering, how can I tidy the code up and make multiple onClick events more readable?

$('.info_2').on('click', function() {
  $('#nav-wrapper').toggleClass('hidden_nav');
  $('#card-wrapper').toggleClass('centre_share');
  $('.E_info').toggleClass('display');
  $('#info-btn').css('opacity', '0');
  $('#nav-wrapper').delay(300).toggleClass('hidden');
  $('#nav-wrapper').removeClass('display_nav');
  $('#nav-wrapper').removeClass('display');
});

$('.info_back').on('click', function() {
  $('#nav-wrapper').removeClass('hidden_nav');
  $('#nav-wrapper').addClass('display_nav');
  $('#nav-wrapper').addClass('display');
  $('#info-btn').css('opacity', '1');
  $('#nav-wrapper').removeClass('hidden');
  $('.E_info').removeClass('display');
  $('.E_info').addClass('hidden');
  $('#card-wrapper').removeClass('centre_share');
});

$('#info-btn').on('click', function(){
  $('#info-btn').toggleClass('close_btn');
  $('.o-card_border').toggleClass('info_display card_active');
  $('.start_title').toggleClass('hidden remove_flow')
  $('#svg_full').attr('class', 'test');
  $('#svg_top').attr('class', 'test');
  $('#svg_bot').attr('class', 'test');
  $('#svg_bot_bot').attr('class', 'test');
  $('#svg_bot_right').attr('class', 'test');
  $('.rectangle_style_frame3 display').toggleClass('hidden');
  $('.triangle_style').toggleClass('hidden');

  $('.bg-info').toggleClass('display');
  $('.info_CharactersInvolved').toggleClass('display');
  $('.info_themes').toggleClass('display');
  $('.E_info').toggleClass('display');
});

The code works fine. I just think it looks really ugly. And the readability of it is painful, especially if you're trying to jump onto the project and learn the codebase.

The worst issues are cases of the same element being selected multiple times in order to apply various methods. Repeat selection of the same element should be avoided because selection of DOM element(s) is expensive.

There are also are cases where multiple selections have the same method applied. This isn't so bad for performance, but leads to unnecessarily bulky source.

The code can be improved with the following techniques :

using method chaining.
using comma separated selectors.
using $(this) to select the same element as the one to which an event handler is attached.
passing space-separated lists of class names to addClass() and .removeClass().

You might end up with something like this :

$('.info_2').on('click', function() {
    $('#card-wrapper').toggleClass('centre_share');
    $('.E_info').toggleClass('display');
    $('#info-btn').css('opacity', '0');
    $('#nav-wrapper').toggleClass('hidden_nav').delay(300).toggleClass('hidden').removeClass('display_nav').removeClass('display');
});

$('.info_back').on('click', function() {
    $('#nav-wrapper').removeClass('hidden_nav hidden').addClass('display_nav display');
    $('#info-btn').css('opacity', '1');
    $('.E_info').removeClass('display').addClass('hidden');
    $('#card-wrapper').removeClass('centre_share');
});

$('#info-btn').on('click', function(){
    $(this).toggleClass('close_btn');
    $('.o-card_border').toggleClass('info_display card_active');
    $('.start_title').toggleClass('hidden remove_flow');
    $('#svg_full, #svg_top, #svg_bot, #svg_bot_bot, #svg_bot_right').attr('class', 'test');
    $('.rectangle_style_frame3 display, .triangle_style').toggleClass('hidden');
    $('.bg-info, .info_CharactersInvolved, .info_themes, .E_info').toggleClass('display');
});


"Which Century" programming exercise I recently wrote this up and am wondering if there is a better way to achieve the results I want.

Essentially, we're taking a given year and adding 'th, nd, rd, st' depending on the digit.

Any suggestions on how to shorten this code and make it better?

Keep in mind that this is taking centuries that change every 01st year. For example, year 2000 is the 20th century, but 2001 is the 21st century.

def which_century(string)
  year = string.to_i
  modifier = 1

  return "1st century" if year < 101

  if year > 100 && string.end_with?("00")
    modifier -= 1
  end

  century = year / 100 + modifier
  p century
  if century > 10 && century < 20
    return century.to_s << "th century."
  end

  second_digit = century % 10

  case second_digit
  when 1 then century.to_s << "st century"
  when 2 then century.to_s << "nd century"
  when 3 then century.to_s << "rd century"
  else
    century.to_s << "th century"
  end
end

puts "Type in a year:"
answer = gets.chomp
p which_century(answer)

Let's clarify the logic.  The code simplification will follow.  The rules are:

"th" years are those ending in 4, 5, 6, 7, 8, 9, or 0.  Additionally, the 11th, 12th, and 13th centuries are exceptions also ending in "th".
Years ending in 1 are "st" years.
Years ending in 2 are "nd" years.
Years ending in 3 are "rd" years.

Now just translate the above into code:

def which_century(string)
  century  = (string.to_f / 100).ceil
  year     = century % 10
  nth_year = (11..13).include?(century) || !(1..3).include?(year)
  return "#{century}th century" if nth_year
  return "#{century}st century" if year == 1
  return "#{century}nd century" if year == 2
  return "#{century}rd century" if year == 3
end


Passing variable pointer as string to another function With Delphi 7 I need to create a TStream variable and use/free it in another part of the code. Unfortunately it cannot be passed as "normal" parameter.

I created a simple application for testing what I need, using some examples I found on Google and some help from SOF (I am not used to pointers).

Here is what I got so far (it uses an Edit, a Memo e 3 Buttons):

function IsObject(Address: Pointer): Boolean; assembler;
asm
  MOV     EAX, [Address]
  CMP     EAX, EAX.vmtSelfPtr
  JNZ     @False
  MOV     Result, True
  JMP     @Exit
@False:
  MOV     Result, False
@Exit:
end;

procedure TForm1.btnSetMem1Click(Sender: TObject);
var
  Stm: TStream;
begin
  Stm := TMemoryStream.Create;
  edtMemID1.Text := IntToStr(Integer(Stm));
end;

procedure TForm1.btnSaveMem1Click(Sender: TObject);
var
  Stm: TStream;
begin
  Stm := Pointer(StrToInt(edtMemID1.Text));
  if IsObject(Stm) then
  try
    memStm1.Lines.SaveToStream(Stm);
    btnSaveMem1.Enabled := False;
    btnLoadMem1.Enabled := True;
    memStm1.Clear;
  except
    FreeAndNil(Stm);
  end;
end;

procedure TForm1.btnLoadMem1Click(Sender: TObject);
var
  Stm: TStream;
begin
  Stm := Pointer(StrToInt(edtMemID1.Text));
  if IsObject(Stm) then
  try
    Stm.Position := 0;
    memStm1.Lines.LoadFromStream(Stm);
  except
    FreeAndNil(Stm);
  end;
end;

procedure TForm1.btnFreeMem1Click(Sender: TObject);
var
  Stm: TStream;
begin
  Stm := Pointer(StrToInt(edtMemID1.Text));
  if IsObject(Stm) then FreeAndNil(Stm);
  btnSaveMem1.Enabled := False;
  btnLoadMem1.Enabled := False;
  edtMemID1.Clear;
end;

procedure TForm1.edtMemID1Change(Sender: TObject);
begin
  btnSaveMem1.Enabled := (Trim(edtMemID1.Text) <> '');
  btnFreeMem1.Enabled := (Trim(edtMemID1.Text) <> '');
end;

The ASM function I found here and seems to be working. I also tested with FastMM and there is no memory leak (if btnFreeMem is used).

Please review this code and maybe give some tips to make it better or avoid problems (memory leaks).

There is not much to be reviewed here.

Function IsObject does it's job correctly and there is not much to be said there.

What would have to be reviewed is the real code that would need IsObject and memory management technique you have shown in your demo. Since your demo is just a demo it doesn't show real life issue you are trying to resolve, and therefore it is impossible to give you any tips and advice on how to improve it.

All I can say is don't ever manage memory like that, and if you really, really feel the need to do so, then post such code here for review. Unless you have some real corner case there, I am quite sure that such code can be thoroughly reviewed and refactored.

So, your code is perfectly fine except you should not be using it anywhere :)


Board-tile game click event OOP pattern This is actually an old 'problem' that I never really knew how to improve, but I'm wondering now if there is a better approach for this problem.

I'm creating MineSweeper with Java and struggling with an OOP aspect of the game. Basically I have a class Square.java, SquareBoms(extends Square) and Field.java. My approach on the Square is that it should only be aware of itself and should not communicate in any way or form with the Field.

However, the Field should check the surrounding SquaresBombs for bombs once a Square has been clicked. So the Square SHOULD message the Field that it has been clicked, but I don't think this is the right way of using OOP.

import java.awt.Color;
import java.awt.Insets;
import java.awt.event.ActionEvent;
import java.awt.event.MouseAdapter;
import java.awt.event.MouseEvent;

import javax.swing.JButton;

public class Square extends JButton {
    Field owner; //This is not OOP as the Square should only be aware of itself and what is in it.
    int coords;
    boolean isChecked = false;
    boolean isMarked = false;
    int bombsAround;

    public Square(Field owner, int coords) {
        this.owner = owner;
        this.coords = coords;
        this.setSize(400, 400);
        this.setMargin(new Insets(0, 0, 0, 0));
        this.addMouseListener(new MouseAdapter(){
            public void mouseClicked(MouseEvent e){
                if (e.getButton() == MouseEvent.BUTTON1) {
                    Clicked();
                }
                else if(e.getButton() == MouseEvent.BUTTON3) {
                    Mark();
                }
            }
        });
    }

    public void Clicked() {
        if(!isChecked) {
            this.setBackground(new Color(238, 238, 238)); // Reset when it had been marked
            isChecked = true;
            this.setEnabled(false);

            owner.checkSquare(this); //This is not OOP as the Square should only be aware of itself and what is in it.
        }
    }

    public void Mark() {
        if(!isChecked) {
            if(!isMarked)
                this.setBackground(Color.GREEN);
            else
                this.setBackground(null);

            isMarked = !isMarked; // toggle on every click
        }
    }

}

UPDATE

My original question was if it was 'allowed' to let a child object know about its parent. After doing some research i've come to the conclusion that this is not wrong in pure OOP-terms.

Separation of Logic and Rendering

The Square object is essentially a button that also contains some information for display, such as the number of surrounding bombs.

The problem you are having is that this GUI element is communicating directly with the game board of your minesweeper game model.  When Square gets clicked, it tells the board to check the amount of bombs.

I'm not very familiar with Swing, but typically I would expect there to be a Scene or Window at the top level of the hierarchy.  Once clicked, the button would tell the controller to tell the game model to perform the necessary action, and then the controller would update the view based on changes to the model.

In this approach you could possibly have a Game object, Board object, and Square object in the game model. On a button click, the coordinates of the Square button object in the view would be passed into the Game, and the appropriate changes would be made to the Board and to the game state.  For example, the coordinates have a bomb, and so the state of the Game is set to GameState.LOSS.

In this approach, you could make the Board object available to the view.
After each move, the view could update based on the new state of the board, marking the squares that have flags, have been uncovered, etc.

You could also do something such as compute the positions of the nearby Square objects at the start of the game, and then you could check for bombs like this:

int bombCount = 0;
for (Position position : square.neighbors) {
    Square neighbor = board.get(position);
    if (neighbor.hasBomb) {
        bombCount++;
    }
}

Or alternatively store a reference to the actual Square objects, if you want.

The advantage of all of this is that if you some day want to use a different graphical representation other than Swing, you could easily take the code for the game model and use a different controller and view. You would still use the model in the same way, passing in the coordinates of the location on the board that is clicked.


Determine if one string is a permutation of the another Can anyone give me some feedback if I'm using std::unordered_multimap optimally? I don't know the ins-and-outs of the std::unordered_multimap implementation.

I'm wondering if comparing two maps iterates through each of the elements in the map, making the algorithm \$O(N^2)\$ or if it just compares the hash of the map or something making the algorithm \$O(N)\$. Or would it just be better to use an array if I'm given this question in an interview?

bool is_permutation(const std::string& s1, const std::string& s2)
{
    assert(s1.size() && s2.size());

    if(s1.length() != s2.length())
        return false;

    std::unordered_multimap<int, char> m1, m2;

    for(int i = 0; i < s1.length(); ++i)
    {
        m1.insert({s1[i], s1[i]});
        m2.insert({s2[i], s2[i]});
    }

    if(m1 == m2)
        return true;

    return false;
}

I would say yes, use an array.

Arrays afford very good memory locality, which is good for cache performance. They essentially are the most basic and efficient hash map if the key is the index.

You only need one array with 256 elements: first count up the number of times each character appears in the first string, and then count down for the number of times each character appears in the second string. Precisely if the two strings are permutations, the array will finish with all zeroes.

As a further hint at relative complexity, according to godbolt.org, your code produces 849 lines of assembly when compiled on the most recent GCC with -O3 optimisation.
The following code produces 53 lines.

bool is_permutation(const std::string& s1, const std::string& s2)
{
    if(s1.length() != s2.length())
        return false;

    // There are usually 256 possible characters but on rare architectures may be more.
    constexpr possible_char_count = std::numeric_limits<unsigned char>::max()+1;
    std::array<size_t, possible_char_count > m1 = {}; // Zero initialized all places

    for(char c : s1) { // Count up for each character in the first string
        m1[static_cast<unsigned char>(c)]++;
    }
    for(char c : s2) { // Count down for each character in the second string
        m1[static_cast<unsigned char>(c)]--;
    }

    for (size_t t : m1) {
        // If any character appears a net nonzero number of times,
        // the two strings are not anagrams.
        if (t != 0) {
            return false;
        }
    }
    return true;
}

Lines of code are not, of themselves, a perfect indicator of what is the most efficient algorithm. After all, you can implement a nice short version of bubble sort! Nevertheless when the underlying approach is the same (that of counting characters) it is a fairly good indicator that one implementation is doing unnecessary work.

One reason to prefer a map to an array is that when the input strings are very short or very homogenous, then the map is likely to be more memory efficient. This may translate into speed improvements by avoiding allocation and inialisation overhead. As always, the only reliable way to decide is to profile with your real world inputs.


Creating an interactive tour for a website I'm using grumble to create an interactive tour for a website I'm working on (I'm aware there is a plugin called crumble that does this but I want to have full control of the functionality).

Basically, I initialise 6 grumbles (this is the amount of tour bubbles I need), then hide the last 5 and then attach each click handler separately. In each click handler I hide the one that is clicked and show the next one.

$(document).ready(function() {

 // First grumble
 $('#someID').grumble({
  text: 'This is a short tour, click to continue',
  angle: 130,
  distance: 10,
  type: 'firstGrumble '
 });

 // Second grumble
 $('#someOtherID').grumble({
  text: 'This is some tour text',
  angle: 30,
  distance: 230,
  type: 'secondGrumble '
 });

 // Third grumble
  $('#someOtherOtherID').grumble({
  text: 'This is some more tour text',
  angle: 5,
  distance: 30,
  type: 'thirdGrumble '
 });

 // Fourth grumble
 $('#anotherID').grumble({
  text: 'This is another bit of tour text',
  angle: 60,
  distance: 30,
  type: 'fourthGrumble '
 });

 // Fifth grumble
 $('#yetAnotherID').grumble({
  text: 'This is also another bit of tour text',
  distance: 30,
  type: 'fifthGrumble '
 });

 // Sixth grumble
 $('#andAnotherID').grumble({
  text: 'This is the end of the tour',
  angle: 300,
  distance: 30,
  type: 'sixthGrumble '
 });

 // Hide the freshly created gumbles
 $('.secondGrumble').hide();
 $('.secondGrumble + div').hide();
 $('.thirdGrumble').hide();
 $('.thirdGrumble + div').hide();
 $('.fourthGrumble').hide();
 $('.fourthGrumble + div').hide();
 $('.fifthGrumble').hide();
 $('.fifthGrumble + div').hide();
 $('.sixthGrumble').hide();
 $('.sixthGrumble + div').hide();

 // Click handler for first grumble
 $(document).on("click", '.firstGrumble + div', function() {
  $('.firstGrumble').hide();
  $('.firstGrumble + div').hide();
  $('.secondGrumble').show();
  $('.secondGrumble + div').show();
 });

 // Click handler for second grumble
 $(document).on("click", '.secondGrumble + div', function() {
  $('.secondGrumble').hide();
  $('.secondGrumble + div').hide();
  $('.thirdGrumble').show();
  $('.thirdGrumble + div').show();
 });

 // Click handler for third grumble
 $(document).on("click", '.thirdGrumble + div', function() {
  $('.thirdGrumble').hide();
  $('.thirdGrumble + div').hide();
  $('.fourthGrumble').show();
  $('.fourthGrumble + div').show();
 });

 // Click handler for fourth grumble
 $(document).on("click", '.fourthGrumble + div', function() {
  $('.fourthGrumble').hide();
  $('.fourthGrumble + div').hide();
  $('.fifthGrumble').show();
  $('.fifthGrumble + div').show();
 });

 // Click handler for fifth grumble
 $(document).on("click", '.fifthGrumble + div', function() {
  $('.fifthGrumble').hide();
  $('.fifthGrumble + div').hide();
  $('.sixthGrumble').show();
  $('.sixthGrumble + div').show();
 });

 // Click handler for sixth grumble
 $(document).on("click", '.sixthGrumble + div', function() {
  $('.sixthGrumble').hide();
  $('.sixthGrumble + div').hide();
 });

});

I want to know a way to make this code more DRY.  I'm thinking some form of an each statement and a counter to create the 6 classes, but I could be wrong.

I would go for an approach as follows:

clickme

clickme

then some javascript fun:

var GrumblingSteps = (function(){
    var grumbleMagic = function($grumbleTarget) {
        //create grumble
        $grumbleTarget.grumble({
            text : $grumbleTarget.data('grumble-text'),
            angle : $grumbleTarget.data('grumble-angle'),
            distance : $grumbleTarget.data('grumble-distance')
        });
        //add click handler to the next in line if any
        if ( $grumbleTarget.data('grumble-next') ) {
            $nextGrumble = $('#'+$grumbleTarget.data('grumble-next'));
            $nextGrumble.on('click', function() {
                $grumbleTarget.grumble('hide');
                grumbleMagic($(this));
            });
        }
    }

    return function(firstGrumble) {
        var $firstGrumble = $(firstGrumble);
        grumbleMagic($firstGrumble);
    }
})();

Someone might be able to change this to a jQuery plugin or so. But for now you use it as follows:

GrumblingSteps('#grumble-1');

  It might have some errors in it since I coded this directly inside this comment box ;)


HackerRank Ransom Note challenge I'm solving HackerRank "Ransom Note" challenge. The code runs fine and cracks it, but I have an issue with its performance. It times out in some cases.

  A kidnapper wrote a ransom note but is worried it will be traced back
  to him. He found a magazine and wants to know if he can cut out whole
  words from it and use them to create an untraceable replica of his
  ransom note. The words in his note are case-sensitive and he must use
  whole words available in the magazine, meaning he cannot use
  substrings or concatenation to create the words he needs.

  Given the words in the magazine and the words in the ransom note,
  print Yes if he can replicate his ransom note exactly using whole
  words from the magazine; otherwise, print No.

  Input Format

  The first line contains two space-separated integers describing the
  respective values of (the number of words in the magazine) and (the
  number of words in the ransom note).

  The second line contains space-separated strings denoting the words
  present in the magazine.

  The third line contains space-separated strings denoting the words
  present in the ransom note.

My implementation:

def ransom_note(magazine, ransom):
    for word in ransom:
        if word in magazine:
            magazine.remove(word)
        else:
            return False
    return True

m, n = map(int, input().strip().split(' '))
magazine = input().strip().split(' ')
ransom = input().strip().split(' ')
answer = ransom_note(magazine, ransom)
if(answer):
    print("Yes")
else:
    print("No")

It's timing out when there are too many items (30.000) on magazine or ransom. Performance talking, what can I improve here?

Your loop has a potential complexity of R * M (R number of words in ransom, M number of words in magazine). Additionally you use remove in the inner loop which which does linear search in magazine.

The solution is to count occurences of words in both, magazine and ransom. this is done in a single sweep and of linear complexity R + M (dict get/set is average linear).

the moste dense and readable solution is

from collections import Counter

def ransom_note(magazine, ransom):
    ran = Counter(ransom)
    mag = Counter(magazine)
    return len(ran - mag) == 0

if you assume a very long magazine and a short ransom message you could go for an early break by counting the ransom first, then do a down count with magazine words. magazine could be a generator in that case reading from an arbitrary large file.

from collections import Counter

def ransom_note(magazine, ransom):
    ran = Counter(ransom)
    if not ran:
        return True # handle edge case
    for m in magazine:
        if m in ran:
            # we need this word
            if ran[m] > 1:
                ran[m] -= 1
                # still need more of that one
            else:
                # the last usage of this word
                ran.pop(m)
                if len(ran) == 0:
                    # we found the last word of our ransom
                    return True
    return False


Is my implementation of FizzBuzz overkill? I am doing FizzBuzz on a take home test (in Unity).  I am sure this is just a competency check, but I went ahead and followed some coding conventions such as using constant fields, dependency injection, comments, etc.  Is this overkill?  If I should "dumb it down", how should I do so?  Also, as seen in the directions, it implies that for multiples of 3 and 5, I should print out "Fizz", "Buzz", and "FizzBuzz"...but I am reading too much into it, right?

I am trying to have an optimal program here in terms of production code.  I hope I am not being too picky and misusing Code Review.

///******************************************
// * FizzBuzz.cs
// *
// * INSTRUCTIONS:
// * Write a program that prints the numbers 1 to 100.
// * For multiples of 3, output the string "Fizz" instead of the number.
// * For multiples of 5, output the string "Buzz" instead of the number.
// * For multiples of 3 & 5, output "FizzBuzz" instead of the number.
// ******************************************/

using UnityEngine;
using System.Collections;

public class FizzBuzz : MonoBehaviour
{
    private const string FIZZ = "Fizz";
    private const string BUZZ = "Buzz";
    private const string FIZZ_BUZZ = "FizzBuzz";

    void Start()
    {
        PrintFizzBuzz(100);
    }

    /// <summary>
    /// Prints the numbers [1, n], unless it is a multiple of 3 or 5.
    /// If a multiple of 3, prints FIZZ.
    /// if a multiple of 5, prints BUZZ.
    /// if a multiple of 3 and 5, prints FIZZ_BUZZ.
    /// </summary>
    /// <param name="n">The maximum number [1, infinity] to print to</param>
    private void PrintFizzBuzz(int n)
    {
        if (n < 1)
        {
            Debug.Log("int n in fizzBuzz.PrintFizzBuzz(int) must be >= 1");
            return;
        }
        for (int i = 1; i <= n; i++)
        {
            if (i % (3 * 5) == 0)
            {
                Debug.Log(FIZZ_BUZZ);
            }
            else if (i % 3 == 0)
            {
                Debug.Log(FIZZ);
            }
            else if (i % 5 == 0)
            {
                Debug.Log(BUZZ);
            }
            else
            {
                Debug.Log(i);
            }
        }
    }
}

You have misunderstood what dependency injection is - what you have done is just use a parameter to set the length of the sequence which is great from a flexibility point of view.

Dependency injection at its simplest level is a class having its dependencies injected into it by something higher up. So, you're writing the fizzbuzz out to Debug - that's a dependency but your class uses it directly. It would be better to have that injected in. In a non-unity world:

// using System;
public class SomeSampleClassThatOutputs
{
    Action<string> _output = s => { };

    public SomeSampleClassThatOutputs(Action<string> output)
    {
        _output = output;
    }

    public void PrintSomething()
    {
        _output("Something");
    }
}

As you can see, the dependency (where to output) is being injected into the class. I used a Func<string> for simplicity but you could easily use a Stream or something.

The caller is now responsible for where the class puts its output:

// Print to Console
var someClass = new SomeSampleClassThatOutputs(Console.WriteLine);
someClass.PrintSomething();

That's the essence of it. You could also use property injection but I don't know enough about Unity to advise on the best way of doing it in Unity.

There's a lot of examples of FizzBuzz around (especially here) so I won't really go into detail about that. Note however, that you have factored out the strings to constants (Fizz, Buzz, FizzBuzz) but you haven't done the same for the numbers (3, 5, 15). You'll notice that they are actually paired together - might want to think about suitable structure for storing that...

C# constants are generally named UpperPascalCase not ALL_CAPS_SNAKE_CASE but I don't know whether Unity follows a different convention.

Anyway, FizzBuzz is generally just a test of whether you can write even the most basic of programs - if you have a solution that works you'll probably get through.

Edit

Just thought I'd nitpick - your comment [1, infinity] is incorrect - it's actually [1, int.MaxValue]


Namespaced JavasScript code I have the following example app: http://dev.driz.co.uk/jsfunction/

Which runs two types of script files with the same outcome.

Script1:

var script1 = {

    showMessage: function( message ) {

        var el = document.getElementById('example1');

        el.innerHTML = message;

    }
}

and script 2:

window.script2 = function() {

    function showMessage( message ) {

        var el = document.getElementById('example2');

        el.innerHTML = message;

    }

    return {

        showMessage: showMessage

    }

}();

They are both namespaced so I can them like:

window.onload = function() {

    script1.showMessage('Hello world!');

    script2.showMessage('How are you?');

};

But what's the better way forward? Script1 is the type I would normally write but it requires you to sometimes call a init function first if you have some events that need running on document load.

For example:

var script = {

    init: function() {

        // some events such as onlick etc..

    }

};

script.init();

Script2 however is a self executing function and therefore would get around this but being a function itself would it allow the same flexibility?

Any thoughts? Suggestions or examples of how people namespace there script files to make them neater and more efficient?

Try module-pattern, or somewhat like the module pattern, using an IIFE

In your case:

(function(ns){

  ns.showMessage = function(message){
    var el = document.getElementById('example1');
    el.innerHTML = message;
  }

}(this.script1 = this.script1 || {}));

window.onload = function() {
  script1.showMessage('Hello world!');
};

Here's the explanation

//basically, an IIFE is a function that executes itself
(function(ns){

  //in here, ns is a reference to our this.namespace

  //also, this IIFE provides us with a local scope
  //we can declare functions and variables that only this scope, by default, sees

  //a "private" variable
  var priv = "I can only be seen in the module";

  //a "private" function
  function privFn(){
    console.log('I am only usable in the module');
  }

  //a "public", or more accurately, an "exposed" variable
  //we attach it to the namespace
  ns.foo = 'I am an exposed value';

  //an exposed function
  ns.bar = function(){
    console.log('exposed!');
    //with an exposed function, we can also call a private function
    privFn();
  }

//this line creates the namespace or uses an existing one
//it then executes the function, passing the created/existing namespace
}(this.namespace = this.namespace || {}));

And so, using our module:

//we can use the exposed entities
namespace.bar();
//exposed!
//I am only usable in the module <- since bar called privFn, it also prints
namespace.foo === 'I am an exposed value'


Separating and organising modules with objects? As I'm creating a 3D Tic-Tac-Toe game, I'm having several modules inside my JavaScript application. For example, there is a 3D module which handles perspective calculations, whereas a Multiplayer module manages everything with relation to online playing.

Is it good practice to put functions of each module in its separate object?

I currently have:

multiplayer.js:

var Multiplayer = {
    socket: new Socket("ws://192.168.0.100:123/test"), // Relevant variables concerning multiplayer stuff

    createSession:
    function() {
       //...
    }
}

renderer.js:

var Renderer = {
    f: 200, // Relevant variables concerning 3D rendering stuff
    g: 200;

    render:
    function() {
        //...
    }
}

and so on.

Is this a good practice of organising my project? Or is there a more efficient way?

I would recommend you use a closure like this:

var Obj = (function() {
    var privateState = null;
    return {
        A: 1,
        B: true,
        C: function() {
            return privateState;
        }
    };
})();

The biggest reason why this is a good idea is that it adds the ability to keep private state within the closure. Normally you can define private properties by convention but as the complexity of an object increases the better it is to keep certain things private.